734664 - Devtools toolbox should display the actual target url when detached

Status: RESOLVED FIXED

(DevTools :: General, defect, P3)

Description

[sys.sgx]

User Agent: Mozilla/5.0 (Windows NT 6.0; rv:11.0) Gecko/20100101 Firefox/11.0
Build ID: 20120309131123

Steps to reproduce:

Open www.google.co.uk, then style editor.
Open www.google.com, then style editor.

Both websites appear as "google" in the style editor.



Expected results:

The real url of the website should appear as there might be differences in the url name. The user should be able to see in which site he's working.

Comment 1

[Cedric Vivier [:cedricv]]

You mean in the window title bar?
I'm not sure this improves anything, lots of URLs would be too long to fit in the title bar.
Also this seems to prioritize a corner case (editing multiple documents with the same title) over 'human-friendliness' of the Style Editor.

Kevin, what do you think?

Comment 2

[Kevin Dangoor]

(In reply to Cedric Vivier [:cedricv] from comment #1)
> You mean in the window title bar?
> I'm not sure this improves anything, lots of URLs would be too long to fit
> in the title bar.
> Also this seems to prioritize a corner case (editing multiple documents with
> the same title) over 'human-friendliness' of the Style Editor.
> 
> Kevin, what do you think?

I think it's more important that we get the Style Editor docked and managed with the rest of the tools... a separate window is easy to lose (something that came up in user studies). The current window set up also behaves a little funny (close the parent tab or navigate away from the page, for example).

Comment 3

[Cedric Vivier [:cedricv]]

I agree, but since we'll keep the possibility of popping out STyle Editor in a window anyways, what to do with this bug?

Comment 4

[Kevin Dangoor]

(In reply to Cedric Vivier [:cedricv] from comment #3)
> I agree, but since we'll keep the possibility of popping out STyle Editor in
> a window anyways, what to do with this bug?

I think it's unlikely that the window it pops out into will be just the Style Editor. The spirit of this bug is probably correct: I think we'd want the popped out window to reflect which URL it is working with.

Comment 5

[sys.sgx]

any updates on this?
thanks

Comment 6

[Panos Astithas (he/him) [:past] (please ni?)]

The style editor is now embedded inside the toolbox. Discussed this in IRC with Paul and we agreed that this would be nice to have.

Comment 7

[David Creswick]

Attachment: patch proposal

How is this? I'm not sure how much I like it placed at the bottom, but it doesn't make sense to place it at the top above the "dock/undock" and "close" buttons. Placing it instead in toolbox.xul, would just be messy code-wise, unless XUL CSS works differently than I assume.

Comment 8

[Girish Sharma [:Optimizer]]

(In reply to David Creswick from comment #7)
> Created attachment 701619 [details] [diff] [review]
> patch proposal
> 
> How is this? I'm not sure how much I like it placed at the bottom, but it
> doesn't make sense to place it at the top above the "dock/undock" and
> "close" buttons. Placing it instead in toolbox.xul, would just be messy
> code-wise, unless XUL CSS works differently than I assume.

I don;t think that adding a status bar is a great idea. You can very well show the url in the title of the window, which infact has a very huge blank space. We should also show which tool is selected, rather than just Developer Tools.

May be in the format of :

<tool> - <url>

as in 

Web Console - https://github.com

and

Inspector - google.com

or maybe we can choose to display the title of the tab instead of the url.

Comment 9

[Paul Rouget [:paul]]

I agree with Optimizer. Better to use the title for that. Showing the tool name could be nit.

I would much prefer to see the URL though.

Comment 10

[David Creswick]

How compatible is that with localizations that don't necessarily read left-to-right (or other language issues I'm not aware of)? What's the convention for setting windows titles with multiple pieces of info?

Comment 11

[David Creswick]

Attachment: patch proposal, revised

Show devtool name and target url in the title bar as suggested by Optimizer. I can't figure out how to get the XML entity "&window.title;" to render. It keeps rendering the literal text.

Comment 12

[Panos Astithas (he/him) [:past] (please ni?)]

You'll need to use a property file for this:
https://developer.mozilla.org/en-US/docs/XUL/Tutorial/Property_Files

Comment 13

[David Creswick]

Attachment: patch proposal, revised

Comment 14

[Paul Rouget [:paul]]

Comment on attachment 702040 [details] [diff] [review]
patch proposal, revised

Thank you for working on that.

I think it would be better if the tittle would be handled on the target side (in Target.jsm).
With a `TabTarget.title` property and a `title-changed` event. Also, the window title needs to be
updated if the title is changed dynamically.

>diff -r 43d65f5d22b2 browser/devtools/framework/Toolbox.jsm
>--- a/browser/devtools/framework/Toolbox.jsm	Sun Jan 13 09:39:32 2013 -0500
>+++ b/browser/devtools/framework/Toolbox.jsm	Mon Jan 14 17:16:04 2013 -0600
>@@ -12,28 +12,16 @@ Cu.import("resource://gre/modules/common
> Cu.import("resource:///modules/devtools/EventEmitter.jsm");
> Cu.import("resource:///modules/devtools/gDevTools.jsm");
> 
> XPCOMUtils.defineLazyModuleGetter(this, "Hosts",
>                                   "resource:///modules/devtools/ToolboxHosts.jsm");
> XPCOMUtils.defineLazyModuleGetter(this, "CommandUtils",
>                                   "resource:///modules/devtools/DeveloperToolbar.jsm");
> 
>-XPCOMUtils.defineLazyGetter(this, "toolboxStrings", function() {
>-  let bundle = Services.strings.createBundle("chrome://browser/locale/devtools/toolbox.properties");
>-  let l10n = function(name) {
>-    try {
>-      return bundle.GetStringFromName(name);
>-    } catch (ex) {
>-      Services.console.logStringMessage("Error reading '" + name + "'");
>-    }
>-  };
>-  return l10n;
>-});
>-

Why do you move that?

>     let frameLoad = function(event) {

Can you explain why the following needs to happen in frameLoad?

>       win.removeEventListener("load", frameLoad, true);
>+
>+      this.refreshTitle();
>+      this._toolbox.on("select", this.refreshTitle.bind(this));
>+      this._toolbox.target.on("navigate", this.refreshTitle.bind(this));

Better to do:
> this.refreshTitle = this.refreshTitle.bind(this);
and then:
> this._toolbox.on("select", this.refreshTitle);

>+    this._toolbox.off("select", this.refreshTitle);
>+    this._toolbox.target.off("navigate", this.refreshTitle);

That won't work, because this.refreshTitle != this.refreshTitle.bind(this).
See comment above.

>   destroy: function WH_destroy() {
>     if (!this._destroyed) {
>       this._destroyed = true;
> 
>       this._window.removeEventListener("unload", this._boundUnload);
>+      this._toolbox.off("select", this.refreshTitle);
>+      if (this._toolbox.target) {
>+        this._toolbox.target.off("navigate", this.refreshTitle);
>+      }

Is this scenario possible?


>+  /**
>+   * Update the window title with the tool name and the URL.
>+   */
>+  refreshTitle: function WH_refreshTitle() {
>+    let toolLabel;
>+    let toolId = this._toolbox.currentToolId;
>+    if (toolId) {
>+      let toolDef = gDevTools.getToolDefinitionMap().get(toolId);
>+      toolLabel = toolDef.label;
>+    } else {
>+      // no tool is selected
>+      toolLabel = Toolbox.l10n("toolbox.window.title");
>+    }
>+    let title = toolLabel + " - " + this._toolbox.target.url;
>+    this._window.document.title = title;
>   }
> }

Don't concatenate in JS. Use a template property for that. See for for example:
http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/devtools/gcli.properties#85

Comment 15

[David Creswick]

(In reply to Paul Rouget [:paul] from comment #14)
> Comment on attachment 702040 [details] [diff] [review]
> patch proposal, revised
> 
> Thank you for working on that.
> 
> I think it would be better if the tittle would be handled on the target side
> (in Target.jsm).
> With a `TabTarget.title` property and a `title-changed` event. Also, the
> window title needs to be
> updated if the title is changed dynamically.

The patch shows the target's url, not the target's document title. I would guess that hooking into the "navigation" event would be sufficient to detect all url changes?

> 
> >diff -r 43d65f5d22b2 browser/devtools/framework/Toolbox.jsm
> >--- a/browser/devtools/framework/Toolbox.jsm	Sun Jan 13 09:39:32 2013 -0500
> >+++ b/browser/devtools/framework/Toolbox.jsm	Mon Jan 14 17:16:04 2013 -0600
> >@@ -12,28 +12,16 @@ Cu.import("resource://gre/modules/common
> > Cu.import("resource:///modules/devtools/EventEmitter.jsm");
> > Cu.import("resource:///modules/devtools/gDevTools.jsm");
> > 
> > XPCOMUtils.defineLazyModuleGetter(this, "Hosts",
> >                                   "resource:///modules/devtools/ToolboxHosts.jsm");
> > XPCOMUtils.defineLazyModuleGetter(this, "CommandUtils",
> >                                   "resource:///modules/devtools/DeveloperToolbar.jsm");
> > 
> >-XPCOMUtils.defineLazyGetter(this, "toolboxStrings", function() {
> >-  let bundle = Services.strings.createBundle("chrome://browser/locale/devtools/toolbox.properties");
> >-  let l10n = function(name) {
> >-    try {
> >-      return bundle.GetStringFromName(name);
> >-    } catch (ex) {
> >-      Services.console.logStringMessage("Error reading '" + name + "'");
> >-    }
> >-  };
> >-  return l10n;
> >-});
> >-
> 
> Why do you move that?

Before, it was defined on the module namespace. The patch moved it to the Toolbox constructor, which is exported from the module. I needed to move it so that ToolboxHost.jsm, a different module, could access the string bundle. After closing this bug, I was thinking of working on unifying the l10n access across all of the devtools. Their string bundles are loaded at least twice, once in framework/ToolDefinition.jsm and at least once (haven't checked) in the devtools themselves.

> 
> >     let frameLoad = function(event) {
> 
> Can you explain why the following needs to happen in frameLoad?

It doesn't need to. That is a vestige from an earlier patch revision in which it was necessary. I'll move it out.

> >       win.removeEventListener("load", frameLoad, true);
> >+
> >+      this.refreshTitle();
> >+      this._toolbox.on("select", this.refreshTitle.bind(this));
> >+      this._toolbox.target.on("navigate", this.refreshTitle.bind(this));
> 
> Better to do:
> > this.refreshTitle = this.refreshTitle.bind(this);
> and then:
> > this._toolbox.on("select", this.refreshTitle);

Okay.

> >+    this._toolbox.off("select", this.refreshTitle);
> >+    this._toolbox.target.off("navigate", this.refreshTitle);
> 
> That won't work, because this.refreshTitle != this.refreshTitle.bind(this).
> See comment above.
> 
> >   destroy: function WH_destroy() {
> >     if (!this._destroyed) {
> >       this._destroyed = true;
> > 
> >       this._window.removeEventListener("unload", this._boundUnload);
> >+      this._toolbox.off("select", this.refreshTitle);
> >+      if (this._toolbox.target) {
> >+        this._toolbox.target.off("navigate", this.refreshTitle);
> >+      }
> 
> Is this scenario possible?

Yes, it's only there because I hit that scenario. The _target property is set to null in Toolbox#destroy before the toolbox host is destroyed.

> Don't concatenate in JS. Use a template property for that.

Okay.

Comment 16

[David Creswick]

Attachment: patch proposal 4

Comment 17

[Paul Rouget [:paul]]

> > I think it would be better if the tittle would be handled on the target side
> > (in Target.jsm).
> > With a `TabTarget.title` property and a `title-changed` event. Also, the
> > window title needs to be
> > updated if the title is changed dynamically.
> 
> The patch shows the target's url, not the target's document title. I would
> guess that hooking into the "navigation" event would be sufficient to detect
> all url changes?

Doe! You're right. Sorry for the confusion :)

Comment 18

[Paul Rouget [:paul]]

Comment on attachment 702536 [details] [diff] [review]
patch proposal 4

>diff --git a/browser/devtools/framework/Toolbox.jsm b/browser/devtools/framework/Toolbox.jsm
>-    let newHost = new Hosts[hostType](this.target.tab);
>+    let newHost = new Hosts[hostType](this);

You are changing the prototype of the Host constructor.
I understand why you're doing this (you need access to the
toolbox), but in term of API, I'm not happy with this.

We're never sure a target has a tab. So I'd like the tab
to be the parameter (so it doesn't need to be checked
in the host code).

Host and Toolbox logics will be too close. I'd like to keep
them as much separated as possible.

I would prefer to see the `refreshTitle` function and its different
calls inside Toolbox.jsm instead.

Maybe the different hosts can expose a "setTitle()" function, and it
would be called from Toolbox.jsm.

Also, in your patch, the title is not updated when we undock
the toolbox into the window, we have to close are re-open the
toolbox to see the title.

>+toolbox.window.title=Developer Tools
>+toolbox.window.titleTemplate=%1$S - %2$S

This needs a comment.

Comment 19

[David Creswick]

(In reply to Paul Rouget [:paul] from comment #18)
> Comment on attachment 702536 [details] [diff] [review]
> patch proposal 4
> 
> >diff --git a/browser/devtools/framework/Toolbox.jsm b/browser/devtools/framework/Toolbox.jsm
> >-    let newHost = new Hosts[hostType](this.target.tab);
> >+    let newHost = new Hosts[hostType](this);
> 
> You are changing the prototype of the Host constructor.
> I understand why you're doing this (you need access to the
> toolbox), but in term of API, I'm not happy with this.
> 
> We're never sure a target has a tab. So I'd like the tab
> to be the parameter (so it doesn't need to be checked
> in the host code).
> 
> Host and Toolbox logics will be too close. I'd like to keep
> them as much separated as possible.
> 
> I would prefer to see the `refreshTitle` function and its different
> calls inside Toolbox.jsm instead.
> 
> Maybe the different hosts can expose a "setTitle()" function, and it
> would be called from Toolbox.jsm.

So you want the title to be the responsibility of the Toolbox, with Toolbox pushing it to the ToolboxHost. Should the title be exposed publicly on Toolbox with a "title" getter and a "title-changed" event or should it be kept private, its only use being to update the host? I'm inclined to keep it private, unless you can think of a reason otherwise.

Comment 20

[David Creswick]

Attachment: patch proposal 5

Comment 21

[Paul Rouget [:paul]]

Comment on attachment 704361 [details] [diff] [review]
patch proposal 5

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

Looks good to me.

Can I ask you to build a test for this?
If you're not sure how, please let me know.

As soon as we have a test, we can land this.

::: browser/devtools/framework/Toolbox.jsm
@@ +25,5 @@
> +        return bundle.GetStringFromName(name);
> +      } else {
> +        let args = Array.prototype.slice.call(arguments, 1);
> +        return bundle.formatStringFromName(name, args, args.length);
> +      }

Maybe you want to use a rest parameter here:
https://developer.mozilla.org/en-US/docs/JavaScript/Reference/rest_parameters

Comment 22

[David Creswick]

Attachment: patch revision 6, with tests

Comment 23

[David Creswick]

Attachment: patch revision 7, with test

Re-upload because the patch didn't apply cleanly anymore. I am requesting another review because I added a test and it is the first test I've written for the mozilla codebase. I assume they are held to the same review policies as code that ships. Correct me if I'm wrong.

Comment 24

[Paul Rouget [:paul]]

Sorry for the delay. I'm a bit busy these days. I'll try to review this patch next week.

Comment 25

[Joe Walker [:jwalker]]

Comment on attachment 708810 [details] [diff] [review]
patch revision 7, with test

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

Paul is reviewed under. r+ from me.
You've run the tests locally, right? Do you have try access? If not, I can do a try run for you.

Comment 26

[Paul Rouget [:paul]]

Comment on attachment 708810 [details] [diff] [review]
patch revision 7, with test

Sorry for the slow review.

>       // If we were asked for a specific tool then we need to wait for the
>       // tool to be ready, otherwise we can just wait for toolbox open
>       if (toolId != null) {
>         toolbox.once(toolId + "-ready", function(event, panel) {
>           this.emit("toolbox-ready", toolbox);
>-          deferred.resolve(toolbox);
>+          promise.then(function () deferred.resolve(toolbox));
>         }.bind(this));
>-        toolbox.open();
>+        let promise = toolbox.open();
>       }

Can you explain why you're doing this.

>+function checkTitle(toolId, url, context) {
>+  let win = Services.wm.getMostRecentWindow("devtools:toolbox");
>+  let definitions = gDevTools.getToolDefinitionMap();
>+  let title = new String(win.document.title);
>+  ok(title.indexOf(url) >= 0,
>+     context + ", url in title");
>+  ok(title.indexOf(definitions.get(toolId).label) >= 0,
>+     context + ", tool name in title");
>+}

Hard code the expected title value:
> is(win.document.title, "inspector = XXX", "Title is right")
Code coverage is larger if you do it this way (you don't rely on a `definitions.get(toolId).label`.

>\ No newline at end of file

You have a couple of these in this patch. Remove them.

As Joe said, we'll need a push to try.

Comment 27

[David Creswick]

(In reply to Joe Walker [:jwalker] from comment #25)
> Comment on attachment 708810 [details] [diff] [review]
> patch revision 7, with test
> 
> Review of attachment 708810 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Paul is reviewed under. r+ from me.
> You've run the tests locally, right? Do you have try access? If not, I can
> do a try run for you.

No, I don't have try access.

Comment 28

[David Creswick]

(In reply to Paul Rouget [:paul] from comment #26)
> Comment on attachment 708810 [details] [diff] [review]
> patch revision 7, with test

> Can you explain why you're doing this.

In short, because the test failed if I didn't. The order in which events are fired in Toolbox#selectTool prevents the title from being set before the test checks it. On my second review of the problem though, I can think of some ways to write the test that render this hunk unnecessary.

> >\ No newline at end of file
> 
> You have a couple of these in this patch. Remove them.

In both cases, I'm adding a newline at end of file where there wasn't one before. The style guide suggests that this is not the wrong thing to do.

Comment 29

[David Creswick]

Attachment: patch revision 8, with test

Comment 30

[Paul Rouget [:paul]]

Comment on attachment 711121 [details] [diff] [review]
patch revision 8, with test

Looks good. Thank you David.

Tests are running here: https://tbpl.mozilla.org/?tree=Try&rev=d556232e66af

Once it's green, I'll land this.

Comment 31

[Paul Rouget [:paul]]

Error: https://tbpl.mozilla.org/php/getParsedLog.php?id=19531088

(might be my fault, I'll look asap)

Comment 32

[Paul Rouget [:paul]]

Attachment: rebased

rebased + hg headers

Comment 33

[Paul Rouget [:paul]]

Running the test locally, I get the same failure:

> uncaught exception - Script error. at resource:///modules/devtools/InspectorPanel.jsm:0

Comment 34

[David Creswick]

I'd say this is the more helpful line:

> JavaScript Error: "TypeError: self.selection is null" {file: "resource://gre/modules/devtools/InspectorPanel.jsm" line: 244}

Comment 35

[David Creswick]

In case it's unclear, I believe bug 840156 should fix the exception in the test.

Comment 36

[Paul Rouget [:paul]]

With bug 840156:
https://tbpl.mozilla.org/?tree=Try&rev=5c1bd2447fa2

Comment 37

[Joe Walker [:jwalker]]

Attachment: re-rebased

Comment 38

[Joe Walker [:jwalker]]

Landed: https://hg.mozilla.org/integration/fx-team/rev/c1abd2ac8de9
Backed out: https://hg.mozilla.org/integration/fx-team/rev/18821382badc

Comment 39

[Paul Rouget [:paul]]

@Joe: What happened?

Comment 40

[Joe Walker [:jwalker]]

https://tbpl.mozilla.org/?tree=Fx-Team&rev=53d22797d694 happened. It caused a ton of oranges, I think because it changed the default window position to 'detached', maybe?

Comment 41

[Girish Sharma [:Optimizer]]

I think there are two things happening :

1) The test for this bug left the toolbox undocked.
2) Some of the tests which involved synthesized keys were calling the keys on to the main window rather than the toolbox window. So they failed in undocked case.

I think both these points need to be fixed as there is a separate bug to run the tests in undocked toolbox.

Comment 42

[David Creswick]

Attachment: patch v10; test now clears Toolbox preferences

Comment 43

[Paul Rouget [:paul]]

(In reply to David Creswick from comment #42)
> Created attachment 717152 [details] [diff] [review]
> patch v10; test now clears Toolbox preferences

Cool. This needs a proper try run (I messed up the last one).
Working on it!

Comment 44

[Paul Rouget [:paul]]

https://tbpl.mozilla.org/?tree=Try&rev=01f1fe77117e

Comment 45

[David Creswick]

Attachment: patch v11

Hopefully this fixes the last of the failures.

Comment 46

[Paul Rouget [:paul]]

https://tbpl.mozilla.org/?tree=Try&rev=2abca9b74b4f

Comment 47

[Paul Rouget [:paul]]

Looks green to me.

Comment 48

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

https://hg.mozilla.org/integration/fx-team/rev/6092bfb91be4

Comment 49

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/6092bfb91be4