Closed Bug 1435084 Opened 3 years ago Closed 3 years ago

Create a pref to load the new console frontend in the Browser Console

Categories

(DevTools :: Console, enhancement, P1)

enhancement

Tracking

(firefox60 fixed)

RESOLVED FIXED
Firefox 60
Tracking Status
firefox60 --- fixed

People

(Reporter: bgrins, Assigned: bgrins)

References

Details

(Whiteboard: [newconsole-mvp] )

Attachments

(1 file)

This'll allow for incremental progress towards Bug 1362023 without requiring all of the test migration / feature work to be done.
Blocks: 1435090
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Priority: -- → P1
Comment on attachment 8947650 [details]
Bug 1435084 - Create a pref to enable the new console UI in the browser console;

https://reviewboard.mozilla.org/r/217372/#review223842

Looks good to me. I have a comment to simplify the code a bit, but this is not really in the scope of this bug.

::: devtools/client/webconsole/hudservice.js:232
(Diff revision 1)
> -    connect().then(getTarget).then(openWindow).then((aWindow) => {
> -      return this.openBrowserConsole(target, aWindow, aWindow)
> +    connect().then(getTarget).then(openWindow).then(({iframeWindow, chromeWindow}) => {
> +      return this.openBrowserConsole(target, iframeWindow, chromeWindow)
>          .then((aBrowserConsole) => {
>            this._browserConsoleDefer.resolve(aBrowserConsole);
>            this._browserConsoleDefer = null;
>          });
>      }, console.error.bind(console));

not really in the scope of this bug, but could this somehow be turned into async/await code ? It would be much more readable.

I also wonder if we could get read of `this._browserConsoleDefer` at the same time ? Or at least, not use defer maybe ?
The only use case seems to be here https://searchfox.org/mozilla-central/rev/f80722d4f3bfb722c5ec53880c4a7efb71285676/devtools/client/webconsole/hudservice.js#181-183 to prevent multiple browser console opening.

::: devtools/client/webconsole/new-console-output/new-console-output-wrapper.js:142
(Diff revision 1)
>            { actor, clipboardText, variableText, message,
>              serviceContainer, openSidebar, rootActorId });
>  
>          // Emit the "menu-open" event for testing.
>          menu.once("open", () => this.emit("menu-open"));
> -        menu.popup(screenX, screenY, this.toolbox);
> +        menu.popup(screenX, screenY, { doc: this.owner.chromeWindow.document });

were there a bug here ?
Attachment #8947650 - Flags: review?(nchevobbe) → review+
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #2)
> ::: devtools/client/webconsole/hudservice.js:232
> (Diff revision 1)
> > -    connect().then(getTarget).then(openWindow).then((aWindow) => {
> > -      return this.openBrowserConsole(target, aWindow, aWindow)
> > +    connect().then(getTarget).then(openWindow).then(({iframeWindow, chromeWindow}) => {
> > +      return this.openBrowserConsole(target, iframeWindow, chromeWindow)
> >          .then((aBrowserConsole) => {
> >            this._browserConsoleDefer.resolve(aBrowserConsole);
> >            this._browserConsoleDefer = null;
> >          });
> >      }, console.error.bind(console));
> 
> not really in the scope of this bug, but could this somehow be turned into
> async/await code ? It would be much more readable.
> 
> I also wonder if we could get read of `this._browserConsoleDefer` at the
> same time ? Or at least, not use defer maybe ?
> The only use case seems to be here
> https://searchfox.org/mozilla-central/rev/
> f80722d4f3bfb722c5ec53880c4a7efb71285676/devtools/client/webconsole/
> hudservice.js#181-183 to prevent multiple browser console opening.
> 

Good idea, I'll file a followup bug to clean this up.

> :::
> devtools/client/webconsole/new-console-output/new-console-output-wrapper.js:
> 142
> (Diff revision 1)
> >            { actor, clipboardText, variableText, message,
> >              serviceContainer, openSidebar, rootActorId });
> >  
> >          // Emit the "menu-open" event for testing.
> >          menu.once("open", () => this.emit("menu-open"));
> > -        menu.popup(screenX, screenY, this.toolbox);
> > +        menu.popup(screenX, screenY, { doc: this.owner.chromeWindow.document });
> 
> were there a bug here ?

Yes, in the Browser Console there is no toolbox so the context menu wouldn't open. The Menu API just expects an object with 'doc' as a key, so I'm changing it to reference the doc of the chromeWindow that gets passed in through hudservice.  This is either the new browserconsole.xul window or the toolbox.xul window depending on if it's the BC or WC.
Blocks: 1436076
Pushed by bgrinstead@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d23fda609203
Create a pref to enable the new console UI in the browser console;r=nchevobbe
https://hg.mozilla.org/mozilla-central/rev/d23fda609203
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.