Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2005623002:
Logic for the Data Saver InfoBar Promo (Closed)
DescriptionLogic for the Data Saver InfoBar Promo
For non-users of Data Saver, show a promotional InfoBar if the following are true:
The user in the DataReductionProxyInfobarPromo field trial.
The user doesn’t have the Data Reduction Proxy enabled.
The user saw the promo in a version >= M51 or installed before the M48 release date 1/26/16.
The user didn’t opt out of the FRE promo card.
The current version is at least two milestones after the last promo was displayed.
The infobar has never been shown and dismissed before.
The request is HTTP.
BUG=610757
Committed: https://crrev.com/3f07536d655fbf235b3c3909c6a19d41f3913f51
Cr-Commit-Position: refs/heads/master@{#402522}
Patch Set 1 #
Total comments: 6
Patch Set 2 : remove field trial and opt out epoch #
Total comments: 12
Patch Set 3 : comments #
Total comments: 62
Patch Set 4 : comments #
Total comments: 32
Patch Set 5 : comments and add test #
Total comments: 28
Patch Set 6 : bengr comments #Patch Set 7 : #Patch Set 8 : dfalcantara comments #Patch Set 9 : no incognito, move to ChromeTabbedActivity #
Total comments: 1
Patch Set 10 : spelling fix #Patch Set 11 : missing returns #Dependent Patchsets: Messages
Total messages: 49 (25 generated)
Description was changed from ========== Logic to display promo, need to write infobar UI BUG= ========== to ========== Data Saver Infobar Promo BUG= ==========
Description was changed from ========== Data Saver Infobar Promo BUG= ========== to ========== Data Saver Infobar Promo For non-users of Data Saver, show a promotional InfoBar if the following are true: The user in the DataReductionProxyInfobarPromo field trial. The user doesn’t have the Data Reduction Proxy enabled. The user saw the promo in a version >= M51. The user didn’t opt out of the FRE promo card. The current version is at least two milestones after the last promo was displayed. The infobar has never been shown and dismissed before. The request is HTTP. BUG=610757 ==========
Description was changed from ========== Data Saver Infobar Promo For non-users of Data Saver, show a promotional InfoBar if the following are true: The user in the DataReductionProxyInfobarPromo field trial. The user doesn’t have the Data Reduction Proxy enabled. The user saw the promo in a version >= M51. The user didn’t opt out of the FRE promo card. The current version is at least two milestones after the last promo was displayed. The infobar has never been shown and dismissed before. The request is HTTP. BUG=610757 ========== to ========== Logic for the Data Saver Infobar Promo For non-users of Data Saver, show a promotional InfoBar if the following are true: The user in the DataReductionProxyInfobarPromo field trial. The user doesn’t have the Data Reduction Proxy enabled. The user saw the promo in a version >= M51. The user didn’t opt out of the FRE promo card. The current version is at least two milestones after the last promo was displayed. The infobar has never been shown and dismissed before. The request is HTTP. BUG=610757 ==========
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #2 (id:60001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60002) has been deleted
Patchset #1 (id:90001) has been deleted
megjablon@chromium.org changed reviewers: + bengr@chromium.org
Ben, this cl is for the logic to display the InfoBar promo. The rest of the native code is here https://codereview.chromium.org/2022313002/ with tests to be added. Can you take a look at DataReductionPromoInfoBar.java to verify that I've covered all cases when we shouldn't show the promo? Thanks!
Patchset #1 (id:110001) has been deleted
On 2016/06/01 00:55:07, megjablon wrote: > Ben, this cl is for the logic to display the InfoBar promo. The rest of the > native code is here https://codereview.chromium.org/2022313002/ with tests to be > added. Can you take a look at DataReductionPromoInfoBar.java to verify that I've > covered all cases when we shouldn't show the promo? Thanks! Why does the user need to have seen the promo in a version >= M51? I'm presuming that starting in M51 we record the milestone in which the promo was shown. Can the logic be that we show the infobar if the promo was shown before and if a milestone is listed it is at least two milestones ago?
Description was changed from ========== Logic for the Data Saver Infobar Promo For non-users of Data Saver, show a promotional InfoBar if the following are true: The user in the DataReductionProxyInfobarPromo field trial. The user doesn’t have the Data Reduction Proxy enabled. The user saw the promo in a version >= M51. The user didn’t opt out of the FRE promo card. The current version is at least two milestones after the last promo was displayed. The infobar has never been shown and dismissed before. The request is HTTP. BUG=610757 ========== to ========== Logic for the Data Saver Infobar Promo For non-users of Data Saver, show a promotional InfoBar if the following are true: The user in the DataReductionProxyInfobarPromo field trial. The user doesn’t have the Data Reduction Proxy enabled. The user saw the promo in a version >= M51 or installed before the M48 release date 1/26/16. The user didn’t opt out of the FRE promo card. The current version is at least two milestones after the last promo was displayed. The infobar has never been shown and dismissed before. The request is HTTP. BUG=610757 ==========
Patchset #2 (id:150001) has been deleted
Description was changed from ========== Logic for the Data Saver Infobar Promo For non-users of Data Saver, show a promotional InfoBar if the following are true: The user in the DataReductionProxyInfobarPromo field trial. The user doesn’t have the Data Reduction Proxy enabled. The user saw the promo in a version >= M51 or installed before the M48 release date 1/26/16. The user didn’t opt out of the FRE promo card. The current version is at least two milestones after the last promo was displayed. The infobar has never been shown and dismissed before. The request is HTTP. BUG=610757 ========== to ========== Logic for the Data Saver InfoBar Promo For non-users of Data Saver, show a promotional InfoBar if the following are true: The user in the DataReductionProxyInfobarPromo field trial. The user doesn’t have the Data Reduction Proxy enabled. The user saw the promo in a version >= M51 or installed before the M48 release date 1/26/16. The user didn’t opt out of the FRE promo card. The current version is at least two milestones after the last promo was displayed. The infobar has never been shown and dismissed before. The request is HTTP. BUG=610757 ==========
bengr@chromium.org changed reviewers: + rajendrant@chromium.org
A few comments to get started. I'm handing this review off to Raj so he can get some practice. Once he signs off, I'll take another look. https://codereview.chromium.org/2005623002/diff/130001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java (right): https://codereview.chromium.org/2005623002/diff/130001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:24: private static final String HTTP_SCHEME = "http://"; Use: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/jav... https://codereview.chromium.org/2005623002/diff/130001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:43: if (!DataReductionPromoUtils.getDataReductionInfoBarPromoFieldTrialEnabled()) return; Shouldn't this just be part of the second run promo field trial? What's the point of separating it out? https://codereview.chromium.org/2005623002/diff/130001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:46: if (!url.startsWith(HTTP_SCHEME)) return; Use this instead: https://code.google.com/p/chromium/codesearch#chromium/src/net/android/java/s...
https://codereview.chromium.org/2005623002/diff/170001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java (right): https://codereview.chromium.org/2005623002/diff/170001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:63: packageInfo = context.getPackageManager().getPackageInfo(context.getPackageName(), 0); wrap this out in a separate function getPackageFirstInstallTime() to return firstInstallTime directly. https://codereview.chromium.org/2005623002/diff/170001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:68: m48ReleaseDateCal.set(2016, 1, 26); Can "2016, 1, 26" be moved as static final https://codereview.chromium.org/2005623002/diff/170001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:74: if (promo_build < 51 && firstInstallTime > m48ReleaseDate) return; m48ReleaseDateCal.getTimeInMillis() can be used directly and m48ReleaseDate can be removed. https://codereview.chromium.org/2005623002/diff/170001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:94: private static int getBuildFromVersionNumber(String version) { Comments please. May be add an example version string. https://codereview.chromium.org/2005623002/diff/170001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:122: public static void launch(WebContents webContents, /s/public/private https://codereview.chromium.org/2005623002/diff/170001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoUtils.java (right): https://codereview.chromium.org/2005623002/diff/170001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoUtils.java:35: * Returns whether any of the Data Reduction Proxies can be displayed. Checks if the proxy is /s/Proxies/Promos
https://codereview.chromium.org/2005623002/diff/130001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java (right): https://codereview.chromium.org/2005623002/diff/130001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:24: private static final String HTTP_SCHEME = "http://"; On 2016/06/02 23:02:11, bengr wrote: > Use: > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/jav... > Done. https://codereview.chromium.org/2005623002/diff/130001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:43: if (!DataReductionPromoUtils.getDataReductionInfoBarPromoFieldTrialEnabled()) return; On 2016/06/02 23:02:11, bengr wrote: > Shouldn't this just be part of the second run promo field trial? What's the > point of separating it out? Done. https://codereview.chromium.org/2005623002/diff/130001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:46: if (!url.startsWith(HTTP_SCHEME)) return; On 2016/06/02 23:02:11, bengr wrote: > Use this instead: > https://code.google.com/p/chromium/codesearch#chromium/src/net/android/java/s... Done. https://codereview.chromium.org/2005623002/diff/170001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java (right): https://codereview.chromium.org/2005623002/diff/170001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:63: packageInfo = context.getPackageManager().getPackageInfo(context.getPackageName(), 0); On 2016/06/03 17:07:49, Raj wrote: > wrap this out in a separate function getPackageFirstInstallTime() to return > firstInstallTime directly. Done. https://codereview.chromium.org/2005623002/diff/170001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:68: m48ReleaseDateCal.set(2016, 1, 26); On 2016/06/03 17:07:49, Raj wrote: > Can "2016, 1, 26" be moved as static final Done. https://codereview.chromium.org/2005623002/diff/170001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:74: if (promo_build < 51 && firstInstallTime > m48ReleaseDate) return; On 2016/06/03 17:07:49, Raj wrote: > m48ReleaseDateCal.getTimeInMillis() can be used directly and m48ReleaseDate can > be removed. Done. https://codereview.chromium.org/2005623002/diff/170001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:94: private static int getBuildFromVersionNumber(String version) { On 2016/06/03 17:07:49, Raj wrote: > Comments please. > May be add an example version string. Done. https://codereview.chromium.org/2005623002/diff/170001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:122: public static void launch(WebContents webContents, On 2016/06/03 17:07:49, Raj wrote: > /s/public/private Done. https://codereview.chromium.org/2005623002/diff/170001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoUtils.java (right): https://codereview.chromium.org/2005623002/diff/170001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoUtils.java:35: * Returns whether any of the Data Reduction Proxies can be displayed. Checks if the proxy is On 2016/06/03 17:07:49, Raj wrote: > /s/Proxies/Promos Done.
lgtm % comments https://codereview.chromium.org/2005623002/diff/190001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2005623002/diff/190001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:544: DataReductionPromoInfoBar.maybeLaunchDataReductionPromoInfoBar( nit: I wonder whether to show the infobar when isFragmentNavigation==False and statusCode==200. WDYT. https://codereview.chromium.org/2005623002/diff/190001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java (right): https://codereview.chromium.org/2005623002/diff/190001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:44: Do we also need to condition on DataReductionProxyInfobarPromo fieldtrial ? https://codereview.chromium.org/2005623002/diff/190001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:63: may be check if either of these *_build variables are -1 and return immediately. Though unlikely for the parse to fail, its better to bail out, rather than to end up showing the infobar on every release.
https://codereview.chromium.org/2005623002/diff/190001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java (right): https://codereview.chromium.org/2005623002/diff/190001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:23: * Generates an InfoBar for data reduction proxy promotion that contains a message and button to nit: Generates an InfoBar to promote the data reduction proxy. The InfoBar contains a message and a button to enable the proxy. https://codereview.chromium.org/2005623002/diff/190001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:29: private static String sTitle; Do these need to be static? I guess that depends on the calling context of launch(). https://codereview.chromium.org/2005623002/diff/190001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:43: if (!DataReductionPromoUtils.canShowDataReductionPromos()) return; Rename DataReductionPromoUtils -> DataReductionProxyPromotionUtils canShowDataReductionPromos -> canShowPromotions https://codereview.chromium.org/2005623002/diff/190001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:45: // Only show the promo on HTTP pages. nit everywhere: promo -> promotion https://codereview.chromium.org/2005623002/diff/190001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:48: // Don't show the InfoBar promo if neither the FRE or second run promo have been shown. nit: spell out "first run experience (FRE)" on the first mention of FRE in this file. nit: have -> has https://codereview.chromium.org/2005623002/diff/190001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:49: if (!DataReductionPromoUtils.getDisplayedDataReductionPromo()) return; I would separate these questions out: if (!DataReductionProxyPromotionUtils.getDisplayedFreCard() && !DataReductionProxyPromotionUtils.getDisplayedInterstitial()) { return; } https://codereview.chromium.org/2005623002/diff/190001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:52: if (DataReductionPromoUtils.getDataReductionFrePromoOptOut()) return; nit: getOptedOutOnFreCard() https://codereview.chromium.org/2005623002/diff/190001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:55: if (DataReductionPromoUtils.getDataReductionInfoBarPromoOptOut()) return; What does it mean to opt out of the infobar promo? I'd instead call this: getDisplayedInfoBar(). https://codereview.chromium.org/2005623002/diff/190001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:57: int current_build = getBuildFromVersionNumber( Use Java style and change build to milestone: currentMilestone = getMilestoneNumberFromVersion(...) https://codereview.chromium.org/2005623002/diff/190001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:59: int promo_build = getBuildFromVersionNumber( See above. lastPromotionMilestone https://codereview.chromium.org/2005623002/diff/190001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:61: int infobar_promo_build = getBuildFromVersionNumber( infobarMilestone https://codereview.chromium.org/2005623002/diff/190001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:63: On 2016/06/04 00:03:49, Raj wrote: > may be check if either of these *_build variables are -1 and return immediately. > Though unlikely for the parse to fail, its better to bail out, rather than to > end up showing the infobar on every release. Yes, absolutely, please check. https://codereview.chromium.org/2005623002/diff/190001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:64: Calendar m48ReleaseDateCal = Calendar.getInstance(TimeZone.getTimeZone("UTC")); rename: m48ReleaseDataCal -> releaseDateOfM48 Leading with an "m" makes it look like a member variable. https://codereview.chromium.org/2005623002/diff/190001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:66: long firstInstallTime = getPackageFirstInstallTime(context); Should also check that this is a sane time, like not a hundred million years in the future. https://codereview.chromium.org/2005623002/diff/190001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:69: // show the infobar for FRE promos that were shown for builds 48 to 51. builds -> milestones https://codereview.chromium.org/2005623002/diff/190001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:70: if (promo_build < 51 && firstInstallTime > m48ReleaseDateCal.getTimeInMillis()) return; promotionMilestone https://codereview.chromium.org/2005623002/diff/190001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:97: private static int getBuildFromVersionNumber(String version) { getMilestoneNumberFromVersionString https://codereview.chromium.org/2005623002/diff/190001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:118: * Gets the time at which this app was first installed in milliseconds. Gets the time at which this app was first installed in milliseconds since the epoch? https://codereview.chromium.org/2005623002/diff/190001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:121: * @return The time at which this app was first installed. Document that this is in milliseconds since the epoch. https://codereview.chromium.org/2005623002/diff/190001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:123: private static long getPackageFirstInstallTime(Context context) { What does "first" mean? If at times a, b, c, a package is installed, then uninstalled, then re-installed, respectively, does this return a or c? If "a" then the logic is busted because re-installation reshows the FRE and promos. If "c" then change the name to getPackageInstallTime() https://codereview.chromium.org/2005623002/diff/190001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:146: sTitle = title; Why do we need to store this stuff? https://codereview.chromium.org/2005623002/diff/190001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoUtils.java (right): https://codereview.chromium.org/2005623002/diff/190001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoUtils.java:13: * Helper functions for displaying the various Data Reduction Proxy promos. The promo screens everywhere: promos -> promotions https://codereview.chromium.org/2005623002/diff/190001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoUtils.java:16: public class DataReductionPromoUtils { Rename as DataReductionProxyPromotionUtils https://codereview.chromium.org/2005623002/diff/190001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoUtils.java:30: private static final String SHARED_PREF_FRE_PROMO_OPT_OUT = "fre_promo_opt_out"; Are we stuck with the string? Otherwise rename "data_reduction_fre_promo_opt_out" https://codereview.chromium.org/2005623002/diff/190001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoUtils.java:32: "data_reduction_infobar_promo_opt_out"; Again, I don't understand how you can opt out of a promo that is opt in. https://codereview.chromium.org/2005623002/diff/190001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoUtils.java:35: * Returns whether any of the Data Reduction Promos can be displayed. Checks if the proxy is Promos -> Proxy promotions Also, you capitalize Data Reduction Proxy in some places but not all. I think these days I prefer data reduction proxy, but regardless, please be consistent. https://codereview.chromium.org/2005623002/diff/190001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoUtils.java:53: public static void saveDataReductionPromoDisplayed() { recordDisplayOfPromotion()? Also, which promotion is this for? I would be explicit. We have three promotions: the FreCard, the Interstitial (if that's what it is), and the InfoBar. https://codereview.chromium.org/2005623002/diff/190001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoUtils.java:70: public static boolean getDisplayedDataReductionPromo() { You can remove DataReduction from all of the method names.
Patchset #4 (id:210001) has been deleted
Please make sure all the naming looks correct. There were a lot of renames and if you could double check for me that the logic didn't get changed with that, it would be great. Thanks! https://chromiumcodereview.appspot.com/2005623002/diff/190001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://chromiumcodereview.appspot.com/2005623002/diff/190001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:544: DataReductionPromoInfoBar.maybeLaunchDataReductionPromoInfoBar( On 2016/06/04 00:03:49, Raj wrote: > nit: I wonder whether to show the infobar when isFragmentNavigation==False and > statusCode==200. WDYT. Done. https://chromiumcodereview.appspot.com/2005623002/diff/190001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java (right): https://chromiumcodereview.appspot.com/2005623002/diff/190001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:23: * Generates an InfoBar for data reduction proxy promotion that contains a message and button to On 2016/06/06 21:07:59, bengr wrote: > nit: Generates an InfoBar to promote the data reduction proxy. The InfoBar > contains a message and a button to enable the proxy. Done. https://chromiumcodereview.appspot.com/2005623002/diff/190001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:29: private static String sTitle; On 2016/06/06 21:07:59, bengr wrote: > Do these need to be static? I guess that depends on the calling context of > launch(). We set these in the static launch. We can't set them any later because we won't have the Android context to get the strings when native calls the constructor for this. We could pass the strings from native as the GeneratedPasswordSavedInfoBarDelegate does. I just opted to use the structure we had in our previous DataReductionProxyInfoBar which was used to the ssl experiment that I just removed. https://chromiumcodereview.appspot.com/2005623002/diff/190001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:43: if (!DataReductionPromoUtils.canShowDataReductionPromos()) return; On 2016/06/06 21:07:59, bengr wrote: > Rename DataReductionPromoUtils -> DataReductionProxyPromotionUtils > > canShowDataReductionPromos -> canShowPromotions We use promo for all the other drp promotion code. If we want to rename, I say we do that for all promotions in a separate cl so we don't have half the code using promotion and half using promo. However, i don't see the need to. I think promo is concise and a clear abbreviation. https://chromiumcodereview.appspot.com/2005623002/diff/190001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:44: On 2016/06/04 00:03:50, Raj wrote: > Do we also need to condition on DataReductionProxyInfobarPromo fieldtrial ? Nope, bengr decided we will just use the existing field trial for promos which is checked in native by DataReductionProxyPromotionUtils#canShowPromotions https://chromiumcodereview.appspot.com/2005623002/diff/190001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:45: // Only show the promo on HTTP pages. On 2016/06/06 21:08:00, bengr wrote: > nit everywhere: promo -> promotion See above https://chromiumcodereview.appspot.com/2005623002/diff/190001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:48: // Don't show the InfoBar promo if neither the FRE or second run promo have been shown. On 2016/06/06 21:08:00, bengr wrote: > nit: spell out "first run experience (FRE)" on the first mention of FRE in this > file. > nit: have -> has Done. https://chromiumcodereview.appspot.com/2005623002/diff/190001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:49: if (!DataReductionPromoUtils.getDisplayedDataReductionPromo()) return; On 2016/06/06 21:08:00, bengr wrote: > I would separate these questions out: > if (!DataReductionProxyPromotionUtils.getDisplayedFreCard() && > !DataReductionProxyPromotionUtils.getDisplayedInterstitial()) { > return; > } We don't have the logic that separates these out. Since they are mutually exclusive, when we wrote the code we simply stored one boolean value indicating if a data reduction proxy promo had been shown to the user. https://chromiumcodereview.appspot.com/2005623002/diff/190001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:52: if (DataReductionPromoUtils.getDataReductionFrePromoOptOut()) return; On 2016/06/06 21:07:59, bengr wrote: > nit: getOptedOutOnFreCard() Done. https://chromiumcodereview.appspot.com/2005623002/diff/190001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:55: if (DataReductionPromoUtils.getDataReductionInfoBarPromoOptOut()) return; On 2016/06/06 21:07:59, bengr wrote: > What does it mean to opt out of the infobar promo? I'd instead call this: > getDisplayedInfoBar(). Done. https://chromiumcodereview.appspot.com/2005623002/diff/190001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:57: int current_build = getBuildFromVersionNumber( On 2016/06/06 21:07:59, bengr wrote: > Use Java style and change build to milestone: currentMilestone = > getMilestoneNumberFromVersion(...) Done. https://chromiumcodereview.appspot.com/2005623002/diff/190001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:59: int promo_build = getBuildFromVersionNumber( On 2016/06/06 21:07:59, bengr wrote: > See above. lastPromotionMilestone Done. https://chromiumcodereview.appspot.com/2005623002/diff/190001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:61: int infobar_promo_build = getBuildFromVersionNumber( On 2016/06/06 21:07:59, bengr wrote: > infobarMilestone Done. https://chromiumcodereview.appspot.com/2005623002/diff/190001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:63: On 2016/06/06 21:07:59, bengr wrote: > On 2016/06/04 00:03:49, Raj wrote: > > may be check if either of these *_build variables are -1 and return > immediately. > > Though unlikely for the parse to fail, its better to bail out, rather than to > > end up showing the infobar on every release. > > Yes, absolutely, please check. We expect lastPromoMilestone and lastInfobarPromoMilestone to be -1 if they're not there which can happen if the last promo was shown before m51 or the promo infobar hasn't been shown. All the below conditions are written with this in mind. https://chromiumcodereview.appspot.com/2005623002/diff/190001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:64: Calendar m48ReleaseDateCal = Calendar.getInstance(TimeZone.getTimeZone("UTC")); On 2016/06/06 21:08:00, bengr wrote: > rename: m48ReleaseDataCal -> releaseDateOfM48 > > Leading with an "m" makes it look like a member variable. Done. https://chromiumcodereview.appspot.com/2005623002/diff/190001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:66: long firstInstallTime = getPackageFirstInstallTime(context); On 2016/06/06 21:08:00, bengr wrote: > Should also check that this is a sane time, like not a hundred million years in > the future. A value in the future doesn't matter because we'll return anyway, but for a sane value in the past, what date should we choose? https://chromiumcodereview.appspot.com/2005623002/diff/190001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:69: // show the infobar for FRE promos that were shown for builds 48 to 51. On 2016/06/06 21:07:59, bengr wrote: > builds -> milestones Done. https://chromiumcodereview.appspot.com/2005623002/diff/190001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:70: if (promo_build < 51 && firstInstallTime > m48ReleaseDateCal.getTimeInMillis()) return; On 2016/06/06 21:07:59, bengr wrote: > promotionMilestone Done. https://chromiumcodereview.appspot.com/2005623002/diff/190001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:97: private static int getBuildFromVersionNumber(String version) { On 2016/06/06 21:07:59, bengr wrote: > getMilestoneNumberFromVersionString Done. https://chromiumcodereview.appspot.com/2005623002/diff/190001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:118: * Gets the time at which this app was first installed in milliseconds. On 2016/06/06 21:07:59, bengr wrote: > Gets the time at which this app was first installed in milliseconds since the > epoch? Done. https://chromiumcodereview.appspot.com/2005623002/diff/190001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:121: * @return The time at which this app was first installed. On 2016/06/06 21:07:59, bengr wrote: > Document that this is in milliseconds since the epoch. Done. https://chromiumcodereview.appspot.com/2005623002/diff/190001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:123: private static long getPackageFirstInstallTime(Context context) { On 2016/06/06 21:08:00, bengr wrote: > What does "first" mean? If at times a, b, c, a package is installed, then > uninstalled, then re-installed, respectively, does this return a or c? If "a" > then the logic is busted because re-installation reshows the FRE and promos. If > "c" then change the name to getPackageInstallTime() Done. However, I believe android calls this first install time because when you update an app it goes through an "install". https://chromiumcodereview.appspot.com/2005623002/diff/190001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:146: sTitle = title; On 2016/06/06 21:07:59, bengr wrote: > Why do we need to store this stuff? See https://chromiumcodereview.appspot.com/2022313002/ and comment above about strings https://chromiumcodereview.appspot.com/2005623002/diff/190001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoUtils.java (right): https://chromiumcodereview.appspot.com/2005623002/diff/190001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoUtils.java:13: * Helper functions for displaying the various Data Reduction Proxy promos. The promo screens On 2016/06/06 21:08:00, bengr wrote: > everywhere: promos -> promotions See comment in DataReductionPromoInfoBar https://chromiumcodereview.appspot.com/2005623002/diff/190001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoUtils.java:16: public class DataReductionPromoUtils { On 2016/06/06 21:08:00, bengr wrote: > Rename as DataReductionProxyPromotionUtils See comment in DataReductionPromoInfoBar https://chromiumcodereview.appspot.com/2005623002/diff/190001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoUtils.java:30: private static final String SHARED_PREF_FRE_PROMO_OPT_OUT = "fre_promo_opt_out"; On 2016/06/06 21:08:00, bengr wrote: > Are we stuck with the string? Otherwise rename > "data_reduction_fre_promo_opt_out" yes we're stuck with this :/ we can move it over if we want. https://chromiumcodereview.appspot.com/2005623002/diff/190001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoUtils.java:32: "data_reduction_infobar_promo_opt_out"; On 2016/06/06 21:08:00, bengr wrote: > Again, I don't understand how you can opt out of a promo that is opt in. Removed. https://chromiumcodereview.appspot.com/2005623002/diff/190001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoUtils.java:35: * Returns whether any of the Data Reduction Promos can be displayed. Checks if the proxy is On 2016/06/06 21:08:00, bengr wrote: > Promos -> Proxy promotions > > Also, you capitalize Data Reduction Proxy in some places but not all. I think > these days I prefer data reduction proxy, but regardless, please be consistent. Done. https://chromiumcodereview.appspot.com/2005623002/diff/190001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoUtils.java:53: public static void saveDataReductionPromoDisplayed() { On 2016/06/06 21:08:00, bengr wrote: > recordDisplayOfPromotion()? > > Also, which promotion is this for? I would be explicit. We have three > promotions: the FreCard, the Interstitial (if that's what it is), and the > InfoBar. This code is simply being moved out of the class it was in before which showed the second run promo UI, but also had generic promo methods used by the FRE. We don't have separate pref values for whether the fre promo and second run promo was shown because they are mutually exclusive. If we want to refactor this, let's to it in a separate bug. https://chromiumcodereview.appspot.com/2005623002/diff/190001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoUtils.java:70: public static boolean getDisplayedDataReductionPromo() { On 2016/06/06 21:08:00, bengr wrote: > You can remove DataReduction from all of the method names. Done.
Patchset #4 (id:230001) has been deleted
https://codereview.chromium.org/2005623002/diff/190001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java (right): https://codereview.chromium.org/2005623002/diff/190001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:44: On 2016/06/10 00:12:06, megjablon wrote: > On 2016/06/04 00:03:50, Raj wrote: > > Do we also need to condition on DataReductionProxyInfobarPromo fieldtrial ? > > Nope, bengr decided we will just use the existing field trial for promos which > is checked in native by DataReductionProxyPromotionUtils#canShowPromotions hmm, Does that mean we cannot start the infobar promo from 1% ? Or could we reduce the existing fieldtrial from 100% to 1% and then bump up slowly. https://codereview.chromium.org/2005623002/diff/250001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunFlowSequencer.java (right): https://codereview.chromium.org/2005623002/diff/250001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunFlowSequencer.java:120: return DataReductionPromoUtils.canShowPromos() nit: just wondering if this fixes anything. I hope kPromoAllowed is populated when canShowPromos() gets called in the FRE flow. I believe IsIncludedInPromoFieldTrial() is called in UI thread, so there is no IO-UI race conditions in populating this. Pls check. https://codereview.chromium.org/2005623002/diff/250001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java (right): https://codereview.chromium.org/2005623002/diff/250001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:41: public static void maybeLaunchDataReductionPromoInfoBar(Context context, nit: DataReduction is not needed here, since its also in the class name. would maybeLaunchInfoBarPromo() work ? https://codereview.chromium.org/2005623002/diff/250001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:63: if (currentMilestone == -1) return; nit: You are right. I agree the conditions below take into account when lastPromoMilestone=-1. Still it would be readable for new users if lastPromoMilestone==-1 check is explicit. https://codereview.chromium.org/2005623002/diff/250001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:92: * @return The milestone of the given version string. nit: pls add 'or -1 on failure.' https://codereview.chromium.org/2005623002/diff/250001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:119: * @return The time at which this app was first installed in milliseconds since the epoch. nit: or 0 if install time cannot be retrieved. https://codereview.chromium.org/2005623002/diff/250001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoUtils.java (right): https://codereview.chromium.org/2005623002/diff/250001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoUtils.java:21: private static final String SHARED_PREF_DISPLAYED_PROMO = "displayed_data_reduction_fre_promo"; nit: I wonder whether to group the FRE or secondrun related strings together and have a common comment. Since its not obvious DISPLAYED_PROMO, DISPLAYED_PROMO_TIME_MS, DISPLAYED_PROMO_VERSION correspond to FRE or secondrun. // FRE or Second run promos. private static final String SHARED_PREF_DISPLAYED_PROMO = "displayed_data_reduction_fre_promo"; private static final String SHARED_PREF_DISPLAYED_PROMO_TIME_MS = "displayed_data_reduction_promo_time_ms"; ... // Infobar promo. ...
https://codereview.chromium.org/2005623002/diff/250001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2005623002/diff/250001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:545: if (statusCode != 200) return; /s/200/HttpURLConnection.HTTP_OK import java.net.URLConnection;
https://codereview.chromium.org/2005623002/diff/250001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2005623002/diff/250001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:544: if (isFragmentNavigation) return; Would it make sense to move these checks into maybeLaunchDataReductionPromoInfoBar, so this class needs to know as little as possible about the DRP promo implementation? https://codereview.chromium.org/2005623002/diff/250001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java (right): https://codereview.chromium.org/2005623002/diff/250001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:27: private static final String M48_RELEASE_DATE = "2016-01-26"; nit: M48_STABLE_RELEASE_DATA, if that's what it is. https://codereview.chromium.org/2005623002/diff/250001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:65: Calendar releaseDateOfM48 = Calendar.getInstance(TimeZone.getTimeZone("UTC")); releaseDateOfM48Stable https://codereview.chromium.org/2005623002/diff/250001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:67: long firstInstallTime = getPackageInstallTime(context); firstInstallTime -> packageInstallTime https://codereview.chromium.org/2005623002/diff/250001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:69: // The boolean pref storing if the user opted out on the First Run Experience promo was Wording is a little confusing. How about: The boolean pref that stores whether the user opted out on the FRE was added in M51. So if Chrome was installed after the FRE promo was added (M48) and before M51, assume the user opted out from the FRE and don't show the infobar. https://codereview.chromium.org/2005623002/diff/250001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoUtils.java (right): https://codereview.chromium.org/2005623002/diff/250001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoUtils.java:16: public class DataReductionPromoUtils { Is there really no way to add tests to all of this? Every class should have unit tests. https://codereview.chromium.org/2005623002/diff/250001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoUtils.java:34: * proxy is allowed by the config, already on, or if the user is managed. By what config? Why does it matter that the proxy pref is managed? Add answers to these questions to the comments. https://codereview.chromium.org/2005623002/diff/250001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoUtils.java:48: * Saves shared prefs indicating that the data reduction proxy First Run Experience or second Why do you capitalize First Run Experience, but not second run promo? I think both should be lower case everywhere. I don't care if you abbreviate as FRE or fre, but you should be consistent. https://codereview.chromium.org/2005623002/diff/250001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoUtils.java:104: public static boolean getOptedOutOnFreCard() { getOptedOutOnFrePromo
Patchset #5 (id:270001) has been deleted
Patchset #5 (id:270002) has been deleted
https://codereview.chromium.org/2005623002/diff/250001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2005623002/diff/250001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:544: if (isFragmentNavigation) return; On 2016/06/20 15:17:11, bengr wrote: > Would it make sense to move these checks into > maybeLaunchDataReductionPromoInfoBar, so this class needs to know as little as > possible about the DRP promo implementation? Done. https://codereview.chromium.org/2005623002/diff/250001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:545: if (statusCode != 200) return; On 2016/06/13 02:30:36, Raj wrote: > /s/200/HttpURLConnection.HTTP_OK > import java.net.URLConnection; Done. https://codereview.chromium.org/2005623002/diff/250001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunFlowSequencer.java (right): https://codereview.chromium.org/2005623002/diff/250001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunFlowSequencer.java:120: return DataReductionPromoUtils.canShowPromos() On 2016/06/10 20:43:30, Raj wrote: > nit: just wondering if this fixes anything. > I hope kPromoAllowed is populated when canShowPromos() gets called in the FRE > flow. > I believe IsIncludedInPromoFieldTrial() is called in UI thread, so there is no > IO-UI race conditions in populating this. Pls check. This is all called on the UI thread, so we don't have any race conditions. I changed this to isDataReductionProxyManaged() because that's the condition we care about. kPromoAllowed is the same population as the DataReductionProxyFREPromo field trial so we don't need to check both and since it's the first run the DRP shouldn't be enabled already so we don't need to check that either. https://codereview.chromium.org/2005623002/diff/250001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java (right): https://codereview.chromium.org/2005623002/diff/250001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:27: private static final String M48_RELEASE_DATE = "2016-01-26"; On 2016/06/20 15:17:11, bengr wrote: > nit: M48_STABLE_RELEASE_DATA, if that's what it is. Done. https://codereview.chromium.org/2005623002/diff/250001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:41: public static void maybeLaunchDataReductionPromoInfoBar(Context context, On 2016/06/10 20:43:30, Raj wrote: > nit: DataReduction is not needed here, since its also in the class name. > would maybeLaunchInfoBarPromo() work ? Done. https://codereview.chromium.org/2005623002/diff/250001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:63: if (currentMilestone == -1) return; On 2016/06/10 20:43:30, Raj wrote: > nit: You are right. I agree the conditions below take into account when > lastPromoMilestone=-1. > Still it would be readable for new users if lastPromoMilestone==-1 check is > explicit. The issue is that we expect lastPromoMilestone==-1 if the user saw the promo before M51, but we would still want to show the promo. This check would mean that we don't show the promo to anyone who saw the promo before M51, which is when the promo version pref was added. I updated the cases and comments below to hopefully reflect this. https://codereview.chromium.org/2005623002/diff/250001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:65: Calendar releaseDateOfM48 = Calendar.getInstance(TimeZone.getTimeZone("UTC")); On 2016/06/20 15:17:11, bengr wrote: > releaseDateOfM48Stable Done. https://codereview.chromium.org/2005623002/diff/250001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:67: long firstInstallTime = getPackageInstallTime(context); On 2016/06/20 15:17:11, bengr wrote: > firstInstallTime -> packageInstallTime Done. https://codereview.chromium.org/2005623002/diff/250001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:69: // The boolean pref storing if the user opted out on the First Run Experience promo was On 2016/06/20 15:17:11, bengr wrote: > Wording is a little confusing. How about: > > The boolean pref that stores whether the user opted out on the FRE was added in > M51. So if Chrome was installed after the FRE promo was added (M48) and before > M51, assume the user opted out from the FRE and don't show the infobar. Done. https://codereview.chromium.org/2005623002/diff/250001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:92: * @return The milestone of the given version string. On 2016/06/10 20:43:30, Raj wrote: > nit: pls add 'or -1 on failure.' Done. https://codereview.chromium.org/2005623002/diff/250001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:119: * @return The time at which this app was first installed in milliseconds since the epoch. On 2016/06/10 20:43:30, Raj wrote: > nit: or 0 if install time cannot be retrieved. Done. https://codereview.chromium.org/2005623002/diff/250001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoUtils.java (right): https://codereview.chromium.org/2005623002/diff/250001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoUtils.java:16: public class DataReductionPromoUtils { On 2016/06/20 15:17:12, bengr wrote: > Is there really no way to add tests to all of this? Every class should have unit > tests. Done. https://codereview.chromium.org/2005623002/diff/250001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoUtils.java:21: private static final String SHARED_PREF_DISPLAYED_PROMO = "displayed_data_reduction_fre_promo"; On 2016/06/10 20:43:30, Raj wrote: > nit: I wonder whether to group the FRE or secondrun related strings together and > have a common comment. > Since its not obvious DISPLAYED_PROMO, DISPLAYED_PROMO_TIME_MS, > DISPLAYED_PROMO_VERSION correspond to FRE or secondrun. > > // FRE or Second run promos. > private static final String SHARED_PREF_DISPLAYED_PROMO = > "displayed_data_reduction_fre_promo"; > private static final String SHARED_PREF_DISPLAYED_PROMO_TIME_MS = > "displayed_data_reduction_promo_time_ms"; > ... > > // Infobar promo. > ... Done. https://codereview.chromium.org/2005623002/diff/250001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoUtils.java:34: * proxy is allowed by the config, already on, or if the user is managed. On 2016/06/20 15:17:12, bengr wrote: > By what config? > > Why does it matter that the proxy pref is managed? > > Add answers to these questions to the comments. Done. https://codereview.chromium.org/2005623002/diff/250001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoUtils.java:48: * Saves shared prefs indicating that the data reduction proxy First Run Experience or second On 2016/06/20 15:17:12, bengr wrote: > Why do you capitalize First Run Experience, but not second run promo? I think > both should be lower case everywhere. I don't care if you abbreviate as FRE or > fre, but you should be consistent. Done. https://codereview.chromium.org/2005623002/diff/250001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoUtils.java:104: public static boolean getOptedOutOnFreCard() { On 2016/06/20 15:17:12, bengr wrote: > getOptedOutOnFrePromo Done.
megjablon@chromium.org changed reviewers: + dfalcantara@chromium.org
dfalcantara: For everything. PTAL, thanks!
lgtm with some suggestions. I defer to dfalcantara. https://codereview.chromium.org/2005623002/diff/300001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java (right): https://codereview.chromium.org/2005623002/diff/300001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:70: if (currentMilestone == -1) return; Maybe make this more restrictive? For instance, you know that a milestone less than 52 is invalid because the code hadn't landed before then. At the very least return if < 0. Also, will this ever be true? This seems more like something you'd want to check instead of silently return from. https://codereview.chromium.org/2005623002/diff/300001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:104: private static int getMilestoneFromVersionNumber(String version) { Is there a better file for this method? I would expect this to be with other methods that deal with versions. If not, I guess here is ok. https://codereview.chromium.org/2005623002/diff/300001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:106: return -1; Should this throw an exception instead of returning -1?
https://chromiumcodereview.appspot.com/2005623002/diff/300001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java (right): https://chromiumcodereview.appspot.com/2005623002/diff/300001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:70: if (currentMilestone == -1) return; On 2016/06/27 01:53:14, bengr wrote: > Maybe make this more restrictive? For instance, you know that a milestone less > than 52 is invalid because the code hadn't landed before then. At the very > least return if < 0. > > Also, will this ever be true? This seems more like something you'd want to check > instead of silently return from. Removed since I've added an illegal argument exception so this shouldn't need to be checked. https://chromiumcodereview.appspot.com/2005623002/diff/300001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:104: private static int getMilestoneFromVersionNumber(String version) { On 2016/06/27 01:53:14, bengr wrote: > Is there a better file for this method? I would expect this to be with other > methods that deal with versions. If not, I guess here is ok. I don't see anywhere better because we get the version string from native, but dfalcantara may know better. https://chromiumcodereview.appspot.com/2005623002/diff/300001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:106: return -1; On 2016/06/27 01:53:14, bengr wrote: > Should this throw an exception instead of returning -1? Done.
https://chromiumcodereview.appspot.com/2005623002/diff/300001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://chromiumcodereview.appspot.com/2005623002/diff/300001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:543: int statusCode) { This should be done in onDeferredStartupForActivity() instead. onDidNavigateMainFrame is called whenever any page starts loading, meaning that the promo logic will be janking up startup and every page load after that. https://chromiumcodereview.appspot.com/2005623002/diff/300001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java (right): https://chromiumcodereview.appspot.com/2005623002/diff/300001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:24: * Generates an InfoBar to promote the data reduction proxy. The InfoBar contains a message and a nit: Because of weird history with Android infobars, I believe the correct nomenclature is "infobar" in comments and "InfoBar" when linking to the class itself https://chromiumcodereview.appspot.com/2005623002/diff/300001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:74: long prackageInstallTime = getPackageInstallTime(context); prackage? https://chromiumcodereview.appspot.com/2005623002/diff/300001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:102: * @return The milestone of the given version string. -1 on failure. nit: Pull out -1 as a private static final int. https://chromiumcodereview.appspot.com/2005623002/diff/300001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:104: private static int getMilestoneFromVersionNumber(String version) { On 2016/06/27 18:25:20, megjablon wrote: > On 2016/06/27 01:53:14, bengr wrote: > > Is there a better file for this method? I would expect this to be with other > > methods that deal with versions. If not, I guess here is ok. > > I don't see anywhere better because we get the version string from native, but > dfalcantara may know better. Well, there's the VersionNumberGetter in the omaha package. Wouldn't hurt to put in there. https://chromiumcodereview.appspot.com/2005623002/diff/300001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:113: if (pieces.length != 4) { nit: put returns on the same lines to be consistent with your block above https://chromiumcodereview.appspot.com/2005623002/diff/300001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:130: * zero if the time cannot be retrieved. nit: Pull out zero into a constant https://chromiumcodereview.appspot.com/2005623002/diff/300001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoUtils.java (right): https://chromiumcodereview.appspot.com/2005623002/diff/300001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoUtils.java:60: AboutVersionStrings versionStrings = PrefServiceBridge.getInstance() nit: Put PrefServiceBridge.getInstance().getAboutVersionStrings() all on the second line? https://chromiumcodereview.appspot.com/2005623002/diff/300001/chrome/android/... File chrome/android/javatests/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoUtilsTest.java (right): https://chromiumcodereview.appspot.com/2005623002/diff/300001/chrome/android/... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoUtilsTest.java:27: loadNativeLibraryAndInitBrowserProcess(); You should be passing around an AdvancedMockContext, which will let you mock out the SharedPreferences. You're currently editing the Application's SharedPreferences directly, which could result in unforeseen consequences between test runs.
https://chromiumcodereview.appspot.com/2005623002/diff/300001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://chromiumcodereview.appspot.com/2005623002/diff/300001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:543: int statusCode) { On 2016/06/27 18:49:22, dfalcantara wrote: > This should be done in onDeferredStartupForActivity() instead. > onDidNavigateMainFrame is called whenever any page starts loading, meaning that > the promo logic will be janking up startup and every page load after that. We want this to be displayed only when the user navigates to a page that fits certain criteria. See https://docs.google.com/a/google.com/document/d/1IEvXX6ZSV7npIFyS_xt8Xa7i10i5... How expensive is checking a pref? We can move up that check in maybeLaunchPromoInfoBar so that it immediately returns once the promo has been shown. https://chromiumcodereview.appspot.com/2005623002/diff/300001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java (right): https://chromiumcodereview.appspot.com/2005623002/diff/300001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:24: * Generates an InfoBar to promote the data reduction proxy. The InfoBar contains a message and a On 2016/06/27 18:49:22, dfalcantara wrote: > nit: Because of weird history with Android infobars, I believe the correct > nomenclature is "infobar" in comments and "InfoBar" when linking to the class > itself Done. https://chromiumcodereview.appspot.com/2005623002/diff/300001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:74: long prackageInstallTime = getPackageInstallTime(context); On 2016/06/27 18:49:22, dfalcantara wrote: > prackage? Done. https://chromiumcodereview.appspot.com/2005623002/diff/300001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:102: * @return The milestone of the given version string. -1 on failure. On 2016/06/27 18:49:22, dfalcantara wrote: > nit: Pull out -1 as a private static final int. Acknowledged. https://chromiumcodereview.appspot.com/2005623002/diff/300001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:104: private static int getMilestoneFromVersionNumber(String version) { On 2016/06/27 18:49:22, dfalcantara wrote: > On 2016/06/27 18:25:20, megjablon wrote: > > On 2016/06/27 01:53:14, bengr wrote: > > > Is there a better file for this method? I would expect this to be with other > > > methods that deal with versions. If not, I guess here is ok. > > > > I don't see anywhere better because we get the version string from native, but > > dfalcantara may know better. > > Well, there's the VersionNumberGetter in the omaha package. Wouldn't hurt to > put in there. Done. https://chromiumcodereview.appspot.com/2005623002/diff/300001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:113: if (pieces.length != 4) { On 2016/06/27 18:49:22, dfalcantara wrote: > nit: put returns on the same lines to be consistent with your block above Acknowledged. https://chromiumcodereview.appspot.com/2005623002/diff/300001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:130: * zero if the time cannot be retrieved. On 2016/06/27 18:49:22, dfalcantara wrote: > nit: Pull out zero into a constant Done. https://chromiumcodereview.appspot.com/2005623002/diff/300001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoUtils.java (right): https://chromiumcodereview.appspot.com/2005623002/diff/300001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoUtils.java:60: AboutVersionStrings versionStrings = PrefServiceBridge.getInstance() On 2016/06/27 18:49:22, dfalcantara wrote: > nit: Put PrefServiceBridge.getInstance().getAboutVersionStrings() all on the > second line? Done. https://chromiumcodereview.appspot.com/2005623002/diff/300001/chrome/android/... File chrome/android/javatests/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoUtilsTest.java (right): https://chromiumcodereview.appspot.com/2005623002/diff/300001/chrome/android/... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoUtilsTest.java:27: loadNativeLibraryAndInitBrowserProcess(); On 2016/06/27 18:49:22, dfalcantara wrote: > You should be passing around an AdvancedMockContext, which will let you mock out > the SharedPreferences. You're currently editing the Application's > SharedPreferences directly, which could result in unforeseen consequences > between test runs. I set the AdvancedMockContext using ContextUtils.initApplicationContextForTests. This seems like it should work rather than passing it around.
https://chromiumcodereview.appspot.com/2005623002/diff/300001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://chromiumcodereview.appspot.com/2005623002/diff/300001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:543: int statusCode) { On 2016/06/27 20:17:02, megjablon wrote: > On 2016/06/27 18:49:22, dfalcantara wrote: > > This should be done in onDeferredStartupForActivity() instead. > > onDidNavigateMainFrame is called whenever any page starts loading, meaning > that > > the promo logic will be janking up startup and every page load after that. > > We want this to be displayed only when the user navigates to a page that fits > certain criteria. See > https://docs.google.com/a/google.com/document/d/1IEvXX6ZSV7npIFyS_xt8Xa7i10i5... > > How expensive is checking a pref? We can move up that check in > maybeLaunchPromoInfoBar so that it immediately returns once the promo has been > shown. Not too expensive: after the first read of the shared_preferences.xml file they are all cached in memory. It's probably less expensive than the scheme equality check. I'm mainly concerned because you're putting this into ChromeActivity. This is the base class for _everything_, including help pages, custom tabs, and web apps. You're adding a check for a one-time promotion for every single page load across all of chrome, ever. There really should be a better place or way to do this. https://chromiumcodereview.appspot.com/2005623002/diff/300001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java (right): https://chromiumcodereview.appspot.com/2005623002/diff/300001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:42: * document (for example scrolling to a named anchoror PopState). anchoror https://chromiumcodereview.appspot.com/2005623002/diff/300001/chrome/android/... File chrome/android/javatests/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoUtilsTest.java (right): https://chromiumcodereview.appspot.com/2005623002/diff/300001/chrome/android/... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoUtilsTest.java:27: loadNativeLibraryAndInitBrowserProcess(); On 2016/06/27 20:17:02, megjablon wrote: > On 2016/06/27 18:49:22, dfalcantara wrote: > > You should be passing around an AdvancedMockContext, which will let you mock > out > > the SharedPreferences. You're currently editing the Application's > > SharedPreferences directly, which could result in unforeseen consequences > > between test runs. > > I set the AdvancedMockContext using ContextUtils.initApplicationContextForTests. > This seems like it should work rather than passing it around. Huh, neat. Guess that call is pretty new.
https://chromiumcodereview.appspot.com/2005623002/diff/300001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://chromiumcodereview.appspot.com/2005623002/diff/300001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:543: int statusCode) { On 2016/06/27 21:10:49, dfalcantara wrote: > On 2016/06/27 20:17:02, megjablon wrote: > > On 2016/06/27 18:49:22, dfalcantara wrote: > > > This should be done in onDeferredStartupForActivity() instead. > > > onDidNavigateMainFrame is called whenever any page starts loading, meaning > > that > > > the promo logic will be janking up startup and every page load after that. > > > > We want this to be displayed only when the user navigates to a page that fits > > certain criteria. See > > > https://docs.google.com/a/google.com/document/d/1IEvXX6ZSV7npIFyS_xt8Xa7i10i5... > > > > How expensive is checking a pref? We can move up that check in > > maybeLaunchPromoInfoBar so that it immediately returns once the promo has been > > shown. > > Not too expensive: after the first read of the shared_preferences.xml file they > are all cached in memory. It's probably less expensive than the scheme equality > check. > > I'm mainly concerned because you're putting this into ChromeActivity. This is > the base class for _everything_, including help pages, custom tabs, and web > apps. You're adding a check for a one-time promotion for every single page load > across all of chrome, ever. There really should be a better place or way to do > this. Are you checking if the tab is Incognito. IIRC data saver isn't enabled for incognito tabs; popping an infobar up in that situation seems like it'd send the wrong message.
Patchset #9 (id:380001) has been deleted
On 2016/06/27 21:18:21, dfalcantara wrote: > https://chromiumcodereview.appspot.com/2005623002/diff/300001/chrome/android/... > File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java > (right): > > https://chromiumcodereview.appspot.com/2005623002/diff/300001/chrome/android/... > chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:543: int > statusCode) { > On 2016/06/27 21:10:49, dfalcantara wrote: > > On 2016/06/27 20:17:02, megjablon wrote: > > > On 2016/06/27 18:49:22, dfalcantara wrote: > > > > This should be done in onDeferredStartupForActivity() instead. > > > > onDidNavigateMainFrame is called whenever any page starts loading, meaning > > > that > > > > the promo logic will be janking up startup and every page load after that. > > > > > > We want this to be displayed only when the user navigates to a page that > fits > > > certain criteria. See > > > > > > https://docs.google.com/a/google.com/document/d/1IEvXX6ZSV7npIFyS_xt8Xa7i10i5... > > > > > > How expensive is checking a pref? We can move up that check in > > > maybeLaunchPromoInfoBar so that it immediately returns once the promo has > been > > > shown. > > > > Not too expensive: after the first read of the shared_preferences.xml file > they > > are all cached in memory. It's probably less expensive than the scheme > equality > > check. > > > > I'm mainly concerned because you're putting this into ChromeActivity. This is > > the base class for _everything_, including help pages, custom tabs, and web > > apps. You're adding a check for a one-time promotion for every single page > load > > across all of chrome, ever. There really should be a better place or way to > do > > this. > > Are you checking if the tab is Incognito. IIRC data saver isn't enabled for > incognito tabs; popping an infobar up in that situation seems like it'd send the > wrong message. Added incognito check (good call) and moved to ChromeTabbedActivity
lgtm https://chromiumcodereview.appspot.com/2005623002/diff/400001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java (right): https://chromiumcodereview.appspot.com/2005623002/diff/400001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:44: * document (for example scrolling to a named anchoror PopState). nit: anchoror
The CQ bit was checked by megjablon@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rajendrant@chromium.org, bengr@chromium.org, dfalcantara@chromium.org Link to the patchset: https://chromiumcodereview.appspot.com/2005623002/#ps440001 (title: "missing returns")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Logic for the Data Saver InfoBar Promo For non-users of Data Saver, show a promotional InfoBar if the following are true: The user in the DataReductionProxyInfobarPromo field trial. The user doesn’t have the Data Reduction Proxy enabled. The user saw the promo in a version >= M51 or installed before the M48 release date 1/26/16. The user didn’t opt out of the FRE promo card. The current version is at least two milestones after the last promo was displayed. The infobar has never been shown and dismissed before. The request is HTTP. BUG=610757 ========== to ========== Logic for the Data Saver InfoBar Promo For non-users of Data Saver, show a promotional InfoBar if the following are true: The user in the DataReductionProxyInfobarPromo field trial. The user doesn’t have the Data Reduction Proxy enabled. The user saw the promo in a version >= M51 or installed before the M48 release date 1/26/16. The user didn’t opt out of the FRE promo card. The current version is at least two milestones after the last promo was displayed. The infobar has never been shown and dismissed before. The request is HTTP. BUG=610757 ==========
Message was sent while issue was closed.
Committed patchset #11 (id:440001)
Message was sent while issue was closed.
Description was changed from ========== Logic for the Data Saver InfoBar Promo For non-users of Data Saver, show a promotional InfoBar if the following are true: The user in the DataReductionProxyInfobarPromo field trial. The user doesn’t have the Data Reduction Proxy enabled. The user saw the promo in a version >= M51 or installed before the M48 release date 1/26/16. The user didn’t opt out of the FRE promo card. The current version is at least two milestones after the last promo was displayed. The infobar has never been shown and dismissed before. The request is HTTP. BUG=610757 ========== to ========== Logic for the Data Saver InfoBar Promo For non-users of Data Saver, show a promotional InfoBar if the following are true: The user in the DataReductionProxyInfobarPromo field trial. The user doesn’t have the Data Reduction Proxy enabled. The user saw the promo in a version >= M51 or installed before the M48 release date 1/26/16. The user didn’t opt out of the FRE promo card. The current version is at least two milestones after the last promo was displayed. The infobar has never been shown and dismissed before. The request is HTTP. BUG=610757 Committed: https://crrev.com/3f07536d655fbf235b3c3909c6a19d41f3913f51 Cr-Commit-Position: refs/heads/master@{#402522} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/3f07536d655fbf235b3c3909c6a19d41f3913f51 Cr-Commit-Position: refs/heads/master@{#402522} |
