940647 - Move first time bookmarks info creation from browser app to operator variant app

Status: RESOLVED FIXED

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

Description

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Right now, we create first bookmarks based on operator variant on first browser load. Since the bookmarks are moving to the system datastore, we should have this happen via the FTU. This will require the FTU having access to the system datastore, as well as updating the bookmark loader to load to the datastore, not the browser db.

Comment 1

[Ben Francis [:benfrancis]]

Hi Kyle,

I think this is required for bug 938288

Are you thinking we would migrate homescreen bookmarks to a datastore as part of this work? As I understand it they're currently stored in IndexedDB in the homescreen app along with the rest of the grid.

Comment 2

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Would the FTU get triggered after a system update?  I don't think it does?  I could be wrong.

Comment 3

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

FTU comes up on update as of bug 939174. Something we're already taking care of. That said, single variant doesn't happen on upgrade, just on first boot. Or at least, it shouldn't. That's something I'll look into.

Comment 4

[Marcelino Veiga Tuimil [:sonmarce]]

(In reply to Kyle Machulis [:kmachulis] [:qdot] from comment #3)
> FTU comes up on update as of bug 939174. Something we're already taking care
> of. That said, single variant doesn't happen on upgrade, just on first boot.
> Or at least, it shouldn't. That's something I'll look into.

Implementation should follow the user story where it was implemented in bug 898392, that implies it is not only related to FTU, it should be included the case of a SIM card is inserted for the first time after FTU

Comment 5

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

The user story on bug 898392 doesn't cover using the FTU in the upgrade path because the idea didn't exist yet. Customizing on upgrade seems like it would cause duplication in contacts, bookmarks, etc.

Comment 6

[Marcelino Veiga Tuimil [:sonmarce]]

Yes, I know, I replied to wrong comment (I was thinking in comment 0). I am not suggesting to implement bookmark customization in FOTA, but as a consequence of removing bookmarks from Browser, we loose bookmark customization, so we should re-implement them for bookmarks in homescreen, and in the same way they were implemented in Browser.

Comment 7

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

As of bug 968668, the default bookmark data got moved to the makefiles in the browser app. We'll need to move those functions over to the system makefiles that came in with bug 968697. Should just be able to cut and paste them over pretty easily. Adding yurenju to our CC list on this since I'll probably have him review also.

Comment 9

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

[Blocking Requested - why for this release]:

Comment 10

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

No longer going to FTU. Now going to operator variant app, to run on first-run-with-sim

Comment 11

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

This isn't just bookmarks. We also need to load top sites, and possibly search engines. So we should just plan on porting everything over.

Comment 12

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Where is search engine information going now? I can move the default settings, but are we still going to still keep a list of them somewhere too? Not seeing anything obvious in the search app.

(First person to this, please un-fb? for others)

Comment 13

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Oh, actually, the browser settings were in the browser DB, not the system settings. Do those need to get ported to the settings system too?

Comment 14

[Kevin Grandon :kgrandon]

(In reply to Kyle Machulis [:kmachulis] [:qdot] from comment #12)
> Where is search engine information going now? I can move the default
> settings, but are we still going to still keep a list of them somewhere too?
> Not seeing anything obvious in the search app.
> 
> (First person to this, please un-fb? for others)

We should already have search information implemented for rocketbar and the search app in settings. It's currently listed under the "Homescreen" panel, but part of the settings work is to move this into the new settings panel that James is working on. Whether or not we need to migrate that data from browser I'm not 100% sure. Perhaps a question for product?

https://github.com/mozilla-b2g/gaia/blob/master/apps/settings/js/vertical_homescreen.js#L119

Leaving ni? on Ben/Dale if I wasn't clear enough, or didn't provide enough info, but feel free to clear if you want.

Comment 15

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

This isn't getting migrated from the browser. We load in default search engine settings and the search engine selection list via operator variant customizations on first SIM insert. Currently that happens in the browser, but I'm moving all of the opvar stuff to the operator variant app, which is what I'm asking about here.

Comment 16

[Kevin Grandon :kgrandon]

I believe we already support this for the search app. There's a good description of this in bug 1031524 comment 12.

Comment 17

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Ah, so we do! Hadn't seen the search customizer module in the operator variant app yet. I'll make part of this bug removing that block from the browser.json in customization then. Thanks Kevin!

Comment 18

[Dale Harvey (:daleharvey)]

Gonna work on top sites customisation in https://bugzilla.mozilla.org/show_bug.cgi?id=1053261, removed as a blocker

Comment 19

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

search engine already happened. Just needs to be bookmarks.

Comment 20

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v1) - Move bookmark opvar creation to opvar app

Comment 21

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v1) - Move bookmark opvar creation to opvar app; r=qDot fb=aus fb=kgrandon

Oops. Gaia patch, linking to pull request and r?'ing :albert since they've worked on browser opvar stuff before.

Comment 22

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8480368 [details] [review]
Patch 1 (v1) - Move bookmark opvar creation to opvar app; r=qDot fb=aus fb=kgrandon

In the long tradition of "OMG VERSION GOES OUT TODAY", I'm gonna slide this in with a gal style self review, though I did get fb from kgrandon and aus on IRC.

Comment 23

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

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

Comment 24

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

And then backed out because I force pushed over the correct commit with an old one with nits fixed. This is probably why we don't self-r things, eh?

New PR up at https://github.com/mozilla-b2g/gaia/pull/23526 - Waiting for this one to actually green on units this time.

Comment 25

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Green'd on try except for Gij's last night, which don't seem capable of becoming unred. None of the offending Gij's seemed dependent on this. Landed revert revert for original patch, and formatting fix.

https://github.com/qdot/gaia/commit/d8fde4d11354d7860aade8815c569ad8748d38e9

Comment 26

[Albert [:albert]]

Comment on attachment 8480368 [details] [review]
Patch 1 (v1) - Move bookmark opvar creation to opvar app; r=qDot fb=aus fb=kgrandon

Sorry for the delay, I was on PTO last week. Ask for review to Carmen, who works in operator variant app.

Comment 27

[Carmen Jimenez Cabezas]

This patch has a side effect. If there's any bookmark defined by single variant, then a single variant app could never be configured to be on the last position of the grid (since bookmarks are not contemplated on the grid configuration and so are always placed after the apps and collections). 
I don't know if this will be a problem for the grid definition, I'll ask our product people about this.

Comment 28

[Carmen Jimenez Cabezas]

Comment on attachment 8480368 [details] [review]
Patch 1 (v1) - Move bookmark opvar creation to opvar app; r=qDot fb=aus fb=kgrandon

I've left some comments on github. I would like to see a new version with the requested changes, specially the one affecting the configuration file. Thanks!!

Comment 29

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Filed followup at bug 1061804, closing this since it merged. Will take care of cleanup there.