Closed Bug 933727 Opened 7 years ago Closed 7 years ago

Land the back-end changes for JSTerm

Categories

(DevTools Graveyard :: Graphic Commandline and Toolbar, defect)

defect
Not set
normal

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)
Summary: Land the backed changes for JSTerm → Land the back-end changes for JSTerm
Attached patch v1 (obsolete) — Splinter Review
This patch broken out: https://github.com/joewalker/gcli/pull/16
Attachment #825892 - Flags: review?(mratcliffe)
Attached patch gcli.patch (obsolete) — Splinter Review
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 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+
Attached patch v2 (obsolete) — Splinter Review
https://github.com/joewalker/gcli/pull/16
Attachment #826051 - Attachment is obsolete: true
Attachment #831562 - Flags: review?(mratcliffe)
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+
Attached patch v3 (obsolete) — Splinter Review
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 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+
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.
Attached patch splitgcli-933727-gcli.patch (obsolete) — Splinter Review
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)
Attached patch splitgcli-933727-devtb.patch (obsolete) — Splinter Review
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)
Attached patch splitgcli-933727-tests.patch (obsolete) — Splinter Review
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)
Attached patch splitgcli-933727-inspect.patch (obsolete) — Splinter Review
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)
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)
Attached patch splitgcli-933727-debug.patch (obsolete) — Splinter Review
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)
Attached patch splitgcli-933727-other.patch (obsolete) — Splinter Review
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)
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+
(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 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+
Attachment #8373288 - Flags: review?(fayearthur) → review+
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+
(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
Attachment #8373270 - Flags: review?(mratcliffe) → review+
Attachment #8373278 - Flags: review?(mratcliffe) → review+
Attachment #8373281 - Flags: review?(mratcliffe) → review+
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
https://hg.mozilla.org/mozilla-central/rev/02ad58b18823
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
No longer depends on: 936451, 949205
No longer blocks: 985015
Depends on: 985015
Depends on: 993985
QA Whiteboard: [qa-]
Depends on: 1038251
Depends on: 1244584
Product: Firefox → DevTools
Product: DevTools → DevTools Graveyard
You need to log in before you can comment on or make changes to this bug.