[DevTools] Run tests only after all UI bits are initialized.

[DevTools] Run tests only after all UI bits are initialized.

Tests use WebInspector.panels, which are initialized only in presentUI.

BUG=397648
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=179146

[dgozman, 2014-07-28 12:17:32]

Take a look please.

[apavlov, 2014-07-28 12:30:31]

https://codereview.chromium.org/426553003/diff/1/Source/devtools/front_end/co...
File Source/devtools/front_end/common/NotificationService.js (left):

https://codereview.chromium.org/426553003/diff/1/Source/devtools/front_end/co...
Source/devtools/front_end/common/NotificationService.js:18: InspectorLoaded:
"InspectorLoaded",
Seva, isn't this a CodeSchool-consumed event? Or does it listen to something
else?

[vsevik, 2014-07-29 08:42:15]

https://codereview.chromium.org/426553003/diff/1/Source/devtools/front_end/Te...
File Source/devtools/front_end/Tests.js (right):

https://codereview.chromium.org/426553003/diff/1/Source/devtools/front_end/Te...
Source/devtools/front_end/Tests.js:888:
WebInspector.notifications.addEventListener(WebInspector.NotificationService.Events.InspectorLoadedForTests,
runTests);
Can we override WebInspector.app.presentUI instead?

https://codereview.chromium.org/426553003/diff/1/Source/devtools/front_end/co...
File Source/devtools/front_end/common/NotificationService.js (left):

https://codereview.chromium.org/426553003/diff/1/Source/devtools/front_end/co...
Source/devtools/front_end/common/NotificationService.js:18: InspectorLoaded:
"InspectorLoaded",
On 2014/07/28 12:30:31, apavlov wrote:
> Seva, isn't this a CodeSchool-consumed event? Or does it listen to something
> else?
This one is not used by codeschool.

https://codereview.chromium.org/426553003/diff/1/Source/devtools/front_end/ma...
File Source/devtools/front_end/main/Main.js (left):

https://codereview.chromium.org/426553003/diff/1/Source/devtools/front_end/ma...
Source/devtools/front_end/main/Main.js:387:
WebInspector.notifications.dispatchEventToListeners(WebInspector.NotificationService.Events.InspectorLoaded);
Could this be used by some third party clients?

[pfeldman, 2014-07-29 08:45:14]

How about we get rid of WebInspector.panels calls instead?

[dgozman, 2014-07-29 08:51:47]

> How about we get rid of WebInspector.panels calls instead?
That's orthogonal to this change. It's just safer to run tests after everything
is initialized. Otherwise, it will regress later with something else, tests will
go flaky and be disabled, etc.

https://codereview.chromium.org/426553003/diff/1/Source/devtools/front_end/Te...
File Source/devtools/front_end/Tests.js (right):

https://codereview.chromium.org/426553003/diff/1/Source/devtools/front_end/Te...
Source/devtools/front_end/Tests.js:888:
WebInspector.notifications.addEventListener(WebInspector.NotificationService.Events.InspectorLoadedForTests,
runTests);
On 2014/07/29 08:42:15, vsevik wrote:
> Can we override WebInspector.app.presentUI instead?

We would have to override all the WebInspector.App hierarchy. I don't see how
it's any better.

https://codereview.chromium.org/426553003/diff/1/Source/devtools/front_end/ma...
File Source/devtools/front_end/main/Main.js (left):

https://codereview.chromium.org/426553003/diff/1/Source/devtools/front_end/ma...
Source/devtools/front_end/main/Main.js:387:
WebInspector.notifications.dispatchEventToListeners(WebInspector.NotificationService.Events.InspectorLoaded);
On 2014/07/29 08:42:15, vsevik wrote:
> Could this be used by some third party clients?

This notification was added in
http://src.chromium.org/viewvc/blink?view=revision&revision=169265, not that
long ago, so I don't think anyone depends on it. How do we check the dependency
though?

[vsevik, 2014-07-29 09:00:36]

I thought panels are created in _createAppUI method when we create and
initialize InspectorView.
Why do we need to postpone loading?
Could you describe where the current implementation fails?

[pfeldman, 2014-07-29 09:01:02]

My worry is that following this route, we can create a comfortable environment
where the tests always pass, but the actual app won't even load. So I guess the
tests should operate same "undefined" front-end condition where they don't know
what part of the app is already initialized.

[dgozman, 2014-07-29 12:44:27]

Agreed with vsevik@ to just rename notification to InspectorUILoadedForTests.

[dgozman, 2014-07-29 12:46:08]

On 2014/07/29 09:01:02, pfeldman wrote:
> My worry is that following this route, we can create a comfortable environment
> where the tests always pass, but the actual app won't even load. So I guess
the
> tests should operate same "undefined" front-end condition where they don't
know
> what part of the app is already initialized.

Most of the tests emulate user actions, which definitely should be done after UI
is initialized.
We don't postpone tests until all backend events have been dispatched, if that
is your concern. In fact, many tests wait for some messages from backend at the
start.

[dgozman, 2014-07-29 12:57:22]

PTAL

[pfeldman, 2014-07-29 13:18:47]

lgtm

[dgozman, 2014-07-29 13:23:12]

The CQ bit was checked by dgozman@chromium.org

[commit-bot: I haz the power, 2014-07-29 13:23:31]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dgozman@chromium.org/426553003/10001

[commit-bot: I haz the power, 2014-07-29 14:29:08]

Change committed as 179146