1069810 - Homescreen application does not handle basic authentication in icon urls.

Status: RESOLVED FIXED

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

Description

[andreas larsson]

If the icon is on a server that requires basic authentication the homescreen application is unable to download it. This patch fixes the problem.

Comment 1

[andreas larsson]

Attachment: patch for handling basic authentication for icons

Comment 2

[Kevin Grandon :kgrandon]

Comment on attachment 8492048 [details] [review]
patch for handling basic authentication for icons

This looks good to me, so I am going to R+, but at the same time add a commit which adds a simple unit test, and land them together. Thank you for the patch.

Comment 3

[Kevin Grandon :kgrandon]

Attachment: Pull request - /w unit test

Adding a unit test to make sure we don't regress this in the future. Carrying review.

Comment 4

[Kevin Grandon :kgrandon]

In master: https://github.com/mozilla-b2g/gaia/commit/c7ef0bf06ce1c98cbe68aa52e2ecd862acb23e9c

Comment 5

[Kevin Grandon :kgrandon]

Comment on attachment 8492250 [details] [review]
Pull request - /w unit test

I think that this will help our partners, so we should consider uplifting this to 2.1.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): Feature implementation.
[User impact] if declined: 
[Testing completed]: Unit tested.
[Risk to taking this patch] (and alternatives if risky): Low risk.
[String changes made]: No.

Comment 6

[[:fabrice] Fabrice Desré]

Comment on attachment 8492250 [details] [review]
Pull request - /w unit test

This patch is wrong. Please use the URL() object instead of doing string concatenation to resolve urls.

Comment 7

[andreas larsson]

Hi, do you mean by using the nsIURL interface? Or this one:
https://developer.mozilla.org/en-US/docs/Web/API/URL.URL ?

Kind regards
Andreas

Comment 8

[[:fabrice] Fabrice Desré]

(In reply to andreas larsson from comment #7)
> Hi, do you mean by using the nsIURL interface? 

No, you can't use that in content code.

> Or this one:
> https://developer.mozilla.org/en-US/docs/Web/API/URL.URL ?

Yep, this one. It does everything you need properly.

Comment 9

[Gregor Wagner [:gwagner]]

Do we still want this in 2.1?

Comment 10

[Josh Cheng [:josh]]

Hi Kai-Zhen,
Please help to land on 2.0M Thanks!

Comment 11

[Kai-Zhen Li [:seinlin][:kli]]

v2.0m: https://github.com/mozilla-b2g/gaia/commit/795f9867f6aa3a82e34b97843e19e2657697d911

Comment 12

[Josh Cheng [:josh]]

Hi Kevin,
Do you like to revise the patch per comment 6 from Fabrice? 
Is this patch good for 2.0M?
Thanks!

Comment 13

[Kevin Grandon :kgrandon]

Well it looks like it's already in 2.0m. I think it should work, but we could always refactor it to address comment 6. Unfortunately I won't have time to do this right now, but if the patch author or someone else wants to do it - feel free to flag me for review. It looks like this was never a 2.1 blocker so I'm not sure if it's needed?

Comment 14

[Josh Cheng [:josh]]

Hi Kevin,
I suppose the change would be:

line 199 of shared/elements/gaia_grid/js/items/grid_item.js

[from] 
  icon = this.app.origin + icon;
[to]
  icon = new URL(icon, this.app.origin);

Is this correct? Thanks!

Comment 15

[Kevin Grandon :kgrandon]

You should write a unit test and verify that the argument order is correct - I'm not sure if it is.

Comment 16

[[:fabrice] Fabrice Desré]

You should rather resolve the url against the manifest url, not the origin.

Comment 17

[Josh Cheng [:josh]]

Hi Kai-Zhen,
I am sorry that this fix is incompleted and we need to backout this from 2.0M and wait until I fix bug 1126709.
Thanks!

Comment 18

[Kevin Grandon :kgrandon]

(In reply to Josh Cheng [:josh] from comment #17)
> Hi Kai-Zhen,
> I am sorry that this fix is incompleted and we need to backout this from
> 2.0M and wait until I fix bug 1126709.
> Thanks!

Why are we spending so much time on this? I don't think it was ever deemed a blocker and I'm not sure we have a good use case for it?

Comment 19

[Kai-Zhen Li [:seinlin][:kli]]

It is reverted on v2.0m: 
https://github.com/mozilla-b2g/gaia/commit/79f923e7205299bb4ea774ce6ce0921820e5a6bd

Comment 20

[Kai-Chih Hu [:khu]]

Reopen it based on last few comments. 
After the triage with device group, we decided not to + for 2.0 and 2.1.

Comment 21

[Gregor Wagner [:gwagner]]

(In reply to Kevin Hu [:khu] from comment #20)
> Reopen it based on last few comments. 
> After the triage with device group, we decided not to + for 2.0 and 2.1.

The proper workflow here is to set the branch specific flags and not re-open if it wasn't backed out on master.

Comment 22

[Kai-Chih Hu [:khu]]

Thanks.