1399779 - DAMP should track the actual opening of the Style Editor

Status: NEW

(DevTools :: Style Editor, enhancement, P3)

Description

[Patrick Brosset <:pbro>]

Right now our performance test (DAMP) tracks the time it takes to complete showToolbox for a given tool.

In the case of the style-editor, this time doesn't correspond to really everything the panel needs to be fully functional.

In particular, this code

this.cssSheet.getMediaRules().then(this._onMediaRulesChanged, console.error);
http://searchfox.org/mozilla-central/source/devtools/client/styleeditor/StyleSheetEditor.jsm#129-130

runs when the editor for the stylesheet shown by default is displayed the first time. We don't wait for this promise the resolve.

We could wait for it to resolve, and have a  StyleSheetEditor.init method that returns a promise. This way we could wait for it to finish from the panel init code:

http://searchfox.org/mozilla-central/source/devtools/client/styleeditor/StyleEditorUI.jsm#392

This would allow tracking the full opening of the panel on DAMP.

When done, I believe we should be able to remove this code from the styleditor/test/head.js file:
let onMediaListChanged = ui.once("media-list-changed");

Comment 1

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Should we change DAMP to instead listen for some explicit event like "tool-is-ready" (a good name left for others to think of...) that each tool can emit at the spot where it is believe to be "done"?

This seems like it could be more predictable than chasing promises as code evolves.

I believe something similar was done to analyze performance of Firefox OS apps...?  Maybe :zibi would have suggestions.

Comment 2

[Alexandre Poirot [:ochameau]]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #1)
> Should we change DAMP to instead listen for some explicit event like
> "tool-is-ready" (a good name left for others to think of...) that each tool
> can emit at the spot where it is believe to be "done"?

Sounds fine to me, but I'm not sure it brings a ton of value.
The main advantage is that you won't necessarely have to chain all the promises.
By chaining I mean have all the promises end up being on the resolve path of gDevTools.showToolbox.
It may be useful.

> This seems like it could be more predictable than chasing promises as code
> evolves.

But we would still have to chase all the promises.
Or at least all the meaningful ones. Like the one we Patrick is mentioning.
We would still have to chases all of them before firing tool-is-ready.

Also from my experience in the inspector, with more than one component loading,
I saw that we have to wait for all components to load.
At the end, it may be easier to propagate the promises, like what we do for showToolbox.
Each component returns a promise and we Promise.all() them before resolving showToolbox.


To reply to you parallel comment in bug 1407826.
I think we would need such event in the context of page reload.
We can use showToolbox promise when testing toolbox opening, but we miss something to watch tool updates on page reload.
But I imagine that this event will be easier to emit if we still chain and chase all the promises.