Closed
Bug 1204812
Opened 9 years ago
Closed 9 years ago
Consider leaving Console.jsm behind in /toolkit
Categories
(DevTools :: General, defect)
DevTools
General
Tracking
(firefox44 fixed)
RESOLVED
FIXED
Firefox 44
Tracking | Status | |
---|---|---|
firefox44 | --- | fixed |
People
(Reporter: jryans, Assigned: ochameau)
References
(Blocks 1 open bug)
Details
Attachments
(3 files, 3 obsolete files)
(deleted),
patch
|
mossop
:
review+
jryans
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jryans
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review |
Console.jsm is used in many places outside DevTools, so it may make more sense to leave it back in toolkit.
If we want to do that, it's probably simplest to put it in /toolkit/modules (no reason for a DevTools folder with one file) and just install it to its previous path.
Comment 1•9 years ago
|
||
I agree!
Assignee | ||
Comment 2•9 years ago
|
||
Assignee | ||
Comment 3•9 years ago
|
||
Assignee | ||
Comment 4•9 years ago
|
||
This is quite messy as we just moved it,
but better move it now, in the same cycle (Fx44).
I moved the shims to toolkit/devtools/
And moved Console.jsm to toolkit/modules/
I could have kept the shims in /devtools/shared/shims/
and also kept/move Console.jsm back to toolkit/devtools/.
But hopefully, after some deprecation time, we could drop the shims and the toolkit/devtools/ folder completely.
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → poirot.alex
Assignee | ||
Comment 5•9 years ago
|
||
Comment on attachment 8671352 [details] [diff] [review]
Move Console.jsm to toolkit/modules/
What do you think about my various choices?
Attachment #8671352 -
Flags: feedback?(jryans)
Reporter | ||
Comment 6•9 years ago
|
||
Comment on attachment 8671352 [details] [diff] [review]
Move Console.jsm to toolkit/modules/
Review of attachment 8671352 [details] [diff] [review]:
-----------------------------------------------------------------
* Moving the real file to toolkit/modules makes sense to me
* You also rewrote the path to match other toolkit modules, also makes sense (even though it's now a 3rd path for this module)
* I might have put the shim in toolkit/shims
Overall, seems good.
::: toolkit/moz.build
@@ +6,5 @@
>
> DIRS += [
> 'components',
> 'content',
> + 'devtools',
Maybe we could call this "shims" here?
It feels a little odd to revive toolkit/devtools for one compatibility file.
Attachment #8671352 -
Flags: feedback?(jryans) → feedback+
Assignee | ||
Comment 7•9 years ago
|
||
Moved the shim from /toolkit/devtools/shims/ to /toolkit/modules/shims/
as it looks weird to introduce a new folder in /toolkit for just one js file
and it feels logic to put it in modules.
Mossop, Are you ok in moving devtool's Console.jsm to toolkit/modules/?
This module is used by a lot of code in /browser, but also in /services
and addons. This module doesn't have a strong dependency on devtools,
it is more dependent to dom/ console written in C++ than code from /devtools.
Attachment #8671352 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8672646 -
Flags: review?(dtownsend)
Comment 8•9 years ago
|
||
Comment on attachment 8672646 [details] [diff] [review]
Move Console.jsm in toolkit/modules/ v2
Yes, moving Console.jsm to toolkit/modules is fine (I'd expect devtools to still be responsible for it in general though). I would rather leave the shim in devtools/shared/shims for now, no need to make a whole new directory just for that one file.
There are a bunch of places in tree that use the old path, are you going to update them here too?
Attachment #8672646 -
Flags: review?(dtownsend) → feedback+
Assignee | ||
Comment 9•9 years ago
|
||
Assignee | ||
Comment 10•9 years ago
|
||
(In reply to Dave Townsend [:mossop] from comment #8)
> Comment on attachment 8672646 [details] [diff] [review]
> Move Console.jsm in toolkit/modules/ v2
>
> Yes, moving Console.jsm to toolkit/modules is fine
Great!
> (I'd expect devtools to still be responsible for it in general though).
Sounds fine.
> I would rather leave the
> shim in devtools/shared/shims for now, no need to make a whole new directory
> just for that one file.
Done. I did that in the expectation that devtools would become a system addon,
but we are far from there yet.
>
> There are a bunch of places in tree that use the old path, are you going to
> update them here too?
Yes, that the second patch attached to this bug ;)
Attachment #8672646 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8673126 -
Flags: review?(jryans)
Attachment #8673126 -
Flags: review?(dtownsend)
Reporter | ||
Updated•9 years ago
|
Attachment #8673126 -
Flags: review?(jryans) → review+
Updated•9 years ago
|
Attachment #8673126 -
Flags: review?(dtownsend) → review+
Assignee | ||
Comment 12•9 years ago
|
||
Comment on attachment 8673127 [details] [diff] [review]
Rewrite the URLs v2
This is just a sed #devtools/shared/Console.jsm#Console.jsm# over the whole codebase.
Are you fine being the only reviewer for this?
Try looks good, I spawn some retry on some orange. There is some failure on videopuppeteer test suite, but I imagine that's not related to this patch.
Attachment #8673127 -
Flags: review?(jryans)
Reporter | ||
Comment 13•9 years ago
|
||
Comment on attachment 8673127 [details] [diff] [review]
Rewrite the URLs v2
Review of attachment 8673127 [details] [diff] [review]:
-----------------------------------------------------------------
I believe it's fine for me to review this, since DevTools implicitly reviewed the same change I made when moving the files originally. Also, this change is quite mechanical, since they should all be updated.
Attachment #8673127 -
Flags: review?(jryans) → review+
Reporter | ||
Comment 14•9 years ago
|
||
Comment on attachment 8673126 [details] [diff] [review]
Move Console.jsm in toolkit/modules/ v3
Review of attachment 8673126 [details] [diff] [review]:
-----------------------------------------------------------------
::: devtools/shared/shims/moz.build
@@ +15,5 @@
> 'Loader.jsm',
> 'Simulator.jsm',
> ]
> +
> +# Extra compability layer for transitional URL used in middle of fx44 cycle
Nit: compatibility
Assignee | ||
Comment 15•9 years ago
|
||
Assignee | ||
Comment 16•9 years ago
|
||
Merged the two previous patch with the nit addressed.
Assignee | ||
Comment 17•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/fcd050cd03e3959f4a21df42a47cdb881bfef2cf
Bug 1204812 - Keep Console.jsm in toolkit/modules/ r=jryans,Mossop
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox44:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•