877262 - Start using the new jetpack loader in the web console

Status: RESOLVED FIXED

(DevTools :: Console, defect)

Comment 1

[Mihai Sucan [:msucan]]

Attachment: proposed patch

Lots of changes for making the webconsole to use the jetpack loader.

Concerns / notes:

* I renamed HUDService.jsm to hudservice.js and cleaned it up a bit. Later on I thought, maybe we should rename to to manager.js. Any preference? It would fit the scratchpad manager, style editor, etc.

* Not too happy about having to use a path like devtools/toolkit/webconsole for require(). We should merge browser/devtools and toolkit/devtools.

* Needed to add a function to the devtools global: btoa(). This is used by some network-related jsm and this function is available in jsm's, but not in require()d scripts.

Try push: https://tbpl.mozilla.org/?tree=Try&rev=3e61a98e2fec

Patch needs more testing, but it's ready for review. All devtools tests passed on my machine. Thanks!

Comment 2

[Mihai Sucan [:msucan]]

Attachment: updated patch - fixes for the 'tools reload' cmd

Played with the 'tools reload' command and it seems it didn't quite work. On Saturday I forgot to update SrcDirProvider to include the new devtools/toolkit path. I also removed the preprocessing for webconsole.xul so it can be reloaded.

Patch changes are here:
https://github.com/mihaisucan/mozilla-patch-queue/commit/d8324ebf21e7169ef94079c077e643b63235e1e2

Comment 3

[Mihai Sucan [:msucan]]

My patch queue: 887273, 812618, 877262.

Comment 4

[Dave Camp (:dcamp)]

Comment on attachment 769751 [details] [diff] [review]
updated patch - fixes for the 'tools reload' cmd

Review of attachment 769751 [details] [diff] [review]:
-----------------------------------------------------------------

Clearing review request - the JSM vs. CommonJS lifetime issue needs addressing, otherwise it looks good.

::: browser/devtools/commandline/test/browser_cmd_calllog.js
@@ +52,5 @@
>      Services.obs.removeObserver(onWebConsoleOpen, "web-console-created");
>  
>      subject.QueryInterface(Ci.nsISupportsString);
>      let hud = HUDService.getHudReferenceById(subject.data);
> +    ok(hud, "console open");

These small unrelated changes/renames in an otherwise mostly mechanical patch makes reviewing a bit tougher, for next time.

::: browser/devtools/shared/widgets/SideMenuWidget.jsm
@@ +19,5 @@
> +
> +XPCOMUtils.defineLazyGetter(this, "NetworkHelper", function() {
> +  return devtools.require("devtools/toolkit/webconsole/network-helper");
> +});
> +

JSMs live for the entire duration of the application, but modules loaded by devtools.require will only last until the next 'tools reload'.  XPCOMUtils.defineLazyGetter will save the original NetworkHelper for as long as the JSM, even across 'tools reload'.

::: browser/devtools/shared/widgets/VariablesView.jsm
@@ +31,5 @@
> +});
> +
> +XPCOMUtils.defineLazyGetter(this, "NetworkHelper", function() {
> +  return devtools.require("devtools/toolkit/webconsole/network-helper");
> +});

Same here.

::: browser/devtools/webconsole/hudservice.js
@@ +137,1 @@
>     */

Did you mean all this hudservice change to be included in this patch?

I don't feel too comfortable reviewing deep hudservice changes, can you add a reviewer for console changes please?

::: browser/devtools/webconsole/test/browser_console_variables_view_while_debugging_and_inspecting.js
@@ +37,5 @@
>    gThread = gDebuggerController.activeThread;
>    gStackframes = gDebuggerController.StackFrames;
>  
> +  info("openInspector");
> +  executeSoon(() => openInspector(inspectorOpened));

Why did you add this?

::: toolkit/devtools/Loader.jsm
@@ +49,5 @@
>          "": "resource://gre/modules/commonjs/",
>          "main": "resource:///modules/devtools/main.js",
>          "devtools": "resource:///modules/devtools",
>          "devtools/server": "resource://gre/modules/devtools/server",
> +        "devtools/toolkit": "resource://gre/modules/devtools/toolkit",

Could you please just add devtools/toolkit/webconsole specifically?

devtools/toolkit shouldn't stick around long, and it'll be nice to limit its scope as much as possible.

Comment 5

[Mihai Sucan [:msucan]]

(In reply to Dave Camp (:dcamp) from comment #4)
> Comment on attachment 769751 [details] [diff] [review]
> updated patch - fixes for the 'tools reload' cmd
> 
> Review of attachment 769751 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Clearing review request - the JSM vs. CommonJS lifetime issue needs
> addressing, otherwise it looks good.

Thanks for the review!


> ::: browser/devtools/commandline/test/browser_cmd_calllog.js
> @@ +52,5 @@
> >      Services.obs.removeObserver(onWebConsoleOpen, "web-console-created");
> >  
> >      subject.QueryInterface(Ci.nsISupportsString);
> >      let hud = HUDService.getHudReferenceById(subject.data);
> > +    ok(hud, "console open");
> 
> These small unrelated changes/renames in an otherwise mostly mechanical
> patch makes reviewing a bit tougher, for next time.

I have moved all of the "HUDConsoleUI merge into HUDService" changes into a separate patch which I will ask Rob to review (part 1). The switch to the jetpack loader with only minimal changes will be part 2 patch.

> ::: browser/devtools/shared/widgets/SideMenuWidget.jsm
> @@ +19,5 @@
> > +
> > +XPCOMUtils.defineLazyGetter(this, "NetworkHelper", function() {
> > +  return devtools.require("devtools/toolkit/webconsole/network-helper");
> > +});
> > +
> 
> JSMs live for the entire duration of the application, but modules loaded by
> devtools.require will only last until the next 'tools reload'. 
> XPCOMUtils.defineLazyGetter will save the original NetworkHelper for as long
> as the JSM, even across 'tools reload'.

Yep, when I updated the patch I only touched browser.js and I forgot about the other files... Fixed now.

> ::: browser/devtools/webconsole/hudservice.js
> @@ +137,1 @@
> >     */
> 
> Did you mean all this hudservice change to be included in this patch?
> 
> I don't feel too comfortable reviewing deep hudservice changes, can you add
> a reviewer for console changes please?

See above about patch split. Will ask Rob for HUDService changes review in a separate patch.


> :::
> browser/devtools/webconsole/test/
> browser_console_variables_view_while_debugging_and_inspecting.js
> @@ +37,5 @@
> >    gThread = gDebuggerController.activeThread;
> >    gStackframes = gDebuggerController.StackFrames;
> >  
> > +  info("openInspector");
> > +  executeSoon(() => openInspector(inspectorOpened));
> 
> Why did you add this?

This is no longer needed. It's remnant from attempts to debug test failures. I have removed all changes in this test now.


> ::: toolkit/devtools/Loader.jsm
> @@ +49,5 @@
> >          "": "resource://gre/modules/commonjs/",
> >          "main": "resource:///modules/devtools/main.js",
> >          "devtools": "resource:///modules/devtools",
> >          "devtools/server": "resource://gre/modules/devtools/server",
> > +        "devtools/toolkit": "resource://gre/modules/devtools/toolkit",
> 
> Could you please just add devtools/toolkit/webconsole specifically?
> 
> devtools/toolkit shouldn't stick around long, and it'll be nice to limit its
> scope as much as possible.

Make sense. Fixed now.

Patches are currently here:
https://github.com/mihaisucan/mozilla-patch-queue/tree/master/patches-webconsole

I still need to fix a known test failure on Mac, then I will submit the patches for review - next week. Thanks!

Comment 6

[Mihai Sucan [:msucan]]

Attachment: part 1 - merge HUDConsoleUI into HUDService and clean it up

As suggested by Dave, I did split the patch into two parts. This patch merges HUDConsoleUI into HUDService. Up until this point we had HUDConsoleUI which provided API to open the web console and the browser console. We also had the separate HUDService object which provided API used by HUDConsoleUI, tests and the web console code. It does not make much sense to have this split, and to expose these two new objects in the new jetpack loader-based approach.

As always, feedback is welcome. Thanks!

Comment 7

[Mihai Sucan [:msucan]]

Attachment: part 2 - switch to the jetpack loader

This is the patch with minimal changes for switching to the jetpack loader, also updated to address your review comments.

Try push: https://tbpl.mozilla.org/?tree=Try&rev=d9024f1bcd70

Questions and concerns:

1. How do we get the styles to reload as well? This is a major "pain point" for me with the work I am doing for the web console output. I still need to run make and restart the browser for CSS changes. Can we just add the style paths or...?

2. I remember you had some issues with the loader patches having bounced back - increased heap count or something. Was that a one-time-only issue or are we going to see that again with every tool we switch? How can I check if this patch is causing any regressions?

Thank you!

Comment 8

[Rob Campbell [:rc] (:robcee)]

-__-

Comment 9

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 775690 [details] [diff] [review]
part 1 - merge HUDConsoleUI into HUDService and clean it up

Review of attachment 775690 [details] [diff] [review]:
-----------------------------------------------------------------

looks good. A couple of questions below. I would've preferred the OS X accesskey stuff in a separate bug, but I'm not going to make you change that now.

::: browser/devtools/webconsole/HUDService.jsm
@@ +52,5 @@
>  //// The HUD service
>  
>  function HUD_SERVICE()
>  {
> +  this.consoles = new Map();

does this finally get rid of hudReferences?

@@ +173,5 @@
> +    let target = devtools.TargetFactory.forTab(window.gBrowser.selectedTab);
> +    let toolbox = gDevTools.getToolbox(target);
> +
> +    return toolbox && toolbox.currentToolId == "webconsole" ?
> +        toolbox.destroy() :

I'm only looking at this because it got moved.

Are we supposed to be toggling the toolbox via the webconsole? This feels like something the Framework should handle.

I guess were we going to make the webconsole (or jsterm) the persistent thing that we wanted, we could do that there. bug 862558.

::: browser/devtools/webconsole/test/browser_console_keyboard_accessibility.js
@@ +58,4 @@
>        EventUtils.synthesizeKey("t", { ctrlKey: true });
> +      ok(!hud.ui.getFilterState("network"), "accesskey for Network works");
> +      EventUtils.synthesizeKey("t", { ctrlKey: true });
> +      ok(hud.ui.getFilterState("network"), "accesskey for Network works (again)");

odd. There were no tests in here before?

::: browser/devtools/webconsole/test/browser_webconsole_bug_601177_log_levels.js
@@ +75,1 @@
>  }

nice update to this test.

::: browser/devtools/webconsole/webconsole.js
@@ +632,5 @@
> +
> +    if (Services.appinfo.OS == "Darwin") {
> +      let net = this.document.querySelector("toolbarbutton[category=net]");
> +      let accesskey = net.getAttribute("accesskeyMacOSX");
> +      net.setAttribute("accesskey", accesskey);

why are we doing this here? and why just for the Net filter?

::: browser/devtools/webconsole/webconsole.xul
@@ +22,5 @@
>          width="900" height="350"
>          persist="screenX screenY width height sizemode">
>    <script type="text/javascript" src="chrome://global/content/globalOverlay.js"/>
>    <script type="text/javascript" src="webconsole.js"/>
> +  <script type="text/javascript"><![CDATA[

why is this in a CDATA blob? needed for XUL?

::: toolkit/devtools/webconsole/WebConsoleUtils.jsm
@@ +262,5 @@
>    {
>      let desc = null;
>      while (aObject) {
>        try {
> +        if ((desc = Object.getOwnPropertyDescriptor(aObject, aProp))) {

um. why the extra parens?

@@ +1259,5 @@
>  
>      let results = doc.evaluate(aXPath, aContext, null,
>                                 Ci.nsIDOMXPathResult.ANY_TYPE, null);
>      let node;
> +    while ((node = results.iterateNext())) {

same question here.

Comment 10

[Mihai Sucan [:msucan]]

(In reply to Rob Campbell [:rc] (:robcee) from comment #9)
> Comment on attachment 775690 [details] [diff] [review]
> part 1 - merge HUDConsoleUI into HUDService and clean it up
> 
> Review of attachment 775690 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> looks good. A couple of questions below. I would've preferred the OS X
> accesskey stuff in a separate bug, but I'm not going to make you change that
> now.

Thanks for the review.

The OS X accesskey change was needed to drop the preprocessing of the webconsole.xul file. It is only a code change - not a change of existing shortcuts or functionality for users.


> ::: browser/devtools/webconsole/HUDService.jsm
> @@ +52,5 @@
> >  //// The HUD service
> >  
> >  function HUD_SERVICE()
> >  {
> > +  this.consoles = new Map();
> 
> does this finally get rid of hudReferences?

Yes.


> @@ +173,5 @@
> > +    let target = devtools.TargetFactory.forTab(window.gBrowser.selectedTab);
> > +    let toolbox = gDevTools.getToolbox(target);
> > +
> > +    return toolbox && toolbox.currentToolId == "webconsole" ?
> > +        toolbox.destroy() :
> 
> I'm only looking at this because it got moved.
> 
> Are we supposed to be toggling the toolbox via the webconsole? This feels
> like something the Framework should handle.

Yes and no. Yes, because this is not directly the job of the HUDService - that to manage anything related to the toolbox. No, because we need a simple method to open the web console for the given tab.

browser.js only calls HUDService.toggleWebConsole() or toggleBrowserConsole(). If we move this code out of the HUDService, we would have to put it inside the gDevTools object, which may or may not be better. A second option would be to put this kind of code in every place where we want to open the console - eg. in tests head.js and in browser.js, etc. That would be too much.


> I guess were we going to make the webconsole (or jsterm) the persistent
> thing that we wanted, we could do that there. bug 862558.

Yep, we can make changes there or in some other bug.


> browser/devtools/webconsole/test/browser_console_keyboard_accessibility.js
> @@ +58,4 @@
> >        EventUtils.synthesizeKey("t", { ctrlKey: true });
> > +      ok(!hud.ui.getFilterState("network"), "accesskey for Network works");
> > +      EventUtils.synthesizeKey("t", { ctrlKey: true });
> > +      ok(hud.ui.getFilterState("network"), "accesskey for Network works (again)");
> 
> odd. There were no tests in here before?

On Mac Ctrl-t (for Network) performs the button action (it seems this is how accesskeys work) without changing focus. On Linux and Windows Alt-t only gives focus to the toolbar button. This is why the test change.

I found it odd that the focus test works on Mac OS X when I use the preprocessed xul, but not with this patch. I did manual testing on Mac and I found that Ctrl-t does work in the same way, with and without the patch (the button action is performed, without changing focus). It may have been just a timing thing that made it work.

> ::: browser/devtools/webconsole/webconsole.js
> @@ +632,5 @@
> > +
> > +    if (Services.appinfo.OS == "Darwin") {
> > +      let net = this.document.querySelector("toolbarbutton[category=net]");
> > +      let accesskey = net.getAttribute("accesskeyMacOSX");
> > +      net.setAttribute("accesskey", accesskey);
> 
> why are we doing this here? and why just for the Net filter?

Sorry, this is a weird.

Only the net filter button had a conflict with an existing keyboard shortcut (ctrl-n) on mac. We can add a variant for mac to all buttons, or we can remove accesskeys on Macs altogether (it has been suggested, previously). I would be happy to remove accesskeys for Mac users.


> ::: browser/devtools/webconsole/webconsole.xul
> @@ +22,5 @@
> >          width="900" height="350"
> >          persist="screenX screenY width height sizemode">
> >    <script type="text/javascript" src="chrome://global/content/globalOverlay.js"/>
> >    <script type="text/javascript" src="webconsole.js"/>
> > +  <script type="text/javascript"><![CDATA[
> 
> why is this in a CDATA blob? needed for XUL?

If we add < or > or other "special" characters we may want to use a CDATA for the script. Is this not needed for XUL?


> ::: toolkit/devtools/webconsole/WebConsoleUtils.jsm
> @@ +262,5 @@
> >    {
> >      let desc = null;
> >      while (aObject) {
> >        try {
> > +        if ((desc = Object.getOwnPropertyDescriptor(aObject, aProp))) {
> 
> um. why the extra parens?
> 
> @@ +1259,5 @@
> >  
> >      let results = doc.evaluate(aXPath, aContext, null,
> >                                 Ci.nsIDOMXPathResult.ANY_TYPE, null);
> >      let node;
> > +    while ((node = results.iterateNext())) {
> 
> same question here.

These two changes are lowering the number of strict warnings.

Please let me know if further changes are need. Thanks!

Comment 11

[Dave Camp (:dcamp)]

Comment on attachment 775692 [details] [diff] [review]
part 2 - switch to the jetpack loader

Review of attachment 775692 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good other than:

::: browser/devtools/shared/DeveloperToolbar.jsm
@@ +51,5 @@
>  let Telemetry = devtools.require("devtools/shared/telemetry");
>  
>  const converters = require("gcli/converters");
>  
> +Object.defineProperty(this, "ConsoleServiceListener", {

ConsoleServiceListener is used in the toolbar, whose lifetime might outlive the tools.  You probably should listen for "devtools-unloaded" on the observer service, see gDevTools for an example.

Comment 12

[Mihai Sucan [:msucan]]

Attachment: part 2 - switch to the jetpack loader v0.2

Thanks for pointing that out. I have updated the patch accordingly and I opened bug 877262, as discussed on IRC.

Comment 13

[Mihai Sucan [:msucan]]

Attachment: part 1 - merge HUDConsoleUI into HUDService and clean it up v0.2

Rebased the patch and addressed review comments.

Comment 14

[Mihai Sucan [:msucan]]

Attachment: part 2 - switch to the jetpack loader v0.3

Patch rebase.

Comment 15

[Mihai Sucan [:msucan]]

Landed:

https://hg.mozilla.org/integration/fx-team/rev/3f8a99f7e0ea
https://hg.mozilla.org/integration/fx-team/rev/497504e7f952

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/3f8a99f7e0ea
https://hg.mozilla.org/mozilla-central/rev/497504e7f952

Comment 17

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 785371 [details] [diff] [review]
part 2 - switch to the jetpack loader v0.3

Review of attachment 785371 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/devtools/webconsole/network-helper.js
@@ +254,5 @@
>        let aChannel = aRequest.QueryInterface(Ci.nsIChannel);
>        let contentCharset = aChannel.contentCharset || aCharset;
>  
>        // Read the content of the stream using contentCharset as encoding.
> +      aCallback(this.readAndConvertFromStream(aInputStream, contentCharset));

`this` is not what you think it is here, which causes errors like

************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "'[JavaScript Error: "this.readAndConvertFromStream is not a function" {file: "resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/toolkit/webconsole/network-helper.js" line: 258}]' when calling method: [nsIRequestObserver::onStopRequest]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0"  data: yes]
************************************************************

Comment 18

[Mihai Sucan [:msucan]]

(In reply to :Ms2ger from comment #17)
> Comment on attachment 785371 [details] [diff] [review]
> part 2 - switch to the jetpack loader v0.3
> 
> Review of attachment 785371 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/devtools/webconsole/network-helper.js
> @@ +254,5 @@
> >        let aChannel = aRequest.QueryInterface(Ci.nsIChannel);
> >        let contentCharset = aChannel.contentCharset || aCharset;
> >  
> >        // Read the content of the stream using contentCharset as encoding.
> > +      aCallback(this.readAndConvertFromStream(aInputStream, contentCharset));
> 
> `this` is not what you think it is here, which causes errors like
> ...

Thanks for reporting this issue. I've opened bug 956757 and submitted a patch.