Closed Bug 940647 Opened 6 years ago Closed 5 years ago

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

Categories

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

x86_64
Linux
defect
Not set

Tracking

(feature-b2g:2.1)

RESOLVED FIXED
2.1 S3 (29aug)
feature-b2g 2.1

People

(Reporter: qdot, Assigned: qdot)

References

Details

(Whiteboard: [systemsfe] )

Attachments

(1 file, 1 obsolete file)

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.
Blocks: 938288
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.
Whiteboard: [ucid:browser104] [systemsfe] [1.4:p2]
Would the FTU get triggered after a system update?  I don't think it does?  I could be wrong.
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.
Target Milestone: --- → 1.4 S1 (14feb)
(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
Depends on: 898392
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.
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.
Whiteboard: [ucid:browser104] [systemsfe] [1.4:p2] → [ucid:browser104] [systemsfe] [1.4:p2] [p=8] [1.4 SP 1 (14feb)]
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.
Duplicate of this bug: 972133
Whiteboard: [ucid:browser104] [systemsfe] [1.4:p2] [p=8] [1.4 SP 1 (14feb)] → [ucid:browser104] [systemsfe] [1.4:p2] [p=8]
Target Milestone: 1.4 S1 (14feb) → 1.4 S2 (28feb)
blocking-b2g: --- → backlog
Target Milestone: 1.4 S2 (28feb) → ---
[Blocking Requested - why for this release]:
blocking-b2g: backlog → 2.1?
Whiteboard: [ucid:browser104] [systemsfe] [1.4:p2] [p=8] → [systemsfe]
blocking-b2g: 2.1? → 2.1+
blocking-b2g: 2.1+ → ---
feature-b2g: --- → 2.1
No longer going to FTU. Now going to operator variant app, to run on first-run-with-sim
Summary: Move first time bookmark creation from browser app to FTU → Move first time bookmark creation from browser app to operator variant app
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.
Summary: Move first time bookmark creation from browser app to operator variant app → Move first time bookmarks/top sites/search engine info creation from browser app to operator variant app
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)
Flags: needinfo?(kgrandon)
Flags: needinfo?(dale)
Flags: needinfo?(bfrancis)
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?
(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.
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.
I believe we already support this for the search app. There's a good description of this in bug 1031524 comment 12.
Flags: needinfo?(kgrandon)
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!
Flags: needinfo?(dale)
Flags: needinfo?(bfrancis)
Target Milestone: --- → 2.1 S3 (29aug)
Gonna work on top sites customisation in https://bugzilla.mozilla.org/show_bug.cgi?id=1053261, removed as a blocker
No longer blocks: 938288
Summary: Move first time bookmarks/top sites/search engine info creation from browser app to operator variant app → Move first time bookmarks/search engine info creation from browser app to operator variant app
search engine already happened. Just needs to be bookmarks.
Summary: Move first time bookmarks/search engine info creation from browser app to operator variant app → Move first time bookmarks info creation from browser app to operator variant app
Oops. Gaia patch, linking to pull request and r?'ing :albert since they've worked on browser opvar stuff before.
Attachment #8480363 - Attachment is obsolete: true
Attachment #8480368 - Flags: review?(alberto.crespellperez)
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.
Attachment #8480368 - Attachment description: Patch 1 (v1) - Move bookmark opvar creation to opvar app → Patch 1 (v1) - Move bookmark opvar creation to opvar app; r=qDot fb=aus fb=kgrandon
Attachment #8480368 - Flags: review+
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.
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 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.
Attachment #8480368 - Flags: review?(alberto.crespellperez) → review?(carmen.jimenezcabezas)
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 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!!
Attachment #8480368 - Flags: review?(carmen.jimenezcabezas)
Filed followup at bug 1061804, closing this since it merged. Will take care of cleanup there.
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.