Closed
Bug 1151909
Opened 10 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)
DevTools
Inspector
Tracking
(firefox53 fixed)
RESOLVED
FIXED
Firefox 53
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)
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.
Reporter | ||
Comment 1•10 years ago
|
||
This will require changing some of the progress listening code in the progress listener for tab actor:
https://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/webbrowser.js#2299
https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIWebProgressListener
Reporter | ||
Updated•10 years ago
|
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]
Reporter | ||
Comment 2•10 years ago
|
||
The tab watcher in firebug might be useful: https://getfirebug.com/developer/api/firebug1.5/symbols/src/content_firebug_tabWatcher.js.html
Updated•10 years ago
|
Whiteboard: [devedition-40] → [devedition-40][difficulty=hard]
Comment 3•10 years ago
|
||
Setting devedition-40 bugs to p3, filter on FB20EAC4-3FC3-48D9-B15F-0587C3987C25
Priority: -- → P3
Reporter | ||
Updated•10 years ago
|
Updated•9 years ago
|
Whiteboard: [devedition-40][difficulty=hard] → [polish-backlog][difficulty=hard]
Comment 5•9 years ago
|
||
(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
Reporter | ||
Updated•9 years ago
|
Reporter | ||
Updated•8 years ago
|
Updated•8 years ago
|
Blocks: top-inspector-bugs
Assignee | ||
Comment 8•8 years ago
|
||
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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 11•8 years ago
|
||
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.
Assignee | ||
Comment 12•8 years ago
|
||
Assignee | ||
Comment 13•8 years ago
|
||
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 17•8 years ago
|
||
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.
Comment 18•8 years ago
|
||
(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 19•8 years ago
|
||
mozreview-review |
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 20•8 years ago
|
||
mozreview-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+
Assignee | ||
Comment 21•8 years ago
|
||
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Attachment #8810419 -
Attachment is obsolete: true
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → poirot.alex
Assignee | ||
Comment 27•8 years ago
|
||
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.
Assignee | ||
Updated•8 years ago
|
Attachment #8813218 -
Flags: review?(pbrosset)
Assignee | ||
Comment 28•8 years ago
|
||
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 29•8 years ago
|
||
mozreview-review |
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+
Assignee | ||
Comment 30•8 years ago
|
||
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.
Assignee | ||
Comment 31•8 years ago
|
||
Looks like bug 1316238 fixes the test failure:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2bdc719922aac2e02ad398d8fce23ab3f925e853
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 35•8 years ago
|
||
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
Comment 36•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/7f137d9fa7ba
https://hg.mozilla.org/mozilla-central/rev/c307daee56b5
https://hg.mozilla.org/mozilla-central/rev/fb4f5c7e082a
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox53:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Updated•8 years ago
|
No longer blocks: top-inspector-bugs
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•