1375351 - (photon) - Clean up workaround code and resources

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, enhancement)

Description

[Julian Chu [:walkingice]]

There will be UI refresh in version-57, and we want to merge code into version-56 (as early as possible). However, we don't want to see those changes in version-56. Therefore we added some workaround.

In Version-56, our target is to build different APK files by different flavor. Australis-flavor gives original Fennec UI. And Photon-flavor gives Photon UI.

* In bug 1374959, added *SkinConfig.java* for RunTime detection
* In bug 1372486, let different flavor use different resources path
* there will be some if-else statements to change behavior for different flavors.

These things should be removed after version-56, to ensure our code won't looks terrible due to those workaround.

File this bug to help to track the removing tasks.

Comment 1

[Jing-wei Wu [:jwu]]

Attachment: Bug 1375351 - Part 1: Remove Australis flavor.

We should now only maintain Photon flavor. Remove all Australis related configuration in build scripts.

Review commit: https://reviewboard.mozilla.org/r/166362/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/166362/

Comment 2

[Jing-wei Wu [:jwu]]

Attachment: Bug 1375351 - Part 2: Remove resources that are only used in Australis.

Review commit: https://reviewboard.mozilla.org/r/166364/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/166364/

Comment 3

[Jing-wei Wu [:jwu]]

Attachment: Bug 1375351 - Part 3: Remove SkinConfig.

SkinConfig is used to detect current skin is Australis or Photon at runtime.
Since we don't support Australis anymore in Nightly, this class should be removed.

Review commit: https://reviewboard.mozilla.org/r/166366/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/166366/

Comment 4

[Jing-wei Wu [:jwu]]

Attachment: Bug 1375351 - Part 4: Remove unused resources.

Resources don't be used in Photon should be removed to prevent lint error.

Review commit: https://reviewboard.mozilla.org/r/166368/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/166368/

Comment 5

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8895231 [details]
Bug 1375351 - Part 3: Remove SkinConfig.

https://reviewboard.mozilla.org/r/166366/#review171562

::: mobile/android/app/src/photon/java/org/mozilla/gecko/toolbar/ToolbarDisplayLayout.java
(Diff revision 1)
>      public void setPrivateMode(boolean isPrivate) {
>          super.setPrivateMode(isPrivate);
>          mSiteSecurity.setPrivateMode(isPrivate);
>  
> -        // Bug 1375351 should change class type to ThemedImageButton to avoid casting
> +        ((ThemedImageButton) mStop).setPrivateMode(isPrivate);
> -        if (SkinConfig.isPhoton()) {

Do you think we should add type and null check here?

::: mobile/android/base/java/org/mozilla/gecko/tabs/TabStripItemView.java
(Diff revision 1)
>      public int getTabId() {
>          return id;
>      }
>  
>      @Override
> -    protected void onSizeChanged(int width, int height, int oldWidth, int oldHeight) {

maybe add the reason in commit message about why they are removed

Comment 6

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8895230 [details]
Bug 1375351 - Part 2: Remove resources that are only used in Australis.

https://reviewboard.mozilla.org/r/166364/#review171564

Comment 7

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8895232 [details]
Bug 1375351 - Part 4: Remove unused resources.

https://reviewboard.mozilla.org/r/166368/#review171558

I believe you've run the lint check already :)

Comment 8

[Jing-wei Wu [:jwu]]

Comment on attachment 8895231 [details]
Bug 1375351 - Part 3: Remove SkinConfig.

https://reviewboard.mozilla.org/r/166366/#review171562

> Do you think we should add type and null check here?

Maybe directly declare mStop as ThemedImageButton then no casting would be needed here. I would file another patch to fix it.

> maybe add the reason in commit message about why they are removed

Overwrite onSizeChanged() in Australis is to calculate the tab strip curve and touch region. Since in Photon we don't draw curve anymore, I just remove all related code without comments to keep the class as clean as possible.

Comment 9

[Jing-wei Wu [:jwu]]

Comment on attachment 8895231 [details]
Bug 1375351 - Part 3: Remove SkinConfig.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/166366/diff/1-2/

Comment 10

[Jing-wei Wu [:jwu]]

Comment on attachment 8895232 [details]
Bug 1375351 - Part 4: Remove unused resources.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/166368/diff/1-2/

Comment 11

[Jing-wei Wu [:jwu]]

(In reply to Nevin Chen [:nechen] from comment #7)
> Comment on attachment 8895232 [details]
> Bug 1375351 - Part 4: Remove unused resources.
> 
> https://reviewboard.mozilla.org/r/166368/#review171558
> 
> I believe you've run the lint check already :)

Here is the try result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e5c97ec7c3ce04e15f21b26cbc3e72c961df5703

Comment 12

[Julian Chu [:walkingice]]

Comment on attachment 8895230 [details]
Bug 1375351 - Part 2: Remove resources that are only used in Australis.

https://reviewboard.mozilla.org/r/166364/#review171580

If lint says OK, then it should be OK

Comment 13

[Julian Chu [:walkingice]]

Comment on attachment 8895231 [details]
Bug 1375351 - Part 3: Remove SkinConfig.

https://reviewboard.mozilla.org/r/166366/#review171582

Comment 14

[Julian Chu [:walkingice]]

Comment on attachment 8895232 [details]
Bug 1375351 - Part 4: Remove unused resources.

https://reviewboard.mozilla.org/r/166368/#review171584

Comment 15

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8895229 [details]
Bug 1375351 - Part 1: Remove Australis flavor.

https://reviewboard.mozilla.org/r/166362/#review171762

Thanks for doing this!  If it's green on try (android-api-15, android-api-15-gradle, android-gradle-dependencies), it's good for me.

::: mobile/android/app/build.gradle:110
(Diff revision 1)
> -        // The default user interface.
> -        australis {
> -            dimension "skin"
> -        }
> -
>          // A new user interface, currently under development.

Update the comment: perhaps "Since Firefox 57, the mobile user interface has followed the Photon design.  Before Firefox 57, the user interface followed the Australis design."

Or remove the comment entirely :)

Comment 16

[Max Liu [:maliu]]

Comment on attachment 8895229 [details]
Bug 1375351 - Part 1: Remove Australis flavor.

https://reviewboard.mozilla.org/r/166362/#review171984

Comment 17

[Jing-wei Wu [:jwu]]

Comment on attachment 8895229 [details]
Bug 1375351 - Part 1: Remove Australis flavor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/166362/diff/1-2/

Comment 18

[Jing-wei Wu [:jwu]]

Comment on attachment 8895230 [details]
Bug 1375351 - Part 2: Remove resources that are only used in Australis.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/166364/diff/1-2/

Comment 19

[Jing-wei Wu [:jwu]]

Comment on attachment 8895231 [details]
Bug 1375351 - Part 3: Remove SkinConfig.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/166366/diff/2-3/

Comment 20

[Jing-wei Wu [:jwu]]

Comment on attachment 8895232 [details]
Bug 1375351 - Part 4: Remove unused resources.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/166368/diff/2-3/

Comment 21

[Jing-wei Wu [:jwu]]

The try result is here:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=47e59641167bd9c01396c62bf8e0da99eebebf33

android-gradle-dependencies isn't included because our try server doesn't recognize this platform.

Comment 22

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8895229 [details]
Bug 1375351 - Part 1: Remove Australis flavor.

https://reviewboard.mozilla.org/r/166362/#review172312

::: mobile/android/app/build.gradle:114
(Diff revision 2)
> -        }
> -
> -        // A new user interface, currently under development.
>          photon {
>              dimension "skin"
>          }

I guess at a later point we could remove the "skin" flavor dimension too?

Comment 23

[Jing-wei Wu [:jwu]]

(In reply to Sebastian Kaspari (:sebastian) from comment #22)
> Comment on attachment 8895229 [details]
> Bug 1375351 - Part 1: Remove Australis flavor.
> 
> https://reviewboard.mozilla.org/r/166362/#review172312
> 
> ::: mobile/android/app/build.gradle:114
> (Diff revision 2)
> > -        }
> > -
> > -        // A new user interface, currently under development.
> >          photon {
> >              dimension "skin"
> >          }
> 
> I guess at a later point we could remove the "skin" flavor dimension too?

Yes, I think when we're ready to merge all files inside "photon/" back to "main/", the skin flavor should be removed as well.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

Autoland can't push this until all pending issues in MozReview are marked as resolved.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/eabb4e8483b3
Part 1: Remove Australis flavor. r=maliu,nalexander,sebastian
https://hg.mozilla.org/integration/autoland/rev/b95d713ab80a
Part 2: Remove resources that are only used in Australis. r=nechen,walkingice
https://hg.mozilla.org/integration/autoland/rev/303c7feb7373
Part 3: Remove SkinConfig. r=nechen,walkingice
https://hg.mozilla.org/integration/autoland/rev/6e7538512758
Part 4: Remove unused resources. r=nechen,walkingice

Comment 26

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/eabb4e8483b3
https://hg.mozilla.org/mozilla-central/rev/b95d713ab80a
https://hg.mozilla.org/mozilla-central/rev/303c7feb7373
https://hg.mozilla.org/mozilla-central/rev/6e7538512758