sys.sgx Reporter
	Description • 13 years ago

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.

	Cedric Vivier [:cedricv]
	Comment 1 • 13 years ago

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?

	Kevin Dangoor
	Comment 2 • 13 years ago

(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).

	Cedric Vivier [:cedricv]
	Comment 3 • 13 years ago

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

	Kevin Dangoor
	Comment 4 • 13 years ago

(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.

	sys.sgx Reporter
	Comment 5 • 12 years ago

any updates on this?
thanks

	Panos Astithas (he/him) [:past] (please ni?)
	Comment 6 • 12 years ago

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.

	David Creswick Assignee
	Comment 7 • 12 years ago

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.

	Girish Sharma [:Optimizer]
	Comment 8 • 12 years ago

(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.

	Paul Rouget [:paul]
	Comment 9 • 12 years ago

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.

	David Creswick Assignee
	Comment 10 • 12 years ago

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?

	David Creswick Assignee
	Comment 11 • 12 years ago

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.

	Panos Astithas (he/him) [:past] (please ni?)
	Comment 12 • 12 years ago

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

	Paul Rouget [:paul]
	Comment 14 • 12 years ago

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

	David Creswick Assignee
	Comment 15 • 12 years ago

(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.

	Paul Rouget [:paul]
	Comment 17 • 12 years ago

> > 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 :)

	Paul Rouget [:paul]
	Comment 18 • 12 years ago

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.

	David Creswick Assignee
	Comment 19 • 12 years ago

(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.

	Paul Rouget [:paul]
	Comment 21 • 12 years ago

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

	David Creswick Assignee
	Comment 23 • 12 years ago

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.

	Paul Rouget [:paul]
	Comment 24 • 12 years ago

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

	Joe Walker [:jwalker]
	Comment 25 • 12 years ago

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.

	Paul Rouget [:paul]
	Comment 26 • 12 years ago

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.

	David Creswick Assignee
	Comment 27 • 12 years ago

(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.

	David Creswick Assignee
	Comment 28 • 12 years ago

(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.

	Paul Rouget [:paul]
	Comment 30 • 12 years ago

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.

	Paul Rouget [:paul]
	Comment 31 • 12 years ago

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

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

	Paul Rouget [:paul]
	Comment 32 • 12 years ago

rebased + hg headers

	Paul Rouget [:paul]
	Comment 33 • 12 years ago

Running the test locally, I get the same failure:

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

	David Creswick Assignee
	Comment 34 • 12 years ago

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}

	David Creswick Assignee
	Comment 35 • 12 years ago

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

	Paul Rouget [:paul]
	Comment 36 • 12 years ago

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

	Joe Walker [:jwalker]
	Comment 38 • 12 years ago

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

	Paul Rouget [:paul]
	Comment 39 • 12 years ago

@Joe: What happened?

	Joe Walker [:jwalker]
	Comment 40 • 12 years ago

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?

	Girish Sharma [:Optimizer]
	Comment 41 • 12 years ago

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.

	Paul Rouget [:paul]
	Comment 43 • 12 years ago

(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!

	Paul Rouget [:paul]
	Comment 44 • 12 years ago

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

	David Creswick Assignee
	Comment 45 • 12 years ago

Hopefully this fixes the last of the failures.

	Paul Rouget [:paul]
	Comment 46 • 12 years ago

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

	Paul Rouget [:paul]
	Comment 47 • 12 years ago

Looks green to me.

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]
	Comment 48 • 12 years ago

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

	Tim Taubert [:ttaubert] (inactive)
	Comment 49 • 12 years ago

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