1048927 - Enable 'add to home' for all web content

Status: RESOLVED FIXED

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

Description

[Chris Lord [:cwiiis]]

Bug 1045828 added 'add to home' for searches only. We want to be able to add any web content to the home-screen.

Comment 1

[Chris Lord [:cwiiis]]

I didn't quite get this done today, here's my WIP branch: https://github.com/Cwiiis/gaia/tree/browser-overflow-menu

It works, except e.me results aren't sanitized, so quite often the search URL doesn't match the actual URL when you visit the page and so you end up being able to add something already added. I'm not sure of the best way to fix this.

We'll need to either sanitize e.me url results, or do some kind of fuzzy matching if we want this to be reliable. Preferably the former (this is easily enough fixed on browser, but the search app would need to visit the url to find out where it ends up redirecting to for this to work as intended...) Alternatively, we can find some hacky ways around this that work for the common-ish case.

cc'ing kgrandon for comment on whether we want to fix this properly or if hacks are ok.

Comment 2

[Kevin Grandon :kgrandon]

I feel like the right way of doing this is to just abandon the search URL concept, and just use the current value of the browser iframe. I'm not entirely sure why we don't have this concept, so I think we should revisit it. In the meantime if a hack gets you unblocked, I'm fine with that also.

Comment 3

[Chris Lord [:cwiiis]]

(In reply to Kevin Grandon :kgrandon from comment #2)
> I feel like the right way of doing this is to just abandon the search URL
> concept, and just use the current value of the browser iframe. I'm not
> entirely sure why we don't have this concept, so I think we should revisit
> it. In the meantime if a hack gets you unblocked, I'm fine with that also.

I think we should do this too, but where this becomes a problem is if you add to the home-screen via the search results and long-pressing on an icon. If you do it this way, we'd have to resolve the URL to add the bookmark... I don't necessarily think that's a bad thing though and would lean towards doing that than trying to hack something up (I can't think of any hacks that I can't also think of trivial ways to break)

Comment 4

[Chris Lord [:cwiiis]]

Attachment: Enable add-to-home for web content

So, this has the drawback now that if you add a search result from e.me or a collection, its URL likely won't match if you bookmark the same thing from the browser.

I think we need to land this and deal with that in a separate bug.

Comment 5

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 8468457 [details] [review]
Enable add-to-home for web content

Just want to get some answers before r+'ing. Feel free to ask r?.

Comment 6

[Chris Lord [:cwiiis]]

(In reply to Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) from comment #5)
> Comment on attachment 8468457 [details] [review]
> Enable add-to-home for web content
> 
> Just want to get some answers before r+'ing. Feel free to ask r?.

All reasonable concerns, I think this patch could go another round.

Comment 7

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Chris Lord [:cwiiis] from comment #6)
> (In reply to Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails,
> needinfo? please) from comment #5)
> > Comment on attachment 8468457 [details] [review]
> > Enable add-to-home for web content
> > 
> > Just want to get some answers before r+'ing. Feel free to ask r?.
> 
> All reasonable concerns, I think this patch could go another round.

Chris... sorry for bug 1049831 that will ask you to rebase your work...

The main idea behing it is that I realized that I need to review some dead code and you need to move it around and ensure it works while nobody is going to use it anyway. It's too late to go back to the old wrapper stuff.
Sorry for any inconvenience.

Comment 8

[Chris Lord [:cwiiis]]

Comment on attachment 8468457 [details] [review]
Enable add-to-home for web content

All review comments addressed, I believe. (not to say there won't be new ones :))

Note with the new isSearch function, this will often return false erroneously until bug 1049569 lands. I think this is ok though, as the consequences aren't a big deal.

Comment 9

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 8468457 [details] [review]
Enable add-to-home for web content

r+ with nits (in the PR) and it would be nice to a few new tests.

Comment 10

[Chris Lord [:cwiiis]]

Comment on attachment 8468457 [details] [review]
Enable add-to-home for web content

Sorry for the churn, but I hadn't noticed the test failures before and I want to get your ok on the changes to app_chrome_test.js. I've addressed your nits too.

wrt to tests, kgrandon has said that he's going to kick off writing a marionette test for this, after which I can write tests for all the options in the overflow menu.

Comment 11

[Chris Lord [:cwiiis]]

Merged: https://github.com/mozilla-b2g/gaia/commit/dc7315b624468f99fb95576a03090be8905e38a3