964476 - [B2G][E.me] Twitter doesn't get added to homescreen from e.me

Status: RESOLVED FIXED

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

Description

[Tony Nguyen]

Description:
Twitter icon doesn't appear when user attempts to add it to homescreen from e.me.

Repro Steps:
1) Updated Buri to BuildID: 20140127072047
2) Navigate to e.me (swipe homescreen left to right)
3) Tap on the Social collection
4) Tap on Twitter icon
5) Swipe on the bottom right of screen and tap on the Star icon
6) Tap the grey Twitter button
7) Tap on "Add to Home Screen" button
8) Tap Home button two times


Actual:
Twitter is not installed to homescreen

Expected:
Twitter is installed to the homescreen

Environmental Variables:
Device: Buri Master M-C mozRIL
BuildID: 20140127072047
Gaia: 75ea7d07cdb590722634016410e12819faf82e5a
Gecko: 53376ef850fc
Version: 29.0a1
v1.2-device.cfg

Attached: logcat

Comment 1

[Tony Nguyen]

Attachment: logcat

Comment 2

[Natalya Kot [:nkot]]

Failing one of the smoketest cases - adding Twitter from E.me to the Homescreen.

The issue doesn't reproduce on v1.3

Device: Buri v1.3 
BuildID: 20140127004002
Gaia: 25a45a836a4a21a30f63fa7b544b42e8b781180a
Gecko: c40099a42c1f
Version: 28.0a2
V1.2-device.cfg

Comment 3

[Sarah Parsons]

This issue started to occur the Buri 1.4 Build ID: 20140126040203

Gaia   f382061fe95750d584a9078175c421a36892afc9
SourceStamp 3f1dd2a8e972
BuildID 20140126040203
Version 29.0a1

Last working Buri 1.4 Build ID: 20140125040202

Gaia   f382061fe95750d584a9078175c421a36892afc9
SourceStamp 9e06d42c2a6a
BuildID 20140125040202
Version 29.0a1

Comment 4

[Sarah Parsons]

This regression window is missing some information, I will update it as soon as possible. 

(In reply to Sarah Parsons from comment #3)
> This issue started to occur the Buri 1.4 Build ID: 20140126040203
> 
> Gaia   f382061fe95750d584a9078175c421a36892afc9
> SourceStamp 3f1dd2a8e972
> BuildID 20140126040203
> Version 29.0a1
> 
> Last working Buri 1.4 Build ID: 20140125040202
> 
> Gaia   f382061fe95750d584a9078175c421a36892afc9
> SourceStamp 9e06d42c2a6a
> BuildID 20140125040202
> Version 29.0a1

Comment 5

[Jason Smith [:jsmith]]

There's no way this regression range is right - there's no Gaia changes in the regression range & the m-c regression range points to fx-team changes only.

Comment 6

[Sarah Parsons]

We were aware the regression window from comment 3 was incorrect, see comment 4.

Correct regression window:

This issue started to occur on the Buri 1.4 Build ID:20140123040608

Gaia   00d8d05f0d0730a3cbf17635ad6a6b197a2ce7c9
SourceStamp 1a75d37e1e39
BuildID 20140123040608
Version 29.0a1

Last working Buri 1.4 Build ID: 20140122040205

Gaia   8d18b1e2fd06c84a879f99f6e8ca1f104eeacb13
SourceStamp 8f4ecbf938cd
BuildID 20140122040205
Version 29.0a1

Comment 7

[Jason Smith [:jsmith]]

https://github.com/mozilla-b2g/gaia/compare/8d18b1e2fd06c84a879f99f6e8ca1f104eeacb13...00d8d05f0d0730a3cbf17635ad6a6b197a2ce7c9

Comment 8

[Cristian Rodriguez (:crdlc)]

It is working on master right now for me, please could you confirm it? thanks

Comment 9

[Sarah Parsons]

This issue still reproduces on the Buri 1.4 Build ID: 20140129040244

Gaia   59e477229162926981cc00cf5d6c53f9614f17dd
SourceStamp 7e79536aca0a
BuildID 20140129040244
Version 29.0a1

Comment 10

[Cristian Rodriguez (:crdlc)]

Could someone explain me what is 1.4? master? Thx

Comment 11

[Jason Smith [:jsmith]]

(In reply to Cristian Rodriguez (:crdlc) from comment #10)
> Could someone explain me what is 1.4? master? Thx

Yup - 1.4 = Master.

Comment 12

[Jason Smith [:jsmith]]

Confirmed this on a 1/30/2014 1.4 build as well.

Comment 13

[Cristian Rodriguez (:crdlc)]

Well, I am taking a look at bug 964889 1.3+ and after that, will take this one, thanks Jason

Comment 14

[Cristian Rodriguez (:crdlc)]

This bug 960660 is the problem in system

https://github.com/mozilla-b2g/gaia/commit/a1e985d3a045daa796812712dfb81486becc5a9b#diff-cfa1d9813ade22ddb10164655efc52b9R604

When we run Twitter or Youtube, a mozbrowsericonchange event is dispatched and this.config.icon is an object with a href attribute in that moment instead of a data:image provided by ev.me. The homescreen is expecting an icon url string instead of an object.href.

The patch is very simple:

https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/app_chrome.js#L327

-          icon: dataset.icon,
+          icon: typeof dataset.icon === 'object' ? dataset.icon.href : dataset.icon,

But the problem IMHO is that with this bug 960660, the icon will be a favicon instead of the rounded icon provided by Ev.me and it does not make sense IMHO

Comment 15

[Dale Harvey (:daleharvey)]

I can take this, sorry I should have checked

I dont think that should be the patch, the icon changed is relating to the favicon used in places, while I do think we should look at consolidating the logic of what icon we use, we are gonna need to do a icon storage service and I think that problem should be fixed then, as for this I just think I should change 

    this.config.icon = evt.detail; 

to 

    this.config.favicon = evt.detail;

I didnt intend to squash others data when I did that

Comment 16

[Dale Harvey (:daleharvey)]

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

Comment 17

[Dale Harvey (:daleharvey)]

Also filed and took https://bugzilla.mozilla.org/show_bug.cgi?id=966137 which I will fix tomorrow

Comment 18

[Kevin Grandon :kgrandon]

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

It's simple enough and makes sense. Dale - will bug 966137 include testing for this issue? It seems like we need or are missing some homescreen tests that would have caught this.

Comment 19

[Dale Harvey (:daleharvey)]

https://bugzilla.mozilla.org/show_bug.cgi?id=966137 ensures I dont regress places with this patch, its failure is just bitrot + orange factor. We dont have tests that would / should have caught this, not sure whether they would need to happen between e.me or homescreen

Comment 20

[Dale Harvey (:daleharvey)]

https://github.com/mozilla-b2g/gaia/commit/bcecb91f5ef7b0d9e426dbda489baee424eed93c

Taking a look at tests