(photon) - Clean up workaround code and resources

RESOLVED FIXED in Firefox 57

Status

()

Firefox for Android
General
RESOLVED FIXED
8 months ago
6 months ago

People

(Reporter: walkingice, Assigned: jwu)

Tracking

(Blocks: 2 bugs)

unspecified
Firefox 57
All
Unspecified
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

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

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments)

(Reporter)

Description

8 months ago
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 5

6 months ago
mozreview-review
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 6

6 months ago
mozreview-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 7

6 months ago
mozreview-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+
(Assignee)

Comment 8

6 months ago
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 11

6 months ago
(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
(Reporter)

Comment 12

6 months ago
mozreview-review
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+
(Reporter)

Comment 13

6 months ago
mozreview-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+
(Reporter)

Comment 14

6 months ago
mozreview-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)

Updated

6 months ago
Assignee: walkingice0204 → topwu.tw

Comment 15

6 months ago
mozreview-review
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 16

6 months ago
mozreview-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+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 21

6 months ago
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.
(Assignee)

Updated

6 months ago
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+
(Assignee)

Comment 23

6 months ago
(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
(Assignee)

Updated

6 months ago
Keywords: checkin-needed

Comment 25

6 months ago
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]
(Assignee)

Updated

6 months ago
Blocks: 1391551
You need to log in before you can comment on or make changes to this bug.