Closed Bug 1186077 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox for Android Graveyard :: General, defect)

All
Android
defect
Not set
normal

Tracking

(firefox43 fixed)

RESOLVED FIXED
Firefox 43
Tracking Status
firefox43 --- fixed

People

(Reporter: mcomella, Assigned: sergej, Mentored)

References

Details

(Whiteboard: [lang=java])

Attachments

(1 file, 1 obsolete file)

Attached patch Instant icon load (obsolete) — Splinter Review
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.
Attachment #8638014 - Flags: review?(mhaigh)
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. :)
Attachment #8638014 - Flags: review?(michael.l.comella)
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.
Attachment #8638014 - Flags: review?(michael.l.comella)
Attachment #8638014 - Flags: review?(mhaigh)
Attachment #8638014 - Flags: feedback+
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.
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.
Flags: needinfo?(michael.l.comella)
Assignee: nobody → sergej
Flags: needinfo?(michael.l.comella)
Sorry, I didn't mean to clear the flag – are you still working on this Sergej? If so, I'll give you a response.
Flags: needinfo?(sergej)
Still waiting for info
Flags: needinfo?(sergej)
(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.
Attachment #8638014 - Attachment is obsolete: true
Attachment #8659217 - Flags: review?(michael.l.comella)
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.
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 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!
Attachment #8659217 - Flags: review?(michael.l.comella) → review+
Can you commit this?
Flags: needinfo?(michael.l.comella)
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
Flags: needinfo?(michael.l.comella) → needinfo?(sergej)
Flags: needinfo?(sergej)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/8d3e78909a64
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
Sergej, if you're interested in more bugs, may I recommend bug 1056031 or bug 1201623?
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.