Closed
Bug 1248601
Opened 8 years ago
Closed 8 years ago
Add the <xul:toolbar> of the Developer Toolbar dynamically
Categories
(DevTools Graveyard :: Graphic Commandline and Toolbar, defect)
DevTools Graveyard
Graphic Commandline and Toolbar
Tracking
(firefox47 fixed)
RESOLVED
FIXED
Firefox 47
Tracking | Status | |
---|---|---|
firefox47 | --- | fixed |
People
(Reporter: ochameau, Assigned: ochameau)
References
Details
Attachments
(2 files, 3 obsolete files)
1.91 KB,
patch
|
jwalker
:
review+
|
Details | Diff | Splinter Review |
11.18 KB,
patch
|
ochameau
:
review+
|
Details | Diff | Splinter Review |
See bug 1248348. Today, a <xul:toolbar> and various sub xul content is hardcoded within browser.xul: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.xul#1118 We should instead add it dynamically so that the reload addon can remove and readd it, possibly, differently. Today, the hot reload addon uses some hack wrappers in order to ensure that DeveloperToolbar dependencies are correctly reloaded. Doing that is also going to cleanup hardcoded stuff for devtools from browser.xul!
Assignee | ||
Comment 1•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=acf28b1cb94d
Assignee | ||
Comment 2•8 years ago
|
||
This patch is quite simple. It just creates the XUL elements during DeveloperToolbar instanciation, instead of fetch some existing elements hardcoded in browser.xul. There is one workaround around xul:textarea. As the element is added to the DOM later, its binding is not always ready. textarea.value happens to be undefined if accessed too quickly. Waiting for try results. I would also need to remove the elements in destroy().
Assignee | ||
Comment 3•8 years ago
|
||
Try was green. New patch, new try, with proper cleanup. Still need to play with this and the reload addon.
Assignee | ||
Updated•8 years ago
|
Attachment #8719850 -
Attachment is obsolete: true
Assignee | ||
Comment 4•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=77432379ea32
Assignee | ||
Comment 5•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=25f22d14cc59
Assignee | ||
Comment 6•8 years ago
|
||
Quite naive operation. I just create the xul elements on-demand in JS instead of hardcoding them in browser.xul. But because XUL is XUL... it comes to a price. When doing that, xul xbl bindings may not be fully set and I had to workaround here in there. Especially in the second patch I'm about to attach. The wins of such patch are: * non-developer saves a few bytes by not having dev toolbars things always around * reload addon can now customize dev toolbar! * dev toolbar code is now more self contained and can easily be enabled/disabled A green try with many other patches: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6989b13865cb
Attachment #8723602 -
Flags: review?(jwalker)
Assignee | ||
Updated•8 years ago
|
Attachment #8720002 -
Attachment is obsolete: true
Assignee | ||
Comment 7•8 years ago
|
||
This is gross but prevents any issues with xbl laziness.
Attachment #8723603 -
Flags: review?(jwalker)
Comment 8•8 years ago
|
||
Comment on attachment 8723603 [details] [diff] [review] Fix focus race due to dynamic XUL insertion. Review of attachment 8723603 [details] [diff] [review]: ----------------------------------------------------------------- Yup, that's gross! It feels like an intermittent test failure waiting to happen, but if it's what we need to do... ::: devtools/client/shared/developer-toolbar.js @@ +499,5 @@ > + // If the toolbar was just inserted, the <textbox> may still have > + // its binding in process of being applied and not be focusable yet > + let waitForBinding = () => { > + // mInputField is a xbl field of <xul:textbox> > + if (typeof(this._input.mInputField) != "undefined") { if (typeof this._input.mInputField != "undefined") ?
Attachment #8723603 -
Flags: review?(jwalker) → review+
Comment 9•8 years ago
|
||
Comment on attachment 8723602 [details] [diff] [review] patch v3 Review of attachment 8723602 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/client/shared/developer-toolbar.js @@ +303,5 @@ > + close.setAttribute("oncommand", "DeveloperToolbar.hide();"); > + close.setAttribute("tooltiptext", "developerToolbarCloseButton.tooltiptext"); > + > + if (isMac) { > + toolbar.appendChild(close); I think it would make this a bit clearer if we moved this down and then did: if (isMac) { toolbar.appendChild(close); toolbar.appendChild(stack); } else { toolbar.appendChild(stack); toolbar.appendChild(close); } I wish the XUL had a comment explaining this hack. If you know, could you add it? @@ +331,5 @@ > + if (!isMac) { > + toolbar.appendChild(close); > + } > + > + let bottomBox = this._doc.getElementById("browser-bottombox"); We have both the parameter 'doc' and the member 'this._doc' and they refer to the same thing (I think). Maybe we don't need the parameter? @@ +336,5 @@ > + this._element = toolbar; > + bottomBox.appendChild(this._element); > + > + this._errorCounterButton = this._doc > + .getElementById("developer-toolbar-toolbox-button"); this._errorCounterButton = toolboxBtn; ? We could probably merge these 2 lines into the lines above
Attachment #8723602 -
Flags: review?(jwalker) → review+
Assignee | ||
Comment 10•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=06134fad6e23
Assignee | ||
Comment 11•8 years ago
|
||
(In reply to Joe Walker [:jwalker] (needinfo me or ping on irc) from comment #8) > Comment on attachment 8723603 [details] [diff] [review] > Fix focus race due to dynamic XUL insertion. > > Review of attachment 8723603 [details] [diff] [review]: > ----------------------------------------------------------------- > > Yup, that's gross! > It feels like an intermittent test failure waiting to happen, but if it's > what we need to do... Hopefully, this explicit wait is effective enough to prevent any failure. It isn't like waiting just for Xms, we are waiting for it to be ready. But do not hesitate to ping me if you need anything wrong/suspicious after it lands.
Assignee | ||
Comment 12•8 years ago
|
||
(In reply to Joe Walker [:jwalker] (needinfo me or ping on irc) from comment #9) > I think it would make this a bit clearer if we moved this down and then did: > > if (isMac) { > toolbar.appendChild(close); > toolbar.appendChild(stack); > } > else { > toolbar.appendChild(stack); > toolbar.appendChild(close); > } > > I wish the XUL had a comment explaining this hack. If you know, could you > add it? This is just that the close button is on the left on mac and right on other platorms.
Assignee | ||
Updated•8 years ago
|
Attachment #8723602 -
Attachment is obsolete: true
Assignee | ||
Comment 14•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/9b8fd7fc3d8d9babbc0668230acc4f65e15640bd Bug 1248601 - Register the Developer Toolbar dynamically. r=jwalker https://hg.mozilla.org/integration/fx-team/rev/fe9a225a5eb94b245ab44ffa52946f4693508495 Bug 1248601 - Fix focus race due to dynamic XUL insertion. r=jwalker
Comment 15•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/9b8fd7fc3d8d https://hg.mozilla.org/mozilla-central/rev/fe9a225a5eb9
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox47:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Updated•6 years ago
|
Product: Firefox → DevTools
Updated•6 years ago
|
Product: DevTools → DevTools Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•