1186077 - Default favicon (globe) is briefly shown on about:home before updating to favicon

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

And it looks bad.

Comment 1

[Sergej Kravcenko]

Attachment: Instant icon load

One of the possible solution. Code block in loadFavicon is for performance reasons because PAGE_SHOW message will try to load icon second time. Maybe it's better to move that check there. Help me with this, not sure about possible side effects.

Comment 2

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

Comment on attachment 8638014 [details] [diff] [review]
Instant icon load

I think I have more context than Martyn, but we'll see who gets to it first. :)

Comment 3

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

Comment on attachment 8638014 [details] [diff] [review]
Instant icon load

Review of attachment 8638014 [details] [diff] [review]:
-----------------------------------------------------------------

Unfortunately, this patch doesn't seem to work - while the initially logic seemed sound, we load the favicons asynchronously so the flicker will occur. Instead, we should probably conditionally clear the favicon.

Comment 4

[Sergej Kravcenko]

Strange, about favicons are always in cache and async loading is not used. You still sees the globe icon? Or we are talking about previous page favicon?
Currently when you enter about:home address you see two icon changes, prev page - globe - search, after the patch it should be prev page - search. Not sure if it possible to change favicon before address bar changes its mode from edit to display.

Comment 5

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

By the way, you can use the "Need more information from" field to give a user a persistent notification so they'll be more likely to see your comments or questions.

I'll add one for myself and get back to you.

Comment 6

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

Sorry, I didn't mean to clear the flag – are you still working on this Sergej? If so, I'll give you a response.

Comment 7

[Sergej Kravcenko]

Still waiting for info

Comment 8

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

(In reply to Sergej Kravcenko from comment #4)
> Strange, about favicons are always in cache and async loading is not used.

I could be wrong – I remember doing a little code digging but perhaps I speculated a bit.

> You still sees the globe icon? Or we are talking about previous page favicon?

The globe icon. STR:

1) Open the browser
2) Hit the 3-dot menu
3) Hit "New Tab"

See: https://www.youtube.com/watch?v=P1jrRYH0Fpg&feature=youtu.be

This video is taken on an N4, Android 5.1.

I see this also (but it's harder to tell) when I navigate to mozilla.org (i.e. the top site) and press the new tab button.

Comment 9

[Sergej Kravcenko]

Attachment: Fix for about:home favicon load

Comment 10

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

Comment on attachment 8659217 [details] [diff] [review]
Fix for about:home favicon load

Review of attachment 8659217 [details] [diff] [review]:
-----------------------------------------------------------------

Unfortunately, I don't have my dev machine at the moment so I won't be able to review this until Monday.

However, do you mind explaining a little how this fixes the issue? It's a little hard to see.

Comment 11

[Sergej Kravcenko]

Line "tabToSelect.addFavicon(url, Favicons.browserToolbarFaviconSize, "");" adds non existing icon to the tab icon list, which leads to default globe icon in the first round of icon update. Second round of icon update extracts icon from tab url and correct is found in the cache. If we remove this line, correct icon will be taken from cache right away.

Changes in Tab.java required to eliminate some flicker when user changes page address to "about:home" on arbitrary loaded page.

Comment 12

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

Comment on attachment 8659217 [details] [diff] [review]
Fix for about:home favicon load

Review of attachment 8659217 [details] [diff] [review]:
-----------------------------------------------------------------

Great! Looks good to me!

Thanks Sergej!

Comment 13

[Sergej Kravcenko]

Can you commit this?

Comment 14

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

Forgot to post try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ef35d96b6411 (all green)

(In reply to Sergej Kravcenko from comment #13)
> Can you commit this?

You can add the "checkin-needed" keyword [1] to get your patch checked in.

[1]: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch#Getting_the_patch_checked_into_the_tree

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/8d3e78909a64

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8d3e78909a64

Comment 17

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

Sergej, if you're interested in more bugs, may I recommend bug 1056031 or bug 1201623?