Last Comment Bug 752681 - Galaxy Note and other mid-sized devices receive XUL Fennec's phone UI, instead of Native Fennec
: Galaxy Note and other mid-sized devices receive XUL Fennec's phone UI, instea...
Status: RESOLVED FIXED
:
Product: Fennec Graveyard
Classification: Graveyard
Component: General (show other bugs)
: Trunk
: All Android
: -- normal (vote)
: Firefox 15
Assigned To: Matt Brubeck (:mbrubeck)
:
Mentors:
Depends on: 752017
Blocks: 719560
  Show dependency treegraph
 
Reported: 2012-05-07 14:01 PDT by Matt Brubeck (:mbrubeck)
Modified: 2012-05-16 03:35 PDT (History)
6 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (1.97 KB, patch)
2012-05-07 14:39 PDT, Matt Brubeck (:mbrubeck)
no flags Details | Diff | Splinter Review
alternate patch (3.11 KB, patch)
2012-05-10 12:47 PDT, Matt Brubeck (:mbrubeck)
blassey.bugs: review+
mark.finkle: approval‑mozilla‑aurora+
mark.finkle: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Matt Brubeck (:mbrubeck) 2012-05-07 14:01:22 PDT
When we publish split builds (Native Fennec + XUL Fennec), our goal is to serve XUL Fennec only to devices that use the tablet UI.  The tablet UI is enabled for screenSize="xlarge" devices, which includes 9- and 10-inch tablets.

However, because of this patch from bug 719560, we will also serve XUL Fennec to screenSize="large" devices, which includes 7-inch tablets and some very large phones like the Samsung Galaxy Note.  XUL Fennec runs with the phone layout on those devices:
https://hg.mozilla.org/mozilla-central/rev/e3bb8af42f98

That patch was needed in order to install XUL Fennec on Tegra boards for testing, because the Tegras have "large" screens.  (See bug 719560 comment 22.)  But for the builds we ship to the market, we want XUL Fennec on "xlarge" devices only.

So on mozilla-beta branch and mozilla-release, we need tinderbox builds that are Tegra-compatible and official release builds that are not.  Since both types of builds currently use the same branding on Android, we can't do that just by editing the branding files.   Is there some configure flag we can use?  Any other suggestions?
Comment 1 Matt Brubeck (:mbrubeck) 2012-05-07 14:20:05 PDT
I didn't notice that bug 752017 was already filed for a decision on whether we want to fix this bug.
Comment 2 Matt Brubeck (:mbrubeck) 2012-05-07 14:39:58 PDT
Created attachment 621744 [details] [diff] [review]
patch

Currently it looks like the config differences between tinderbox builds and release builds are the --with-official-branding flag and the --enable-update-channel setting.

This patch builds XUL fennec in xlarge-only mode only when --with-official-branding is enabled.

If we'd rather have a more specific flag, we'd need to add it to the build automation.
Comment 3 Kevin Brosnan [:kbrosnan] 2012-05-07 14:51:40 PDT
The thought was in driver triage to block XUL Fennec on the Galaxy Note and it would fall back to native Fennec instead of changing the screen size targets.
Comment 4 Matt Brubeck (:mbrubeck) 2012-05-07 15:02:45 PDT
(In reply to Kevin Brosnan [:kbrosnan] from comment #3)
> The thought was in driver triage to block XUL Fennec on the Galaxy Note and
> it would fall back to native Fennec instead of changing the screen size
> targets.

Is it possible to block devices on a per-APK basis?  If so, that sounds fine.  We should also block it for other screenSize="large" devices like the 7" Galaxy Tab.
Comment 5 Matt Brubeck (:mbrubeck) 2012-05-07 16:51:31 PDT
(In reply to Kevin Brosnan [:kbrosnan] from comment #3)
> The thought was in driver triage to block XUL Fennec on the Galaxy Note and
> it would fall back to native Fennec instead of changing the screen size
> targets.

Based on my own testing with a split product in the Google Play Store, there's no way to block a device for just one APK.  You can only exclude devices for the entire product (i.e. both APKs).
Comment 6 Brad Lassey [:blassey] (use needinfo?) 2012-05-07 16:57:41 PDT
Comment on attachment 621744 [details] [diff] [review]
patch

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

this will result in the official builds failing to run tests.

I think the right solution is to black list the note in the market for the xul build
Comment 7 Matt Brubeck (:mbrubeck) 2012-05-07 17:00:01 PDT
Comment on attachment 621744 [details] [diff] [review]
patch

(In reply to Brad Lassey [:blassey] from comment #6)
> this will result in the official builds failing to run tests.

Do we run automation on official builds?

(Even if we do, as philor will tell you, no one checks the test results on the release branch anyway...)

> I think the right solution is to black list the note in the market for the
> xul build

This is not possible.  See comment 5.
Comment 8 JP Rosevear [:jpr] 2012-05-08 09:06:05 PDT
From bug 752017:

"Native Phone UI on 5" devices for sure vs. the tablet UI. Might be slightly odd, but no odder than anything else on a 5" device. Definitely makes more sense that the 10" tablet UI."
Comment 9 Matt Brubeck (:mbrubeck) 2012-05-09 09:58:03 PDT
(In reply to Matt Brubeck (:mbrubeck) from comment #7)
> > this will result in the official builds failing to run tests.
> 
> Do we run automation on official builds?

As far as I can tell, we do *not* run automated tests on official builds.
Comment 10 Alex Keybl [:akeybl] 2012-05-10 12:38:01 PDT
Matt let me know that this will need to land on both MOBILE130_2012050119_RELBRANCH and tip of beta since it blocks our go to build for the XF13b2 spin associated with FN14b1. Would that be possible today?
Comment 11 Matt Brubeck (:mbrubeck) 2012-05-10 12:39:41 PDT
(In reply to Alex Keybl [:akeybl] from comment #10)
> Matt let me know that this will need to land on both
> MOBILE130_2012050119_RELBRANCH and tip of beta since it blocks our go to
> build for the XF13b2 spin associated with FN14b1. Would that be possible
> today?

Yes, I can land this today if I get r+ today.
Comment 12 Matt Brubeck (:mbrubeck) 2012-05-10 12:47:37 PDT
Created attachment 622860 [details] [diff] [review]
alternate patch

This switches to setting MOZ_MOBILE_COMPAT in the mozconfig, which seems a bit cleaner.  Untested.
Comment 13 Matt Brubeck (:mbrubeck) 2012-05-10 13:00:46 PDT
(In reply to Matt Brubeck (:mbrubeck) from comment #12)
> This switches to setting MOZ_MOBILE_COMPAT in the mozconfig, which seems a
> bit cleaner.  Untested.

I've now tested this approach successfully in local builds with and without MOZ_MOBILE_COMPAT in the mozconfig.
Comment 14 Matt Brubeck (:mbrubeck) 2012-05-10 14:08:24 PDT
(In reply to Brad Lassey [:blassey] from comment #6)
> this will result in the official builds failing to run tests.

Aki confirmed that we do not run tests on official builds.  (Well, he also thought it was possible that we run them but don't display the results anywhere, which amounts to the same thing.)
Comment 15 Brad Lassey [:blassey] (use needinfo?) 2012-05-14 07:17:47 PDT
Comment on attachment 622860 [details] [diff] [review]
alternate patch

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

::: mobile/xul/config/mozconfigs/android/release
@@ +17,5 @@
>  ac_add_options --enable-update-channel=${MOZ_UPDATE_CHANNEL}
>  
>  export JAVA_HOME=/tools/jdk6
>  export MOZILLA_OFFICIAL=1
> +export MOZ_MOBILE_COMPAT=Tablets

why is this needed? shouldn't the configure.sh changes be sufficient?
Comment 16 Matt Brubeck (:mbrubeck) 2012-05-14 08:35:17 PDT
(In reply to Brad Lassey [:blassey] from comment #15)
> ::: mobile/xul/config/mozconfigs/android/release
> > +export MOZ_MOBILE_COMPAT=Tablets
> 
> why is this needed? shouldn't the configure.sh changes be sufficient?

I'm moving this configuration from configure.sh to the mozconfig.  Previously we set MOZ_MOBILE_COMPAT in configure.sh depending on the branding.  With this patch, it defaults to "All" in configure.in, but in official builds the mozconfig explicitly sets it to "Tablets".
Comment 18 Matt Brubeck (:mbrubeck) 2012-05-14 20:22:03 PDT
Comment on attachment 622860 [details] [diff] [review]
alternate patch

We need this patch on the mobile 13.0b2 relbranch for the next respin of XUL Fennec 13 beta.  We will also need this on ESR 10 for the Native Fennec 1.0 final release, assuming we continue to serve XUL Fennec ESR 10 to tablet users on the release channel.  I'll request esr10 approval after this has been released on beta.

[Approval Request Comment]
User impact if declined: Users of some devices receive XUL Fennec instead of Native Fennec.

Testing completed (on m-c, etc.): Landed on m-i, but this patch only affects "official" beta and release builds, so it can't be tested in tinderbox builds or nightlies.  I tested it in local builds that I uploaded to my own Google Play Store account.

Risk to taking this patch (and alternatives if risky): This patch is XUL-Fennec-only, and it only changes the list of supported screen types in the Android manifest.  It should be very low-risk.

String or UUID changes made by this patch: None.

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Comment 19 Ed Morley [:emorley] 2012-05-15 06:31:53 PDT
https://hg.mozilla.org/mozilla-central/rev/672f5159bcdd
Comment 20 Matt Brubeck (:mbrubeck) 2012-05-15 13:19:12 PDT
Found a line that was left in by accident; removed it in a followup patch:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7b425021598e

Landed on beta (including 13.0b2 relbranch) and aurora:
https://hg.mozilla.org/releases/mozilla-beta/rev/88ee14110a74
https://hg.mozilla.org/releases/mozilla-beta/rev/fdf0d6f54fd4
https://hg.mozilla.org/releases/mozilla-aurora/rev/7bac62505304
Comment 21 Ed Morley [:emorley] 2012-05-16 03:35:08 PDT
(In reply to Matt Brubeck (:mbrubeck) from comment #20)
> Found a line that was left in by accident; removed it in a followup patch:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/7b425021598e

https://hg.mozilla.org/mozilla-central/rev/7b425021598e

Note You need to log in before you can comment on or make changes to this bug.