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)

Product:
Firefox for Android Graveyard
Component:
General
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox56 fixed, firefox57 verified)

Status:
VERIFIED FIXED
Milestone:
Firefox 56
Tracking Flags:
Tracking Status
firefox56 --- fixed
firefox57 --- verified

People

(Reporter: wesley_huang, Assigned: jwu)

References

Details

Attachments

(3 files)

Bug 1366704 - Part 1: Implement ThemedListView for Photon visual refresh.
59 bytes, text/x-review-board-request
sebastian
: review+
walkingice
: review+
Details
Bug 1366704 - Part 2: Support changing theme for search suggestion custom views.
59 bytes, text/x-review-board-request
sebastian
: review+
walkingice
: review+
Details
Bug 1366704 - Part 3: Support search suggestion visual refresh in private mode.
59 bytes, text/x-review-board-request
sebastian
: review+
walkingice
: review+
Details 
      No description provided.
Assignee: nobody → topwu.tw
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)
Attachment #8881873 - Flags: review?(cnevinchen) → review?(s.kaspari)
Attachment #8881874 - Flags: review?(cnevinchen) → review?(s.kaspari)
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 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+
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 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+
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 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 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 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 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+
Blocks: 1379652
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
Keywords: checkin-needed
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
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
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)
Status: RESOLVED → VERIFIED
status-firefox57: --- → verified
QA Contact: oana.horvath
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

Creator:
Created:
Updated:
Size: