Place the web console in its own iframe

RESOLVED FIXED in Firefox 17

Status

defect
P1
normal
RESOLVED FIXED
8 years ago
a year ago

People

(Reporter: jwalker, Assigned: msucan)

Tracking

Trunk
Firefox 17
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 6 obsolete attachments)

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.
OS: Windows 7 → All
Hardware: x86_64 → All

Comment 1

8 years ago
(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.
Assignee

Comment 2

8 years ago
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.
Depends on: async-webconsole
Summary: Place the web console in it's own iframe → Place the web console in its own iframe
Assignee

Updated

8 years ago
Whiteboard: [webconsole]
Component: Developer Tools → Developer Tools: Console
Bug 673148 isn't going to happen anytime soon, is it?
No longer depends on: async-webconsole
Assignee

Comment 4

8 years ago
(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.
filter on pegasus
Priority: -- → P1
Depends on: async-webconsole

Updated

7 years ago
Blocks: 704110
Assignee

Comment 6

7 years ago
Ready to start work on this now that bug 673148 has landed.
Assignee: nobody → mihai.sucan
Status: NEW → ASSIGNED
Assignee

Comment 7

7 years ago
Posted patch proposed patch (obsolete) — Splinter Review
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!
Attachment #635377 - Flags: review?(rcampbell)
Attachment #635377 - Flags: review?(dcamp)
Attachment #635377 - Flags: review?(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?
Assignee

Comment 9

7 years ago
Dão: thanks!
Attachment #635377 - Attachment is obsolete: true
Attachment #635377 - Flags: review?(rcampbell)
Attachment #635377 - Flags: review?(dcamp)
Attachment #635377 - Flags: review?(dao)
Attachment #635801 - Flags: review?(rcampbell)
Attachment #635801 - Flags: review?(dcamp)
Attachment #635801 - Flags: review?(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.
Attachment #635801 - Flags: review?(dao) → review+
Assignee

Comment 11

7 years ago
Posted patch address dão's latest comment (obsolete) — Splinter Review
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.
Attachment #635801 - Attachment is obsolete: true
Attachment #635801 - Flags: review?(rcampbell)
Attachment #635801 - Flags: review?(dcamp)
Attachment #636267 - Flags: review?(rcampbell)
Attachment #636267 - Flags: review?(dcamp)
(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.
Assignee

Updated

7 years ago
Blocks: 768094
Assignee

Updated

7 years ago
Blocks: 768103
Assignee

Updated

7 years ago
Blocks: 768096
Assignee

Comment 13

7 years ago
Posted patch rebased patch (obsolete) — Splinter Review
Rebased the patch.
Attachment #636267 - Attachment is obsolete: true
Attachment #636267 - Flags: review?(rcampbell)
Attachment #636267 - Flags: review?(dcamp)
Attachment #637459 - Flags: review?(rcampbell)
Attachment #637459 - Flags: review?(dcamp)
Assignee

Updated

7 years ago
Blocks: 676722
Assignee

Comment 14

7 years ago
Posted patch another patch rebase (obsolete) — Splinter Review
Attachment #637459 - Attachment is obsolete: true
Attachment #637459 - Flags: review?(rcampbell)
Attachment #637459 - Flags: review?(dcamp)
Attachment #645313 - Flags: review?(rcampbell)

Updated

7 years ago
Blocks: 777011
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.
Attachment #645313 - Flags: review?(rcampbell) → review+
PS, might want to run through try again to make sure that test failure isn't real.
Assignee

Comment 17

7 years ago
(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.
Assignee

Comment 18

7 years ago
Posted patch address review comments (obsolete) — Splinter Review
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
Attachment #645313 - Attachment is obsolete: true
Assignee

Comment 19

7 years ago
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
Attachment #646149 - Attachment is obsolete: true
Assignee

Comment 20

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

Landed:
https://hg.mozilla.org/integration/fx-team/rev/637461e0e2ad
Attachment #646606 - Attachment description: gcli test fixes → [in-fx-team] gcli test fixes
Assignee

Updated

7 years ago
Whiteboard: [webconsole] → [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/637461e0e2ad
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 17
Depends on: 778597
(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.
Blocks: 782305
No longer blocks: 782305
Depends on: 782305
Depends on: 782306

Updated

a year ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.