1041620 - Add system browser icon to homescreen

Status: RESOLVED FIXED

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

Comment 1

[Kevin Grandon :kgrandon]

What does this for exactly? Can you attach a spec?

Comment 2

[Kevin Grandon :kgrandon]

Oh right, is this for what we previously used a bookmark for?

Comment 3

[Dale Harvey (:daleharvey)]

Yup exactly, either we enable the ability to specify a custom bookmark that triggers |https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/wrapper_factory.js#L38| or we make the search an application with an icon and special case the system to recognise that app and expand the rocketbar

Thinking about it I still think the current bookmark is the best solution, gonna think about it a bit before implementating it

Comment 4

[[:fabrice] Fabrice Desré]

Could we make bookmarks as activity wrappers, ie. a bookmark would be (icon, name, grid_state?, activity_payload) ?

Comment 5

[Dale Harvey (:daleharvey)]

and have the system app as the handler to the 'rocketbar' activity? that sounds fairly nice to me, I think the grid component may already be fairly closely set up to that already

Comment 6

[[:fabrice] Fabrice Desré]

Yes, exactly. Bonus point, that will let us to things like bookmarking shortcuts to an address book entry, etc.

Comment 7

[Dale Harvey (:daleharvey)]

So probably worth going through this in work hours, but laying out the ways we can do this.

1. As currently but fired via activity

The launching of the start page is fairly clean via an activity, however dealing with the search app + rocketbar having to show and hide the start page is messy.

2. 'Start page' as an application

The system will need a special case to 1. show the rocketbar expanded on the 'start page', 2. Hide the search results window when you there is no search in progress

The search page has to request screenshots from the search app, the is currently easy as we already have an open iac in the search window

A 'Start page app' breaks from the app window management model, if a user opens start page, clicks a link to make that app navigate, when we press on the start page icon again we want to open a new instance (wondering if we actually want that behaviour for all applications though (user opens twitter, navigates to a url that replaces the twitter app content, they can press back to go back to the app, but if they dont and open twitter again, it should open a new instance)

Comment 8

[Kevin Grandon :kgrandon]

I would also like to compare with the approach of using the Search app with an icon as Ben suggested. We should perform some minimum level of due-diligence to consider all options, and go with what's best. I'm more than happy to spin a patch for this to compare it if needed.

Comment 10

[Ben Francis [:benfrancis]]

Hope you don't mind, but I think the title of this bug was confusing.

Comment 11

[Dale Harvey (:daleharvey)]

No bother

So talking through the UX specs and experimenting a couple of different ways, I think I have found the cleanest way to implement this, we can have a index.html that is the new tabs and a search.html that is opened via searchWindow and acts completely as it currently does. we will need some code to detect the search.index.html is open and keep rocketbar expanded, I think thats reasonable.

There is 1 fairly annoying part that I cant figure out how to implement in a clean way, the screenshots for the tab page is currently driven through IAC which is brittle.

So the current state is: 
 1. System app checks various events and uses that to keep a places datastore
 2. Search app syncs that datastore, it maintains an index to keep track of the top sites that are to be displayed
 3. Search app sends an IAC message to the system app to take a screenshot if a currently visited page is on that top site list
 4. System app takes a screenshot and stores it in places
 5. Search app recieves the change and can display the screenshot

This is brittle and tightly coupled, however its fairly efficient since we only take screenshots of loaded pages (but does mean if the page is closed between all this we dont have a screenshot), it also means we arent garbage collecting screenshots.

There are a few ways I can think of to implement this

 1. Change the IAC to a meta tag, a permanent IAC channel is a lot more complex to maintain (particularly when we have shared context between the search and system) - this is slightly less brittle, but just as coupled, still doesnt consider garbage collection etc, its just a slightly less hacky verion of a very hacky version.

 2. Have the Search app take its own screenshots - This is architecturally a lot cleaner, however loading a mozbrowser frame in the background purely to take a screenshot is hugely expensive, its probably not even gonna work at all on low memory devices and is going to be a big performance drag at other times.

 3. Have the System app keep track of the top visited sites - This moves a lot of logic into the system app which is icky, currently the system app does dumb writes to the places datastore, since datastores dont have indexes we would need to create another local indexeddb store in the system app to keep track of the most visited sites and any change in that logic would need to be implemented in the system app.

Frankly I dont like any of these options, for implementing the first browser icon / topsites I will start with switching to the metatagadded thing as it simplifies the implementation, but I would love to hear some nicer solution that I havent thought of

Comment 12

[Dale Harvey (:daleharvey)]

So after discussing this with UX and playing around with the implementation, the window manager seems to really not like me having searchWindow open a seperate window with search.html, there are also various build issues with that setup.

My current plan is a new Browser2 application, completely standalone, reuses the sync module from https://bugzilla.mozilla.org/show_bug.cgi?id=1042116, I talked with UX and they expect the top Sites page to have as any other application with a proper background and transparent search results overlayed so that makes this behaviour vastly easier.

Comment 13

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

(In reply to Dale Harvey (:daleharvey) from comment #12)
> So after discussing this with UX and playing around with the implementation,
> the window manager seems to really not like me having searchWindow open a
> seperate window with search.html, there are also various build issues with
> that setup.
> 
> My current plan is a new Browser2 application, completely standalone, reuses
> the sync module from https://bugzilla.mozilla.org/show_bug.cgi?id=1042116, I
> talked with UX and they expect the top Sites page to have as any other
> application with a proper background and transparent search results
> overlayed so that makes this behaviour vastly easier.

I'm really not sure about the right path to implement that. But going with a separate app for now sounds good enough. 

The screenshot stuff seems an issue, but I'm not sure to understand why we can't just read the one from the Places datastore (if any ?).

Comment 14

[Dale Harvey (:daleharvey)]

The issue is we dont want to store a screenshot for every loaded page, the top sites app is the one that knows what urls we want screenshots of, the system app is the one that can take the screenshots, so we have to do some hacky way for the top sites to tell the system app what sites to take screenshots of.

Comment 15

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

(In reply to Dale Harvey (:daleharvey) PTO 12th August from comment #14)
> The issue is we dont want to store a screenshot for every loaded page, the
> top sites app is the one that knows what urls we want screenshots of, the
> system app is the one that can take the screenshots, so we have to do some
> hacky way for the top sites to tell the system app what sites to take
> screenshots of.

I assume this is because the frecency counter in Places.js is not enough compared to all the things done by the search app ?

Comment 16

[Dale Harvey (:daleharvey)]

Yup, the datastore doesnt have a index by frecency, so it would require syncing to a local idb store, however the way we have architected the places syncing it should actually be relatively trivial to share that data across system and search app, we only need to keep the last X (6) in memory so duplicating it isnt an issue, that gets rid of the whole need for the screenshot iac which is 90% of the hackiness of this

Comment 17

[Kevin Grandon :kgrandon]

Hey Dale - being that it's critical that we get this in this week, and you are on PTO - I'm going to go ahead and steal this from you for the time being. If you do get back and actively working on this before I land a patch feel free to steal this back.

Sorry for stealing - only doing so because this is urgent. Thanks!

Comment 18

[Kevin Grandon :kgrandon]

Attachment: Github pull request

This pull requests revives the previous newtab page inside of the search app as the launch_path for the search app. The search app is given an icon on the homescreen which launches this new launch path.

Comment 19

[Kevin Grandon :kgrandon]

Comment on attachment 8471070 [details] [review]
Github pull request

Hey guys, this restores most of the "start page" functionality that we previously had. It makes the Search app display an icon on the homepage, and that will launch a different page based on launch_path. It's not 100% to spec yet, but I think it gets us to a good place again to iterate on it. Ben could you give this a review? 

Alive - also a change in app_window_factory that you should probably take a look at. Do you think this is ok for now? Should we try to have Rocketbar intercept the open-app event perhaps?

Comment 20

[(inactive after 6/18) Alive Kuo [:alive]]

Could you elaborate: who is launching search app via open-app event?

Comment 21

[Kevin Grandon :kgrandon]

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #20)
> Could you elaborate: who is launching search app via open-app event?

Rocketbar launches the search window, and this fires an open-app event. In this case we do not want the default actions inside of window factory to happen.

Comment 22

[Ben Francis [:benfrancis]]

Comment on attachment 8471070 [details] [review]
Github pull request

I would have preferred to approach this differently by implementing one feature at a time fully to spec, but I think this code is fine in that it mainly just gives access to the features which are already landed behind a pref, but with a new cleaner approach to showing the new tab page.

Follow-ups will therefore be needed for:
* Rocketbar is not expanded on the new tab page
* The new tab page stays in the task manager after the user taps a top site or history item (I actually quite like this but it isn't to UX spec)
* The Browser icon is called "Search" and has a search icon. The spec requires it to be called "Browser" and have a browser icon but for "Search or enter address" placeholder text to appear in the Rocketbar on the new tab page.
* We're displaying hard-coded data for default top sites which was used for user testing. We need to re-implement the feature we have in the browser app to define default top sites at build time and on first SIM entry and hook this up to a real places database.
* The history list doesn't display date headings as per the spec, which is currently impossible because we're not recording all "visits" in a places database so we only know the last time a place was visited
* There's no landscape view of top sites

Interaction spec here: https://mozilla.app.box.com/s/lbw2wzw3p4jvxs24k4sg/1/2099951272/19877711201/1
Visual spec here: https://mozilla.app.box.com/s/ac65vfmuq1ywl9s2wkqc

This patch turns on a partial implementation of the following features:
Bug 962610 - [User Story] Browser Chrome: Top Sites
Bug 962612 - [User Story] Browser Chrome: View History
Bug 941270 - [User Story] Browser: New window

But these user stories should not be closed until this functionality is fully to spec.

I defer to Alive on the app_window_factory.js part because it seems pretty dirty to hard-code "newtab.html".

Comment 23

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Kevin Grandon :kgrandon from comment #21)
> (In reply to Alive Kuo [:alive][NEEDINFO!] from comment #20)
> > Could you elaborate: who is launching search app via open-app event?
> 
> Rocketbar launches the search window, and this fires an open-app event. In
> this case we do not want the default actions inside of window factory to
> happen.

AFAIK search window is not launched via mozApps API..

Comment 24

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8471070 [details] [review]
Github pull request

I don't read the spec(where is it?) and don't know if it's up to spec, but the code looks fine.

Please add a test in app_window_factory_test.

Comment 25

[Kevin Grandon :kgrandon]

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #23)
> AFAIK search window is not launched via mozApps API..

Looking at this code again, I think what might be happening is that this is getting called from IAC. Perhaps we are trying to send an IAC to the search app before it's opened? In any case I will investigate, we might be able to remove this.

Comment 26

[Kevin Grandon :kgrandon]

Master: https://github.com/mozilla-b2g/gaia/commit/041babb2f2ecd426b909c7906d977d23f75de48f