1366704 - (photon) follow up of search suggestion on the visual refresh of private mode

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, enhancement)

Comment 1

[Jing-wei Wu [:jwu]]

Attachment: Bug 1366704 - Part 1: Implement ThemedListView for Photon visual refresh.

Review commit: https://reviewboard.mozilla.org/r/152904/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/152904/

Comment 2

[Jing-wei Wu [:jwu]]

Attachment: Bug 1366704 - Part 2: Support changing theme for search suggestion custom views.

Review commit: https://reviewboard.mozilla.org/r/152906/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/152906/

Comment 3

[Jing-wei Wu [:jwu]]

Attachment: Bug 1366704 - Part 3: Support search suggestion visual refresh in private mode.

Review commit: https://reviewboard.mozilla.org/r/152908/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/152908/

Comment 4

[Nevin Chen(Not active on Bugzilla)]

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.

Comment 5

[Julian Chu [:walkingice]]

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

[Julian Chu [:walkingice]]

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?

Comment 7

[Jing-wei Wu [:jwu]]

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

[Julian Chu [:walkingice]]

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?

Comment 9

[Jing-wei Wu [:jwu]]

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 10

[Jing-wei Wu [:jwu]]

Comment on attachment 8881872 [details]
Bug 1366704 - Part 1: Implement ThemedListView for Photon visual refresh.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/152904/diff/1-2/

Comment 11

[Jing-wei Wu [:jwu]]

Comment on attachment 8881873 [details]
Bug 1366704 - Part 2: Support changing theme for search suggestion custom views.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/152906/diff/1-2/

Comment 12

[Jing-wei Wu [:jwu]]

Comment on attachment 8881874 [details]
Bug 1366704 - Part 3: Support search suggestion visual refresh in private mode.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/152908/diff/1-2/

Comment 13

[Julian Chu [:walkingice]]

Comment on attachment 8881872 [details]
Bug 1366704 - Part 1: Implement ThemedListView for Photon visual refresh.

https://reviewboard.mozilla.org/r/152904/#review158982

Comment 14

[Jing-wei Wu [:jwu]]

Comment on attachment 8881872 [details]
Bug 1366704 - Part 1: Implement ThemedListView for Photon visual refresh.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/152904/diff/2-3/

Comment 15

[Jing-wei Wu [:jwu]]

Comment on attachment 8881873 [details]
Bug 1366704 - Part 2: Support changing theme for search suggestion custom views.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/152906/diff/2-3/

Comment 16

[Jing-wei Wu [:jwu]]

Comment on attachment 8881874 [details]
Bug 1366704 - Part 3: Support search suggestion visual refresh in private mode.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/152908/diff/2-3/

Comment 17

[Sebastian Kaspari (:sebastian; :pocmo)]

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.

Comment 18

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8881873 [details]
Bug 1366704 - Part 2: Support changing theme for search suggestion custom views.

https://reviewboard.mozilla.org/r/152906/#review160772

Comment 19

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8881874 [details]
Bug 1366704 - Part 3: Support search suggestion visual refresh in private mode.

https://reviewboard.mozilla.org/r/152908/#review160778

Comment 20

[Jing-wei Wu [:jwu]]

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 21

[Jing-wei Wu [:jwu]]

Comment on attachment 8881872 [details]
Bug 1366704 - Part 1: Implement ThemedListView for Photon visual refresh.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/152904/diff/3-4/

Comment 22

[Jing-wei Wu [:jwu]]

Comment on attachment 8881873 [details]
Bug 1366704 - Part 2: Support changing theme for search suggestion custom views.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/152906/diff/3-4/

Comment 23

[Jing-wei Wu [:jwu]]

Comment on attachment 8881874 [details]
Bug 1366704 - Part 3: Support search suggestion visual refresh in private mode.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/152908/diff/3-4/

Comment 24

[Pulsebot]

Pushed by [e-mail]:
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

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9cf3756e48b3
https://hg.mozilla.org/mozilla-central/rev/714e8049f1a5
https://hg.mozilla.org/mozilla-central/rev/91c6be339537

Comment 26

[Oana Horvath [:ohorvath]]

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)