1341275 - Activity stream - Website icon from Context menu is inconsistent

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P1)

Description

[Carmen Fat [:cfat] - Ecosystem QA]

Environment: 
Devices:  
- HTC 10 (Android 6.0.1);
- Nexus 5 (Android 6.0.1)
Build: Nightly 54.0a1 (2017-02-21);

Prerequisites: Activity stream on Android is enabled.

Steps to reproduce:
1. Open Fennec;
2. Go to Activity stream Panel;
3. Tap on the 3 dots context menu icon of a website several times (either from Top sites or Highlights areas);
4. Repeat step 3 for various websites.

Expected result:
Context menu is triggered, icons are displayed properly, there are no UI issues.

Actual result:
The icon from the context menu is inconsistent: sometimes it appears as big as in the tiles, sometimes only the favicon is displayed.

Notes:
Please see the Screencast attached.
This is not reproducible on Oneplus Two (Android 6.0.1) and LG G4 (Android 5.1).

Comment 1

[Carmen Fat [:cfat] - Ecosystem QA]

Attachment: Inconsistent website icon.mp4

Comment 2

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

I am able to reproduce on my N5.

Comment 3

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

When opening the context menus, I noticed that `FaviconView.formatImage` is called twice:
- FaviconView.onSizeChanged -> formatImage
- onIconResponse in BottomSheetContextMenu -> ... -> formatImage

When onSizeChanged is called first, everything looks great. When onIconResponse is called first, we see the unexpected behavior.

Now to figure out why it's called twice and whether or not it should be called twice...

Comment 4

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

When onIconResponse is called first, it calls showNoImage because the size of the ImageView isn't set yet and it's early returning. `showNoImage` sets `mDominantColor = 0`, overriding the value we wanted to assign. When onSizeChanged is called second, mDominantColor is still 0 so we don't fill with the proper color.

Comment 5

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

I have the patches completed for review but I'm blocked on pushing to reviewboard because my SSH access expired (bug 1381164). This patch isn't urgent so I'll wait – NI self to remember to follow-up on this bug.

Comment 6

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Bug 1341275: Remove unused constants in FaviconView.

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

Comment 7

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Bug 1341275: Rm redundant mActualHeight/Width variables.

These are already present in View.getHeight/Width and:
1) Having duplicated values can lead to bugs
2) Having them around makes it harder to see what's important in this code.

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

Comment 8

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Bug 1341275: Rm redundant check for size change in onSizeChanged.

onSizeChanged should only be called when the size actually changes. However,
I did notice weird behavior in the debugger where `formatImage` appeared to be
called twice from onSizeChanged, however, I was unable to reproduce this
behavior with log statements.

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

Comment 9

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Bug 1341275: Replace color 0 with Color.TRANSPARENT.

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

Comment 10

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Bug 1341275: Add mShouldShowImage flag to FaviconView.

I verified this fixed the issue by:
- Attaching a debugger and checking against the STR call order (comment 3).
- Ensuring the favicons in the SearchEngineRow looked correct

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

Comment 11

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8887105 [details]
Bug 1341275: Remove unused constants in FaviconView.

https://reviewboard.mozilla.org/r/157840/#review162950

Comment 12

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8887106 [details]
Bug 1341275: Rm redundant mActualHeight/Width variables.

https://reviewboard.mozilla.org/r/157842/#review162952

::: mobile/android/base/java/org/mozilla/gecko/widget/FaviconView.java:137
(Diff revision 1)
>       * normal sized ones, replaces null bitmaps with the default Favicon, and fills all remaining space
>       * in this view with the coloured background.
>       */
>      private void formatImage() {
>          // We're waiting for both onSizeChanged and updateImage to be called before scaling.
> -        if (mIconBitmap == null || mActualWidth == 0 || mActualHeight == 0) {
> +        if (mIconBitmap == null || getWidth() == 0 || getHeight() == 0) {

I assume you checked (or know) that getWidth() and getHeight() return the right values this point already? I don't remember the complete flow but it takes some time until those methods return the measured size (There's also getMeasuredWidth()/getMeasuredHeight()).

Comment 13

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8887107 [details]
Bug 1341275: Rm redundant check for size change in onSizeChanged.

https://reviewboard.mozilla.org/r/157844/#review162954

Comment 14

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8887108 [details]
Bug 1341275: Replace color 0 with Color.TRANSPARENT.

https://reviewboard.mozilla.org/r/157846/#review162956

Comment 15

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8887109 [details]
Bug 1341275: Add mShouldShowImage flag to FaviconView.

https://reviewboard.mozilla.org/r/157848/#review162962

Comment 16

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8887106 [details]
Bug 1341275: Rm redundant mActualHeight/Width variables.

https://reviewboard.mozilla.org/r/157842/#review162952

> I assume you checked (or know) that getWidth() and getHeight() return the right values this point already? I don't remember the complete flow but it takes some time until those methods return the measured size (There's also getMeasuredWidth()/getMeasuredHeight()).

I didn't think about this – thanks for catching it. That being said, I looked this up:

1) `getMeasuredWidth/Height` returns the dimension the view measures for itself so that the parent can decide to give it its full size whereas `getWidth/Height` returns the dimension the parent actually assigned the child. `mActualWidth/Height` correspond to the actual size so `getMeasured*` is not useful to us here.

2) `getWidth/Height` is a computation from `mBottom` & friends, which get assigned from `setBottom` or `setFrame`.

Both `setFrame` and `setBottom` (& friends) first assign the new values then call `sizeChange`, which calls `onSizeChanged`. All of our code waits for `onSizeChanged` before using width/height values so they should be valid. In fact, this is where `mActualWidth/Height` was getting assigned in the first place.

I'll add a comment to make sure this is clear to developers coming across the code.

---

Bonus info: `setFrame` will be called from `layout` so the `getWidth/Height` values should be set after the first layout.

Comment 17

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Bug 1341275 - review: Add comment explaining FaviconView.getWidth non-zero values.

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

Comment 18

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8887171 [details]
Bug 1341275 - review: Add comment explaining FaviconView.getWidth non-zero values.

https://reviewboard.mozilla.org/r/157912/#review163042

r=trivial.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d565d004c4af
Remove unused constants in FaviconView. r=sebastian
https://hg.mozilla.org/integration/autoland/rev/8f2c2231ebfd
Rm redundant mActualHeight/Width variables. r=sebastian
https://hg.mozilla.org/integration/autoland/rev/fd4091dc602f
Rm redundant check for size change in onSizeChanged. r=sebastian
https://hg.mozilla.org/integration/autoland/rev/c3b8710c0d73
Replace color 0 with Color.TRANSPARENT. r=sebastian
https://hg.mozilla.org/integration/autoland/rev/f961f2145216
Add mShouldShowImage flag to FaviconView. r=sebastian
https://hg.mozilla.org/integration/autoland/rev/d7b2b1eb46a0
review: Add comment explaining FaviconView.getWidth non-zero values. r=mcomella

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d565d004c4af
https://hg.mozilla.org/mozilla-central/rev/8f2c2231ebfd
https://hg.mozilla.org/mozilla-central/rev/fd4091dc602f
https://hg.mozilla.org/mozilla-central/rev/c3b8710c0d73
https://hg.mozilla.org/mozilla-central/rev/f961f2145216
https://hg.mozilla.org/mozilla-central/rev/d7b2b1eb46a0

Comment 21

[Gerry Chang [:gchang]]

We are about to go to 55 RC. Mark 55 won't fix.

Comment 22

[Bogdan Surd, QA [:bsurd, NI]]

Devices:
 - HTC 10 (Android 7.0);
 - Nexus 5 (Android 6.0.1);
 - Note 4 (Android 5.0.1).

Verified this in the latest Nightly using the steps in Comment 0. Long tapped instead of tapping on 3 dot menu since the design has changed. The issue was no longer reproducible.