It is time. Requesting tracking to make us aware of when the rollback option will go away.

Presumably this would fix bug 1075797.

Let's get an assignee

Our resources have got a lot bigger since the summer. Paring them down would be great.

Martyn, please review, land, and flag for uplift. I removed all the resources I found that are mirrored on both old tablet and new tablet - we can uplift this to get some APK savings. Note that the rest of the work to be done in this bug has neglible APK savings. In order to make sure I didn't break any runtime references, I created null bitmaps to fill the place of the removed old tablet resources. They have a header of: <!-- Bug 1106935: Temporary replacement for old tablet resources --> To make them easy to find.

(In reply to Michael Comella (:mcomella) [PTO until 1/14] from comment #4) > I removed all the resources I found that are mirrored on both old tablet and > new tablet I did this with: ls m/a/b/resources/drawable-large-xhdpi-v11/* And checking which items were named twice (* and new_tablet_*).

Add files from m/a/b/drawable-xlarge-xhdpi-v11/.

Comment on attachment 8543131 [details] [diff] [review] Part 1: Replace old tablet pngs with null XML resources I want this part in 36.

Comment on attachment 8543133 [details] [diff] [review] Part 1: Replace old tablet pngs with null XML resources Review of attachment 8543133 [details] [diff] [review]: ----------------------------------------------------------------- LGTM

Comment on attachment 8543133 [details] [diff] [review] Part 1: Replace old tablet pngs with null XML resources Approval Request Comment [Feature/regressing bug #]: New tablet UI (bug 1014156) [User impact if declined]: The APK in 36 will contain unused png assets and be unnecessarily large. [Describe test coverage new/current, TBPL]: None [Risks and why]: Low - we're basically removing some images. In the best bad case, those images will disappear from the UI because we mirror each removed asset with a null resource. Worst case, we crash because the assets cannot be found. [String/UUID change made/needed]: None

https://hg.mozilla.org/releases/mozilla-aurora/rev/7f35fd48c3b1 https://hg.mozilla.org/releases/mozilla-beta/rev/f154bf489b34 Setting the status for 36/37 to fixed for tracking purposes. Feel free to set them back to affected if more is intended to land there at some point.

/r/3143 - Bug 1106935 - Part 2: Remove old tablet branches from BrowserToolbar. /r/3145 - Bug 1106935 - Part 3: Remove tablet browser_toolbar.xml and orphaned resources. /r/3147 - Bug 1106935 - Part 4: Move new_tablet_browser_toolbar to browser_toolbar and update associated code. /r/3149 - Bug 1106935 - Part 5: Rename BrowserToolbarNewTablet -> BrowserToolbarTablet. Pull down these commits: hg pull review -r 20cda66a6b901a556283f9627b5156c66ab0a3ac

Micro-commits! Yes! Let's land these as we go along, to avoid epic bitrot.

Finkle, I see no reason this needs to be in 37 (besides part 1, which is to remove the pngs, which is in 36) - any concerns with clearing the tracking flag?

Agreed. Changed to "+"

Comment on attachment 8556778 [details] MozReview Request: bz://1106935/mcomella /r/3143 - Bug 1106935 - Part 2: Remove old tablet branches from BrowserToolbar. /r/3145 - Bug 1106935 - Part 3: Remove tablet browser_toolbar.xml and orphaned resources. /r/3147 - Bug 1106935 - Part 4: Move new_tablet_browser_toolbar to browser_toolbar and update associated code. /r/3149 - Bug 1106935 - Part 5: Rename BrowserToolbarNewTablet -> BrowserToolbarTablet. /r/3173 - Bug 1106935 - Part 6: Replace NewTabletUI.isEnabled with HardwareUtils.isTablet where simple. r=mhaigh Pull down these commits: hg pull review -r 9264730533081acf138d1d789ec93598802d9e90

Comment on attachment 8556778 [details] MozReview Request: bz://1106935/mcomella https://reviewboard.mozilla.org/r/3141/#review2667 Looks good - would be good to hit up those remaining TODOs. Loving the micro commits :) ::: mobile/android/base/home/TopSitesThumbnailView.java (Diff revision 2) > +import org.mozilla.gecko.util.HardwareUtils; Stick this import up by the other org.mozilla imports ::: mobile/android/base/toolbar/BrowserToolbarTablet.java (Diff revision 2) > - if (selectedTab != null && selectedTab.canDoForward()) { > + // TODO: Remove this hack in favor of resources when old tablet is removed. Are we not removing the old tablet code? ::: mobile/android/base/toolbar/BrowserToolbarTablet.java (Diff revision 2) > + // Do nothing; ; -> . ::: mobile/android/base/toolbar/BrowserToolbarPhoneBase.java (Diff revision 2) > +import org.mozilla.gecko.widget.ThemedImageView; Stick this import up by the other org.mozilla imports ::: mobile/android/base/toolbar/BrowserToolbarTablet.java (Diff revision 2) > - @Override > + // TODO: Move this to *TabletBase when old tablet is removed. Are we not removing the old tablet code?

Comment on attachment 8556778 [details] MozReview Request: bz://1106935/mcomella /r/3143 - Bug 1106935 - Part 2: Remove old tablet branches from BrowserToolbar. /r/3145 - Bug 1106935 - Part 3: Remove tablet browser_toolbar.xml and orphaned resources. /r/3147 - Bug 1106935 - Part 4: Move new_tablet_browser_toolbar to browser_toolbar and update associated code. /r/3149 - Bug 1106935 - Part 5: Rename BrowserToolbarNewTablet -> BrowserToolbarTablet. /r/3173 - Bug 1106935 - Part 6: Replace NewTabletUI.isEnabled with HardwareUtils.isTablet where simple. r=mhaigh /r/3255 - Bug 1106935 - review: Correct import style; fix comment grammar. r=mhaigh Pull down these commits: hg pull review -r 209864d6670b8c2316539b33eaafa5149671d1ff

https://reviewboard.mozilla.org/r/3141/#review2677 As per comment 15, I'd like to land these as we go along so we don't epic bitrot - it all doesn't need to land together. > Are we not removing the old tablet code? Easy issues first (see above). > Are we not removing the old tablet code? I was trying to do the easy issues first - eventually, everything will get removed, don't you worry!

Comment on attachment 8556778 [details] MozReview Request: bz://1106935/mcomella Comments as above. Didn't mean to cancel the review, rb flow is still a bit alien to me!

remote: https://hg.mozilla.org/integration/fx-team/rev/f1215e82b80c remote: https://hg.mozilla.org/integration/fx-team/rev/82782dff1b40 remote: https://hg.mozilla.org/integration/fx-team/rev/fb5c06b33638 remote: https://hg.mozilla.org/integration/fx-team/rev/ca24935d3b8a remote: https://hg.mozilla.org/integration/fx-team/rev/a08c30389a72 remote: https://hg.mozilla.org/integration/fx-team/rev/a9cc1659d77f

Comment on attachment 8556778 [details] MozReview Request: bz://1106935/mcomella /r/3143 - Bug 1106935 - Remove new tablet branch in MenuItemActionBar. r=mhaigh /r/3145 - Bug 1106935 - Move BrowserToolbarTablet menu button right margin hack to xml. r=mhaigh /r/3147 - Bug 1106935 - Remove MOZ_ANDROID_NEW_TABLET_UI confvar. r=nalexander /r/3149 - Bug 1106935 - Remove new tablet branch in GeckoMenuInflater. r=mhaigh /r/3173 - Bug 1106935 - Remove new tablet favicon size branch from Favicons. r=mhaigh /r/3255 - Bug 1106935 - Remove new tablet branch in selecting default toolbar favicons. r=mhaigh Pull down these commits: hg pull review -r 6ca02c382b85580353f57192eecff8e23e12c5ad

https://reviewboard.mozilla.org/r/3147/#review2713 Is there a reason you've left the configure.in references? I'd rather remove all traces and deal with any potential build errors for the (rare!) folks who defined it manually. Kill it with fire, no need for re-review.

Looks like this was an RB mistake, but my comments stand. Follow-up?

https://hg.mozilla.org/mozilla-central/rev/f1215e82b80c https://hg.mozilla.org/mozilla-central/rev/82782dff1b40 https://hg.mozilla.org/mozilla-central/rev/fb5c06b33638 https://hg.mozilla.org/mozilla-central/rev/ca24935d3b8a https://hg.mozilla.org/mozilla-central/rev/a08c30389a72 https://hg.mozilla.org/mozilla-central/rev/a9cc1659d77f

https://reviewboard.mozilla.org/r/3147/#review2727 Ignorance - I added a new commit to deal with this.

Comment on attachment 8556778 [details] MozReview Request: bz://1106935/mcomella /r/3143 - Bug 1106935 - Remove new tablet branch in MenuItemActionBar. r=mhaigh /r/3145 - Bug 1106935 - Move BrowserToolbarTablet menu button right margin hack to xml. r=mhaigh /r/3147 - Bug 1106935 - Remove MOZ_ANDROID_NEW_TABLET_UI confvar. r=nalexander /r/3149 - Bug 1106935 - Remove new tablet branch in GeckoMenuInflater. r=mhaigh /r/3173 - Bug 1106935 - Remove new tablet favicon size branch from Favicons. r=mhaigh /r/3255 - Bug 1106935 - Remove new tablet branch in selecting default toolbar favicons. r=mhaigh /r/3317 - Bug 1106935 - Remove MOZ_ANDROID_NEW_TABLET_UI from configure.in. r=nalexander Pull down these commits: hg pull review -r 7ccf97e8c1ef0040e0a27d158a95782fd17d0f7e

Comment on attachment 8556778 [details] MozReview Request: bz://1106935/mcomella https://reviewboard.mozilla.org/r/3141/#review2965 Ship It!

Comment on attachment 8556778 [details] MozReview Request: bz://1106935/mcomella https://reviewboard.mozilla.org/r/3141/#review3105 Ship It!

https://hg.mozilla.org/integration/fx-team/rev/9080887b3cc4 https://hg.mozilla.org/integration/fx-team/rev/fb066050757e https://hg.mozilla.org/integration/fx-team/rev/78cb4369b396 https://hg.mozilla.org/integration/fx-team/rev/319511aa757a https://hg.mozilla.org/integration/fx-team/rev/f292df38d7eb https://hg.mozilla.org/integration/fx-team/rev/efa9411df1c4 https://hg.mozilla.org/integration/fx-team/rev/f8366cf7e7c8

Backed out for blowing up API9 tests. Please verify that this is green on Try before pushing again. https://hg.mozilla.org/integration/fx-team/rev/c17419e63808 https://treeherder.mozilla.org/logviewer.html#?job_id=2025871&repo=fx-team

Gotcha: 13:08:51 INFO - 02-18 13:06:29.343 E/GeckoApp( 590): Exception starting favicon cache. Corrupt resources? 13:08:51 INFO - 02-18 13:06:29.343 E/GeckoApp( 590): java.lang.IllegalStateException: Null default favicon was returned from the resources system! That was hard to track down - Richard, why don't we just throw when the Favicon cache cannot be inited? See: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoApp.java?rev=d3ac91f56ef2#1189

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ca5b82311a3f NI self to land when green (pending review).

Comment on attachment 8556778 [details] MozReview Request: bz://1106935/mcomella /r/3143 - Bug 1106935 - Remove MOZ_ANDROID_NEW_TABLET_UI from configure.in. r=nalexander /r/3145 - Bug 1106935 - Remove new tablet branch in selecting default toolbar favicons. r=mhaigh /r/3147 - Bug 1106935 - Remove new tablet favicon size branch from Favicons. r=mhaigh /r/3149 - Bug 1106935 - Remove new tablet branch in GeckoMenuInflater. r=mhaigh /r/3173 - Bug 1106935 - Remove MOZ_ANDROID_NEW_TABLET_UI confvar. r=nalexander /r/3255 - Bug 1106935 - Move BrowserToolbarTablet menu button right margin hack to xml. r=mhaigh /r/3317 - Bug 1106935 - Remove new tablet branch in MenuItemActionBar. r=mhaigh /r/4029 - Bug 1106935 - Get bitmap from Drawable, rather than BitmapFactory. Pull down these commits: hg pull review -r 936b05446cbc3ffcfda1a297d769f02699e7a275

https://reviewboard.mozilla.org/r/4029/#review3167 Ship It! ::: mobile/android/base/favicons/Favicons.java (Diff revision 1) > - defaultFavicon = BitmapFactory.decodeResource(res, R.drawable.toolbar_favicon_default); > + defaultFavicon = ((BitmapDrawable) res.getDrawable(R.drawable.toolbar_favicon_default)).getBitmap(); This'll now throw a CCE instead of returning null for us to check on the line below. Consider checking: ``` if (res instanceof BitmapDrawable) { defaultFavicon = ... } else { throw new IllegalStateException("toolbar_favicon_default wasn't a bitmap resource!"); } ```

https://reviewboard.mozilla.org/r/4029/#review3169 > This'll now throw a CCE instead of returning null for us to check on the line below. Consider checking: > > ``` > if (res instanceof BitmapDrawable) { > defaultFavicon = ... > } else { > throw new IllegalStateException("toolbar_favicon_default wasn't a bitmap resource!"); > } > ``` Assuming we continue to catch rather than crash (https://bugzilla.mozilla.org/show_bug.cgi?id=1106935#c34), wfm.

(In reply to Michael Comella (:mcomella) from comment #34) > That was hard to track down - Richard, why don't we just throw when the > Favicon cache cannot be inited? To answer this specific question: because we shouldn't get there (as the log message notes, to get in this state the default favicon needs to be missing from the APK), but if we can't, it's worth trying to be a functional browser. That kinda assumes that it works; if it doesn't, then yes, we should consider crashing in an obvious fashion instead.

Comment on attachment 8556778 [details] MozReview Request: bz://1106935/mcomella rnewman, comment 37.

(In reply to Richard Newman [:rnewman] from comment #39) > That kinda assumes that it works; if it doesn't, then yes, we should > consider crashing in an obvious fashion instead. Sure, seems like it doesn't work - can you file a bug to crash instead, or file a bug to fix it (and add a test! :P), to your preference?

green try in comment 35. The landed patch will be slightly different, w/ rnewman's nit from comment 37: I don't expect the changes to break anything.

remote: https://hg.mozilla.org/integration/fx-team/rev/2ba0223fe2b4 remote: https://hg.mozilla.org/integration/fx-team/rev/d34cbc2d9328 remote: https://hg.mozilla.org/integration/fx-team/rev/85da10774705 remote: https://hg.mozilla.org/integration/fx-team/rev/e08a3fda1ada remote: https://hg.mozilla.org/integration/fx-team/rev/fffa9d369acd remote: https://hg.mozilla.org/integration/fx-team/rev/ced200b5b4ab remote: https://hg.mozilla.org/integration/fx-team/rev/6d82ed89b839 remote: https://hg.mozilla.org/integration/fx-team/rev/42b42d67f1a8

https://hg.mozilla.org/mozilla-central/rev/2ba0223fe2b4 https://hg.mozilla.org/mozilla-central/rev/d34cbc2d9328 https://hg.mozilla.org/mozilla-central/rev/85da10774705 https://hg.mozilla.org/mozilla-central/rev/e08a3fda1ada https://hg.mozilla.org/mozilla-central/rev/fffa9d369acd https://hg.mozilla.org/mozilla-central/rev/ced200b5b4ab https://hg.mozilla.org/mozilla-central/rev/6d82ed89b839 https://hg.mozilla.org/mozilla-central/rev/42b42d67f1a8

Michael, did everything land for this bug, or is it open for a reason?

(In reply to Richard Newman [:rnewman] from comment #46) > Michael, did everything land for this bug, or is it open for a reason? Everything did not land yet - this is going to take a while and is low priority.

Comment on attachment 8556778 [details] MozReview Request: bz://1106935/mcomella /r/3143 - Bug 1106935 - Remove unused imports from testAboutPage. r=mhaigh /r/3145 - Bug 1106935 - Add XML attributes to dimens. r=mhaigh /r/3147 - Bug 1106935 - Use Android resource system on tablet in ToolbarDisplayLayout. r=mhaigh /r/3149 - Bug 1106935 - Remove unused NewTabletUI imports. r=mhaigh /r/3173 - Bug 1106935 - Use resource system for tablets in ThumbnailHelper. r=mhaigh Pull down these commits: hg pull -r 0ca8c927063473dc475be984405a8aed04b30590 https://reviewboard-hg.mozilla.org/gecko/

https://reviewboard.mozilla.org/r/3173/#review2669 Ship It! ::: mobile/android/base/home/TopSitesThumbnailView.java (Diff revision 1) > +import org.mozilla.gecko.util.HardwareUtils; Move this code up by the other moz imports ::: mobile/android/base/ThumbnailHelper.java (Diff revision 4) > + res.getValue(R.dimen.thumbnail_aspect_ratio, outValue, true); Much nicer way of doing it!

https://reviewboard.mozilla.org/r/3147/#review5391 Ship It! ::: mobile/android/base/resources/drawable-large-v11/site_security_level.xml (Diff revision 4) > +<?xml version="1.0" encoding="utf-8"?> I don't know if this is a limitation of the systems we use, but it'd be good to have these marked as file name changes, rather than a file deleted and a new file added, so we can keep the history intact.

https://reviewboard.mozilla.org/r/3145/#review5389 ::: mobile/android/base/resources/values/dimens.xml (Diff revision 4) > + <dimen name="custom_match_parent">-1px</dimen> I've just submmitted this in one of my Tab Queue bugs. Are you sure the px on the end there is right? We're trying to emulate the default values of match_parent or wrap_content, which are defined as (http://goo.gl/KE5s26): <attr name="layout_width" format="dimension"> <enum name="fill_parent" value="-1" /> <enum name="match_parent" value="-1" /> <enum name="wrap_content" value="-2" /> </attr> This is a more thorough write up of the method than the SO anwser, if you're interested: http://blog.danlew.net/2015/01/06/handling-android-resources-with-non-standard-formats/

https://reviewboard.mozilla.org/r/3143/#review2671 Ship It! ::: mobile/android/base/toolbar/BrowserToolbarPhoneBase.java (Diff revision 1) > +import org.mozilla.gecko.widget.ThemedImageView; Move this import up by the other moz imports

https://reviewboard.mozilla.org/r/3149/#review5397 Ship It!

Comment on attachment 8556778 [details] MozReview Request: bz://1106935/mcomella https://reviewboard.mozilla.org/r/3141/#review5399 Am I right in thinking that the NewTabletUI file is still alive after these patches?

https://reviewboard.mozilla.org/r/3141/#review5413 Yes, we're doing this in parts so I haven't finished removing everything yet.

https://reviewboard.mozilla.org/r/3147/#review5417 > I don't know if this is a limitation of the systems we use, but it'd be good to have these marked as file name changes, rather than a file deleted and a new file added, so we can keep the history intact. hg notates them as file add/deletes but I did run the `hg mv` command, and it does store that history. Note that when using `hg log`, you also have to specify the `--follow` flag to get this history, e.g. `hg log -f site_security_level.xml`.

https://reviewboard.mozilla.org/r/3173/#review5419 > Move this code up by the other moz imports It already is. I'm a bit confused because I haven't touched this file with the latest push - I wonder if this is a reviewboard error?

https://reviewboard.mozilla.org/r/3143/#review5421 > Move this import up by the other moz imports Same - it's with the other moz imports. It looks like this is from a previous push ("Diff revision 1" as opposed to 4).

https://reviewboard.mozilla.org/r/3145/#review5425 > I've just submmitted this in one of my Tab Queue bugs. Are you sure the px on the end there is right? > > We're trying to emulate the default values of match_parent or wrap_content, which are defined as (http://goo.gl/KE5s26): > > <attr name="layout_width" format="dimension"> > <enum name="fill_parent" value="-1" /> > <enum name="match_parent" value="-1" /> > <enum name="wrap_content" value="-2" /> > </attr> > > This is a more thorough write up of the method than the SO anwser, if you're interested: http://blog.danlew.net/2015/01/06/handling-android-resources-with-non-standard-formats/ Nice find! I think px is used in the SO answer because (I think) it doesn't get scaled by the system, and thus its value is used directly, but your answer is definitely more correct - I'll switch over to using that.

Comment on attachment 8556778 [details] MozReview Request: bz://1106935/mcomella /r/3143 - Bug 1106935 - Remove unused imports from testAboutPage. r=mhaigh /r/3145 - Bug 1106935 - Add XML attributes to dimens. r=mhaigh /r/3147 - Bug 1106935 - Use Android resource system on tablet in ToolbarDisplayLayout. r=mhaigh /r/3149 - Bug 1106935 - Remove unused NewTabletUI imports. r=mhaigh /r/3173 - Bug 1106935 - Use resource system for tablets in ThumbnailHelper. r=mhaigh /r/6571 - Bug 1106935 - Remove NewTabletUI branch in TabsPanel. r=mhaigh /r/6573 - Bug 1106935 - Remove references to old tablet sidebar. r=mhaigh /r/6575 - Bug 1106935 - Remove unused tabs panel style resources. r=mhaigh /r/6577 - Bug 1106935 - Remove NewTabletUI branch in PrivateTabsPanel. r=mhaigh /r/6579 - Bug 1106935 - Remove NewTabletUI branch in ActionBarViewFlipper. r=mhaigh /r/6581 - Bug 1106935 - Remove NewTabletUI branch in BrowserToolbarTabletBase. r=mhaigh /r/6583 - Bug 1106935 - Remove NewTabletUI branches in NewTabletUI. r=mhaigh /r/6585 - Bug 1106935 - Remove NewTabletUI class. r=mhaigh Pull down these commits: hg pull -r 1e216b09beb6574695a103c7c13a530468aa422f https://reviewboard-hg.mozilla.org/gecko/

I removed your NewTabletUI class. :D Note that I also filed bug 1150742 for even more followup madness (the distinction being this bug is primarily about removing the branches in code as opposed to research).

https://reviewboard.mozilla.org/r/6585/#review5715 Looking good - can't wait to get this landed!

Comment on attachment 8556778 [details] MozReview Request: bz://1106935/mcomella r+ via irc: 06:44 <mcomella> mhaigh: Is this an r+? https://bugzilla.mozilla.org/show_bug.cgi?id=1106935#c63 06:44 <mhaigh> mcomella: yeah - weird - I ticked the ship it option. anyway - SHIP IT!

https://hg.mozilla.org/integration/fx-team/rev/98b329aed166 https://hg.mozilla.org/integration/fx-team/rev/0af8086b3bc9 https://hg.mozilla.org/integration/fx-team/rev/c594a2788d16 https://hg.mozilla.org/integration/fx-team/rev/f19635d67587 https://hg.mozilla.org/integration/fx-team/rev/0302e869e440 https://hg.mozilla.org/integration/fx-team/rev/a5e6cf8b9c9c https://hg.mozilla.org/integration/fx-team/rev/bf88029f521e https://hg.mozilla.org/integration/fx-team/rev/e581a1f51297 https://hg.mozilla.org/integration/fx-team/rev/c1befe2261fc https://hg.mozilla.org/integration/fx-team/rev/57c22ce3666a https://hg.mozilla.org/integration/fx-team/rev/dff4d833af14 https://hg.mozilla.org/integration/fx-team/rev/ce8b48cf665e

https://hg.mozilla.org/mozilla-central/rev/98b329aed166 https://hg.mozilla.org/mozilla-central/rev/0af8086b3bc9 https://hg.mozilla.org/mozilla-central/rev/c594a2788d16 https://hg.mozilla.org/mozilla-central/rev/f19635d67587 https://hg.mozilla.org/mozilla-central/rev/0302e869e440 https://hg.mozilla.org/mozilla-central/rev/a5e6cf8b9c9c https://hg.mozilla.org/mozilla-central/rev/bf88029f521e https://hg.mozilla.org/mozilla-central/rev/e581a1f51297 https://hg.mozilla.org/mozilla-central/rev/c1befe2261fc https://hg.mozilla.org/mozilla-central/rev/57c22ce3666a https://hg.mozilla.org/mozilla-central/rev/dff4d833af14 https://hg.mozilla.org/mozilla-central/rev/ce8b48cf665e

Michael, can you please continue to handle these uplifts? They tend to be time-consuming and conflict-prone.

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #67) > Michael, can you please continue to handle these uplifts? They tend to be > time-consuming and conflict-prone. The rest of this does not need to be uplifted - part 1 was the only changeset that needed to land on 38+.