1032721 - Re-enable browser_inspector_infobar.js when oranges are fixed

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

This is a continuation of Bug 1028609, browser_inspector_infobar.js had a very regular orange and was proving difficult to fix so the test was disabled.

We should fix the orange and re-enable the test.

Comment 1

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: infobar-timing-out-1032721.patch

You have already reviewed the bulk of this patch in bug 1028609 so you just need to focus on the tests, which are pretty simple.

I have split browser_inspector_infobar.js into two and now only try to get the findbar height on transitionend.

I have also added browser_inspector_transition.js, which is really just testing our once method but is also a useful reference point.

Comment 2

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

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

Comment 3

[Patrick Brosset <:pbro>]

Comment on attachment 8450267 [details] [diff] [review]
infobar-timing-out-1032721.patch

Review of attachment 8450267 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me.
I just would like the highlightNode function to be renamed. Other than that, if the problems goes away (and it looks that way looking at the try push), then great!

::: browser/devtools/inspector/test/browser_inspector_transition.js
@@ +1,4 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +

This test might be better named "browser_inspector_highlight-after-transition.js" or something similar.

@@ +21,5 @@
> +  gBrowser.removeCurrentTab();
> +});
> +
> +function* checkDivHeight(inspector) {
> +  let div = content.document.querySelector("div");

can be replaced with:
let div = getNode("div");

::: browser/devtools/inspector/test/head.js
@@ +95,5 @@
> + *        The instance of InspectorPanel currently loaded in the toolbox
> + * @return a promise that resolves when the inspector is updated with the new
> + * node
> + */
> +function highlightNode(nodeOrSelector, inspector) {

This is something I commented about in the previous bug: I think it makes more sense to name this function 'selectAndHighlightNode' cause that's what it does. Just 'highlightNode' may give the wrong impression.

Also, why not just:
let selectAndHighlight = Task.async(function*(nodeOrSelector, inspector) {
  let updated = inspector.toolbox.once("highlighter-ready");
  yield selectNode(nodeOrSelector, inspector, "test-highlight");
  yield updated;
});

Comment 4

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: infobar-timing-out-1032721.patch

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #3)
> Comment on attachment 8450267 [details] [diff] [review]
> infobar-timing-out-1032721.patch
> 
> Review of attachment 8450267 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good to me.
> I just would like the highlightNode function to be renamed. Other than that,
> if the problems goes away (and it looks that way looking at the try push),
> then great!
> 
> ::: browser/devtools/inspector/test/browser_inspector_transition.js
> @@ +1,4 @@
> > +/* This Source Code Form is subject to the terms of the Mozilla Public
> > + * License, v. 2.0. If a copy of the MPL was not distributed with this
> > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> > +
> 
> This test might be better named
> "browser_inspector_highlight-after-transition.js" or something similar.
> 

Done

> @@ +21,5 @@
> > +  gBrowser.removeCurrentTab();
> > +});
> > +
> > +function* checkDivHeight(inspector) {
> > +  let div = content.document.querySelector("div");
> 
> can be replaced with:
> let div = getNode("div");
> 

Done

> ::: browser/devtools/inspector/test/head.js
> @@ +95,5 @@
> > + *        The instance of InspectorPanel currently loaded in the toolbox
> > + * @return a promise that resolves when the inspector is updated with the new
> > + * node
> > + */
> > +function highlightNode(nodeOrSelector, inspector) {
> 
> This is something I commented about in the previous bug: I think it makes
> more sense to name this function 'selectAndHighlightNode' cause that's what
> it does. Just 'highlightNode' may give the wrong impression.
> 

Done

> Also, why not just:
> let selectAndHighlight = Task.async(function*(nodeOrSelector, inspector) {
>   let updated = inspector.toolbox.once("highlighter-ready");
>   yield selectNode(nodeOrSelector, inspector, "test-highlight");
>   yield updated;
> });

Because then this one function would be defined differently than all others in head.js.

Maybe in the future we could change all methods to use the pattern but, in my opinion, it is much harder to read... especially by new contributors. A high price to pay for very little benefit.

Comment 5

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: infobar-timing-out-1032721.patch

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

Comment 6

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8455057 [details] [diff] [review]
infobar-timing-out-1032721.patch

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

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d0b5e87f69f5