859042 - Create onload and DOMContentLoaded markers in the timeline

Status: RESOLVED FIXED

(DevTools :: Performance Tools (Profiler/Timeline), defect, P1)

Description

[Victor Porof [:vporof][:vp]]

Along with bug 859041.

Comment 1

[Victor Porof [:vporof][:vp]]

Moving into Developer Tools: Netmonitor component. Filter on NETMONITORAMA.

Comment 2

[Jeff Griffiths (:canuckistani) (:⚡︎)]

Re-adjusting priority - having these markers is critical for letting developers profile page load.

Comment 3

[Victor Porof [:vporof][:vp]]

I can do this if nobody else volunteers.

Comment 4

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(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

Comment 5

[Jordan Santell [:jsantell] [@jsantell]]

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?

Comment 6

[Victor Porof [:vporof][:vp]]

Attachment: v1

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 8

[Jordan Santell [:jsantell] [@jsantell]]

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.

Comment 9

[Jordan Santell [:jsantell] [@jsantell]]

Ah ok I didn't see you pinged smaug for the cpp changes :)

Comment 10

[Victor Porof [:vporof][:vp]]

(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.

Comment 11

[Victor Porof [:vporof][:vp]]

Broke down this bug into two parts. The netmonitor frontend bits are in bug 1218078, next on my list.

Comment 12

[Victor Porof [:vporof][:vp]]

Attachment: v2

Addressed comments.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

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+

Comment 14

[Victor Porof [:vporof][:vp]]

Attachment: v3

Addressed comments.

Comment 15

[Pulsebot]

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

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/502d5eecac97

Comment 17

[Will Bamberg [:wbamberg]]

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?

Comment 18

[Jordan Santell [:jsantell] [@jsantell]]

lgtm!

Comment 19

[Sebastian Zartner [:sebo]]

(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