Closed
Bug 933727
Opened 11 years ago
Closed 10 years ago
Land the back-end changes for JSTerm
Categories
(DevTools Graveyard :: Graphic Commandline and Toolbar, defect)
DevTools Graveyard
Graphic Commandline and Toolbar
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 30
People
(Reporter: jwalker, Assigned: jwalker)
References
Details
Attachments
(1 file, 11 obsolete files)
1.56 MB,
patch
|
Details | Diff | Splinter Review |
We're splitting landing JSTerm into parts. The first part is the largest, it gets rid of the huge gcli.jsm file and replaces it with a virgin copy of the relevant parts of the GCLI repo along with the loader changes to make it all work nicely. Along the way many bugs were fixed. (See the blocks list)
Assignee | ||
Updated•11 years ago
|
Summary: Land the backed changes for JSTerm → Land the back-end changes for JSTerm
Assignee | ||
Comment 1•11 years ago
|
||
Try: https://tbpl.mozilla.org/?tree=Try&rev=89c5d17d10ac
Assignee | ||
Comment 2•11 years ago
|
||
This patch broken out: https://github.com/joewalker/gcli/pull/16
Attachment #825892 -
Flags: review?(mratcliffe)
Assignee | ||
Comment 3•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=b542ee9df548
Attachment #825892 -
Attachment is obsolete: true
Attachment #825892 -
Flags: review?(mratcliffe)
Attachment #826051 -
Flags: review?(mratcliffe)
Comment 4•11 years ago
|
||
Comment on attachment 826051 [details] [diff] [review] gcli.patch Review of attachment 826051 [details] [diff] [review]: ----------------------------------------------------------------- r+ Please also address comments in https://github.com/joewalker/gcli/pull/16 ... there is nothing serious, mostly grammar errors in comments.
Attachment #826051 -
Flags: review?(mratcliffe) → review+
Assignee | ||
Comment 5•11 years ago
|
||
https://github.com/joewalker/gcli/pull/16
Attachment #826051 -
Attachment is obsolete: true
Attachment #831562 -
Flags: review?(mratcliffe)
Comment 6•11 years ago
|
||
Comment on attachment 831562 [details] [diff] [review] v2 Review of attachment 831562 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me, r+
Attachment #831562 -
Flags: review?(mratcliffe) → review+
Assignee | ||
Comment 7•10 years ago
|
||
This update fixes a number of issues with remoting. See https://github.com/joewalker/gcli/pull/16 for the extra changes. It's green on try: https://tbpl.mozilla.org/?tree=Try&rev=2cba025dbd1f
Attachment #831562 -
Attachment is obsolete: true
Attachment #8341655 -
Flags: review?(mratcliffe)
Comment 8•10 years ago
|
||
Comment on attachment 8341655 [details] [diff] [review] v3 Review of attachment 8341655 [details] [diff] [review]: ----------------------------------------------------------------- Comments are on Github, just a few comment nits but otherwise everything looks great. r+
Attachment #8341655 -
Flags: review?(mratcliffe) → review+
Assignee | ||
Comment 9•10 years ago
|
||
Ms2ger, This patch looks huge because it splits out a large file into a number of smaller files. As part of that I wanted to copy several resources. The makefile in question is the one a little over half way down just after Loader.jsm. If this isn't the best way to do it (it works though) perhaps you could point me at the right way. Thanks.
Assignee | ||
Comment 10•10 years ago
|
||
Recent try build: https://tbpl.mozilla.org/?tree=Try&rev=763dd3465433
Assignee | ||
Comment 11•10 years ago
|
||
Mike: It's probably easier to finish this off at https://github.com/joewalker/gcli/pull/16.
Attachment #8341655 -
Attachment is obsolete: true
Attachment #8373270 -
Flags: review?(mratcliffe)
Assignee | ||
Comment 12•10 years ago
|
||
These changes update the developer toolbar to work with the split out GCLI, including making it use promises rather than callbacks, and removing function names.
Attachment #8373278 -
Flags: review?(mratcliffe)
Assignee | ||
Comment 13•10 years ago
|
||
Everything that matches browser_gcli_*.js, you've seen already in https://github.com/joewalker/gcli/pull/16 A number of tests that match browser_cmd_*.js have been converted from callbacks to promises/yield, so their *much* easier to follow now. I've also done a fair bit of splitting tests up to reduce the chances of intermittents.
Attachment #8373281 -
Flags: review?(mratcliffe)
Assignee | ||
Comment 14•10 years ago
|
||
Mike is reviewing the code that splits up GCLI. This part updates the imports and fixes a test which now gives a more precise error message.
Attachment #8373283 -
Flags: review?(pbrosset)
Assignee | ||
Comment 15•10 years ago
|
||
Mike is reviewing the code that splits up GCLI. This part updates the imports and updates the unit tests to use promise/yield.
Attachment #8373288 -
Flags: review?(fayearthur)
Assignee | ||
Comment 16•10 years ago
|
||
Mike is reviewing the code that splits up GCLI. This part: - updates the imports for the new location - updates a test to use promise/yield - removes 'completed: false' from the tests because everything is now async
Attachment #8373294 -
Flags: review?(past)
Assignee | ||
Comment 17•10 years ago
|
||
Rob, this is a collection of import updates for the splitting out of GCLI. I'm not sure it's worth splitting them out further, but I can if you prefer. Also one instance of removing 'completed: false' from a test because everything is now async.
Attachment #8373297 -
Flags: review?(rcampbell)
Assignee | ||
Comment 18•10 years ago
|
||
Green on try FWIW: https://tbpl.mozilla.org/?tree=Try&rev=9960de973951
Comment 19•10 years ago
|
||
Comment on attachment 8373294 [details] [diff] [review] splitgcli-933727-debug.patch Review of attachment 8373294 [details] [diff] [review]: ----------------------------------------------------------------- LGTM.
Attachment #8373294 -
Flags: review?(past) → review+
Comment 20•10 years ago
|
||
o.m.g.
Assignee | ||
Comment 21•10 years ago
|
||
(In reply to Rob Campbell [:rc] (:robcee) from comment #20) > o.m.g. Which I interpret to mean "that's a big patch", which is true. It disolves gcli.jsm into it's parts so it's like adding the whole of GCLI twice. However it is also more work than I would have liked. I don't want that to happen again.
Comment 22•10 years ago
|
||
Comment on attachment 8373297 [details] [diff] [review] splitgcli-933727-other.patch Review of attachment 8373297 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/devtools/scratchpad/CmdScratchpad.jsm @@ +3,5 @@ > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > this.EXPORTED_SYMBOLS = [ ]; > > +const devtools = Components.utils.import("resource://gre/modules/devtools/Loader.jsm", {}).devtools; you could destructure this with: const {devtools} = Cu.import("...loader.jsm", {}); ::: browser/devtools/tilt/CmdTilt.jsm @@ +7,5 @@ > > this.EXPORTED_SYMBOLS = [ ]; > > Components.utils.import('resource://gre/modules/XPCOMUtils.jsm'); > +const devtools = Components.utils.import("resource://gre/modules/devtools/Loader.jsm", {}).devtools; you could destructure this with: const {devtools} = Cu.import("...loader.jsm", {}); I don't know if you like that better or not, but we use it elsewhere.
Attachment #8373297 -
Flags: review?(rcampbell) → review+
Updated•10 years ago
|
Attachment #8373288 -
Flags: review?(fayearthur) → review+
Comment 23•10 years ago
|
||
Comment on attachment 8373283 [details] [diff] [review] splitgcli-933727-inspect.patch Review of attachment 8373283 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/devtools/inspector/CmdInspect.jsm @@ +9,4 @@ > XPCOMUtils.defineLazyModuleGetter(this, "gDevTools", > "resource:///modules/devtools/gDevTools.jsm"); > + > +let devtools = Cu.import("resource://gre/modules/devtools/Loader.jsm", {}).devtools; I have the same comment than Rob here, I'm used to doing: let {devtools} = Cu.import("resources:..."); instead. I've seen this used in a lot of places in the code and I like this as it's a little bit shorter. Also, makes it easier to extend when new exported symbols appear. @@ +9,5 @@ > XPCOMUtils.defineLazyModuleGetter(this, "gDevTools", > "resource:///modules/devtools/gDevTools.jsm"); > + > +let devtools = Cu.import("resource://gre/modules/devtools/Loader.jsm", {}).devtools; > +var gcli = devtools.require('gcli/index'); s/var/let ? I think gcli primarily uses var rather than let or const, so it might not be worth changing them all. ::: browser/devtools/responsivedesign/CmdResize.jsm @@ +10,5 @@ > GetStringFromName("brandShortName"); > > this.EXPORTED_SYMBOLS = [ ]; > > +const devtools = Cu.import("resource://gre/modules/devtools/Loader.jsm", {}).devtools; Same comment as previously.
Attachment #8373283 -
Flags: review?(pbrosset) → review+
Comment 24•10 years ago
|
||
(In reply to Joe Walker [:jwalker] from comment #21) > (In reply to Rob Campbell [:rc] (:robcee) from comment #20) > > o.m.g. > > Which I interpret to mean "that's a big patch", which is true. It disolves > gcli.jsm into it's parts so it's like adding the whole of GCLI twice. > However it is also more work than I would have liked. I don't want that to > happen again. you've interpreted my outburst correctly. I pity the reviewers on this, though hopefully it's mostly a remove+add operation. marking this as assigned. Curious to see where the front-end work for this lives. Bug 911456?
Status: NEW → ASSIGNED
Updated•10 years ago
|
Attachment #8373270 -
Flags: review?(mratcliffe) → review+
Updated•10 years ago
|
Attachment #8373278 -
Flags: review?(mratcliffe) → review+
Updated•10 years ago
|
Attachment #8373281 -
Flags: review?(mratcliffe) → review+
Assignee | ||
Comment 25•10 years ago
|
||
Attachment #8373270 -
Attachment is obsolete: true
Attachment #8373278 -
Attachment is obsolete: true
Attachment #8373281 -
Attachment is obsolete: true
Attachment #8373283 -
Attachment is obsolete: true
Attachment #8373288 -
Attachment is obsolete: true
Attachment #8373294 -
Attachment is obsolete: true
Attachment #8373297 -
Attachment is obsolete: true
Assignee | ||
Comment 26•10 years ago
|
||
Green on try: https://tbpl.mozilla.org/?tree=Try&rev=29849ad8ba40
Assignee | ||
Comment 27•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Fx-Team&rev=02ad58b18823 https://hg.mozilla.org/integration/fx-team/rev/02ad58b18823
Comment 28•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/02ad58b18823
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
Assignee | ||
Updated•10 years ago
|
Depends on: 979030
Updated•10 years ago
|
Updated•10 years ago
|
QA Whiteboard: [qa-]
Updated•6 years ago
|
Product: Firefox → DevTools
Updated•6 years ago
|
Product: DevTools → DevTools Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•