Closed
Bug 1366704
Opened 7 years ago
Closed 7 years ago
(photon) follow up of search suggestion on the visual refresh of private mode
Categories
(Firefox for Android Graveyard :: General, enhancement)
Firefox for Android Graveyard
General
Tracking
(firefox56 fixed, firefox57 verified)
VERIFIED
FIXED
Firefox 56
People
(Reporter: wesley_huang, Assigned: jwu)
References
Details
Attachments
(3 files)
No description provided.
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → topwu.tw
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 4•7 years ago
|
||
Comment on attachment 8881872 [details] Bug 1366704 - Part 1: Implement ThemedListView for Photon visual refresh. Sorry I'm not familiar with this part of code. I'll borrow Sebastian's wisdom about this.
Attachment #8881872 -
Flags: review?(cnevinchen) → review?(s.kaspari)
Updated•7 years ago
|
Attachment #8881873 -
Flags: review?(cnevinchen) → review?(s.kaspari)
Updated•7 years ago
|
Attachment #8881874 -
Flags: review?(cnevinchen) → review?(s.kaspari)
Comment 5•7 years ago
|
||
mozreview-review |
Comment on attachment 8881872 [details] Bug 1366704 - Part 1: Implement ThemedListView for Photon visual refresh. https://reviewboard.mozilla.org/r/152904/#review158620 ::: mobile/android/base/java/org/mozilla/gecko/widget/themed/ThemedView.java.frag:121 (Diff revision 1) > resetTheme(); > } > > @Override > protected void onLayout(boolean changed, int left, int top, int right, int bottom) { > +//#ifndef ABSTRACT_VIEW_GROUP Why don't call super.onLayout? ::: mobile/android/base/moz.build:1012 (Diff revision 1) > + 'widget/themed/ThemedListView.java', > 'widget/themed/ThemedRelativeLayout.java', > 'widget/themed/ThemedTextSwitcher.java', > 'widget/themed/ThemedTextView.java', > 'widget/themed/ThemedView.java', > + 'widget/themed/ThemedViewGroup.java', above three patches, none is using ThemedViewGroup. Is there any purpose to create this class?
Comment 6•7 years ago
|
||
mozreview-review |
Comment on attachment 8881873 [details] Bug 1366704 - Part 2: Support changing theme for search suggestion custom views. https://reviewboard.mozilla.org/r/152906/#review158628 ::: mobile/android/base/java/org/mozilla/gecko/home/TwoLinePageRow.java:84 (Diff revision 1) > > mFavicon = (FaviconView) findViewById(R.id.icon); > } > > @Override > - protected void onAttachedToWindow() { > + public void onAttachedToWindow() { I am curious about this. Probably we should update python generator?
Attachment #8881873 -
Flags: review?(walkingice0204) → review+
Assignee | ||
Comment 7•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8881872 [details] Bug 1366704 - Part 1: Implement ThemedListView for Photon visual refresh. https://reviewboard.mozilla.org/r/152904/#review158620 > Why don't call super.onLayout? ViewGroup is an abstract class and its `onLayout()` is an abstract method. That's is why ThemedViewGroup cannot call super.onLayout. > above three patches, none is using ThemedViewGroup. Is there any purpose to create this class? Sorry! ThemedViewGroup is used in our prototype in github repository but never used in this final version. I'll file another patch and without its file.
Comment 8•7 years ago
|
||
mozreview-review |
Comment on attachment 8881874 [details] Bug 1366704 - Part 3: Support search suggestion visual refresh in private mode. https://reviewboard.mozilla.org/r/152908/#review158632 ::: mobile/android/app/src/photon/java/org/mozilla/gecko/home/SearchEngineRow.java:495 (Diff revision 1) > + mUserEnteredTextView.setPrivateMode(isPrivate); > + > + final int childCount = mSuggestionView.getChildCount(); > + for (int i = 0; i < childCount; i++) { > + final View child = mSuggestionView.getChildAt(i); > + if (child instanceof SuggestionItem) { There is a method `hideRecycledSuggestions` which makes some children gone. Maybe we can also check the visibility as well? ::: mobile/android/base/java/org/mozilla/gecko/home/BrowserSearch.java:483 (Diff revision 1) > + public void onTabChanged(Tab tab, Tabs.TabEvents msg, String data) { > + if (tab == null) { > + return; > + } > + > + switch (msg) { if there is only one case, maybe we can just use if-statement? ::: mobile/android/base/java/org/mozilla/gecko/home/BrowserSearch.java:485 (Diff revision 1) > + return; > + } > + > + switch (msg) { > + case SELECTED: > + if (mList != null) { we register listener after mList is initialized/assigned. If this null checking necessary? ::: mobile/android/base/java/org/mozilla/gecko/home/BrowserSearch.java:1335 (Diff revision 1) > return super.onTouchEvent(event); > } > + > + @Override > + public void setPrivateMode(boolean isPrivate) { > + final boolean currentMode = isPrivateMode(); just nit. I think `final boolean modeChanged = this.isPrivateMode() == isPrivateMode` will give better semantic. ::: mobile/android/base/java/org/mozilla/gecko/home/TwoLinePageRow.java:43 (Diff revision 1) > public class TwoLinePageRow extends ThemedLinearLayout > implements Tabs.OnTabsChangedListener { > > protected static final int NO_ICON = 0; > > - private final TextView mTitle; > + private final FadedSingleColorTextView mTitle; I might not fully understand. It seems using `ThemedTextView` would be enough here?
Attachment #8881874 -
Flags: review?(walkingice0204) → review+
Assignee | ||
Comment 9•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8881874 [details] Bug 1366704 - Part 3: Support search suggestion visual refresh in private mode. https://reviewboard.mozilla.org/r/152908/#review158632 > There is a method `hideRecycledSuggestions` which makes some children gone. Maybe we can also check the visibility as well? If we check the visibility and ignore invisible views, these invisible views would have to check if they are in normal/private mode as well when they change their visibility. It might increase the complexity for both `setVisibility()` and `setPrivateMode()` because of influencing each other.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 13•7 years ago
|
||
mozreview-review |
Comment on attachment 8881872 [details] Bug 1366704 - Part 1: Implement ThemedListView for Photon visual refresh. https://reviewboard.mozilla.org/r/152904/#review158982
Attachment #8881872 -
Flags: review?(walkingice0204) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 17•7 years ago
|
||
mozreview-review |
Comment on attachment 8881872 [details] Bug 1366704 - Part 1: Implement ThemedListView for Photon visual refresh. https://reviewboard.mozilla.org/r/152904/#review160768 ::: mobile/android/base/java/org/mozilla/gecko/widget/themed/ThemedListView.java:22 (Diff revision 3) > +import android.content.res.TypedArray; > +import android.graphics.drawable.ColorDrawable; > +import android.graphics.drawable.Drawable; > +import android.util.AttributeSet; > + > +public class ThemedListView extends android.widget.ListView FYI: Eventually we wanted to replace all ListViews with RecyclerViews. ::: mobile/android/base/java/org/mozilla/gecko/widget/themed/ThemedView.java.frag:77 (Diff revision 3) > @Override > - public void onAttachedToWindow() { > + protected void onAttachedToWindow() { > super.onAttachedToWindow(); > > if (autoUpdateTheme) > theme.addListener(this); > } > > @Override > - public void onDetachedFromWindow() { > + protected void onDetachedFromWindow() { Why are those now public? I can't find the reason for that in the code.
Attachment #8881872 -
Flags: review?(s.kaspari) → review+
Comment 18•7 years ago
|
||
mozreview-review |
Comment on attachment 8881873 [details] Bug 1366704 - Part 2: Support changing theme for search suggestion custom views. https://reviewboard.mozilla.org/r/152906/#review160772
Attachment #8881873 -
Flags: review?(s.kaspari) → review+
Comment 19•7 years ago
|
||
mozreview-review |
Comment on attachment 8881874 [details] Bug 1366704 - Part 3: Support search suggestion visual refresh in private mode. https://reviewboard.mozilla.org/r/152908/#review160778
Attachment #8881874 -
Flags: review?(s.kaspari) → review+
Assignee | ||
Comment 20•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8881872 [details] Bug 1366704 - Part 1: Implement ThemedListView for Photon visual refresh. https://reviewboard.mozilla.org/r/152904/#review160768 > Why are those now public? I can't find the reason for that in the code. Actually the modifier is changed from public back to protected. The modifier of these two methods defined View.java are both protected, it's no nessary to promote to public. https://android.googlesource.com/platform/frameworks/base/+/a175a5b/core/java/android/view/View.java#9500
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 24•7 years ago
|
||
Pushed by cbook@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/9cf3756e48b3 Part 1: Implement ThemedListView for Photon visual refresh. r=sebastian,walkingice https://hg.mozilla.org/integration/autoland/rev/714e8049f1a5 Part 2: Support changing theme for search suggestion custom views. r=sebastian,walkingice https://hg.mozilla.org/integration/autoland/rev/91c6be339537 Part 3: Support search suggestion visual refresh in private mode. r=sebastian,walkingice
Keywords: checkin-needed
Comment 25•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/9cf3756e48b3 https://hg.mozilla.org/mozilla-central/rev/714e8049f1a5 https://hg.mozilla.org/mozilla-central/rev/91c6be339537
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Comment 26•7 years ago
|
||
Verified as fixed on Nightly 56.0a1, (2017-08-10). Devices: Huawei Honor 8 (Android 6.0) Motorola Nexus 6 (Android 7.0) Prestigio Grace X5 (Android 4.4.2) Huawei Honor 8 (Android 6.0)
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
•