Closed Bug 1151909 Opened 9 years ago Closed 8 years ago

The markup view doesn't initialize until the page is fully loaded but it should be able to open on DOMContentLoaded

Categories

(DevTools :: Inspector, defect, P2)

Product:
DevTools
Component:
Inspector
Type:
defect

Tracking

(firefox53 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 53
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

(Reporter: bgrins, Assigned: ochameau)

References

(Blocks 1 open bug)

Details

(Whiteboard: [polish-backlog][difficulty=hard])

Attachments

(3 files, 1 obsolete file)

Bug 1151909 - Make the inspector actor wait for DOMContentLoaded instead of load.
58 bytes, text/x-review-board-request
pbro
: review+
Details
Bug 1151909 - Fix browser_markup_load_01.js race by listening for events before executing the action that trigger them.
58 bytes, text/x-review-board-request
pbro
: review+
Details
Bug 1151909 - Make the highlighter work on DOMContentLoaded instead of load.
58 bytes, text/x-review-board-request
pbro
: review+
Details 
This makes it feel sluggish on pages that take a long time for the 'load' event to fire.  We should be able to initialize on DOMContentLoaded instead.

To reproduce, open the inspector on a page like https://soundcloud.com/explore, then reload the page.  See how long it takes for the markup view to show back up.
Summary: The markup view doesn't initialize until the page is fully loaded → The markup view doesn't initialize until the page is fully loaded but it should be able to open on DOMContentLoaded
Whiteboard: [devedition-40]
Whiteboard: [devedition-40] → [devedition-40][difficulty=hard]
Setting devedition-40 bugs to p3, filter on FB20EAC4-3FC3-48D9-B15F-0587C3987C25
Priority: -- → P3
See Also: → 1036324
Whiteboard: [devedition-40][difficulty=hard] → [polish-backlog][difficulty=hard]
Blocks: 1208350
(In reply to Brian Grinstead [:bgrins] from comment #2)
> The tab watcher in firebug might be useful:
> https://getfirebug.com/developer/api/firebug1.5/symbols/src/
> content_firebug_tabWatcher.js.html

The current code for this can be found here:
https://github.com/firebug/firebug/blob/master/extension/content/firebug/chrome/tabWatcher.js

Sebastian
See Also: → 1105470
See Also: → 1277348
Moving to P2 (filter on CLIMBING SHOES).
Priority: P3 → P2
Bug 1277348 is going to fix one scenario. When opening the devtools against a still loading page, it will only wait for DOMContentLoaded instead of load.
But inspector is still going to wait for load event if you reload the page after the inspector is opened.
We can listen for window-ready event from the TabActor, which is fired very early, once the Window object is ready, before any JS script from the page is executed and before DOMContentLoaded.
I already sent such patch to try and it seems to be fine, it just highlights one buggy test having a race:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0c18d0713809b9d7cf2f88b8afd54711d3d44f93

I think it is worth testing that. You can see the DOM being created.
But it may also be CPU intensive and not that useful. It may be better to listen for window-ready, but then wait for the DOMContentLoaded event.
Depends on: 1154645
Comment on attachment 8810419 [details]
Bug 1151909 - Wait for DOMContentLoaded

I kept this apart to allow you testing loading the markup view the earliest we can, before <body> is created. But I think we should start we loading the markup-view on DOMContentLoaded. It is less stressful for slow hardware.
We may always come back later to that, optimize the inspector performances and load on window-ready.

Note that it looks like Chrome starts loading its markup view before DOMContentLoaded as we see <body> being created.
(In reply to Alexandre Poirot [:ochameau] from comment #17)
> Comment on attachment 8810419 [details]
> Bug 1151909 - Wait for DOMContentLoaded
> 
> I kept this apart to allow you testing loading the markup view the earliest
> we can, before <body> is created. But I think we should start we loading the
> markup-view on DOMContentLoaded. It is less stressful for slow hardware.
> We may always come back later to that, optimize the inspector performances
> and load on window-ready.
> 
> Note that it looks like Chrome starts loading its markup view before
> DOMContentLoaded as we see <body> being created.
I've tested both approaches, and I think that waiting for DOMContentLoaded is a better solution.
Showing the DOM on window-ready is cool because the inspector is immediately showing something, but it's showing something that can't be used yet. Things update all the time, and might actually slow down the loading of the page. Also, the body node doesn't exist yet, so the inspector can't select it which is what it should normally do by default, and the rule-view is empty.
Waiting for DOMContentLoaded preserves that feature (body gets selected and the rule-view shows its rules). And content in the inspector appears way faster than it currently does in dev-edition.
So I vote for this solution.
Comment on attachment 8808631 [details]
Bug 1151909 - Make the inspector actor wait for DOMContentLoaded instead of load.

https://reviewboard.mozilla.org/r/91428/#review93752

The code changes in this patch look good, but consider my comment in the bug about waiting for DOMContentLoaded.
Attachment #8808631 - Flags: review?(pbrosset) → review+
Comment on attachment 8808632 [details]
Bug 1151909 - Fix browser_markup_load_01.js race by listening for events before executing the action that trigger them.

https://reviewboard.mozilla.org/r/91430/#review93754
Attachment #8808632 - Flags: review?(pbrosset) → review+
Attachment #8810419 - Attachment is obsolete: true
Assignee: nobody → poirot.alex
Looks like the markup view also need some tweaks to be also ready earlier on DOMContentLoaded.
With just the first two patches here is what you get:
* open a website
* open the inspector
* load another slow loading website like: lemonde.fr
* click on pick an element
* over elements
* you will see the highlighter change the selected element in the markup view without seeing the node highlighted on the website [<== this is the weird new behavior that doesn't exists today, as the markup view AND the highlighter are not working until load]
* once the website is fully loaded, elements starts being highlighted.
Attachment #8813218 - Flags: review?(pbrosset)
In that last patch I only modified the CanvasFrameAnonymousContentHelper.
There is still various highlighter code based on navigate but I don't know if that's any usefull to modify them? There is still highlighters.js, box-model and css-grid.
Comment on attachment 8813218 [details]
Bug 1151909 - Make the highlighter work on DOMContentLoaded instead of load.

https://reviewboard.mozilla.org/r/94718/#review95548

> In that last patch I only modified the CanvasFrameAnonymousContentHelper.
> There is still various highlighter code based on navigate but I don't know if that's any usefull to modify them?
> There is still highlighters.js, box-model and css-grid.

Yeah, let's keep your patch simple like this and maybe filed bugs for the other highlighters you mentioned. :gl is doing work on the css-grid highlighter now, so it would be good to let him know about this required change in a new bug.
Attachment #8813218 - Flags: review?(pbrosset) → review+
Looks like it may make devtools/client/inspector/test/browser_inspector_search-05.js even more intermittent:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d31c121abd9234ed01f8fbb105857ebb6c73ba62&selectedJob=31829177

I should push again with bug 1316238 to see if that fix it.
Depends on: 1316238
Pushed by apoirot@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7f137d9fa7ba
Make the inspector actor wait for DOMContentLoaded instead of load. r=pbro
https://hg.mozilla.org/integration/autoland/rev/c307daee56b5
Fix browser_markup_load_01.js race by listening for events before executing the action that trigger them. r=pbro
https://hg.mozilla.org/integration/autoland/rev/fb4f5c7e082a
Make the highlighter work on DOMContentLoaded instead of load. r=pbro
No longer blocks: top-inspector-bugs
Depends on: 1333711
Depends on: 1341756
Depends on: 1347761
Depends on: 1365075
Depends on: 1367372
Depends on: 1378133
Depends on: 1414597
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: