Closed Bug 1375351 Opened 7 years ago Closed 7 years ago

(photon) - Clean up workaround code and resources

Categories

(Firefox for Android Graveyard :: General, enhancement)

All
Unspecified
enhancement
Not set
normal

Tracking

(firefox57 fixed)

RESOLVED FIXED
Firefox 57
Tracking Status
firefox57 --- fixed

People

(Reporter: walkingice, Assigned: jwu)

References

Details

(Whiteboard: [FNC][SPT57.1][INT])

Attachments

(4 files)

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 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
Attachment #8895231 - Flags: review?(cnevinchen) → review+
Comment on attachment 8895230 [details]
Bug 1375351 - Part 2: Remove resources that are only used in Australis.

https://reviewboard.mozilla.org/r/166364/#review171564
Attachment #8895230 - Flags: review?(cnevinchen) → review+
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 :)
Attachment #8895232 - Flags: review?(cnevinchen) → review+
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.
(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 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
Attachment #8895230 - Flags: review?(walkingice0204) → review+
Comment on attachment 8895231 [details]
Bug 1375351 - Part 3: Remove SkinConfig.

https://reviewboard.mozilla.org/r/166366/#review171582
Attachment #8895231 - Flags: review?(walkingice0204) → review+
Comment on attachment 8895232 [details]
Bug 1375351 - Part 4: Remove unused resources.

https://reviewboard.mozilla.org/r/166368/#review171584
Attachment #8895232 - Flags: review?(walkingice0204) → review+
Assignee: walkingice0204 → topwu.tw
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 :)
Attachment #8895229 - Flags: review?(nalexander) → review+
Comment on attachment 8895229 [details]
Bug 1375351 - Part 1: Remove Australis flavor.

https://reviewboard.mozilla.org/r/166362/#review171984
Attachment #8895229 - Flags: review?(max) → review+
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.
Keywords: checkin-needed
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?
Attachment #8895229 - Flags: review?(s.kaspari) → review+
(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.
Autoland can't push this until all pending issues in MozReview are marked as resolved.
Keywords: checkin-needed
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
Whiteboard: [FNC][SPT57.1][INT]
Blocks: 1391551
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: