1093469 - [Browser]Browser Homescreen Bookmark Icons are not same for the website(defaults icon is shown)

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Browser, defect)

Description

[vsireesha246]

In V2.0 Version this issue is reproduced.

STR:
1.Open Browser app
2.Browser any website having images which is having Prev and Next buttons to navigate.
3.Try to add first image as bookmark to Homescreen.The icon added to Homescreen is proper.
4.Navigate images using next buttons and try to add 3 or 4th or any random one to Homescreen.
5.The default icon is added to Homescreen.

Expected:Proper website Icon need to be added for the same website.Not the default one icon.

Comment 1

[vsireesha246]

Attachment: Bug_1093469.patch

Hi Ben,

Would you please review the attached patch and let me know your review comments.

Thanks..
Sireesha

Comment 2

[Kevin Grandon :kgrandon]

Thank you for the patch.

Comment 3

[yifan [:yifan][:yliao]]

Thank you for the patch! BrowserDB.setAndLoadIconForPage is async, there's possibility that it finishes after BrowserDB.getPlace.

Comment 4

[Ben Francis [:benfrancis]]

Comment on attachment 8516489 [details] [diff] [review]
Bug_1093469.patch

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

Thank you for the patch but I'm afraid it looks like this patch will have no effect.

Can you provide an example of a web page which exhibits this bug?

Also, I'm not sure what version of the browser app you are patching, the line numbers don't seem to match. Can you provide a pull request on GitHub for future patches?

The browser app should already load a favicon from the default location /favicon.ico here https://github.com/mozilla-b2g/gaia/blob/v2.0/apps/browser/js/browser.js#L436

::: apps/browser/js/browser.js
@@ +905,3 @@
>      if (!this.currentTab.url || UrlHelper.isNotURL(this.currentTab.url)) {
>        // TODO: don't silently fail here
>        return;

If !this.currentTab.iconUrl is true, the code will return here.

@@ +906,5 @@
>        // TODO: don't silently fail here
>        return;
> +    } else {
> +      // If no icon URL found yet, try loading from default location
> +      if (!this.currentTab.iconUrl) {

This code will never be reached.

Comment 5

[vsireesha246]

Can you provide an example of a web page which exhibits this bug?

m.daum.net and select any first image and add it to Homescreen and then navigate to next images(using next button) and the add it to Homescreen.
-----------------------------------------------------------------------------------------------------------------
Also, I'm not sure what version of the browser app you are patching, the line numbers don't seem to match. Can you provide a pull request on GitHub for future patches?

In Version V2.0 i added this patch
------------------------------------------------------------------------------------------------------------------
The browser app should already load a favicon from the default location /favicon.ico here https://github.com/mozilla-b2g/gaia/blob/v2.0/apps/browser/js/browser.js#L436

Yes,but in this case the iconurl is null or undefined causing this problem and adding default icon to Homescreen

Comment 6

[Ben Francis [:benfrancis]]

Attachment: https://github.com/mozilla-b2g/gaia/pull/25890

Sorry vsireesha, I mis-read your patch. I see what you're trying to do now.

Unfortunately the patch you submitted may suffer from race conditions as yifan pointed out.

I've supplied an alternative patch.

The underlying issue here is actually that we set iconUrl to null on a loadstart event and if it has not been set by an iconchange event by the time loadend comes around we try to set it to the default /favicon.ico.

In the case of the example you give, we are getting locationchange events after the initial page has loaded but no loadstart or loadend (possibly due to use of the History API). When you go to bookmark the URL it doesn't have an icon set in the places database so you get the placeholder icon. This patch ensures you will get the /favicon.ico instead. It also removes an apparently un-necessary round-trip to the database.

Note that this patch is against 2.0 so will require approval for uplift to the 2.0 branch. I expect this is unlikely to be approved at this stage.

Comment 7

[Dale Harvey (:daleharvey)]

Comment on attachment 8518218 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/25890

In general if people are submitting patches, I would point then towards what is wrong and how to fix their patch as opposed to taking it, but the code looks fine.

Comment 8

[Ben Francis [:benfrancis]]

Comment on attachment 8518218 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/25890

vsireesha246, we can ask for approval for this patch to be landed on 2.0 but I think that's very unlikely at this stage as it isn't marked as a blocker.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): n/a
[User impact] if declined: Some web pages will have the default icon when added to the homescreen, possibly just pages where the History API is used to change the URL without loading a new web page.
[Testing completed]: Manual only, this legacy code does not have extensive test coverage.
[Risk to taking this patch] (and alternatives if risky): Small JavaScript change, relatively low risk of regression.
[String changes made]: none.

Comment 9

[bhavana bajaj [:bajaj]]

Comment on attachment 8518218 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/25890


Its too late to land this change on 2.0 and this does not look like a ship-blocker, so will have to be fixed in 2.1

Comment 10

[Ben Francis [:benfrancis]]

This bug is already fixed in 2.1 and 2.2. Will have to set WONTFIX on 2.0.

Comment 11

[Lance（leave from Mozilla）]

Hi vsireesha246,
    Can you privode the bug video for me? Thanks.

Comment 12

[vsireesha246]

Sorry I can't upload video,but it is easily reproducible by following the mentioned STR in bug.

Comment 13

[Lance（leave from Mozilla）]

This bug has been verified to fail on Flame 2.1, 2.2.
See attachment: 1548.MP4 and logcat_1548.txt
Reproducing rate: 5/5

1.Open Browser app
2.Browser any website(ex:http://photo.poco.cn/special_topic/topic_id-7873-p-1.html#content_hash) having images which is having Prev and Next buttons to navigate.
3.Tap "..." to add first image web to Homescreen.
**The icon added to Homescreen is proper.
4.Navigate images using next buttons and try to add 3 or 4th or any random one to Homescreen.

Actual result:
The default icon is added to Homescreen.

Expected:
Proper website Icon need to be added for the same website.Not the default one icon.

Flame 2.1 version:
Gaia-Rev        38e17b0219cbc50a4ad6f51101898f89e513a552
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-2g34_v2_1/rev/8b92c4b8f59a
Build-ID        20141205001201
Version         34.0
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20141205.035305
FW-Date         Fri Dec  5 03:53:16 EST 2014
Bootloader      L1TC00011880

Flame 2.2 version:
Gaia-Rev        4cdeee67b449db90aae9384337311547c280093c
Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/d7c76fe69e9a
Build-ID        20141209160204
Version         37.0a1
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20141209.192623
FW-Date         Tue Dec  9 19:26:39 EST 2014
Bootloader      L1TC00011880

Comment 14

[Lance（leave from Mozilla）]

Attachment: logcat_1548.txt

Comment 15

[Lance（leave from Mozilla）]

Attachment: 1548.MP4