Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2633603002:
Disable app banners in incognito. (Closed)
|
Created:
3 years, 11 months ago by dominickn Modified:
3 years, 11 months ago CC:
chromium-reviews, dominickn+watch_chromium.org, pkotwicz+watch_chromium.org, zpeng+watch_chromium.org, asvitkine+watch_chromium.org, agrieve+watch_chromium.org, chrome-apps-syd-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDisable app banners in incognito.
The add to homescreen menu item is disabled in incognito mode. For
consistency, this CL disables app banners when a site is opened in
incognito. Browser tests and instrumentation tests are added to ensure
the correct functionality.
BUG=680778
Review-Url: https://codereview.chromium.org/2633603002
Cr-Commit-Position: refs/heads/master@{#444223}
Committed: https://chromium.googlesource.com/chromium/src/+/8cc0b4685ce52aac20f0c42b4938be23bd462872
Patch Set 1 #
Total comments: 6
Patch Set 2 : Metrics checking #
Total comments: 4
Patch Set 3 : Comment + rebase #
Messages
Total messages: 32 (18 generated)
The CQ bit was checked by dominickn@chromium.org to run a CQ dry run
dominickn@chromium.org changed reviewers: + benwells@chromium.org, dfalcantara@chromium.org
PTAL, thanks!
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.
https://codereview.chromium.org/2633603002/diff/1/chrome/android/javatests/sr... File chrome/android/javatests/src/org/chromium/chrome/browser/banners/AppBannerManagerTest.java (right): https://codereview.chromium.org/2633603002/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/banners/AppBannerManagerTest.java:617: waitUntilNoInfoBarsExist(); You may need to update this if another patch lands first... https://codereview.chromium.org/2633603002/diff/1/chrome/browser/banners/app_... File chrome/browser/banners/app_banner_manager_browsertest.cc (right): https://codereview.chromium.org/2633603002/diff/1/chrome/browser/banners/app_... chrome/browser/banners/app_banner_manager_browsertest.cc:148: if (!browser->profile()->IsOffTheRecord()) { Can you add an expectation that there are zero histograms in off the record?
Thanks! https://codereview.chromium.org/2633603002/diff/1/chrome/android/javatests/sr... File chrome/android/javatests/src/org/chromium/chrome/browser/banners/AppBannerManagerTest.java (right): https://codereview.chromium.org/2633603002/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/banners/AppBannerManagerTest.java:617: waitUntilNoInfoBarsExist(); On 2017/01/13 04:48:16, benwells wrote: > You may need to update this if another patch lands first... Acknowledged. https://codereview.chromium.org/2633603002/diff/1/chrome/browser/banners/app_... File chrome/browser/banners/app_banner_manager_browsertest.cc (right): https://codereview.chromium.org/2633603002/diff/1/chrome/browser/banners/app_... chrome/browser/banners/app_banner_manager_browsertest.cc:148: if (!browser->profile()->IsOffTheRecord()) { On 2017/01/13 04:48:16, benwells wrote: > Can you add an expectation that there are zero histograms in off the record? I feel that doesn't belong here. I don't know of any code that avoids making a metrics call when in incognito; it should be the responsibility of the metrics system itself to no-op. Is that reasonable? If you feel strongly I can add it, but it seems out of place.
https://codereview.chromium.org/2633603002/diff/1/chrome/browser/banners/app_... File chrome/browser/banners/app_banner_manager_browsertest.cc (right): https://codereview.chromium.org/2633603002/diff/1/chrome/browser/banners/app_... chrome/browser/banners/app_banner_manager_browsertest.cc:148: if (!browser->profile()->IsOffTheRecord()) { On 2017/01/13 04:55:32, dominickn wrote: > On 2017/01/13 04:48:16, benwells wrote: > > Can you add an expectation that there are zero histograms in off the record? > > I feel that doesn't belong here. I don't know of any code that avoids making a > metrics call when in incognito; it should be the responsibility of the metrics > system itself to no-op. Is that reasonable? If you feel strongly I can add it, > but it seems out of place. Recording metrics in incognito happens, but that isn't really the problem. The problem is that one of the values that is logged is that the user was in an incognito session. That should never be logged (which I only learned recently).
The CQ bit was checked by dominickn@chromium.org to run a CQ dry run
Thanks! https://codereview.chromium.org/2633603002/diff/1/chrome/browser/banners/app_... File chrome/browser/banners/app_banner_manager_browsertest.cc (right): https://codereview.chromium.org/2633603002/diff/1/chrome/browser/banners/app_... chrome/browser/banners/app_banner_manager_browsertest.cc:148: if (!browser->profile()->IsOffTheRecord()) { On 2017/01/13 04:59:06, benwells wrote: > On 2017/01/13 04:55:32, dominickn wrote: > > On 2017/01/13 04:48:16, benwells wrote: > > > Can you add an expectation that there are zero histograms in off the record? > > > > I feel that doesn't belong here. I don't know of any code that avoids making a > > metrics call when in incognito; it should be the responsibility of the metrics > > system itself to no-op. Is that reasonable? If you feel strongly I can add it, > > but it seems out of place. > > Recording metrics in incognito happens, but that isn't really the problem. The > problem is that one of the values that is logged is that the user was in an > incognito session. That should never be logged (which I only learned recently). Oh I understand. Done (and even caught a bug whilst doing it)
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
dominickn@chromium.org changed reviewers: + isherman@chromium.org
+isherman: PTAL at histograms, thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
histograms.xml lgtm
https://codereview.chromium.org/2633603002/diff/20001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/banners/AppBannerManagerTest.java (right): https://codereview.chromium.org/2633603002/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/banners/AppBannerManagerTest.java:617: waitUntilNoInfoBarsExist(); It's weird to be testing for the negative here, especially because waitUntilNoInfoBarsExist is meant for waiting until infobars go away, not for them to never show up. That check will immediately pass for any number of reasons.
https://codereview.chromium.org/2633603002/diff/20001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/banners/AppBannerManagerTest.java (right): https://codereview.chromium.org/2633603002/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/banners/AppBannerManagerTest.java:617: waitUntilNoInfoBarsExist(); On 2017/01/13 19:03:38, dfalcantara (load balance plz) wrote: > It's weird to be testing for the negative here, especially because > waitUntilNoInfoBarsExist is meant for waiting until infobars go away, not for > them to never show up. That check will immediately pass for any number of > reasons. Hmm. There's several tests here which follow this pattern (load page, wait till app banner manager is done, wait until no infobars exist) - they repeat it and expect the infobar to be shown the second time. Not really sure how best to proceed here (perhaps remove the test entirely?)
lgtm, don't have to change anything. https://codereview.chromium.org/2633603002/diff/20001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/banners/AppBannerManagerTest.java (right): https://codereview.chromium.org/2633603002/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/banners/AppBannerManagerTest.java:617: waitUntilNoInfoBarsExist(); On 2017/01/16 02:40:50, dominickn wrote: > On 2017/01/13 19:03:38, dfalcantara (load balance plz) wrote: > > It's weird to be testing for the negative here, especially because > > waitUntilNoInfoBarsExist is meant for waiting until infobars go away, not for > > them to never show up. That check will immediately pass for any number of > > reasons. > > Hmm. There's several tests here which follow this pattern (load page, wait till > app banner manager is done, wait until no infobars exist) - they repeat it and > expect the infobar to be shown the second time. Not really sure how best to > proceed here (perhaps remove the test entirely?) No, it's probably fine... in many of these cases, the function is effectively just checking that there aren't any infobars at all. I learned from one of Ben's CLs that an infobar will immediately register as existing when the tab creates an infobar but won't necessarily be on screen yet. It should probably just assert that instead, given what we figured out there.
The CQ bit was checked by dominickn@chromium.org to run a CQ dry run
Thanks! https://codereview.chromium.org/2633603002/diff/20001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/banners/AppBannerManagerTest.java (right): https://codereview.chromium.org/2633603002/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/banners/AppBannerManagerTest.java:617: waitUntilNoInfoBarsExist(); On 2017/01/17 19:13:59, dfalcantara (load balance plz) wrote: > On 2017/01/16 02:40:50, dominickn wrote: > > On 2017/01/13 19:03:38, dfalcantara (load balance plz) wrote: > > > It's weird to be testing for the negative here, especially because > > > waitUntilNoInfoBarsExist is meant for waiting until infobars go away, not > for > > > them to never show up. That check will immediately pass for any number of > > > reasons. > > > > Hmm. There's several tests here which follow this pattern (load page, wait > till > > app banner manager is done, wait until no infobars exist) - they repeat it and > > expect the infobar to be shown the second time. Not really sure how best to > > proceed here (perhaps remove the test entirely?) > > No, it's probably fine... in many of these cases, the function is effectively > just checking that there aren't any infobars at all. I learned from one of > Ben's CLs that an infobar will immediately register as existing when the tab > creates an infobar but won't necessarily be on screen yet. > > It should probably just assert that instead, given what we figured out there. Done.
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.
The CQ bit was checked by dominickn@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from benwells@chromium.org, isherman@chromium.org, dfalcantara@chromium.org Link to the patchset: https://codereview.chromium.org/2633603002/#ps40001 (title: "Comment + rebase")
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": 40001, "attempt_start_ts": 1484700950626880,
"parent_rev": "88a9a4f79d7fe6b05b2def6cbf158c935b1040b1", "commit_rev":
"8cc0b4685ce52aac20f0c42b4938be23bd462872"}
Message was sent while issue was closed.
Description was changed from ========== Disable app banners in incognito. The add to homescreen menu item is disabled in incognito mode. For consistency, this CL disables app banners when a site is opened in incognito. Browser tests and instrumentation tests are added to ensure the correct functionality. BUG=680778 ========== to ========== Disable app banners in incognito. The add to homescreen menu item is disabled in incognito mode. For consistency, this CL disables app banners when a site is opened in incognito. Browser tests and instrumentation tests are added to ensure the correct functionality. BUG=680778 Review-Url: https://codereview.chromium.org/2633603002 Cr-Commit-Position: refs/heads/master@{#444223} Committed: https://chromium.googlesource.com/chromium/src/+/8cc0b4685ce52aac20f0c42b4938... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/8cc0b4685ce52aac20f0c42b4938... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
