Put the new tablet UI behind a build flag

RESOLVED FIXED in Firefox 35

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: lucasr, Assigned: lucasr)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 35
All
Android
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

4 years ago
To avoid letting unused code ride the trains. Even though the new UI will land pref'd off in m-c, we unconditionally ship the new resources/code in the APK right now.
(Assignee)

Updated

4 years ago
Blocks: 1014156
(Assignee)

Updated

4 years ago
See Also: → bug 1073474
(Assignee)

Comment 1

4 years ago
Created attachment 8498134 [details] [diff] [review]
Put the new tablet UI behind a build flag (r=nalexander)
(Assignee)

Comment 2

4 years ago
Comment on attachment 8498134 [details] [diff] [review]
Put the new tablet UI behind a build flag (r=nalexander)

This is not a complete solution (a few classes/resources still ship a build with new tablet UI disabled) but I consider this good enough as a temporary setup. This patches ensures new tablet resources and tab strip classes are not included in the APK when the build flag is disabled. I had to keep a couple of classes in because they're used in common code, namely BrowserToolbarNewTablet and TabsGridLayout.

To be clear: the new tablet UI is still disabled by default even when new tablet build flag is enabled.

ps: This also fixes some remaining issues related to bug 1073474.
Attachment #8498134 - Flags: review?(nalexander)
Blocks: 1039789
Status: NEW → ASSIGNED
Comment on attachment 8498134 [details] [diff] [review]
Put the new tablet UI behind a build flag (r=nalexander)

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

::: configure.in
@@ +4896,5 @@
>      AC_DEFINE(MOZ_ANDROID_SHARE_OVERLAY)
>  fi
>  
>  dnl ========================================================
> +dnl = Include New Tablet UI on Android

I'd probably comment that this is not long for this world.
Attachment #8498134 - Flags: feedback+
Comment on attachment 8498134 [details] [diff] [review]
Put the new tablet UI behind a build flag (r=nalexander)

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

Looks fine to me.

::: configure.in
@@ +4897,5 @@
>  fi
>  
>  dnl ========================================================
> +dnl = Include New Tablet UI on Android
> +dnl ========================================================

Yeah, calling something /new/ has a tendency to drift.  Can we call it tablet-UI-on-top or tablet2 or anything else?  If you're 100% sure this dies quickly, roll on.

::: mobile/android/base/moz.build
@@ +516,5 @@
> +        'tabs/TabStripAdapter.java',
> +        'tabs/TabStripItemView.java',
> +        'tabs/TabStripView.java'
> +    ]
> +    ANDROID_RES_DIRS += [ SRCDIR + '/newtablet/res' ]

Urgh, newtablet?  Also, this will need care to integrate into Eclipse correctly.  I can help with this, but please file a ticket.

::: mobile/android/confvars.sh
@@ +80,5 @@
>  else
>    MOZ_ANDROID_SEARCH_ACTIVITY=
>  fi
>  
> +# Enable the new tablet UI in pre-release builds

Just double-checking: pre-release == {Nightly, Aurora}; and that's what you want.
Attachment #8498134 - Flags: review?(nalexander) → review+
(Assignee)

Updated

4 years ago
Blocks: 1075797
(Assignee)

Comment 5

4 years ago
(In reply to Nick Alexander :nalexander from comment #4)
> Comment on attachment 8498134 [details] [diff] [review]
> Put the new tablet UI behind a build flag (r=nalexander)
> 
> Review of attachment 8498134 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks fine to me.
> 
> ::: configure.in
> @@ +4897,5 @@
> >  fi
> >  
> >  dnl ========================================================
> > +dnl = Include New Tablet UI on Android
> > +dnl ========================================================
> 
> Yeah, calling something /new/ has a tendency to drift.  Can we call it
> tablet-UI-on-top or tablet2 or anything else?  If you're 100% sure this dies
> quickly, roll on.

As I explained on IRC, the newtablet directories and resource names are just a mechanism to allow us to have both current and new tablet UIs in parallel. We'll do a mass rename/move of new tablet resources and classes once we make it official in Nightly. 

> ::: mobile/android/base/moz.build
> @@ +516,5 @@
> > +        'tabs/TabStripAdapter.java',
> > +        'tabs/TabStripItemView.java',
> > +        'tabs/TabStripView.java'
> > +    ]
> > +    ANDROID_RES_DIRS += [ SRCDIR + '/newtablet/res' ]
> 
> Urgh, newtablet?  Also, this will need care to integrate into Eclipse
> correctly.  I can help with this, but please file a ticket.

Filed bug 1075797.

> ::: mobile/android/confvars.sh
> @@ +80,5 @@
> >  else
> >    MOZ_ANDROID_SEARCH_ACTIVITY=
> >  fi
> >  
> > +# Enable the new tablet UI in pre-release builds
> 
> Just double-checking: pre-release == {Nightly, Aurora}; and that's what you
> want.

Not entirely sure, to be honest. I think I'll make it Nightly only just in case. Thanks for pointing this out.
No longer blocks: 1075797
(Assignee)

Comment 6

4 years ago
(In reply to Richard Newman [:rnewman] from comment #3)
> Comment on attachment 8498134 [details] [diff] [review]
> Put the new tablet UI behind a build flag (r=nalexander)
> 
> Review of attachment 8498134 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: configure.in
> @@ +4896,5 @@
> >      AC_DEFINE(MOZ_ANDROID_SHARE_OVERLAY)
> >  fi
> >  
> >  dnl ========================================================
> > +dnl = Include New Tablet UI on Android
> 
> I'd probably comment that this is not long for this world.

Done.
(Assignee)

Comment 7

4 years ago
Decided to keep this in Nightly & Aurora after discussing this with mfinkle.

https://hg.mozilla.org/integration/fx-team/rev/21f27ccab222
https://hg.mozilla.org/mozilla-central/rev/21f27ccab222
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.