Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1339613004:
Add Data Reduction Proxy card to Clank FRE in EM (Closed)
DescriptionAdd Data Reduction Proxy card to Clank FRE in EM
The card enables the Data Reduction Proxy by default and will be
shown in emerging markets via the DataReductionProxyFREPromo field trial.
BUG=511599, 535756
Committed: https://crrev.com/bb2b200f796a348a235eb953b501452cb228d80f
Cr-Commit-Position: refs/heads/master@{#351252}
Patch Set 1 : #Patch Set 2 : #
Total comments: 4
Patch Set 3 : fixing strings #
Total comments: 16
Patch Set 4 : addressing comments #
Total comments: 13
Patch Set 5 : ui fixes and comments #
Total comments: 9
Patch Set 6 : comments #
Total comments: 6
Patch Set 7 : nits #
Total comments: 4
Patch Set 8 : comments #Messages
Total messages: 27 (7 generated)
Patchset #1 (id:1) has been deleted
megjablon@chromium.org changed reviewers: + newt@chromium.org
newt@chromium.org changed reviewers: + aurimas@chromium.org
aurimas: Could you take a look since you're the first run UI expert? I just looked at android_chrome_strings.grd. https://codereview.chromium.org/1339613004/diff/40001/chrome/android/java/str... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/1339613004/diff/40001/chrome/android/java/str... chrome/android/java/strings/android_chrome_strings.grd:761: Data Saver Enabled use "Sentence case", e.g. "Data Saver enabled" with a lowecase "e". That's the Android platform default, and we use it consistently in Chrome for Android. https://codereview.chromium.org/1339613004/diff/40001/chrome/android/java/str... chrome/android/java/strings/android_chrome_strings.grd:1570: <message name="IDS_FRE_NEXT" desc="Text for Data Reduction Proxy page next button"> I'd add this at the top of this file under the "generic strings" section (and make the description general, of course). Also, you probably want [CHAR-LIMIT=20] since this is button text. (See the CHAR-LIMIT guidelines at the top of this file.)
https://codereview.chromium.org/1339613004/diff/40001/chrome/android/java/str... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/1339613004/diff/40001/chrome/android/java/str... chrome/android/java/strings/android_chrome_strings.grd:761: Data Saver Enabled On 2015/09/23 17:15:06, newt wrote: > use "Sentence case", e.g. "Data Saver enabled" with a lowecase "e". That's the > Android platform default, and we use it consistently in Chrome for Android. Done. https://codereview.chromium.org/1339613004/diff/40001/chrome/android/java/str... chrome/android/java/strings/android_chrome_strings.grd:1570: <message name="IDS_FRE_NEXT" desc="Text for Data Reduction Proxy page next button"> On 2015/09/23 17:15:06, newt wrote: > I'd add this at the top of this file under the "generic strings" section (and > make the description general, of course). Also, you probably want > [CHAR-LIMIT=20] since this is button text. (See the CHAR-LIMIT guidelines at the > top of this file.) Done.
https://codereview.chromium.org/1339613004/diff/60001/chrome/android/java/res... File chrome/android/java/res/layout/fre_data_reduction_proxy.xml (right): https://codereview.chromium.org/1339613004/diff/60001/chrome/android/java/res... chrome/android/java/res/layout/fre_data_reduction_proxy.xml:7: <org.chromium.chrome.browser.firstrun.DataReductionProxyView Where can I find mocks for what it is supposed to look like? https://codereview.chromium.org/1339613004/diff/60001/chrome/android/java/res... chrome/android/java/res/layout/fre_data_reduction_proxy.xml:54: android:id="@+id/data_reduction_title_2" Why does the title move from above to bellow the illustration? We don't do that in any other FRE screen. https://codereview.chromium.org/1339613004/diff/60001/chrome/android/java/res... chrome/android/java/res/layout/fre_data_reduction_proxy.xml:110: android:gravity="end|center_vertical" Why is button text at the end instead of center? If there is only one button it should show in the center like ToS screen. https://codereview.chromium.org/1339613004/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunActivity.java (right): https://codereview.chromium.org/1339613004/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunActivity.java:106: if (FieldTrialList.findFullName("DataReductionProxyFREPromo").equals("Enabled")) { I don't think finch is available this early into loading process? User hasn't accepted ToS so there is no way we are fetching finch configs for them. https://codereview.chromium.org/1339613004/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunActivity.java:254: RecordUserAction.record("MobileFre.FreShown"); Why do we need this additional histogram? https://codereview.chromium.org/1339613004/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunActivity.java:302: Log.w("TEST USER ACTIONS", "MobileFre.DataReductionProxyCardShown"); Why is this a warning? https://codereview.chromium.org/1339613004/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunActivity.java:305: RecordUserAction.record("MobileFre.DataReductionProxyDisabled"); Why mark it as action? Let's make it a histogram with the choice that they made.
https://codereview.chromium.org/1339613004/diff/60001/chrome/android/java/res... File chrome/android/java/res/layout/fre_data_reduction_proxy.xml (right): https://codereview.chromium.org/1339613004/diff/60001/chrome/android/java/res... chrome/android/java/res/layout/fre_data_reduction_proxy.xml:7: <org.chromium.chrome.browser.firstrun.DataReductionProxyView On 2015/09/24 18:28:08, aurimas wrote: > Where can I find mocks for what it is supposed to look like? https://docs.google.com/document/d/1JaHECNpOGb-Kf8I-Bcc0tcz7Hyt6oWjtEjA4MiBIN... https://codereview.chromium.org/1339613004/diff/60001/chrome/android/java/res... chrome/android/java/res/layout/fre_data_reduction_proxy.xml:54: android:id="@+id/data_reduction_title_2" On 2015/09/24 18:28:08, aurimas wrote: > Why does the title move from above to bellow the illustration? We don't do that > in any other FRE screen. The title on the vertical screen is below the image, but it doesn't make sense to have it "below" the image, which would just be to the right instead of across the top, in the horizontal view. https://codereview.chromium.org/1339613004/diff/60001/chrome/android/java/res... chrome/android/java/res/layout/fre_data_reduction_proxy.xml:110: android:gravity="end|center_vertical" On 2015/09/24 18:28:08, aurimas wrote: > Why is button text at the end instead of center? If there is only one button it > should show in the center like ToS screen. See mock. https://codereview.chromium.org/1339613004/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunActivity.java (right): https://codereview.chromium.org/1339613004/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunActivity.java:106: if (FieldTrialList.findFullName("DataReductionProxyFREPromo").equals("Enabled")) { On 2015/09/24 18:28:08, aurimas wrote: > I don't think finch is available this early into loading process? User hasn't > accepted ToS so there is no way we are fetching finch configs for them. We're aware of this. M48 is expected to have this functionality. https://docs.google.com/document/d/1KS8QlEe-Ev2AJm5DheIwvpBeGCJtBmO_Vmpa0Wg2d... https://codereview.chromium.org/1339613004/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunActivity.java:254: RecordUserAction.record("MobileFre.FreShown"); On 2015/09/24 18:28:08, aurimas wrote: > Why do we need this additional histogram? So we can get the % of FREs that have the Data Reduction Proxy card. https://codereview.chromium.org/1339613004/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunActivity.java:302: Log.w("TEST USER ACTIONS", "MobileFre.DataReductionProxyCardShown"); On 2015/09/24 18:28:08, aurimas wrote: > Why is this a warning? Ah deleted the wrong line! Was using to verify. Removed and added the metrics. https://codereview.chromium.org/1339613004/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunActivity.java:305: RecordUserAction.record("MobileFre.DataReductionProxyDisabled"); On 2015/09/24 18:28:08, aurimas wrote: > Why mark it as action? Let's make it a histogram with the choice that they made. Done.
Patchset #4 (id:80001) has been deleted
https://codereview.chromium.org/1339613004/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunActivity.java (right): https://codereview.chromium.org/1339613004/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunActivity.java:106: if (FieldTrialList.findFullName("DataReductionProxyFREPromo").equals("Enabled")) { On 2015/09/24 at 19:17:28, megjablon wrote: > On 2015/09/24 18:28:08, aurimas wrote: > > I don't think finch is available this early into loading process? User hasn't > > accepted ToS so there is no way we are fetching finch configs for them. > > We're aware of this. M48 is expected to have this functionality. https://docs.google.com/document/d/1KS8QlEe-Ev2AJm5DheIwvpBeGCJtBmO_Vmpa0Wg2d... According to the doc finch will fetch after EULA is accepted. You added this code to run before EULA is accepted. https://codereview.chromium.org/1339613004/diff/100001/chrome/android/java/re... File chrome/android/java/res/layout/fre_data_reduction_proxy.xml (right): https://codereview.chromium.org/1339613004/diff/100001/chrome/android/java/re... chrome/android/java/res/layout/fre_data_reduction_proxy.xml:60: android:text="@string/data_reduction_promo_title" Why do we use two different titles for landscape and portrait? https://codereview.chromium.org/1339613004/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/firstrun/DataReductionProxyFirstRunFragment.java (right): https://codereview.chromium.org/1339613004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/firstrun/DataReductionProxyFirstRunFragment.java:41: mNextButton = (Button) view.findViewById(R.id.next_button); This button is used once, we dont need to make it into a member variable, just scope it to this method. Same with the switch. https://codereview.chromium.org/1339613004/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunActivity.java (right): https://codereview.chromium.org/1339613004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunActivity.java:254: RecordUserAction.record("MobileFre.FreShown"); This is already covered by MobileFre.SignInChoice https://codereview.chromium.org/1339613004/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoScreen.java (right): https://codereview.chromium.org/1339613004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoScreen.java:57: // Only show the promo if the FRE card was not shown. If we treat them the same way, why do we have a different pref for this promo vs fre promo? https://codereview.chromium.org/1339613004/diff/100001/chrome/android/java/st... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/1339613004/diff/100001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:763: <message name="IDS_DATA_REDUCTION_ENABLE_SWITCH" desc="Text for the switch the user presses if they want to enable/disable Data Saver" > "Text for the switch that the user presses if they want to enable/disable Data Saver feature" https://codereview.chromium.org/1339613004/diff/100001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:764: Data Saver enabled We should switch the text for the switch to "Data Saver disabled". We do that in settings for toggles.
https://codereview.chromium.org/1339613004/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunActivity.java (right): https://codereview.chromium.org/1339613004/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunActivity.java:106: if (FieldTrialList.findFullName("DataReductionProxyFREPromo").equals("Enabled")) { On 2015/09/24 20:59:10, aurimas wrote: > On 2015/09/24 at 19:17:28, megjablon wrote: > > On 2015/09/24 18:28:08, aurimas wrote: > > > I don't think finch is available this early into loading process? User > hasn't > > > accepted ToS so there is no way we are fetching finch configs for them. > > > > We're aware of this. M48 is expected to have this functionality. > https://docs.google.com/document/d/1KS8QlEe-Ev2AJm5DheIwvpBeGCJtBmO_Vmpa0Wg2d... > > According to the doc finch will fetch after EULA is accepted. You added this > code to run before EULA is accepted. This is the EULA agreement in the Android set up. That will kick off the finch download before Chrome is even opened for the first time. https://codereview.chromium.org/1339613004/diff/100001/chrome/android/java/re... File chrome/android/java/res/layout/fre_data_reduction_proxy.xml (right): https://codereview.chromium.org/1339613004/diff/100001/chrome/android/java/re... chrome/android/java/res/layout/fre_data_reduction_proxy.xml:60: android:text="@string/data_reduction_promo_title" On 2015/09/24 20:59:10, aurimas wrote: > Why do we use two different titles for landscape and portrait? Done. https://codereview.chromium.org/1339613004/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/firstrun/DataReductionProxyFirstRunFragment.java (right): https://codereview.chromium.org/1339613004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/firstrun/DataReductionProxyFirstRunFragment.java:41: mNextButton = (Button) view.findViewById(R.id.next_button); On 2015/09/24 20:59:10, aurimas wrote: > This button is used once, we dont need to make it into a member variable, just > scope it to this method. > > Same with the switch. Done. https://codereview.chromium.org/1339613004/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunActivity.java (right): https://codereview.chromium.org/1339613004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunActivity.java:254: RecordUserAction.record("MobileFre.FreShown"); On 2015/09/24 20:59:10, aurimas wrote: > This is already covered by MobileFre.SignInChoice And this is reported even if the sign in card isn't shown, correct? https://codereview.chromium.org/1339613004/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoScreen.java (right): https://codereview.chromium.org/1339613004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoScreen.java:57: // Only show the promo if the FRE card was not shown. On 2015/09/24 20:59:10, aurimas wrote: > If we treat them the same way, why do we have a different pref for this promo vs > fre promo? Done. https://codereview.chromium.org/1339613004/diff/100001/chrome/android/java/st... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/1339613004/diff/100001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:763: <message name="IDS_DATA_REDUCTION_ENABLE_SWITCH" desc="Text for the switch the user presses if they want to enable/disable Data Saver" > On 2015/09/24 20:59:10, aurimas wrote: > "Text for the switch that the user presses if they want to enable/disable Data > Saver feature" Done. https://codereview.chromium.org/1339613004/diff/100001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:764: Data Saver enabled On 2015/09/24 20:59:10, aurimas wrote: > We should switch the text for the switch to "Data Saver disabled". We do that in > settings for toggles. Done.
https://codereview.chromium.org/1339613004/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunActivity.java (right): https://codereview.chromium.org/1339613004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunActivity.java:254: RecordUserAction.record("MobileFre.FreShown"); On 2015/09/24 at 23:40:34, megjablon wrote: > On 2015/09/24 20:59:10, aurimas wrote: > > This is already covered by MobileFre.SignInChoice > > And this is reported even if the sign in card isn't shown, correct? Yes, you can look within this file. https://codereview.chromium.org/1339613004/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/firstrun/DataReductionProxyFirstRunFragment.java (right): https://codereview.chromium.org/1339613004/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/firstrun/DataReductionProxyFirstRunFragment.java:34: final Switch mEnableDataSaverSwitch = (Switch) view s/mEnableDataSaverSwitch/enableDataSaverSwitch https://codereview.chromium.org/1339613004/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/firstrun/DataReductionProxyFirstRunFragment.java:36: Button mNextButton = (Button) view.findViewById(R.id.next_button); s/mNextButton/nextButton https://codereview.chromium.org/1339613004/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunActivity.java (right): https://codereview.chromium.org/1339613004/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunActivity.java:108: if (FieldTrialList.findFullName("DataReductionProxyFREPromo").equals("Enabled")) { > This is the EULA agreement in the Android set up. That will kick off the finch download before Chrome is even opened for the first time. That mean this will only work for devices that ship with Chrome M48 preinstalled because Android setup happens way before play store updates kick off. Are we ok with that? Why aren't we doing a client side check who to show this to? https://codereview.chromium.org/1339613004/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunActivity.java:256: RecordUserAction.record("MobileFre.FreShown"); Remove this action.
https://codereview.chromium.org/1339613004/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/firstrun/DataReductionProxyFirstRunFragment.java (right): https://codereview.chromium.org/1339613004/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/firstrun/DataReductionProxyFirstRunFragment.java:34: final Switch mEnableDataSaverSwitch = (Switch) view On 2015/09/25 23:03:35, aurimas wrote: > s/mEnableDataSaverSwitch/enableDataSaverSwitch Done. https://codereview.chromium.org/1339613004/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/firstrun/DataReductionProxyFirstRunFragment.java:36: Button mNextButton = (Button) view.findViewById(R.id.next_button); On 2015/09/25 23:03:35, aurimas wrote: > s/mNextButton/nextButton Done. https://codereview.chromium.org/1339613004/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunActivity.java (right): https://codereview.chromium.org/1339613004/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunActivity.java:108: if (FieldTrialList.findFullName("DataReductionProxyFREPromo").equals("Enabled")) { On 2015/09/25 23:03:35, aurimas wrote: > > This is the EULA agreement in the Android set up. That will kick off the finch > download before Chrome is even opened for the first time. > That mean this will only work for devices that ship with Chrome M48 preinstalled > because Android setup happens way before play store updates kick off. Are we ok > with that? Why aren't we doing a client side check who to show this to? We're ok with this because any time we can't show the FRE card we have the promo as backup. Since this card enables Data Saver by default, it's only targeted to EM countries which is why we use the Finch config. https://codereview.chromium.org/1339613004/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunActivity.java:256: RecordUserAction.record("MobileFre.FreShown"); On 2015/09/25 23:03:35, aurimas wrote: > Remove this action. Done.
https://codereview.chromium.org/1339613004/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunActivity.java (right): https://codereview.chromium.org/1339613004/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunActivity.java:108: if (FieldTrialList.findFullName("DataReductionProxyFREPromo").equals("Enabled")) { On 2015/09/25 at 23:21:42, megjablon wrote: > On 2015/09/25 23:03:35, aurimas wrote: > > > This is the EULA agreement in the Android set up. That will kick off the finch > > download before Chrome is even opened for the first time. > > That mean this will only work for devices that ship with Chrome M48 preinstalled > > because Android setup happens way before play store updates kick off. Are we ok > > with that? Why aren't we doing a client side check who to show this to? > > We're ok with this because any time we can't show the FRE card we have the promo as backup. Since this card enables Data Saver by default, it's only targeted to EM countries which is why we use the Finch config. As long as you realize that means that this will work only on new devices that ship the right version, so it will not see the uptick for up to possibly a year when people in EM countries start buying such devices. Locally we can target EM countries locally by checking country by other means than finch like getNetworkCountryIso() http://developer.android.com/reference/android/telephony/TelephonyManager.htm...
On 2015/09/25 23:28:16, aurimas wrote: > https://codereview.chromium.org/1339613004/diff/120001/chrome/android/java/sr... > File > chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunActivity.java > (right): > > https://codereview.chromium.org/1339613004/diff/120001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunActivity.java:108: > if (FieldTrialList.findFullName("DataReductionProxyFREPromo").equals("Enabled")) > { > On 2015/09/25 at 23:21:42, megjablon wrote: > > On 2015/09/25 23:03:35, aurimas wrote: > > > > This is the EULA agreement in the Android set up. That will kick off the > finch > > > download before Chrome is even opened for the first time. > > > That mean this will only work for devices that ship with Chrome M48 > preinstalled > > > because Android setup happens way before play store updates kick off. Are we > ok > > > with that? Why aren't we doing a client side check who to show this to? > > > > We're ok with this because any time we can't show the FRE card we have the > promo as backup. Since this card enables Data Saver by default, it's only > targeted to EM countries which is why we use the Finch config. > > As long as you realize that means that this will work only on new devices that > ship the right version, so it will not see the uptick for up to possibly a year > when people in EM countries start buying such devices. > > Locally we can target EM countries locally by checking country by other means > than finch like getNetworkCountryIso() > http://developer.android.com/reference/android/telephony/TelephonyManager.htm... The user will still need to have the version this lands in when they go through the FRE so using getNetworkCountryIso() wouldn't help us much in that respect. Using a Finch config gives us control over what percent is seeing this so we can slowly ramp it up and have control to turn it off if we get too much new traffic too quickly.
LGTM % nits https://chromiumcodereview.appspot.com/1339613004/diff/140001/chrome/android/... File chrome/android/java/res/layout/fre_data_reduction_proxy.xml (right): https://chromiumcodereview.appspot.com/1339613004/diff/140001/chrome/android/... chrome/android/java/res/layout/fre_data_reduction_proxy.xml:41: android:orientation="vertical" > I was under impression that was the default value for the orientation, no? https://chromiumcodereview.appspot.com/1339613004/diff/140001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoScreen.java (right): https://chromiumcodereview.appspot.com/1339613004/diff/140001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoScreen.java:146: public static boolean getDisplayedDataReductionPromo(Context context) { Add javadocs https://chromiumcodereview.appspot.com/1339613004/diff/140001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoScreen.java:151: public static void setDisplayedDataReductionPromo(Context context, boolean displayed) { Add javadocs
megjablon@chromium.org changed reviewers: + asvitkine@chromium.org
asvitkine: histograms.xml newt: DataReductionPromoScreen, DataReductionProxyUma, and android_chrome_strings.grd https://chromiumcodereview.appspot.com/1339613004/diff/140001/chrome/android/... File chrome/android/java/res/layout/fre_data_reduction_proxy.xml (right): https://chromiumcodereview.appspot.com/1339613004/diff/140001/chrome/android/... chrome/android/java/res/layout/fre_data_reduction_proxy.xml:41: android:orientation="vertical" > On 2015/09/28 17:31:00, aurimas wrote: > I was under impression that was the default value for the orientation, no? Done. https://chromiumcodereview.appspot.com/1339613004/diff/140001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoScreen.java (right): https://chromiumcodereview.appspot.com/1339613004/diff/140001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoScreen.java:146: public static boolean getDisplayedDataReductionPromo(Context context) { On 2015/09/28 17:31:00, aurimas wrote: > Add javadocs Done. https://chromiumcodereview.appspot.com/1339613004/diff/140001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoScreen.java:151: public static void setDisplayedDataReductionPromo(Context context, boolean displayed) { On 2015/09/28 17:31:00, aurimas wrote: > Add javadocs Done.
asvitkine: histograms.xml newt: DataReductionPromoScreen, DataReductionProxyUma, and android_chrome_strings.grd
lgtm % comment https://chromiumcodereview.appspot.com/1339613004/diff/160001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunActivity.java (right): https://chromiumcodereview.appspot.com/1339613004/diff/160001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunActivity.java:108: if (FieldTrialList.findFullName("DataReductionProxyFREPromo").equals("Enabled")) { Our best practice is to use startsWith(). This allows you to have multiple group names to use the enabled behavior.
lgtm with comment https://chromiumcodereview.appspot.com/1339613004/diff/160001/chrome/android/... File chrome/android/java/res/layout/fre_data_reduction_proxy.xml (right): https://chromiumcodereview.appspot.com/1339613004/diff/160001/chrome/android/... chrome/android/java/res/layout/fre_data_reduction_proxy.xml:79: android:textStyle="bold" /> Bold? not Roboto Medium (i.e. "android:fontFamily="sans-serif-medium")? Roboto Medium is what we often use instead of bold.
https://codereview.chromium.org/1339613004/diff/160001/chrome/android/java/re... File chrome/android/java/res/layout/fre_data_reduction_proxy.xml (right): https://codereview.chromium.org/1339613004/diff/160001/chrome/android/java/re... chrome/android/java/res/layout/fre_data_reduction_proxy.xml:79: android:textStyle="bold" /> On 2015/09/28 20:12:08, newt wrote: > Bold? not Roboto Medium (i.e. "android:fontFamily="sans-serif-medium")? > > Roboto Medium is what we often use instead of bold. Done. https://codereview.chromium.org/1339613004/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunActivity.java (right): https://codereview.chromium.org/1339613004/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunActivity.java:108: if (FieldTrialList.findFullName("DataReductionProxyFREPromo").equals("Enabled")) { On 2015/09/28 18:52:08, Alexei Svitkine wrote: > Our best practice is to use startsWith(). This allows you to have multiple group > names to use the enabled behavior. Done.
The CQ bit was checked by megjablon@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from aurimas@chromium.org, asvitkine@chromium.org, newt@chromium.org Link to the patchset: https://codereview.chromium.org/1339613004/#ps180001 (title: "comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1339613004/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1339613004/180001
Message was sent while issue was closed.
Committed patchset #8 (id:180001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/bb2b200f796a348a235eb953b501452cb228d80f Cr-Commit-Position: refs/heads/master@{#351252} |
