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)
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)
(deleted),
text/x-review-board-request
|
maliu
:
review+
nalexander
:
review+
sebastian
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
cnevinchen
:
review+
walkingice
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
cnevinchen
:
review+
walkingice
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
cnevinchen
:
review+
walkingice
:
review+
|
Details |
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•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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•7 years ago
|
Assignee: walkingice0204 → topwu.tw
Comment 15•7 years 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•7 years 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•7 years 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•7 years ago
|
Keywords: checkin-needed
Comment 22•7 years ago
|
||
mozreview-review |
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•7 years 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.
Comment 24•7 years ago
|
||
Autoland can't push this until all pending issues in MozReview are marked as resolved.
Keywords: checkin-needed
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 25•7 years 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
Comment 26•7 years ago
|
||
bugherder |
Updated•7 years ago
|
Whiteboard: [FNC][SPT57.1][INT]
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•