Pi Wei Cheng [:piwei] (inactive) Reporter
	Description • 10 years ago

Description:
This bug is created to better describe bug 1058266. It is a regression and is introduced with Browser2.

Repro Steps:
1) Update a Flame device to BuildID: 20140826180827
2) Connect to internet by any method
3) Launch Browser and access Facebook.com by typing it in URL field
4) Once Facebook loads, tap on the '...' icon to the right of URL field, select Add to Home Screen and confirm addition
5) Navigate to Homescreen and observe the shortcut icon being added

Expected: 
Shortcut is created with an icon related to the webpage

Actual:
Shortcut is created with the generic rockship icon

Environmental Variables:
Device: Flame 2.1 Master
BuildID: 20140826180827
Gaia: 6e804a42ab90f4251c7fe8c68731dc1c6abd8006
Gecko: 0753f7b93ab7
Version: 34.0a1 (2.1 Master)
Firmware: V123
User Agent: Mozilla/5.0 (Mobile; rv:33.0) Gecko/33.0 Firefox/33.0
  
Repro frequency: 3/3

Attaching: Logcat.

	Pi Wei Cheng [:piwei] (inactive) Reporter
	Comment 2 • 10 years ago

Correction: On comment 1, the facebook icon is on bottom middle.

	Peter Bylenga [:PBylenga]
	Comment 3 • 10 years ago

[Blocking Requested - why for this release]:
bad user experience/functionality with a core feature.

bug 1058266 isn't a regression because that issue reproduces since Browser2 was implemented (20140818173616 is the build id that has it first).

QAWanted to verify that facebook.com icon behaves in the same way as the other testing done in bug 1058266 and is also not a regression for Browser2 feature.

	Chris Kreinbring [:CKreinbring]
	Comment 4 • 10 years ago

The bug repros on Flame 2.1
Actual result: The Facebook bookmark has the generic rocket icon instead of the Facebook icon.

BuildID: 20140818173616
Gaia: b33b4d9558e0b9eabbfda7be23435e2b38fd40bf
Gecko: 111a1da2a95d
Platform Version: 34.0a1
Firmware Version: V123
User Agent: Mozilla/5.0 (Mobile; rv:33.0) Gecko/33.0 Firefox/33.0

	Joshua Mitchell (Inactive)
	Comment 5 • 10 years ago

issue repros on the first available Browser 2 build so it is not a regression

	yifan [:yifan][:yliao]
	Comment 6 • 10 years ago

Icons are saved by the system browser when handling mozbrowsericonchange events. When browsing to the mobile facebook page, the mozbrowericonchange event is not triggered by gecko so the system browser doesn't get the icon.

Here is the icon link in the mobile facebook web page:

<link href="https://fbstatic-a.akamaihd.net/rsrc.php/v2/y9/r/94cCwk8yDOQ.png" rel="apple-touch-icon-precomposed" sizes="114x114" />

	yifan [:yifan][:yliao]
	Comment 7 • 10 years ago

The "apple-touch-icon-precomposed" icon is not supported. See bug 921014 for more details.

https://bugzilla.mozilla.org/show_bug.cgi?id=921014#c47

	yifan [:yifan][:yliao]
	Comment 8 • 10 years ago

Hi Karl,

According to https://bugzilla.mozilla.org/show_bug.cgi?id=921014#c44 , the 'apple-touch-icon-precomposed' is not going to be supported on FxOS. Could we have web compatibility team's support on this icon issue? Or this should be handled by another team? Thank you. :)

	Karl Dubost💡 :karlcow
	Comment 9 • 10 years ago

Let's see…


When requesting Facebook home page, we are redirected to the mobile.

→ http -v GET https://m.facebook.com/ 'User-Agent: Mozilla/5.0 (Mobile; rv:30.0) Gecko/30.0 Firefox/30.0'

GET / HTTP/1.1
Accept: */*
Accept-Encoding: gzip, deflate
Host: m.facebook.com
User-Agent:  Mozilla/5.0 (Mobile; rv:30.0) Gecko/30.0 Firefox/30.0



HTTP/1.1 200 OK
[… etc…]

    <meta name="viewport" content="initial-scale=1,maximum-scale=1,user-scalable=no" />
    <link href="https://fbstatic-a.akamaihd.net/rsrc.php/v2/ya/r/O2aKM2iSbOw.png" rel="apple-touch-icon-precomposed" sizes="196x196" />
[… etc…]

which has indeed only 1 link.
Wondering what is send to other browsers: 

Safari iOS 6
    <meta name="viewport" content="initial-scale=1,maximum-scale=1,user-scalable=no" />
    <link href="https://fbstatic-a.akamaihd.net/rsrc.php/v2/ya/r/O2aKM2iSbOw.png" rel="apple-touch-icon-precomposed" sizes="60x60" />

So from what I gather, all devices are receiving a version of an icon which is basically made for Apple, be chrome, safari, windows which is a bit surprising.

I guess it's worth contacting Facebook for it and see if we can motivate them to change it given that iOS7 and later do not add any effects to the icons.

	Karl Dubost💡 :karlcow
	Comment 10 • 10 years ago

Harald has good relationships with Facebook. So he might be able to accelerate the process.

	yifan [:yifan][:yliao]
	Comment 11 • 10 years ago

Thank you for the great help! :)

	Gregor Wagner [:gwagner]
	Comment 12 • 10 years ago

(In reply to Joshua Mitchell [:Joshua_M] from comment #5)
> issue repros on the first available Browser 2 build so it is not a regression

Did this work with the old browser?

	Ben Francis [:benfrancis]
	Comment 13 • 10 years ago

It did work on the old browser, but it used the favicon. Are we trying to load a precomposed apple-touch-icon and it's failing?

	Candice Serran (:cserran)
	Comment 14 • 10 years ago

Confirmed blocker: regression and definite icon bugs

	Ben Francis [:benfrancis]
	Comment 15 • 10 years ago

Dale, do you know whether we would fire an iconchange event for a precomposed apple-touch-icon that we can't use?

I would guess that either we're firing an event for an apple-touch-icon we can't use instead of firing the iconchange event with a standard rel=icon link for the icon we can use, or the window manager doesn't fall back to favicon.ico in the document root like the browser app did.

	Karl Dubost💡 :karlcow
	Comment 16 • 10 years ago

(In reply to Ben Francis [:benfrancis] from comment #15)
> Dale, do you know whether we would fire an iconchange event for a
> precomposed apple-touch-icon that we can't use?


See Comment #7

	Dale Harvey (:daleharvey) Assignee
	Comment 17 • 10 years ago

We wont fire for |rel="apple-touch-icon-precomposed"| as Karl clarified, and we currently dont fallback to the favicon at root, we should 1. ask facebook to fix it to remove -precomposed 2. add the fallback to favicon at root

	Ben Francis [:benfrancis]
	Comment 18 • 10 years ago

Alive, it sounds like we want to fall back to "favicon.ico" at the root of the domain if no icon is provided in a link tag. I will look at this when I get back from PTO on Wednesday if you or someone else doesn't steal it by then :)

	Karl Dubost💡 :karlcow
	Comment 19 • 10 years ago

Also note that when iOS (7.1 at least) devices do not find a suitable size for icons for the homescreen. Like for example the tiny 16x16px, they generate a partial screenshot of the home page that they use as an icon. 

See for example on Mozilla Web site where it is the case currently.
https://bug1045961.bugzilla.mozilla.org/attachment.cgi?id=8484668



(Wondering what Sean thinks about it)
Another strategy instead of the fallback to the "crappy" favicon 16x16px could be something like:

1. Parse the domain name      (ex: facebook.com)
2. Identify the first letter  (ex: F)
3. Compute the md5 hash of the domain in Hex
4. Take the 6 first characters of this hash and use it as a background-color
5. Create an icon with a white letter on top the background-color.


Some examples (using python):

>>> import md5
>>> md5.new('facebook.com').hexdigest()[0:6]
'2343ec'
>>> # so a white F on #2343EC
>>> md5.new('twitter.com').hexdigest()[0:6]
'7905d1'
>>> # so a white T on #7905D1
>>> md5.new('wikipedia.org').hexdigest()[0:6]
'465f7f'
>>> # so a white W on #465F7F
>>> md5.new('mozilla.org').hexdigest()[0:6]
'b5292b'
>>> # so a white M on #B5292B


The benefits:
* it avoids a pixelated icon on the home screen. 
* it makes the system a bit playful.

	(inactive after 6/18) Alive Kuo [:alive]
	Comment 20 • 10 years ago

(In reply to Dale Harvey (:daleharvey) from comment #17)
> We wont fire for |rel="apple-touch-icon-precomposed"| as Karl clarified, and
> we currently dont fallback to the favicon at root, we should 1. ask facebook
> to fix it to remove -precomposed 2. add the fallback to favicon at root

I think we should do both. Ask facebook to improve and add the fallback.

	Jason Smith [:jsmith]
	Comment 21 • 10 years ago

Flagging regression to indicate it's a user facing regression from the old browser app --> new system browser.

	Ben Francis [:benfrancis]
	Comment 22 • 10 years ago

Update: I have a patch for this, but after chatting to Dale it seems to do this properly we need to modify IconHelper to work differently.

Dale suggests adding a getIcon(url, placeObj) method and then using this all over the system app.

	Ben Francis [:benfrancis]
	Comment 23 • 10 years ago

Hi Dale, could you provide a bit more information about the refactoring you think is necessary? I'm not sure I fully understand what you want to do here.

IconsHelper is in /shared and is used in multiple places in the system so there's a risk of regressions if we're not careful.

	Dale Harvey (:daleharvey) Assignee
	Comment 24 • 10 years ago

Pretty much as you said

IconHelper.getIcon = function(url, placeObj) { 
  if (!placeObj.icons) { 
    return url + favicon.ico
  } 
  return IconHelper.getBestIcon(placeObj);
}

along those lines, formatting the url correctly and fixing the callers

	Dale Harvey (:daleharvey) Assignee
	Comment 25 • 10 years ago

the placeObj will be optional, right now we pass the icons object through some objects and activities and I want to get rid of that, eventually getIcon can look like

getIcon = function(url, [place]) { 
  if (localCache(url)) { 
    return localCache[url];
  }
  // The optional place argument is a pure optimisation to avoid a read
  if (!place) { 
    place = readPlaces(url);
  }
  return getBestIcon(place); // and cache it
}

Which means everything gets cached locally and its consistent etc etc

	Dale Harvey (:daleharvey) Assignee
	Comment 27 • 10 years ago

Talked with ben and I will steal this one

	Kevin Grandon :kgrandon
	Comment 29 • 10 years ago

Comment on attachment 8489922 [details]
https://github.com/mozilla-b2g/gaia/pull/24088

Makes sense to me. Thanks!

	Kevin Grandon :kgrandon
	Comment 30 • 10 years ago

In mater: https://github.com/mozilla-b2g/gaia/commit/679b6a1f3786d429419cd411693536b23785952b

	Kevin Grandon :kgrandon
	Comment 31 • 10 years ago

Comment on attachment 8489922 [details]
https://github.com/mozilla-b2g/gaia/pull/24088

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): Feature implementation.
[User impact] if declined: Bad ux using the new system browser as often favicons will not be displayed.
[Testing completed]: Manual and unit tested.
[Risk to taking this patch] (and alternatives if risky): I would say low risk as it's already somewhat broken.
[String changes made]: None.

	Ryan VanderMeulen [:RyanVM]
	Comment 32 • 10 years ago

v2.1: https://github.com/mozilla-b2g/gaia/commit/74b03617b47f94743e03f288dc4d5c384ae22373

	Brogan Zumwalt [Inactive]
	Comment 33 • 10 years ago

Issue verified fixed on Flame 2.1 and Flame 2.2

Actual Results: Shortcut is created with an icon related to the webpage

Device: Flame 2.1 (319mb)(Kitkat Base)(Full Flash)
BuildID: 20141023001201
Gaia: 1e48e3e40e0780c0cd07a3457e5fe2efeeb542d1
Gecko: 09fb60a37850
Gonk: 05aa7b98d3f891b334031dc710d48d0d6b82ec1d
Version: 34.0 (2.1)
Firmware: V188
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Device: Flame 2.2 Master (319mb)(Kitkat Base)(Full Flash)
Build ID: 20141023040204
Gaia: 27a1d1baaa8e375b70e043efee67d5f2206c330b
Gecko: 88adcf8fef83
Version: 36.0a1 (Master)
Firmware Version: v188
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0

	:Harald Kirschner :digitarald
	Comment 34 • 10 years ago

Remove ni?