Closed
Bug 1046200
Opened 10 years ago
Closed 10 years ago
Create BrowserApp.isNewTablet()
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 35
People
(Reporter: lucasr, Assigned: lucasr)
References
Details
(Whiteboard: [fixed-larch])
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
mcomella
:
review+
|
Details | Diff | Splinter Review |
Controlled by a shared prefs key. This will allow Nightly users to opt-in to the new tablet UI.
Assignee | ||
Updated•10 years ago
|
No longer blocks: new-tablet-v1
Assignee | ||
Updated•10 years ago
|
Summary: Create HardwareUtils.isNewTablet() → Create BrowserApp.isNewTablet()
Assignee | ||
Comment 1•10 years ago
|
||
Assignee | ||
Comment 2•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8467791 -
Attachment is obsolete: true
Assignee | ||
Comment 3•10 years ago
|
||
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)
Assignee | ||
Comment 4•10 years ago
|
||
Assignee | ||
Comment 5•10 years ago
|
||
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)
Assignee | ||
Comment 6•10 years ago
|
||
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 7•10 years ago
|
||
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+
Assignee | ||
Comment 8•10 years ago
|
||
(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.
Comment 9•10 years ago
|
||
(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
Updated•10 years ago
|
Flags: needinfo?(lucasr.at.mozilla)
Assignee | ||
Comment 10•10 years ago
|
||
(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]
Assignee | ||
Comment 11•10 years ago
|
||
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
Comment 12•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/6f4f1a77de2d https://hg.mozilla.org/mozilla-central/rev/dd71fd335247
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•