975834 - homescreen bookmarks do not work for URLs ending in a file

Status: RESOLVED FIXED

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

Description

[Ben Kelly [:bkelly, not reviewing]]

STR:

1) Open the following URL in your browser:

http://people.mozilla.org/~roc/virtual-list-demo.html

2) Press the favorite star at the bottom of the screen.  Save it to the homescreen.
3) Attempt to open the new homescreen icon.

Expected results:

The page opens correctly

Actual results:

Failure to load.  You can see that it is trying to open a URL with a "/" appended to the end of it.  This won't work for a URL that points at an actual file.

This occurs on my v1.4 buri:

  gecko hg:  1238ef12b996
  gaia git:  15b998c0a57e9415fd694fca2477ff1ee8e81661

I verified this works on my v1.1 helix, so this is a regression.

Comment 1

[Jason Smith [:jsmith]]

We should check if this happens on 1.3 as well.

Comment 2

[Cristian Rodriguez (:crdlc)]

It happens in 1.3 as well

Comment 3

[Cristian Rodriguez (:crdlc)]

Regression from bug 939524

Comment 4

[Cristian Rodriguez (:crdlc)]

Attachment: Patch v1

Comment 5

[[:fabrice] Fabrice Desré]

I don't think the origin in this patch is correct.

Comment 6

[Cristian Rodriguez (:crdlc)]

Actually, the origin is not used in bookmarks, just the bookmarkURL and url properties. The first one is used for indexing bookmarks in homescreen and the second one is the url to open the bookmark in a wrapper. We added "/" at the end of the bookmarkURL in order to distinguish between installed apps and bookmarks. E.g. http://m.facebook.com (app) and http://m.facebook.com/ (bookmark). It was done in bug 939524, comment 13. Apps are -> origin = scheme + "://" + domain + [:port] so you should never have a trailing "/". I didn't change the this.origin in this patch so blocking the patch does not make sense in my honest opinion for this reason. Maybe if you want to set the correct origin, a new one bug could be filled to follow this discussion.

Comment 7

[[:fabrice] Fabrice Desré]

Well, that's a terrible way of distinguishing between apps and bookmarks. Why don't we just use an explicit flag? Please file a followup to clean up all that.

Comment 8

[Cristian Rodriguez (:crdlc)]

Filled bug 976955

Comment 10

[Ben Francis [:benfrancis]]

Whoa, you know that http://example.com/foo is not the same URL as http://example.com/foo/ right?

Surely what distinguishes an app from a bookmark is whether it has a manifest URL, not whether it has a slash on the end of its URL? Could we store the manifest URL for this purpose? Does the homescreen app allow for multiple apps per origin?

Comment 11

[Cristian Rodriguez (:crdlc)]

We know distinguish between them perfectly :) but the problem is not distinguish here. The issue is that apps are indexed in the homescreen by means of origin/bookmarkURL. If we want to do it well, we have to change that code for indexing. I can study to do this in that way but the patch could be riskier thought for sure. A new bug 976955 was created to follow this implementation and to do the stuffs correctly

Comment 12

[Cristian Rodriguez (:crdlc)]

I have another idea, maybe no risky, I will take care of it today

Comment 13

[Cristian Rodriguez (:crdlc)]

Comment on attachment 8380559 [details]
Patch v1

Moving the review to Vivien because I thought that he was on PTO this week but I realized yesterday that he is working hard as usual

Comment 14

[Cristian Rodriguez (:crdlc)]

No more trailing '/' anymore ;) it was a bad idea that day

Comment 15

[Cristian Rodriguez (:crdlc)]

Merged in master:

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

Comment 16

[Cristian Rodriguez (:crdlc)]

Comment on attachment 8380559 [details]
Patch v1

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):
[User impact] if declined: URLs that finish in a file are not bookmarked
[Testing completed]: yes
[Risk to taking this patch] (and alternatives if risky): low
[String changes made]:

Comment 17

[Ryan VanderMeulen [:RyanVM]]

This has conflicts on v1.3. Please rebase.

Comment 18

[Cristian Rodriguez (:crdlc)]

Merged in v1.3:

https://github.com/mozilla-b2g/gaia/commit/9e439c98a05bde90b571701ef0b2dbb4249ef196