875902 - [toolbox] Simplify selectTool and test for panel readiness

Status: RESOLVED FIXED

(DevTools :: Framework, defect)

Description

[David Creswick]

When waiting for panels to load, the toolbox listens for the DOMContentLoaded event, but it should listen for the load event instead. This results in, at the very least, the promise returned by gDevTools.showToolbox(...) resolving before the panel is completely initialized, though only for some tools. This has caused me some problems when writing tests for other bugs.

The difference between the two events is defined here: http://www.whatwg.org/specs/web-apps/current-work/multipage/the-end.html

Comment 1

[David Creswick]

Attachment: patch v1

Comment 2

[Victor Porof [:vporof][:vp]]

I think I remember Paul mentioned that he used DOMContentLoaded on purpose, instead of the load event. I can't seem to recall the exact reason though...

Comment 3

[Dave Camp (:dcamp)]

Comment on attachment 753918 [details] [diff] [review]
patch v1

I think Joe is a better reviewer here.

Comment 4

[Joe Walker [:jwalker]]

Comment on attachment 753918 [details] [diff] [review]
patch v1

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

Hmmm.
I've not heard Paul say we should be using DOMContentLoaded, and it seems to me as though 'load' makes more sense here. But Paul is back in a few days time, so I suggest we wait and ask him.

Personally, I think I'd rather have a panel.ready() function that returns a promise that's resolved when the panel is ready. That means we don't need to expose the panelWin object everywhere. Does that make sense?

Comment 5

[David Creswick]

Attachment: patch v2, rebased

Rebased.

Comment 6

[David Creswick]

Well the other reason to expose panelWin is so that the test has something to test. I suppose that could be moved to an "isReady" property or method.

Comment 7

[David Creswick]

Attachment: patch v3, rewrite

So what is the call on DOMContentLoaded vs load? Either way, this is a slightly better way to write selectTool and loadTool.

Comment 8

[Paul Rouget [:paul]]

(In reply to David Creswick from comment #0)
> When waiting for panels to load, the toolbox listens for the
> DOMContentLoaded event, but it should listen for the load event instead.
> This results in, at the very least, the promise returned by
> gDevTools.showToolbox(...) resolving before the panel is completely
> initialized, though only for some tools.

iirc, we use DOMContentLoaded for performance reasons (showing the tools as soon as possible).

> This has caused me some problems when writing tests for other bugs.

What kind of problems?

Comment 9

[David Creswick]

(In reply to Paul Rouget [:paul] from comment #8)
> (In reply to David Creswick from comment #0)
> 
> iirc, we use DOMContentLoaded for performance reasons (showing the tools as
> soon as possible).

Oh, I suppose that makes sense. I wasn't thinking about the interactive case.

> > This has caused me some problems when writing tests for other bugs.
> 
> What kind of problems?

In my test, I wanted access to the CssRuleView object of the inspector tool, but it isn't available until the window of the tab's iframe has loaded. I reviewed that case again this morning and realized I should probably be waiting for the rule view tab to load instead of the inspector panel. 
 
It wasn't unreasonable to imagine that tools would defer some initialization until the load event fires, so I wrote the test hoping to fix all cases. The options panel, the inspector panel, and the styleeditor panel all report to be ready before their content document's load event fires. It doesn't look like that is actually the problem I thought it was though.

Comment 10

[David Creswick]

Attachment: v4, stripped down to just the improvements in style and test coverage

Hijacking this bug number to simplify selectTool and test for panel readiness.

Comment 11

[David Creswick]

Attachment: v5, bitrot fixed

Comment 12

[Phil Ringnalda (:philor)]

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

Comment 13

[Victor Porof [:vporof][:vp]]

https://hg.mozilla.org/mozilla-central/rev/1d0d9f1d6321