Closed
Bug 734664
Opened 13 years ago
Closed 12 years ago
Devtools toolbox should display the actual target url when detached
Categories
(DevTools :: General, defect, P3)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 22
People
(Reporter: sys.sgx, Assigned: dcrewi)
References
Details
Attachments
(1 file, 11 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review |
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
Updated•13 years ago
|
QA Contact: untriaged → developer.tools.style.editor
Updated•13 years ago
|
Summary: *** Style Editor should display actual url of website → Style Editor should display actual url of website
Updated•13 years ago
|
Priority: P1 → P3
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?
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).
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?
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.
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.
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
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.
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.
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.
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?
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.
Attachment #701619 -
Attachment is obsolete: true
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
Assignee | ||
Comment 13•12 years ago
|
||
Attachment #701655 -
Attachment is obsolete: true
Attachment #702040 -
Flags: review?(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
Attachment #702040 -
Flags: review?(paul) → review-
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.
Assignee | ||
Comment 16•12 years ago
|
||
Attachment #702040 -
Attachment is obsolete: true
Attachment #702536 -
Flags: review?(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 :)
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.
Attachment #702536 -
Flags: review?(paul) → review-
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.
Assignee | ||
Comment 20•12 years ago
|
||
Attachment #702536 -
Attachment is obsolete: true
Attachment #704361 -
Flags: review?(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
Attachment #704361 -
Flags: review?(paul) → review+
Assignee | ||
Comment 22•12 years ago
|
||
Attachment #704361 -
Attachment is obsolete: true
Attachment #705627 -
Flags: review?(paul)
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.
Attachment #705627 -
Attachment is obsolete: true
Attachment #705627 -
Flags: review?(paul)
Attachment #708810 -
Flags: review?(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.
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.
Attachment #708810 -
Flags: review?(paul) → review+
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.
Attachment #708810 -
Flags: review+
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.
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.
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.
Attachment #711121 -
Flags: review+
Comment 31•12 years ago
|
||
Error: https://tbpl.mozilla.org/php/getParsedLog.php?id=19531088 (might be my fault, I'll look asap)
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
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}
Assignee | ||
Comment 35•12 years ago
|
||
In case it's unclear, I believe bug 840156 should fix the exception in the test.
Comment 37•12 years ago
|
||
Attachment #711121 -
Attachment is obsolete: true
Attachment #711354 -
Attachment is obsolete: true
Updated•12 years ago
|
Whiteboard: [fixed-in-fx-team]
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
Whiteboard: [fixed-in-fx-team]
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?
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.
Assignee | ||
Comment 42•12 years ago
|
||
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!
Assignee | ||
Comment 45•12 years ago
|
||
Hopefully this fixes the last of the failures.
Attachment #717152 -
Attachment is obsolete: true
Updated•12 years ago
|
Attachment #715415 -
Attachment is obsolete: true
Updated•12 years ago
|
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
Comment 49•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/6092bfb91be4
Assignee: nobody → dcrewi
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 22
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•