Closed Bug 1046200 Opened 6 years ago Closed 6 years ago

Create BrowserApp.isNewTablet()

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 35

People

(Reporter: lucasr, Assigned: lucasr)

References

Details

(Whiteboard: [fixed-larch])

Attachments

(1 file, 2 obsolete files)

Controlled by a shared prefs key. This will allow Nightly users to opt-in to the new tablet UI.
No longer blocks: new-tablet-v1
Blocks: 1046203
Blocks: 1046206
Summary: Create HardwareUtils.isNewTablet() → Create BrowserApp.isNewTablet()
Blocks: 1047561
Blocks: 1048575
Blocks: 1048903
Attachment #8467791 - Attachment is obsolete: true
Comment on attachment 8468551 [details] [diff] [review]
Introduce NewTabletUI utility API (r=mcomella)

This is the global util API we'll use to split between old and new tablet UIs.
Attachment #8468551 - Flags: review?(michael.l.comella)
Comment on attachment 8468551 [details] [diff] [review]
Introduce NewTabletUI utility API (r=mcomella)

Apologies for the churn.
Attachment #8468551 - Attachment is obsolete: true
Attachment #8468551 - Flags: review?(michael.l.comella)
Comment on attachment 8468588 [details] [diff] [review]
Introduce NewTabletUI utility API (r=mcomella)

Depends on the patch for bug 1047561.
Attachment #8468588 - Flags: review?(michael.l.comella)
Comment on attachment 8468588 [details] [diff] [review]
Introduce NewTabletUI utility API (r=mcomella)

Review of attachment 8468588 [details] [diff] [review]:
-----------------------------------------------------------------

r+ w/ nits.

::: mobile/android/base/NewTabletUI.java
@@ +11,5 @@
> +import org.mozilla.gecko.util.HardwareUtils;
> +
> +public class NewTabletUI {
> +    // XXX: Change default value to false before
> +    // merging to m-c.

As per my comments in bug 1047561, add a comment that the default value in the preferences xml file needs to be changed too.

@@ +16,5 @@
> +    private static final boolean DEFAULT = true;
> +
> +    private static Boolean sNewTabletUI;
> +
> +    public static synchronized boolean isEnabled(Context context) {

nit: Comment that this should be an application context.

@@ +22,5 @@
> +            final SharedPreferences prefs = GeckoSharedPrefs.forApp(context);
> +            sNewTabletUI = prefs.getBoolean(GeckoPreferences.PREFS_NEW_TABLET_UI, DEFAULT);
> +        }
> +
> +        return HardwareUtils.isTablet() && sNewTabletUI;

nit: Why not short-circuit this whole method first on `if (!HardwareUtils.isTablet()) { return false; }`, rather than checking it down here? It's a bit more intuitive.
Attachment #8468588 - Flags: review?(michael.l.comella) → review+
(In reply to Michael Comella (:mcomella) from comment #7)
> Comment on attachment 8468588 [details] [diff] [review]
> Introduce NewTabletUI utility API (r=mcomella)
> 
> Review of attachment 8468588 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ w/ nits.
> 
> ::: mobile/android/base/NewTabletUI.java
> @@ +11,5 @@
> > +import org.mozilla.gecko.util.HardwareUtils;
> > +
> > +public class NewTabletUI {
> > +    // XXX: Change default value to false before
> > +    // merging to m-c.
> 
> As per my comments in bug 1047561, add a comment that the default value in
> the preferences xml file needs to be changed too.

Done. Also added a reminder to the tablet-refresh etherpad. 

> @@ +16,5 @@
> > +    private static final boolean DEFAULT = true;
> > +
> > +    private static Boolean sNewTabletUI;
> > +
> > +    public static synchronized boolean isEnabled(Context context) {
> 
> nit: Comment that this should be an application context.

Well, it doesn't need to be. Any context bound to the same app should work here.

> @@ +22,5 @@
> > +            final SharedPreferences prefs = GeckoSharedPrefs.forApp(context);
> > +            sNewTabletUI = prefs.getBoolean(GeckoPreferences.PREFS_NEW_TABLET_UI, DEFAULT);
> > +        }
> > +
> > +        return HardwareUtils.isTablet() && sNewTabletUI;
> 
> nit: Why not short-circuit this whole method first on `if
> (!HardwareUtils.isTablet()) { return false; }`, rather than checking it down
> here? It's a bit more intuitive.

Good point, done.
(In reply to Lucas Rocha (:lucasr) from comment #8)
> Well, it doesn't need to be. Any context bound to the same app should work
> here.

There's no documentation stating that this should be an application context so I think you are correct.

Any reason this hasn't landed yet?
Status: NEW → ASSIGNED
Flags: needinfo?(lucasr.at.mozilla)
(In reply to Michael Comella (:mcomella) from comment #9)
> (In reply to Lucas Rocha (:lucasr) from comment #8)
> > Well, it doesn't need to be. Any context bound to the same app should work
> > here.
> 
> There's no documentation stating that this should be an application context
> so I think you are correct.
> 
> Any reason this hasn't landed yet?

Tree was closed. Pushed: https://hg.mozilla.org/projects/larch/rev/6f4f1a77de2d
Flags: needinfo?(lucasr.at.mozilla)
Whiteboard: [fixed-larch]
I split the patch incorrectly, forgot to squash the GeckoPreferences pref key constant. Not my best day in git land.

Pushed this to fix the build.
https://hg.mozilla.org/projects/larch/rev/dd71fd335247
https://hg.mozilla.org/mozilla-central/rev/6f4f1a77de2d
https://hg.mozilla.org/mozilla-central/rev/dd71fd335247
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
You need to log in before you can comment on or make changes to this bug.