Create onload and DOMContentLoaded markers in the timeline

RESOLVED FIXED in Firefox 44

Status

()

Firefox
Developer Tools: Performance Tools (Profiler/Timeline)
P1
normal
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: vporof, Assigned: vporof)

Tracking

(Blocks: 2 bugs, {dev-doc-complete})

unspecified
Firefox 44
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox44 fixed)

Details

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

Attachments

(3 attachments)

(Assignee)

Description

5 years ago
Along with bug 859041.
(Assignee)

Updated

5 years ago
Summary: [netmonitor] Show onload and DOMContentReady markers in the timeline → [netmonitor] Show onload and DOMContentLoaded markers in the timeline
(Assignee)

Comment 1

5 years ago
Moving into Developer Tools: Netmonitor component. Filter on NETMONITORAMA.
Component: Developer Tools → Developer Tools: Netmonitor
Summary: [netmonitor] Show onload and DOMContentLoaded markers in the timeline → Show onload and DOMContentLoaded markers in the timeline
(Assignee)

Updated

5 years ago
Priority: -- → P2
(Assignee)

Updated

5 years ago
See Also: → bug 859045
Whiteboard: [devedition-40]
Blocks: 1174091

Updated

2 years ago
Blocks: 991806
Whiteboard: [devedition-40] → [polish-backlog]
Re-adjusting priority - having these markers is critical for letting developers profile page load.
Priority: P2 → P1
(Assignee)

Comment 3

2 years ago
I can do this if nobody else volunteers.
(In reply to Victor Porof [:vporof][:vp] from comment #3)
> I can do this if nobody else volunteers.

Excellent. As soon as this is fixed I can fix bug 1174091

Honza
Throwing this at you Victor since no one else checking it out -- I assumed this was for performance tools, but wonder if whatever we use for network here we could also use for perf?
Assignee: nobody → vporof
Blocks: 1210159
(Assignee)

Updated

2 years ago
Whiteboard: [polish-backlog] → [polish-backlog] [difficulty=hard]
(Assignee)

Comment 6

2 years ago
Created attachment 8678116 [details] [diff] [review]
v1
Attachment #8678116 - Flags: review?(jsantell)
Attachment #8678116 - Flags: review?(bugs)

Comment 7

2 years ago
Comment on attachment 8678116 [details] [diff] [review]
v1

> nsDocShell::EndPageLoad(nsIWebProgress* aProgress,
>                         nsIChannel* aChannel, nsresult aStatus)
> {
>   if (!aChannel) {
>     return NS_ERROR_NULL_POINTER;
>   }
> 
>   nsCOMPtr<nsIURI> url;
>   nsresult rv = aChannel->GetURI(getter_AddRefs(url));
>   if (NS_FAILED(rv)) {
>     return rv;
>   }
> 
>+  RefPtr<TimelineConsumers> timelines = TimelineConsumers::Get();
>+
>+  if (timelines && timelines->HasConsumer(this)) {
>+    timelines->AddMarkerForDocShell(
>+      this, "document::Load", MarkerTracingType::TIMESTAMP);
>+  }
Why is this here and not in the place where we actually dispatch load event?
I would move the code to nsDocumentViewer::LoadComplete, unless there is some strong reason to put it to docshell.

That would make it also clear whether the code runs in case we're coming from bfcache, since we check there if (!restoring) {



I wonder if we could add a static helper method to TimelineConsumers which
then calls Get() and AddMarkerForDocShell if needed.
Attachment #8678116 - Flags: review?(bugs) → review-
Comment on attachment 8678116 [details] [diff] [review]
v1

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

r+ with css variable name change and moving the marker test with the other marker tests, and match that structure (it's mostly boilerplate).

You should probably have someone else review the cpp changes

::: devtools/client/performance/modules/markers.js
@@ +93,5 @@
>    },
> +  "document::DOMContentLoaded": {
> +    group: 1,
> +    colorName: "graphs-doc-domcontentloaded",
> +    label: "DOMContentLoaded"

I guess we don't need to L10N these.

::: devtools/client/performance/test/browser_timeline-waterfall-load-markers.js
@@ +1,5 @@
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +/**
> + * Tests if the sidebar is updated with "DOMContentLoaded" and "load" markers.

This test isn't testing timeline/waterfall/sidebar anything, it's testing just marker creation -- should go with other tests in `devtools/server/tests/browser/browser_markers*`

::: devtools/client/themes/variables.css
@@ +51,5 @@
>    --theme-graphs-orange: #d97e00;
>    --theme-graphs-red: #e57180;
>    --theme-graphs-grey: #cccccc;
> +  --theme-graphs-doc-domcontentloaded: #00f;
> +  --theme-graphs-doc-load: #f00;

I think calling these `--theme-graphs-full-red` or something is more consistent, as the other styles don't have any semantic meanings (as they are rather arbitrary) -- also, just because chrome uses hard blue and hard red, doesn't mean we have to do the same for our markers, but maybe this doesn't matter if we move it to being a "line" anyway.
Attachment #8678116 - Flags: review?(jsantell) → review+
Ah ok I didn't see you pinged smaug for the cpp changes :)
(Assignee)

Comment 10

2 years ago
(In reply to Olli Pettay [:smaug] from comment #7)
> Comment on attachment 8678116 [details] [diff] [review]
> v1
> 
> > nsDocShell::EndPageLoad(nsIWebProgress* aProgress,
> >                         nsIChannel* aChannel, nsresult aStatus)
> > {
> >   if (!aChannel) {
> >     return NS_ERROR_NULL_POINTER;
> >   }
> > 
> >   nsCOMPtr<nsIURI> url;
> >   nsresult rv = aChannel->GetURI(getter_AddRefs(url));
> >   if (NS_FAILED(rv)) {
> >     return rv;
> >   }
> > 
> >+  RefPtr<TimelineConsumers> timelines = TimelineConsumers::Get();
> >+
> >+  if (timelines && timelines->HasConsumer(this)) {
> >+    timelines->AddMarkerForDocShell(
> >+      this, "document::Load", MarkerTracingType::TIMESTAMP);
> >+  }
> Why is this here and not in the place where we actually dispatch load event?
> I would move the code to nsDocumentViewer::LoadComplete, unless there is
> some strong reason to put it to docshell.
> 
> That would make it also clear whether the code runs in case we're coming
> from bfcache, since we check there if (!restoring) {
> 
> 

The only reason is that I didn't know where we actually dispatched the load event :) I browsed the code for a while and found something that looked like it does what I want it to do.

Indeed nsDocumentViewer::LoadComplete is a much better place.

> 
> I wonder if we could add a static helper method to TimelineConsumers which
> then calls Get() and AddMarkerForDocShell if needed.

It does seem like it's a fairly frequently used method, but I'm not sure giving it "preferential treatment" is that much nicer.
(Assignee)

Updated

2 years ago
Summary: Show onload and DOMContentLoaded markers in the timeline → Create onload and DOMContentLoaded markers in the timeline
Whiteboard: [polish-backlog] [difficulty=hard] → [polish-backlog] [difficulty=medium]
(Assignee)

Updated

2 years ago
Blocks: 1218078
(Assignee)

Comment 11

2 years ago
Broke down this bug into two parts. The netmonitor frontend bits are in bug 1218078, next on my list.
No longer blocks: 1218078
(Assignee)

Comment 12

2 years ago
Created attachment 8678466 [details] [diff] [review]
v2

Addressed comments.
Attachment #8678466 - Flags: review?(bugs)
Comment on attachment 8678466 [details] [diff] [review]
v2

It is not clear to me what document::Load is about if we notify this way.
Do we want to notify also when we just dispatch pageshow and not load (when coming from bfcache)

There is the comment
"+A marker generated when the document's "load" event is fired."
which hints the patch isn't right
but the code in DocumentViewer should be moved to be somewhere before
EventDispatcher::Dispatch(window, mPresContext, &event, nullptr, &status);
And when moving the code there, no need to null check window.
And because SetReadyStateInternal may destroy worlds, add the code before that and then
you can also reuse the existing docShell variable.

With that, r+
Attachment #8678466 - Flags: review?(bugs) → review+
(Assignee)

Comment 14

2 years ago
Created attachment 8679388 [details] [diff] [review]
v3

Addressed comments.
Attachment #8679388 - Flags: review+

Comment 15

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/502d5eecac97

Comment 16

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/502d5eecac97
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
Keywords: dev-doc-needed
I've added these to the list of markers: https://developer.mozilla.org/en-US/docs/Tools/Performance/Waterfall#Markers

Do we need anything else here?
Flags: needinfo?(jsantell)
lgtm!
Flags: needinfo?(jsantell)
Blocks: 1233321
Depends on: 1233323
Depends on: 1233325
Component: Developer Tools: Netmonitor → Developer Tools: Performance Tools (Profiler/Timeline)
Keywords: dev-doc-needed → dev-doc-complete
(In reply to Will Bamberg [:wbamberg] from comment #17)
> I've added these to the list of markers:
> https://developer.mozilla.org/en-US/docs/Tools/Performance/Waterfall#Markers

Please note bug 1233325.

Also, testing with Nightly 2015-12-16 it sometimes happens that the two events are triggered at the same time according to the performance profile, which looks somewhat suspicious to me, and sometimes they are not recorded at all.
Though I assume both are bugs of the Performance panel. Can you reproduce this? Should I file separate bugs for those?

Sebastian

Updated

2 years ago
No longer blocks: 1233321
No longer depends on: 1233323
You need to log in before you can comment on or make changes to this bug.