So that users can opt-in to use the new tablet UI.

Comment on attachment 8468611 [details] [diff] [review] Add UI to enable/disable the new tablet UI (r=mcomella) Strings are hardcoded because they are not meant to be official l10n material.

Comment on attachment 8468611 [details] [diff] [review] Add UI to enable/disable the new tablet UI (r=mcomella) Review of attachment 8468611 [details] [diff] [review]: ----------------------------------------------------------------- r+ w/ nits. ::: mobile/android/base/resources/xml/preferences_display.xml @@ +24,5 @@ > <CheckBoxPreference android:key="browser.chrome.dynamictoolbar" > android:title="@string/pref_scroll_title_bar2" > android:summary="@string/pref_scroll_title_bar_summary" /> > > + <!-- Title string is hardcoded here because it's not meant to be translated --> nit: Add why is it not meant to be translated. @@ +27,5 @@ > > + <!-- Title string is hardcoded here because it's not meant to be translated --> > + <CheckBoxPreference android:key="android.not_a_preference.new_tablet_ui" > + android:title="Enable new tablet UI" > + android:defaultValue="true" /> This is duplicated here and in the patch in bug 1046200. I imagine it's because we don't seem to be calling `PreferenceManager.setDefaultValues` anywhere (see [1]). I'd make a note in the other patch that this value should be updated too when merging to m-c. [1]: http://stackoverflow.com/a/3911077

Comment on attachment 8468611 [details] [diff] [review] Add UI to enable/disable the new tablet UI (r=mcomella) Review of attachment 8468611 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/resources/xml/preferences_display.xml @@ +27,5 @@ > > + <!-- Title string is hardcoded here because it's not meant to be translated --> > + <CheckBoxPreference android:key="android.not_a_preference.new_tablet_ui" > + android:title="Enable new tablet UI" > + android:defaultValue="true" /> Actually, does this defaultValue even do anything without calling PM.setDefaultValues?

(In reply to Michael Comella (:mcomella) from comment #4) > Comment on attachment 8468611 [details] [diff] [review] > Add UI to enable/disable the new tablet UI (r=mcomella) > > Review of attachment 8468611 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: mobile/android/base/resources/xml/preferences_display.xml > @@ +27,5 @@ > > > > + <!-- Title string is hardcoded here because it's not meant to be translated --> > > + <CheckBoxPreference android:key="android.not_a_preference.new_tablet_ui" > > + android:title="Enable new tablet UI" > > + android:defaultValue="true" /> > > Actually, does this defaultValue even do anything without calling > PM.setDefaultValues? Apparently, it does.

(In reply to Michael Comella (:mcomella) from comment #3) > Comment on attachment 8468611 [details] [diff] [review] > Add UI to enable/disable the new tablet UI (r=mcomella) > > Review of attachment 8468611 [details] [diff] [review]: > ----------------------------------------------------------------- > > r+ w/ nits. > > ::: mobile/android/base/resources/xml/preferences_display.xml > @@ +24,5 @@ > > <CheckBoxPreference android:key="browser.chrome.dynamictoolbar" > > android:title="@string/pref_scroll_title_bar2" > > android:summary="@string/pref_scroll_title_bar_summary" /> > > > > + <!-- Title string is hardcoded here because it's not meant to be translated --> > > nit: Add why is it not meant to be translated. Done. > @@ +27,5 @@ > > > > + <!-- Title string is hardcoded here because it's not meant to be translated --> > > + <CheckBoxPreference android:key="android.not_a_preference.new_tablet_ui" > > + android:title="Enable new tablet UI" > > + android:defaultValue="true" /> > > This is duplicated here and in the patch in bug 1046200. I imagine it's > because we don't seem to be calling `PreferenceManager.setDefaultValues` > anywhere (see [1]). > > I'd make a note in the other patch that this value should be updated too > when merging to m-c. Yep, done. I'm also keeping a list of things we'll need to do before merging to m-c in the tablet-refresh etherpad.

Comment on attachment 8472394 [details] [diff] [review] Add UI to enable/disable the new tablet UI (r=mcomella) Decided to make this a bit more future proof and only show the UI toggle on non-release builds. Updated the robotium tests accordingly.

Comment on attachment 8472394 [details] [diff] [review] Add UI to enable/disable the new tablet UI (r=mcomella) Review of attachment 8472394 [details] [diff] [review]: ----------------------------------------------------------------- Where do you make the change to ensure this only appears on non-release builds? I just see the updated tests. ::: mobile/android/base/tests/testSettingsMenuItems.java @@ +158,5 @@ > // Text reflow - only built if *not* release build > String[] textReflowUi = { StringHelper.TEXT_REFLOW_LABEL }; > settingsMap.get(PATH_DISPLAY).add(textReflowUi); > > + // New tablet UI can only be enabled in non-release build nit: The comment is technically redundant because it's inside the `if (!AppConstants.RELEASE_BUILD)` block, but I'm fine with leaving it in. Don't forget to mark this in the etherpad for removal.

Comment on attachment 8472901 [details] [diff] [review] Add UI to enable/disable the new tablet UI (r=mcomella) (In reply to Michael Comella (:mcomella) from comment #9) > Comment on attachment 8472394 [details] [diff] [review] > Add UI to enable/disable the new tablet UI (r=mcomella) > > Review of attachment 8472394 [details] [diff] [review]: > ----------------------------------------------------------------- > > Where do you make the change to ensure this only appears on non-release > builds? I just see the updated tests. Oops, forgot to squash this part in. > ::: mobile/android/base/tests/testSettingsMenuItems.java > @@ +158,5 @@ > > // Text reflow - only built if *not* release build > > String[] textReflowUi = { StringHelper.TEXT_REFLOW_LABEL }; > > settingsMap.get(PATH_DISPLAY).add(textReflowUi); > > > > + // New tablet UI can only be enabled in non-release build > > nit: The comment is technically redundant because it's inside the `if > (!AppConstants.RELEASE_BUILD)` block, but I'm fine with leaving it in. > > Don't forget to mark this in the etherpad for removal. Good point, done.

Preference forgot to check for isTablet, filed bug above.