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)
3.13 KB,
patch
|
mossop
:
review+
jryans
:
review+
|
Details | Diff | Splinter Review |
71.82 KB,
patch
|
jryans
:
review+
|
Details | Diff | Splinter Review |
74.97 KB,
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
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a67bb592f4c3
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
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cf6005825e9f
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
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7996b70da862
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
https://hg.mozilla.org/mozilla-central/rev/fcd050cd03e3
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
•