Closed Bug 734664 Opened 9 years ago Closed 8 years ago

Devtools toolbox should display the actual target url when detached

Categories

(DevTools :: General, defect, P3)

12 Branch
defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 22

People

(Reporter: sys.sgx, Assigned: dcrewi)

References

Details

Attachments

(1 file, 11 obsolete files)

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.
Severity: normal → major
Component: Untriaged → Developer Tools: Style Editor
OS: Windows Vista → All
Priority: -- → P1
Hardware: x86 → All
Version: 11 Branch → 12 Branch
QA Contact: untriaged → developer.tools.style.editor
Summary: *** Style Editor should display actual url of website → Style Editor should display actual url of website
Priority: P1 → P3
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?
(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).
I agree, but since we'll keep the possibility of popping out STyle Editor in a window anyways, what to do with this bug?
(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.
any updates on this?
thanks
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.
Status: UNCONFIRMED → NEW
Component: Developer Tools: Style Editor → Developer Tools
Ever confirmed: true
Summary: Style Editor should display actual url of website → Devtools toolbox should display the actual target url when detached
Attached patch patch proposal (obsolete) — Splinter Review
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.
(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.
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.
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?
Attached patch patch proposal, revised (obsolete) — Splinter Review
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.
Attachment #701619 - Attachment is obsolete: true
Attached patch patch proposal, revised (obsolete) — Splinter Review
Attachment #701655 - Attachment is obsolete: true
Attachment #702040 - Flags: review?(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
Attachment #702040 - Flags: review?(paul) → review-
(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.
Attached patch patch proposal 4 (obsolete) — Splinter Review
Attachment #702040 - Attachment is obsolete: true
Attachment #702536 - Flags: review?(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 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.
Attachment #702536 - Flags: review?(paul) → review-
(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.
Attached patch patch proposal 5 (obsolete) — Splinter Review
Attachment #702536 - Attachment is obsolete: true
Attachment #704361 - Flags: review?(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
Attachment #704361 - Flags: review?(paul) → review+
Attached patch patch revision 6, with tests (obsolete) — Splinter Review
Attachment #704361 - Attachment is obsolete: true
Attachment #705627 - Flags: review?(paul)
Attached patch patch revision 7, with test (obsolete) — Splinter Review
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.
Attachment #705627 - Attachment is obsolete: true
Attachment #705627 - Flags: review?(paul)
Attachment #708810 - Flags: review?(paul)
Sorry for the delay. I'm a bit busy these days. I'll try to review this patch next week.
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.
Attachment #708810 - Flags: review?(paul) → review+
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.
Attachment #708810 - Flags: review+
(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.
(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.
Attached patch patch revision 8, with test (obsolete) — Splinter Review
Attachment #708810 - Attachment is obsolete: true
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.
Attachment #711121 - Flags: review+
Error: https://tbpl.mozilla.org/php/getParsedLog.php?id=19531088

(might be my fault, I'll look asap)
Attached patch rebased (obsolete) — Splinter Review
rebased + hg headers
Running the test locally, I get the same failure:

> uncaught exception - Script error. at resource:///modules/devtools/InspectorPanel.jsm:0
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}
Depends on: 840156
Blocks: :PaulFx21
In case it's unclear, I believe bug 840156 should fix the exception in the test.
Attached patch re-rebased (obsolete) — Splinter Review
Attachment #711121 - Attachment is obsolete: true
Attachment #711354 - Attachment is obsolete: true
Whiteboard: [fixed-in-fx-team]
@Joe: What happened?
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?
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.
(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!
Attached patch patch v11Splinter Review
Hopefully this fixes the last of the failures.
Attachment #717152 - Attachment is obsolete: true
Looks green to me.
Whiteboard: [land-in-fx-team]
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/6092bfb91be4
Assignee: nobody → dcrewi
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 22
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.