|
|
DescriptionSeperate reloads when reporting previews infobar dismissal
When the previews infobar is dismissed, it would be valuable to track
reloads differently from new navigations. Reloads are not treated as
opt outs by the blacklisting policy, so the page could be served as a new LoFi page or LitePage after a reload
(offline previews are specifically prevented from being shown on any
reload however).
If the user dismisses the infobar and then reloads, this will only be captured as the user dismissing the infobar.
BUG=672634
Committed: https://crrev.com/e0be759f29bd8acd4b83da1aa3e49c645da01ef3
Cr-Commit-Position: refs/heads/master@{#439895}
Patch Set 1 #
Total comments: 4
Patch Set 2 : megjablon comments #
Total comments: 2
Patch Set 3 : holte nit #Patch Set 4 : rebase #Patch Set 5 : rebase error fix #Patch Set 6 : rebase fix #
Messages
Total messages: 52 (37 generated)
The CQ bit was checked by ryansturm@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...
Description was changed from ========== Seperate reloads when reporting previews infobar dismissal When the previews infobar is dismissed, it would be valuable to track reloads differently from new navigations, as reloads are not treated as opt outs, so the page could be served as a new LoFi page or LitePage (offline previews are specifically prevented from being shown on any reload however). BUG= ========== to ========== Seperate reloads when reporting previews infobar dismissal When the previews infobar is dismissed, it would be valuable to track reloads differently from new navigations, as reloads are not treated as opt outs, so the page could be served as a new LoFi page or LitePage (offline previews are specifically prevented from being shown on any reload however). If the user dismisses the infobar and then reloads, this will only be captured as the user dismissing the infobar. BUG= ==========
Description was changed from ========== Seperate reloads when reporting previews infobar dismissal When the previews infobar is dismissed, it would be valuable to track reloads differently from new navigations, as reloads are not treated as opt outs, so the page could be served as a new LoFi page or LitePage (offline previews are specifically prevented from being shown on any reload however). If the user dismisses the infobar and then reloads, this will only be captured as the user dismissing the infobar. BUG= ========== to ========== Seperate reloads when reporting previews infobar dismissal When the previews infobar is dismissed, it would be valuable to track reloads differently from new navigations, as reloads are not treated as opt outs, so the page could be served as a new LoFi page or LitePage (offline previews are specifically prevented from being shown on any reload however). If the user dismisses the infobar and then reloads, this will only be captured as the user dismissing the infobar. BUG=672634 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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_...)
The CQ bit was checked by ryansturm@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: This issue passed the CQ dry run.
ryansturm@chromium.org changed reviewers: + megjablon@chromium.org
megjablon: PTAL, I think this will be valuable for looking at users who recognize that they want a non-preview page, but might refresh the page instead of using the previews UI. We should be able to compare the ratio of reloads to new navigations using PageLoad.PaintTiming.NavigationToFirstContentfulPaint.LoadType.Reload and PageLoad.PaintTiming.NavigationToFirstContentfulPaint.LoadType.NewNavigation as a rough baselines of reload to new navigation ratios. (e.g. If we see that >10-25% of the navigations from a previews page are reloads not using the UI, there is probably a problem).
https://codereview.chromium.org/2557113004/diff/1/chrome/browser/previews/pre... File chrome/browser/previews/previews_infobar_delegate.cc (right): https://codereview.chromium.org/2557113004/diff/1/chrome/browser/previews/pre... chrome/browser/previews/previews_infobar_delegate.cc:106: RecordPreviewsInfoBarAction( Add unit test coverage.
Can you break up the long one sentence paragraph in the description? :) https://codereview.chromium.org/2557113004/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2557113004/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:98720: + <int value="3" label="Infobar dismissed by new navigation"/> Nit: idk that new is necessary here
The CQ bit was checked by ryansturm@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...
Description was changed from ========== Seperate reloads when reporting previews infobar dismissal When the previews infobar is dismissed, it would be valuable to track reloads differently from new navigations, as reloads are not treated as opt outs, so the page could be served as a new LoFi page or LitePage (offline previews are specifically prevented from being shown on any reload however). If the user dismisses the infobar and then reloads, this will only be captured as the user dismissing the infobar. BUG=672634 ========== to ========== Seperate reloads when reporting previews infobar dismissal When the previews infobar is dismissed, it would be valuable to track reloads differently from new navigations. Reloads are not treated as opt outs by the blacklisting policy, so the page could be served as a new LoFi page or LitePage after a reload (offline previews are specifically prevented from being shown on any reload however). If the user dismisses the infobar and then reloads, this will only be captured as the user dismissing the infobar. BUG=672634 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
megjablon: PTAL https://codereview.chromium.org/2557113004/diff/1/chrome/browser/previews/pre... File chrome/browser/previews/previews_infobar_delegate.cc (right): https://codereview.chromium.org/2557113004/diff/1/chrome/browser/previews/pre... chrome/browser/previews/previews_infobar_delegate.cc:106: RecordPreviewsInfoBarAction( On 2016/12/09 17:04:42, Ryan Sturm wrote: > Add unit test coverage. Done. https://codereview.chromium.org/2557113004/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2557113004/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:98720: + <int value="3" label="Infobar dismissed by new navigation"/> On 2016/12/09 20:31:19, megjablon wrote: > Nit: idk that new is necessary here Done.
lgtm
The CQ bit was checked by ryansturm@chromium.org
The CQ bit was unchecked by ryansturm@chromium.org
ryansturm@chromium.org changed reviewers: + holte@chromium.org
holte: PTAL
lgtm https://codereview.chromium.org/2557113004/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2557113004/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:98721: + <int value="4" label="Infboar dismissed by reload"/> Infboar -> Infobar
Didn't see you were OOO. Thanks for the review! https://codereview.chromium.org/2557113004/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2557113004/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:98721: + <int value="4" label="Infboar dismissed by reload"/> On 2016/12/19 22:50:26, Steven Holte (OOO till Dec 27) wrote: > Infboar -> Infobar Done.
The CQ bit was checked by ryansturm@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from holte@chromium.org, megjablon@chromium.org Link to the patchset: https://codereview.chromium.org/2557113004/#ps40001 (title: "holte nit")
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: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by ryansturm@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from holte@chromium.org, megjablon@chromium.org Link to the patchset: https://codereview.chromium.org/2557113004/#ps60001 (title: "rebase")
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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by ryansturm@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: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by ryansturm@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 ryansturm@chromium.org
The CQ bit was checked by ryansturm@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from holte@chromium.org, megjablon@chromium.org Link to the patchset: https://codereview.chromium.org/2557113004/#ps100001 (title: "rebase fix")
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": 1482271687422810, "parent_rev": "d5fdf6971384f684b704776c880b3c81a2704155", "commit_rev": "45440bb3c14c0dc44a06d704142924a95d062e1f"}
Message was sent while issue was closed.
Description was changed from ========== Seperate reloads when reporting previews infobar dismissal When the previews infobar is dismissed, it would be valuable to track reloads differently from new navigations. Reloads are not treated as opt outs by the blacklisting policy, so the page could be served as a new LoFi page or LitePage after a reload (offline previews are specifically prevented from being shown on any reload however). If the user dismisses the infobar and then reloads, this will only be captured as the user dismissing the infobar. BUG=672634 ========== to ========== Seperate reloads when reporting previews infobar dismissal When the previews infobar is dismissed, it would be valuable to track reloads differently from new navigations. Reloads are not treated as opt outs by the blacklisting policy, so the page could be served as a new LoFi page or LitePage after a reload (offline previews are specifically prevented from being shown on any reload however). If the user dismisses the infobar and then reloads, this will only be captured as the user dismissing the infobar. BUG=672634 Review-Url: https://codereview.chromium.org/2557113004 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Seperate reloads when reporting previews infobar dismissal When the previews infobar is dismissed, it would be valuable to track reloads differently from new navigations. Reloads are not treated as opt outs by the blacklisting policy, so the page could be served as a new LoFi page or LitePage after a reload (offline previews are specifically prevented from being shown on any reload however). If the user dismisses the infobar and then reloads, this will only be captured as the user dismissing the infobar. BUG=672634 Review-Url: https://codereview.chromium.org/2557113004 ========== to ========== Seperate reloads when reporting previews infobar dismissal When the previews infobar is dismissed, it would be valuable to track reloads differently from new navigations. Reloads are not treated as opt outs by the blacklisting policy, so the page could be served as a new LoFi page or LitePage after a reload (offline previews are specifically prevented from being shown on any reload however). If the user dismisses the infobar and then reloads, this will only be captured as the user dismissing the infobar. BUG=672634 Committed: https://crrev.com/e0be759f29bd8acd4b83da1aa3e49c645da01ef3 Cr-Commit-Position: refs/heads/master@{#439895} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/e0be759f29bd8acd4b83da1aa3e49c645da01ef3 Cr-Commit-Position: refs/heads/master@{#439895} |