Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2849593002:
Use is_same_document term instead of is_in_page for metrics. (Closed)
|
Created:
3 years, 7 months ago by Eugene But (OOO till 7-30) Modified:
3 years, 7 months ago CC:
chromium-reviews, asvitkine+watch_chromium.org, estark Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionUse is_same_document term instead of is_in_page for metrics.
RecordMainFrameNavigation receives NavigationHandle->IsSameDocument()
for its second argument, so the argument name should be is_same_page.
Also updated histogram comment to be accurate.
BUG=695189
Review-Url: https://codereview.chromium.org/2849593002
Cr-Commit-Position: refs/heads/master@{#470339}
Committed: https://chromium.googlesource.com/chromium/src/+/4645ee94b0d729532c32eb504e7e1fe400206548
Patch Set 1 #Patch Set 2 : Self review #Patch Set 3 : Updated comments #Patch Set 4 : Self review #Patch Set 5 : s/changes/replaces #
Total comments: 2
Patch Set 6 : Addressed review comment #
Messages
Total messages: 29 (14 generated)
eugenebut@chromium.org changed reviewers: + davidben@chromium.org, estark@chromium.org
The CQ bit was checked by eugenebut@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
estark@chromium.org changed reviewers: + elawrence@chromium.org
elawrence, could you please review this in my stead? Looks reasonable to me but since you added these metrics recently you might have opinions.
On 2017/05/01 18:02:06, estark wrote:
> elawrence, could you please review this in my stead? Looks reasonable to me
but
> since you added these metrics recently you might have opinions.
The code change itself looks fine.
The comment change looks wrong to me; the wording is somewhat less clear ("goes
to a document" seems more clear than "changes a document") and the
parentheticals seem to be missing words (it lists the types of navigations that
are /excluded/, not included).
On 2017/05/01 18:19:36, elawrence wrote:
> On 2017/05/01 18:02:06, estark wrote:
> > elawrence, could you please review this in my stead? Looks reasonable to me
> but
> > since you added these metrics recently you might have opinions.
>
> The code change itself looks fine.
>
> The comment change looks wrong to me; the wording is somewhat less clear
("goes
> to a document" seems more clear than "changes a document") and the
> parentheticals seem to be missing words (it lists the types of navigations
that
> are /excluded/, not included).
Thanks! Updated the comments. I used "changes a document object" instead of
"goes to a document", because it's what navigation does (changes window.document
object). PTAL.
On 2017/05/08 18:55:10, Eugene But (OOO till May 8) wrote:
> On 2017/05/01 18:19:36, elawrence wrote:
> > On 2017/05/01 18:02:06, estark wrote:
> > > elawrence, could you please review this in my stead? Looks reasonable to
me
> > but
> > > since you added these metrics recently you might have opinions.
> >
> > The code change itself looks fine.
> >
> > The comment change looks wrong to me; the wording is somewhat less clear
> ("goes
> > to a document" seems more clear than "changes a document") and the
> > parentheticals seem to be missing words (it lists the types of navigations
> that
> > are /excluded/, not included).
> Thanks! Updated the comments. I used "changes a document object" instead of
> "goes to a document", because it's what navigation does (changes
window.document
> object). PTAL.
LGTM, thanks for restoring the exceptions list.
I'm still tripping over the word "changes"; would "replaces" be more accurate?
The reason I ask is that the document object still "changes" on a excluded
same-page navigations (e.g. the scroll position and various DOM properties
likely change), and we're not changing the object per-se, we're unloading it and
replacing it with a different one.
lgtm
On 2017/05/08 19:00:23, elawrence wrote:
> On 2017/05/08 18:55:10, Eugene But (OOO till May 8) wrote:
> > On 2017/05/01 18:19:36, elawrence wrote:
> > > On 2017/05/01 18:02:06, estark wrote:
> > > > elawrence, could you please review this in my stead? Looks reasonable to
> me
> > > but
> > > > since you added these metrics recently you might have opinions.
> > >
> > > The code change itself looks fine.
> > >
> > > The comment change looks wrong to me; the wording is somewhat less clear
> > ("goes
> > > to a document" seems more clear than "changes a document") and the
> > > parentheticals seem to be missing words (it lists the types of navigations
> > that
> > > are /excluded/, not included).
> > Thanks! Updated the comments. I used "changes a document object" instead of
> > "goes to a document", because it's what navigation does (changes
> window.document
> > object). PTAL.
>
> LGTM, thanks for restoring the exceptions list.
>
> I'm still tripping over the word "changes"; would "replaces" be more accurate?
> The reason I ask is that the document object still "changes" on a excluded
> same-page navigations (e.g. the scroll position and various DOM properties
> likely change), and we're not changing the object per-se, we're unloading it
and
> replacing it with a different one.
Sure. Replaced "changes" with "replaces".
The CQ bit was checked by eugenebut@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from elawrence@chromium.org, davidben@chromium.org Link to the patchset: https://codereview.chromium.org/2849593002/#ps80001 (title: "s/changes/replaces")
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...)
On 2017/05/08 20:21:01, commit-bot: I haz the power wrote: > 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...) fwiw, mpearson@ reviewed my last update to these comments.
eugenebut@chromium.org changed reviewers: + isherman@chromium.org
+isherman@ for histograms.xml
Metrics LGTM w/ a nit: https://codereview.chromium.org/2849593002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2849593002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:31961: + pushState/replaceState, same page history navigation. nit: s/same/or same (ditto below)
Thanks everyone! https://codereview.chromium.org/2849593002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2849593002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:31961: + pushState/replaceState, same page history navigation. On 2017/05/09 03:46:56, Ilya Sherman wrote: > nit: s/same/or same (ditto below) Done.
The CQ bit was checked by eugenebut@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from elawrence@chromium.org, isherman@chromium.org, davidben@chromium.org Link to the patchset: https://codereview.chromium.org/2849593002/#ps100001 (title: "Addressed review comment")
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": 100001, "attempt_start_ts": 1494336330727180,
"parent_rev": "915256dfeb6b6f87c5b2d145c08fecfcf247553b", "commit_rev":
"4645ee94b0d729532c32eb504e7e1fe400206548"}
Message was sent while issue was closed.
Description was changed from ========== Use is_same_document term instead of is_in_page for metrics. RecordMainFrameNavigation receives NavigationHandle->IsSameDocument() for its second argument, so the argument name should be is_same_page. Also updated histogram comment to be accurate. BUG=695189 ========== to ========== Use is_same_document term instead of is_in_page for metrics. RecordMainFrameNavigation receives NavigationHandle->IsSameDocument() for its second argument, so the argument name should be is_same_page. Also updated histogram comment to be accurate. BUG=695189 Review-Url: https://codereview.chromium.org/2849593002 Cr-Commit-Position: refs/heads/master@{#470339} Committed: https://chromium.googlesource.com/chromium/src/+/4645ee94b0d729532c32eb504e7e... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/4645ee94b0d729532c32eb504e7e... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
