688981 - Place the web console in its own iframe

Status: RESOLVED FIXED

(DevTools :: Console, defect, P1)

Description

[Joe Walker [:jwalker]]

There are many things that could make this hard - it might require lots of test re-writes, and cause refactoring with needing more than one document to be in use.

Comment 1

[David Dahl :ddahl]

(In reply to Joe Walker from comment #0)
> There are many things that could make this hard - it might require lots of
> test re-writes, and cause refactoring with needing more than one document to
> be in use.

We should try to demonstrate that this is actually a problem before carrying out this change. It will no doubt be more work than we think.

Comment 2

[Mihai Sucan [:msucan]]

Agreed. There are costs and benefits that need to be weighed before we make such big changes to the code.

This is something we can only do post-e10s work. The e10s Web Console work has progressed far too much for us to make such invasive changes now. See bug 673148.

Please note that when we do the iframe switch, we can also move the Web Console into its own real window - we'll no longer need a xul:panel. The e10s work makes things simpler as well.

Comment 3

[Dão Gottwald [:dao]]

Bug 673148 isn't going to happen anytime soon, is it?

Comment 4

[Mihai Sucan [:msucan]]

(In reply to Dão Gottwald [:dao] from comment #3)
> Bug 673148 isn't going to happen anytime soon, is it?

According to current plans, it is going to happen and we will complete that work. We are much too forward with bug 673148 and there's stuff in the patch that's important for fennec support.

I've been taken away by higher priority work (orion and the source editor) - which is why there was no progress with 673148.

I'll get back to 673148 this month, and in full speed once the holidays are over.

Comment 5

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

filter on pegasus

Comment 6

[Mihai Sucan [:msucan]]

Ready to start work on this now that bug 673148 has landed.

Comment 7

[Mihai Sucan [:msucan]]

Attachment: proposed patch

This is the proposed patch. Changes should follow the overall plan I discussed with Rob. Details:

- moved the entire Web Console UI into a separate script: webconsole.js.
- the old HeadsUpDisplay object is now split into two: the WebConsoleFrame and WebConsole objects.
  - WebConsoleFrame is used for managing the UI in the iframe.
  - WebConsole is used by HUDService to wrap the iframe.
- the Web Console UI is loaded in an iframe, the webconsole.xul file.
- in HUDService.jsm we now have only code that manages instances of Web Consoles and only code that deals with the Firefox UI and the iframe integration. This makes it much easier for others to port the Web Console to Thunderbird, Seamonkey and other apps. The WebConsoleFrame specifically avoids any work with the Firefox UI - I tried to keep the code self-contained.
- disabled preprocessing for HUDService.jsm (no longer needed). Preprocessing is now only needed for the webconsole.xul file.
- the whole Web Console code has now migrated to "use strict" - HUDService.jsm, HUDService-content.js and webconsole.js.
- removed hard-coded XUL from webconsole.js - all the elements live now inside webconsole.xul. This makes editing the Web Console UI much easier.
- moved webconsole.css away from browser.xul into webconsole.xul.
- fixed the code such that all tooltips in the Web Console show. Much nicer.
- almost no UI-related methods in the HUDService object. Moved them as needed into the Web Console objects.
- I expect this patch is going to fix a number of "consolecleanup" bugs. Will triage them once this patch lands.

To follow the history of the patch, how it started and how I worked on it, take a look at:
https://github.com/mihaisucan/mozilla-patch-queue/blob/master/patches-webconsole/bug-688981

Please let me know if you find any problems. Thank you!

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


Dão: I am asking for review from you because I took webconsole.css out from browser.xul and made a minor change in browser.css. Any comments you have are appreciated. Thanks!

Comment 8

[Dão Gottwald [:dao]]

Comment on attachment 635377 [details] [diff] [review]
proposed patch

>--- a/browser/themes/winstripe/browser.css
>+++ b/browser/themes/winstripe/browser.css

>+/* Web Console */
>+
>+.hud-box {
>+  border-bottom: 1px solid #aaa;
>+  text-shadow: none;
>+}

Could you rename "hud-box" to something sensible?

>+.hud-box.animated {
>+  -moz-transition: height 100ms;
>+}

And make "animated" an attribute?

Comment 9

[Mihai Sucan [:msucan]]

Attachment: updated patch to address dão's comment

Dão: thanks!

Comment 10

[Dão Gottwald [:dao]]

Comment on attachment 635801 [details] [diff] [review]
updated patch to address dão's comment

>--- a/browser/themes/winstripe/browser.css
>+++ b/browser/themes/winstripe/browser.css

>+.web-console-frame {
>+  border-bottom: 1px solid #aaa;
>+  text-shadow: none;
>+}

text-shadow: none; shouldn't be needed anymore.

>+.web-console-panel {
>+  -moz-appearance: none;
>+  background-color: white;
>+}
>+
>+.web-console-panel > .web-console-frame {
>+  height: 100%;
>+  width: 100%;
>+  background-color: white;
>+}

background-color: white; looks like it can go away, width/height should probably be in the markup or in a content stylesheet.

Comment 11

[Mihai Sucan [:msucan]]

Attachment: address dão's latest comment

Updated per Dão's latest comment.

Dão: thanks for your comments and for the r+! Do note that .web-console-panel { background-color: white } needs to stay. When the iframe is in the xul:panel we add a xul:hbox and a xul:resizer. If I remove background-color: white, I get the bottom of the panel fully transparent.

Comment 12

[Dão Gottwald [:dao]]

(In reply to Mihai Sucan [:msucan] from comment #11)
> If I remove
> background-color: white, I get the bottom of the panel fully transparent.

That's because you set -moz-appearance: none; for reasons I don't see.

Comment 13

[Mihai Sucan [:msucan]]

Attachment: rebased patch

Rebased the patch.

Comment 14

[Mihai Sucan [:msucan]]

Attachment: another patch rebase

Comment 15

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

Comment on attachment 645313 [details] [diff] [review]
another patch rebase

One test failure:

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/webconsole/test/browser_webconsole_position_ui.js | Timed out while waiting for: web console position changed to 'window'

Paul noticed some funny extra padding around the jsterm input line.

+++ b/browser/devtools/webconsole/HUDService.jsm

here we go...

in HUDService.jsm:

I'm tempted to rename this file since we're already doing a huge re-org. Would be nice to remove the HUD name once and for all. Can we rename this file to WebConsoleService.jsm? Only worry is that it'll cause a cascade renaming inside the file itself. e.g., hudRef, HUD_SERVICE, etc. and all the tests would break. Again.

So, maybe it's not worth it. Also, "hud" is easier to type that "WebConsole".

+// The Web Console UI is loaded in an iframe. This constant points to the file
+// to load in the iframe.
+const UI_IFRAME_URL = "chrome://browser/content/devtools/webconsole.xul";

might want to say, "points to the document to load in the iframe". but meh.

-    let hud = new HeadsUpDisplay(aTab);
+    let hud = new WebConsole(aTab);

yes!

in onPopupShown:

-      this.HUDBox.style.height = "auto";
-      this.HUDBox.setAttribute("flex", "1");
+      this.iframe.style.height = "auto";
+      this.iframe.setAttribute("flex", "1");

then down below:

-    space.setAttribute("flex", "1");
+    space.flex = 1;

why no setAttribute?

in _afterPositionConsole:
+      this.iframe.removeEventListener("load", onLoad, true);
+      this.iframeWindow = this.iframe.contentWindow.wrappedJSObject;

do we need the .wrappedJSObject there?

+var HeadsUpDisplayUICommands = {

    vs

let WebConsoleObserver = {

any reason to prefer var over let here? No preference either way, but should be consistent.

in webconsole.js:

+  _initUI: function WCF__initUI()
+  {
+    let doc = this.document;
+
+    this.filterBox = doc.getElementsByClassName("hud-filter-box")[0];
+    this.outputNode = doc.getElementsByClassName("hud-output-node")[0];
+

any reason not to use querySelector here? You won't need to dereference with [0].

+  _initPositionUI: function WCF__initPositionUI()
+  {
+    let doc = this.document;
+
+    let itemAbove = doc.querySelector("*[consolePosition='above']");
+    itemAbove.addEventListener("command", this._onPositionConsoleCommand, false);

can you be a little more specific than *[consolePosition=...]? That's going to be a rather slow search.

+  _initFilterButtons: function WCF__initFilterButtons()
+  {
+    let categories = this.document
+                     .querySelectorAll(".webconsole-filter-button[category]");
+    Array.prototype.forEach.call(categories, function(aButton) {
+      aButton.addEventListener("click", this._toggleFilter, false);

instead of Array.prototype.forEach.call you can just call Array.forEach...

same for the inner forEach.

+  _toggleFilter: function WCF__toggleFilter(aEvent)
...
+    switch (tagName) {
+      case "toolbarbutton": {

this is OK, but I generally only use switch statements for more than 2 cases. Can keep this though.

only other thing I can think of is that you might want to add padding: 0 for pinstripe around the input container. One of those is getting default padding on OS X.

I expect most of the above suggestions are for code that was copied and pasted into webconsole.js. Would be nice to clean them up but not strictly necessary. Overall, I really like the code reorganization.

Comment 16

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

PS, might want to run through try again to make sure that test failure isn't real.

Comment 17

[Mihai Sucan [:msucan]]

(In reply to Rob Campbell [:rc] (:robcee) from comment #15)
> Comment on attachment 645313 [details] [diff] [review]
> another patch rebase
> 
> One test failure:
> 
> TEST-UNEXPECTED-FAIL |
> chrome://mochitests/content/browser/browser/devtools/webconsole/test/
> browser_webconsole_position_ui.js | Timed out while waiting for: web console
> position changed to 'window'

Uh oh. Could be. I just retested on Linux - no failures. Will test on Mac OS and will push again to the try servers. Maybe something changed...

> Paul noticed some funny extra padding around the jsterm input line.

Will check.


> +++ b/browser/devtools/webconsole/HUDService.jsm
> 
> here we go...
> 
> in HUDService.jsm:
> 
> I'm tempted to rename this file since we're already doing a huge re-org.
> Would be nice to remove the HUD name once and for all. Can we rename this
> file to WebConsoleService.jsm? Only worry is that it'll cause a cascade
> renaming inside the file itself. e.g., hudRef, HUD_SERVICE, etc. and all the
> tests would break. Again.
> 
> So, maybe it's not worth it. Also, "hud" is easier to type that "WebConsole".

As discussed in yesterday's meeting, we probably don't need to do this now. We could rename the file when we move it to toolkit.


> +// The Web Console UI is loaded in an iframe. This constant points to the
> file
> +// to load in the iframe.
> +const UI_IFRAME_URL = "chrome://browser/content/devtools/webconsole.xul";
> 
> might want to say, "points to the document to load in the iframe". but meh.

Sure. Engrish ftw! :)

> -    let hud = new HeadsUpDisplay(aTab);
> +    let hud = new WebConsole(aTab);
> 
> yes!

I did this because the new object is no longer the same as the old HeadsUpDisplay. I had a reason for the rename, finally! :)

> in onPopupShown:
> 
> -      this.HUDBox.style.height = "auto";
> -      this.HUDBox.setAttribute("flex", "1");
> +      this.iframe.style.height = "auto";
> +      this.iframe.setAttribute("flex", "1");
> 
> then down below:
> 
> -    space.setAttribute("flex", "1");
> +    space.flex = 1;
> 
> why no setAttribute?

Argh. I want elem.flex = 1 in all cases where possible. IIRC, I had XUL experts who pointed out I don't need to use setAttribute.

> in _afterPositionConsole:
> +      this.iframe.removeEventListener("load", onLoad, true);
> +      this.iframeWindow = this.iframe.contentWindow.wrappedJSObject;
> 
> do we need the .wrappedJSObject there?

Yes. I can't otherwise access some things in the iframe window.

> +var HeadsUpDisplayUICommands = {
> 
>     vs
> 
> let WebConsoleObserver = {
> 
> any reason to prefer var over let here? No preference either way, but should
> be consistent.

No preference, but good point. I shall make this consistent. Good catch!


> in webconsole.js:
> 
> +  _initUI: function WCF__initUI()
> +  {
> +    let doc = this.document;
> +
> +    this.filterBox = doc.getElementsByClassName("hud-filter-box")[0];
> +    this.outputNode = doc.getElementsByClassName("hud-output-node")[0];
> +
> 
> any reason not to use querySelector here? You won't need to dereference with
> [0].

No specific reason. Will fix.


> +  _initPositionUI: function WCF__initPositionUI()
> +  {
> +    let doc = this.document;
> +
> +    let itemAbove = doc.querySelector("*[consolePosition='above']");
> +    itemAbove.addEventListener("command", this._onPositionConsoleCommand,
> false);
> 
> can you be a little more specific than *[consolePosition=...]? That's going
> to be a rather slow search.

Will do.

> +  _initFilterButtons: function WCF__initFilterButtons()
> +  {
> +    let categories = this.document
> +                    
> .querySelectorAll(".webconsole-filter-button[category]");
> +    Array.prototype.forEach.call(categories, function(aButton) {
> +      aButton.addEventListener("click", this._toggleFilter, false);
> 
> instead of Array.prototype.forEach.call you can just call Array.forEach...
> 
> same for the inner forEach.

Good points.


> +  _toggleFilter: function WCF__toggleFilter(aEvent)
> ...
> +    switch (tagName) {
> +      case "toolbarbutton": {
> 
> this is OK, but I generally only use switch statements for more than 2
> cases. Can keep this though.

I wasn't sure which way is "uglier".

> only other thing I can think of is that you might want to add padding: 0 for
> pinstripe around the input container. One of those is getting default
> padding on OS X.

Will look into fixing this.

> I expect most of the above suggestions are for code that was copied and
> pasted into webconsole.js. Would be nice to clean them up but not strictly
> necessary. Overall, I really like the code reorganization.

Great! Thanks for your review!

Will update the patch ASAP.

Comment 18

[Mihai Sucan [:msucan]]

Attachment: address review comments

Addressed Rob's comments. No test failures on my Mac OS and Linux systems. Perhaps the failure you had was with the toolbar theming changes.

Pushed to try:
https://tbpl.mozilla.org/?tree=Try&rev=2f411d80787e

Comment 19

[Mihai Sucan [:msucan]]

Attachment: [in-fx-team] gcli test fixes

Try push was all orange due to some gcli tests that use the web console. I backported changes from the patch for bug 768096 into this patch.

Interdiff:
https://github.com/mihaisucan/mozilla-patch-queue/commit/160b2e5b376a58e2569aaa9ec923c445761f6044

Added a callback argument to jsterm.execute() and a message flush callback mechanism.

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

Comment 20

[Mihai Sucan [:msucan]]

Comment on attachment 646606 [details] [diff] [review]
[in-fx-team] gcli test fixes

Landed:
https://hg.mozilla.org/integration/fx-team/rev/637461e0e2ad

Comment 21

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/637461e0e2ad

Comment 22

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

(In reply to Mihai Sucan [:msucan] from comment #18)
> Created attachment 646149 [details] [diff] [review]
> address review comments
> 
> Addressed Rob's comments. No test failures on my Mac OS and Linux systems.
> Perhaps the failure you had was with the toolbar theming changes.

Nope, wasn't applied at the time. Could've been a quirk of my particular system.