Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(234)

Issue 2557113004: Seperate reloads when reporting previews infobar dismissal (Closed)

Created:
4 years ago by RyanSturm
Modified:
4 years ago
Reviewers:
Steven Holte, megjablon
CC:
chromium-reviews, asvitkine+watch_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

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}

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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+40 lines, -1 line) Patch
M chrome/browser/previews/previews_infobar_delegate.h View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/previews/previews_infobar_delegate.cc View 1 2 3 1 chunk +3 lines, -1 line 0 comments Download
M chrome/browser/previews/previews_infobar_delegate_unittest.cc View 1 2 3 4 5 3 chunks +35 lines, -0 lines 0 comments Download
M tools/metrics/histograms/histograms.xml View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 52 (37 generated)
RyanSturm
megjablon: PTAL, I think this will be valuable for looking at users who recognize that ...
4 years ago (2016-12-09 16:42:22 UTC) #12
RyanSturm
https://codereview.chromium.org/2557113004/diff/1/chrome/browser/previews/previews_infobar_delegate.cc File chrome/browser/previews/previews_infobar_delegate.cc (right): https://codereview.chromium.org/2557113004/diff/1/chrome/browser/previews/previews_infobar_delegate.cc#newcode106 chrome/browser/previews/previews_infobar_delegate.cc:106: RecordPreviewsInfoBarAction( Add unit test coverage.
4 years ago (2016-12-09 17:04:42 UTC) #13
megjablon
Can you break up the long one sentence paragraph in the description? :) https://codereview.chromium.org/2557113004/diff/1/tools/metrics/histograms/histograms.xml File ...
4 years ago (2016-12-09 20:31:20 UTC) #14
RyanSturm
megjablon: PTAL https://codereview.chromium.org/2557113004/diff/1/chrome/browser/previews/previews_infobar_delegate.cc File chrome/browser/previews/previews_infobar_delegate.cc (right): https://codereview.chromium.org/2557113004/diff/1/chrome/browser/previews/previews_infobar_delegate.cc#newcode106 chrome/browser/previews/previews_infobar_delegate.cc:106: RecordPreviewsInfoBarAction( On 2016/12/09 17:04:42, Ryan Sturm wrote: ...
4 years ago (2016-12-17 00:21:54 UTC) #20
megjablon
lgtm
4 years ago (2016-12-19 21:43:56 UTC) #21
RyanSturm
holte: PTAL
4 years ago (2016-12-19 22:15:02 UTC) #25
Steven Holte
lgtm https://codereview.chromium.org/2557113004/diff/20001/tools/metrics/histograms/histograms.xml File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2557113004/diff/20001/tools/metrics/histograms/histograms.xml#newcode98721 tools/metrics/histograms/histograms.xml:98721: + <int value="4" label="Infboar dismissed by reload"/> Infboar ...
4 years ago (2016-12-19 22:50:26 UTC) #26
RyanSturm
Didn't see you were OOO. Thanks for the review! https://codereview.chromium.org/2557113004/diff/20001/tools/metrics/histograms/histograms.xml File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2557113004/diff/20001/tools/metrics/histograms/histograms.xml#newcode98721 tools/metrics/histograms/histograms.xml:98721: ...
4 years ago (2016-12-19 22:55:48 UTC) #27
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2557113004/40001
4 years ago (2016-12-19 22:56:43 UTC) #30
commit-bot: I haz the power
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_compile_dbg/builds/183851) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, ...
4 years ago (2016-12-19 23:25:19 UTC) #32
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2557113004/60001
4 years ago (2016-12-20 18:05:14 UTC) #35
commit-bot: I haz the power
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_ng/builds/357977)
4 years ago (2016-12-20 18:25:15 UTC) #37
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2557113004/100001
4 years ago (2016-12-20 22:08:44 UTC) #47
commit-bot: I haz the power
Committed patchset #6 (id:100001)
4 years ago (2016-12-20 22:46:05 UTC) #50
commit-bot: I haz the power
4 years ago (2016-12-20 22:47:42 UTC) #52
Message was sent while issue was closed.
Patchset 6 (id:??) landed as
https://crrev.com/e0be759f29bd8acd4b83da1aa3e49c645da01ef3
Cr-Commit-Position: refs/heads/master@{#439895}

Powered by Google App Engine
This is Rietveld 408576698