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)

defect
Not set
normal

Tracking

(firefox47 fixed)

RESOLVED FIXED
Firefox 47
Tracking Status
firefox47 --- fixed

People

(Reporter: ochameau, Assigned: ochameau)

References

Details

Attachments

(2 files, 3 obsolete files)

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!
Attached patch patch v1 (obsolete) — Splinter Review
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().
Attached patch patch v2 (obsolete) — Splinter Review
Try was green. New patch, new try, with proper cleanup.
Still need to play with this and the reload addon.
Attachment #8719850 - Attachment is obsolete: true
Attached patch patch v3 (obsolete) — Splinter Review
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)
Attachment #8720002 - Attachment is obsolete: true
This is gross but prevents any issues with xbl laziness.
Attachment #8723603 - Flags: review?(jwalker)
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 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+
(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.
(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.
Attached patch patch v4Splinter Review
Addressed comments.
Attachment #8724405 - Flags: review+
Attachment #8723602 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/9b8fd7fc3d8d
https://hg.mozilla.org/mozilla-central/rev/fe9a225a5eb9
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Blocks: 1252479
Depends on: 1257178
Depends on: 1259024
Product: Firefox → DevTools
Product: DevTools → DevTools Graveyard
You need to log in before you can comment on or make changes to this bug.