STR: 1) Add a LWT on new tablet while in portrait mode (note, I used LWT switcher to do this) 2) Rotate the device Expected: The theme updates Actual: The theme dumps garbage in the added space I think this is because we don't refresh the LWT on configuration change [1]. I'm not sure if this affects other devices. [1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.java?rev=e2759f8d7232#1378

As discussed in our new-tablet-v1 triage meeting, this is (likely) too dangerous to get into 36: looking at 37, or, worst case, v2.

I noticed this also occurs on phone, just in a different and seemingly less intrusive way. I think mBitmap.getWidth() [1] is incorrect - it is different when the device is started in portrait and landscape modes and also "left" is negative when the device is rotated from portrait to landscape. [1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/lwt/LightweightTheme.java#321

maxwidth is: * 1824 (portrait) * 1920 (landscape) Which is calculated from [1]: int maxWidth = Math.max(dm.widthPixels, dm.heightPixels); I'm assuming dm.heightPixels does not include the software system buttons and/or the notification bar. [1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/lwt/LightweightTheme.java#208

Tested on ICS, KitKat, and GB. There is still some weirdness in LWT such as the height of certain elements being incorrect and some incorrect states after rotation, but at least the edges of the bitmap are not getting clamped to the right as before, causing what appeared to be a graphical glitch. However, I think this patch is all the time we should put into LWT for 36.

Note: there may be a better way to initialize the LWT such that we don't need the device height but this is the quick, easy win I feel confident uplifting.

Comment on attachment 8552797 [details] [diff] [review] Use real device dimensions when calculating LWT bitmap sizes f? rnewman on HardwareUtils - is not caching my values an acceptable approach?

Comment on attachment 8552797 [details] [diff] [review] Use real device dimensions when calculating LWT bitmap sizes Review of attachment 8552797 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/lwt/LightweightTheme.java @@ +206,5 @@ > > // Get the max display dimension so we can crop or expand the theme. > + final Point displayDimensions = new Point(); > + HardwareUtils.getDeviceDimensions(mApplication, displayDimensions); > + final int maxWidth = Math.max(displayDimensions.x, displayDimensions.y); (a) Why not just call your method getLargestDimension and have it return a single value? (b) I think setLightweightTheme is the wrong place for this; if the maximum value is varying across orientations, then you're relying on the theme being set in the right orientation, no? So perhaps the solution here is to figure out the situations in which our dimensions are wrong -- when we're in portrait and height is the largest value -- and pad accordingly. ::: mobile/android/base/util/HardwareUtils.java @@ +18,2 @@ > import android.view.ViewConfiguration; > +import android.view.WindowManager; Let's put the gfxey stuff in a separate class; DisplayUtils? @@ +120,5 @@ > + /** > + * Returns the best-guess physical device dimensions, including the system status bars. Note > + * that DisplayMetrics.height/widthPixels does not include the system bars. > + * > + * This method does not cache because the return value is dependent on device orientation. But presumably there are only {2,4} device orientations? @@ +132,5 @@ > + final Display display = ((WindowManager) context.getSystemService(Context.WINDOW_SERVICE)).getDefaultDisplay(); > + final int realWidth; > + final int realHeight; > + > + if (Build.VERSION.SDK_INT >= 17) { Use `Versions`.

(In reply to Richard Newman [:rnewman] from comment #7) > (a) Why not just call your method getLargestDimension and have it return a > single value? Good idea. > (b) I think setLightweightTheme is the wrong place for this; if the maximum > value is varying across orientations, then you're relying on the theme being > set in the right orientation, no? Ideally, it shouldn't vary across orientations - it does because of inconsistent return values from Display.heightPixels - this bug is intended to fix that. > So perhaps the solution here is to figure out the situations in which our > dimensions are wrong -- when we're in portrait and height is the largest > value -- and pad accordingly. This bug adds a method to get the value we expect - no need for potentially ambiguous padding. There is probably a better way of writing the LWT code such that this is not an important value but I'm looking for the quick, safe win here so we can uplift.

Before we used DisplayMetrics.width/heightPixels, which does not include the system bars.

Comment on attachment 8553438 [details] [diff] [review] Use real device dimensions when calculating LWT bitmap sizes Review of attachment 8553438 [details] [diff] [review]: ----------------------------------------------------------------- Good to go. ::: mobile/android/base/util/WindowUtils.java @@ +9,5 @@ > +import android.util.DisplayMetrics; > +import android.util.Log; > +import android.view.Display; > +import android.view.WindowManager; > +import org.mozilla.gecko.AppConstants.Versions; nit: Move this above the android imports @@ +43,5 @@ > + } else if (Versions.feature14Plus) { > + int tempWidth; > + int tempHeight; > + try { > + final Method mGetRawH = Display.class.getMethod("getRawHeight"); nit: Our convention is that new files do not use Hungarian notation - remove the `m`. ;)

Comment on attachment 8553438 [details] [diff] [review] Use real device dimensions when calculating LWT bitmap sizes Review of attachment 8553438 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/util/WindowUtils.java @@ +37,5 @@ > + final DisplayMetrics realMetrics = new DisplayMetrics(); > + display.getRealMetrics(realMetrics); > + > + realWidth = realMetrics.widthPixels; > + realHeight = realMetrics.heightPixels; I think you can simplify each of these clauses to be: if (Versions....) { ... return Math.max(realMetrics.widthPixels, realMetrics.heightPixels); } if ( ... ) Then you can kill all of the 'else's and also realWidth and realHeight.

Updated for nits.

Comment on attachment 8553914 [details] [diff] [review] Use real device dimensions when calculating LWT bitmap sizes Approval Request Comment [Feature/regressing bug #]: New tablet UI (bug 1014156) [User impact if declined]: Users of Lightweight themes will see an unpolished experience when rotating the device where the location of the background is unpredictable and sometimes looks glitchy. [Describe test coverage new/current, TreeHerder]: None [Risks and why]: Low - the change will only affect Lightweight themes. However, we do use a new method for determining the device size that should, but may not, work correctly on *all* devices (because Android). If it doesn't work correctly on some devices, the expected case is that these devices will have a similar experience to what is currently available. In the worst (unlikely) case, Lightweight themes will look completely terrible and basically be unusable. [String/UUID change made/needed]: None

https://hg.mozilla.org/releases/mozilla-aurora/rev/e485039f4301 https://hg.mozilla.org/releases/mozilla-beta/rev/2745f66dac6f

Verified as fixed in builds: Firefox for Android 38.0a1 (2015-01-26) Firefox for Android 37.0a2 (2015-01-26) Device: Asus Transformer Pad TF300T (Android 4.2.1)

Verified as fixed in Firefox for Android 36 Beta 4; Device: Asus Transformer Pad TF300T (Android 4.2.1).