Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2724943003:
Add a trace event in FrameLoader::updateForSameDocumentNavigation. (Closed)
DescriptionAdd a trace event in FrameLoader::updateForSameDocumentNavigation.
Currently, we are exposing History::pushState and Location::setHash
in tracing. These two events are seen as the indicators of a spa
navigation.
However, we found out that FrameLoader::updateForSameDocumentNavigation
is the central point for same document navigation (aka spa navigation).
Therefore, we can remove trace events in History::pushState and
Location::setHash and add a trace event in the central point instead.
BUG=697617
Review-Url: https://codereview.chromium.org/2724943003
Cr-Commit-Position: refs/heads/master@{#455236}
Committed: https://chromium.googlesource.com/chromium/src/+/883d0479494ebd6c5351af89cd575da2e2127030
Patch Set 1 #Patch Set 2 : format #Patch Set 3 : sync #Patch Set 4 : Add a trace event in FrameLoader::updateForSameDocumentNavigation #Patch Set 5 : sync #
Messages
Total messages: 45 (20 generated)
Description was changed from ========== Expose trace event url info. Currently, we aren't exposing any arguments in trace events: History::pushState and Location::setHash. We want to include the url info when we generate a breakdown for a SPA navigation metric. BUG=697617 ========== to ========== Expose trace event url info. Currently, we aren't exposing any arguments in trace events: History::pushState and Location::setHash. We want to include the url info when we generate a breakdown for a SPA navigation metric. Therefore, we are exposing the url for History::pushState and Location::setHash in this cl. BUG=697617 ==========
sunjian@chromium.org changed reviewers: + tdresser@chromium.org
Expose url info for History::pushState and Location::setHash events.
What do we need this for? We should be able to look at the blink user_timing NavigationStart slice proceeding the SPA nav to get the URL (without the hash etc). When do we need the URL including the hash?
On 2017/03/02 14:57:05, tdresser wrote: > What do we need this for? We should be able to look at the blink user_timing > NavigationStart slice proceeding the SPA nav to get the URL (without the hash > etc). When do we need the URL including the hash? I thought it would be nice to include the url information for each in-app navigation when generating the breakdown thing we talked about implementing. The intent is to capture the url when in-app navigation happens.
On 2017/03/02 17:49:00, sunjian wrote: > On 2017/03/02 14:57:05, tdresser wrote: > > What do we need this for? We should be able to look at the blink user_timing > > NavigationStart slice proceeding the SPA nav to get the URL (without the hash > > etc). When do we need the URL including the hash? > > I thought it would be nice to include the url information for each in-app > navigation when generating the breakdown thing we talked about implementing. > The intent is to capture the url when in-app navigation happens. How would this be exposed in the visualization of the breakdown?
On 2017/03/02 18:48:40, tdresser wrote: > On 2017/03/02 17:49:00, sunjian wrote: > > On 2017/03/02 14:57:05, tdresser wrote: > > > What do we need this for? We should be able to look at the blink user_timing > > > NavigationStart slice proceeding the SPA nav to get the URL (without the > hash > > > etc). When do we need the URL including the hash? > > > > I thought it would be nice to include the url information for each in-app > > navigation when generating the breakdown thing we talked about implementing. > > The intent is to capture the url when in-app navigation happens. > > How would this be exposed in the visualization of the breakdown? I expose it through here so that i can extract the url from the url update slice like History::pushState and Loccation::setHash. I think what they did in loading_metric, the example you gave me, was they exposed the url as a metadata in the breakdown. We don't have to have this extra information i think. I thought it would be nice to add that if not too much work. If you go to https://cs.chromium.org/chromium/src/third_party/catapult/tracing/tracing/met... and search for 'Navigation infos' literal, you can see what they did exactly.
LGTM, thanks for clarifying what this would be used for.
The CQ bit was checked by sunjian@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by sunjian@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tdresser@chromium.org Link to the patchset: https://codereview.chromium.org/2724943003/#ps20001 (title: "format")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by sunjian@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tdresser@chromium.org Link to the patchset: https://codereview.chromium.org/2724943003/#ps40001 (title: "sync")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
sunjian@chromium.org changed reviewers: + japhet@chromium.org
Hey Nate, Can you give a quick review of this cl please? Jian
This is a subset of blink's definition of a same-page navigation. Do you want to
include other same-page navigations ("location = #foo", history.replaceState(),
history.back()/forward() or user-initiated back/forward navigation that is
same-page)? Or is there something special about these 2 cases?
On 2017/03/03 23:06:54, Nate Chapin wrote:
> This is a subset of blink's definition of a same-page navigation. Do you want
to
> include other same-page navigations ("location = #foo",
history.replaceState(),
> history.back()/forward() or user-initiated back/forward navigation that is
> same-page)? Or is there something special about these 2 cases?
Yes, i only modified these two cases because right now we can only identify
these two
as indicators for a same page navigation. And i am writing a TBMv2 metric for
same page
navigation. This change is for providing url information for generating a
breakdown. If
you are interested in know more about the background of the project. Here are
two docs
that can more thoroughly explain it:
https://docs.google.com/a/chromium.org/document/d/1otSutPfYQ20L-cyTY6QRWxcyAa...
https://docs.google.com/document/d/1B44Aqym4rRNlghlMNzSR2clIZhdvVyhQ2LMmvRKEk...
Let me know if you have more questions.
On 2017/03/03 23:22:16, sunjian wrote:
> On 2017/03/03 23:06:54, Nate Chapin wrote:
> > This is a subset of blink's definition of a same-page navigation. Do you
want
> to
> > include other same-page navigations ("location = #foo",
> history.replaceState(),
> > history.back()/forward() or user-initiated back/forward navigation that is
> > same-page)? Or is there something special about these 2 cases?
>
> Yes, i only modified these two cases because right now we can only identify
> these two
> as indicators for a same page navigation. And i am writing a TBMv2 metric for
> same page
> navigation. This change is for providing url information for generating a
> breakdown. If
> you are interested in know more about the background of the project. Here are
> two docs
> that can more thoroughly explain it:
>
https://docs.google.com/a/chromium.org/document/d/1otSutPfYQ20L-cyTY6QRWxcyAa...
>
>
https://docs.google.com/document/d/1B44Aqym4rRNlghlMNzSR2clIZhdvVyhQ2LMmvRKEk...
>
> Let me know if you have more questions.
The canonical definition of a same page navigation in blink is that it goes
through FrameLoader::updateForSameDocumentNavigation. Is there a specific reason
to trace these specific API calls rather than that central point for same page
navigations?
I guess my real question is: Do you have/need a different definition of "same
page navigation" than blink's internal one? If so, that's fine, I just didn't
immediately see that from those docs in my quick read.
On 2017/03/03 23:28:37, Nate Chapin wrote:
> On 2017/03/03 23:22:16, sunjian wrote:
> > On 2017/03/03 23:06:54, Nate Chapin wrote:
> > > This is a subset of blink's definition of a same-page navigation. Do you
> want
> > to
> > > include other same-page navigations ("location = #foo",
> > history.replaceState(),
> > > history.back()/forward() or user-initiated back/forward navigation that is
> > > same-page)? Or is there something special about these 2 cases?
> >
> > Yes, i only modified these two cases because right now we can only identify
> > these two
> > as indicators for a same page navigation. And i am writing a TBMv2 metric
for
> > same page
> > navigation. This change is for providing url information for generating a
> > breakdown. If
> > you are interested in know more about the background of the project. Here
are
> > two docs
> > that can more thoroughly explain it:
> >
>
https://docs.google.com/a/chromium.org/document/d/1otSutPfYQ20L-cyTY6QRWxcyAa...
> >
> >
>
https://docs.google.com/document/d/1B44Aqym4rRNlghlMNzSR2clIZhdvVyhQ2LMmvRKEk...
> >
> > Let me know if you have more questions.
>
> The canonical definition of a same page navigation in blink is that it goes
> through FrameLoader::updateForSameDocumentNavigation. Is there a specific
reason
> to trace these specific API calls rather than that central point for same page
> navigations?
>
> I guess my real question is: Do you have/need a different definition of "same
> page navigation" than blink's internal one? If so, that's fine, I just didn't
> immediately see that from those docs in my quick read.
Great catch Nate! We were struggling to find a definition of a same document
navigation in blink. We didn't know there is.
We can put this patch off for a bit. I'll talk to Tim next Monday and have a
discussion about this and make plan for next step.
But thanks for the information. I'll let you know when we need your further
review.
On 2017/03/04 00:00:46, sunjian wrote:
> On 2017/03/03 23:28:37, Nate Chapin wrote:
> > On 2017/03/03 23:22:16, sunjian wrote:
> > > On 2017/03/03 23:06:54, Nate Chapin wrote:
> > > > This is a subset of blink's definition of a same-page navigation. Do you
> > want
> > > to
> > > > include other same-page navigations ("location = #foo",
> > > history.replaceState(),
> > > > history.back()/forward() or user-initiated back/forward navigation that
is
> > > > same-page)? Or is there something special about these 2 cases?
> > >
> > > Yes, i only modified these two cases because right now we can only
identify
> > > these two
> > > as indicators for a same page navigation. And i am writing a TBMv2 metric
> for
> > > same page
> > > navigation. This change is for providing url information for generating a
> > > breakdown. If
> > > you are interested in know more about the background of the project. Here
> are
> > > two docs
> > > that can more thoroughly explain it:
> > >
> >
>
https://docs.google.com/a/chromium.org/document/d/1otSutPfYQ20L-cyTY6QRWxcyAa...
> > >
> > >
> >
>
https://docs.google.com/document/d/1B44Aqym4rRNlghlMNzSR2clIZhdvVyhQ2LMmvRKEk...
> > >
> > > Let me know if you have more questions.
> >
> > The canonical definition of a same page navigation in blink is that it goes
> > through FrameLoader::updateForSameDocumentNavigation. Is there a specific
> reason
> > to trace these specific API calls rather than that central point for same
page
> > navigations?
> >
> > I guess my real question is: Do you have/need a different definition of
"same
> > page navigation" than blink's internal one? If so, that's fine, I just
didn't
> > immediately see that from those docs in my quick read.
>
> Great catch Nate! We were struggling to find a definition of a same document
> navigation in blink. We didn't know there is.
> We can put this patch off for a bit. I'll talk to Tim next Monday and have a
> discussion about this and make plan for next step.
> But thanks for the information. I'll let you know when we need your further
> review.
Switching to only trace FrameLoader::updateForSameDocumentNavigation SGTM.
Description was changed from ========== Expose trace event url info. Currently, we aren't exposing any arguments in trace events: History::pushState and Location::setHash. We want to include the url info when we generate a breakdown for a SPA navigation metric. Therefore, we are exposing the url for History::pushState and Location::setHash in this cl. BUG=697617 ========== to ========== Add a trace event in FrameLoader::updateForSameDocumentNavigation. Currently, we are exposing History::pushState and Location::setHash in tracing. These two events are seen as the indicators of a spa navigation. However, we found out that FrameLoader::updateForSameDocumentNavigation is the central point for same document navigation (aka spa navigation). Therefore, we can remove trace events in History::pushState and Location::setHash and add a trace event in the central point. BUG=697617 ==========
Description was changed from ========== Add a trace event in FrameLoader::updateForSameDocumentNavigation. Currently, we are exposing History::pushState and Location::setHash in tracing. These two events are seen as the indicators of a spa navigation. However, we found out that FrameLoader::updateForSameDocumentNavigation is the central point for same document navigation (aka spa navigation). Therefore, we can remove trace events in History::pushState and Location::setHash and add a trace event in the central point. BUG=697617 ========== to ========== Add a trace event in FrameLoader::updateForSameDocumentNavigation. Currently, we are exposing History::pushState and Location::setHash in tracing. These two events are seen as the indicators of a spa navigation. However, we found out that FrameLoader::updateForSameDocumentNavigation is the central point for same document navigation (aka spa navigation). Therefore, we can remove trace events in History::pushState and Location::setHash and add a trace event in the central point instead. BUG=697617 ==========
I removed the trace events in History::pushState and Location::setHash. And added one in FrameLoader::updateForSameDocumentNavigation instead. Please take another pass! Thanks!
lgtm
The CQ bit was checked by sunjian@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tdresser@chromium.org Link to the patchset: https://codereview.chromium.org/2724943003/#ps60001 (title: "Add a trace event in FrameLoader::updateForSameDocumentNavigation")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Still LGTM.
The CQ bit was checked by sunjian@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from japhet@chromium.org, tdresser@chromium.org Link to the patchset: https://codereview.chromium.org/2724943003/#ps80001 (title: "sync")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1488915282937010,
"parent_rev": "11e9bad5ae19ce1739d95cca9e714c96e5ae6bcd", "commit_rev":
"883d0479494ebd6c5351af89cd575da2e2127030"}
Message was sent while issue was closed.
Description was changed from ========== Add a trace event in FrameLoader::updateForSameDocumentNavigation. Currently, we are exposing History::pushState and Location::setHash in tracing. These two events are seen as the indicators of a spa navigation. However, we found out that FrameLoader::updateForSameDocumentNavigation is the central point for same document navigation (aka spa navigation). Therefore, we can remove trace events in History::pushState and Location::setHash and add a trace event in the central point instead. BUG=697617 ========== to ========== Add a trace event in FrameLoader::updateForSameDocumentNavigation. Currently, we are exposing History::pushState and Location::setHash in tracing. These two events are seen as the indicators of a spa navigation. However, we found out that FrameLoader::updateForSameDocumentNavigation is the central point for same document navigation (aka spa navigation). Therefore, we can remove trace events in History::pushState and Location::setHash and add a trace event in the central point instead. BUG=697617 Review-Url: https://codereview.chromium.org/2724943003 Cr-Commit-Position: refs/heads/master@{#455236} Committed: https://chromium.googlesource.com/chromium/src/+/883d0479494ebd6c5351af89cd57... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/883d0479494ebd6c5351af89cd57... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
