Closed Bug 988172 Opened 7 years ago Closed 7 years ago

Create datastore for bookmarks and extract the UI to add them to system

Categories

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

All
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
1.4 S5 (11apr)

People

(Reporter: crdlc, Assigned: crdlc)

References

Details

(Whiteboard: [systemsfe])

Attachments

(1 file)

190 bytes, text/html
daleharvey
: review+
vingtetun
: feedback+
Details
No description provided.
Assignee: nobody → crdlc
Blocks: 898325
Status: NEW → ASSIGNED
You might want to sync with Kyle or Dale on this so we don't duplicate the work here. They have been working on similar functionality.
I am synchronized with Vivien, Kyle is on PTO and they are working on browser bookmarks code AFAIK
(In reply to Cristian Rodriguez (:crdlc) from comment #2)
> I am synchronized with Vivien, Kyle is on PTO and they are working on
> browser bookmarks code AFAIK

There are no more browser bookarks :) With the integration of the browser into the system we are having just one place for bookmarks and that's in the system. This DB should hold much more than just bookmarks. Dale knows more about the places DB that he is working on.
Attached file Patch v1
Hi Vivien, the code is ready and working fine but before implementing and moving tests I would like to know your feedback about this implementation

Thanks a lot
Attachment #8397118 - Flags: feedback?(21)
Dale is in the cc in this bug. Basically, I am moving bookmarks that lived in the homescreen to a new app called "bookmark" that will control the datastore, the "Add bookmark", "Update bookmark" and "Delete bookmark" UIs (activities). Homescreens will receive by means of an API events about bookmarks like "added", "updated" and/or "deleted". So, these code are decoupled completely from current homescreen and in next weeks both homescreens (vertical and current implementation) have the same bookmarks. It is working right now with the current Browser app and it will work when this app will belong to system app
This definitely needs coordinated with the places work (which is the same work), as gregor mentioned browser bookmarks are going to be replaced by homescreen bookmarks.

Currently we have the places datastore in the system app which currently only stores history, bookmarks will either need to go within the same store or ast least closely integrated, they have an effect on frecency and will likely need to share services like how we handle icons (which is non trivial)

I am gonna look around for previous discussion to link, the best start is probably the spec though - https://mozilla.app.box.com/s/0gm5nqtm6inkbjx6qfn8, also a bunch of discussion about it on https://etherpad.mozilla.org/RB-Eng-Review

It would be helpful to know your plans / background for this change, we are gonna have a meeting about this stuff on tuesday it might help us get the changes alighned

As an aside, when vivien does give feedback, we would very much like to avoid these stores being owned by the system app, however being that it has to store history, thats the only place we can currently have it
According to Vivien's suggestion taken in the IRC the architecture what I am doing is this:

1) A dedicated app called "bookmark" which will control all operations related to bookmarks

https://github.com/crdlc/gaia/tree/bug-988172/apps/bookmark

1.1) Bookmarking

It publishes an activity named "save-bookmark" (like homescreen had in the v1.4)

https://github.com/crdlc/gaia/blob/bug-988172/apps/bookmark/manifest.webapp#L12

This inline activity shows the "Edit link" UI and stores the bookmark descriptor in the datastore

Clients are listening for "added" events and reacting to this new event

https://github.com/crdlc/gaia/blob/bug-988172/apps/homescreen/js/bookmark.js#L44

What does descriptor mean?

* This is an agnostic descriptor without Blobs, etc.. Homescreens will decide what and how to show them

1.2) Edition (bug 988174)

It will publish an activity named "update-bookmark" (like homescreen had in the v1.4).

This inline activity shows the "Update link" UI and updates the bookmark descriptor in the datastore

Clients are listening for "updated" events and reacting to this new event

1.3) Deleting (bug 988177)

It will publish an activity named "delete-bookmark" (like homescreen had in the v1.4).

This inline activity shows the "Delete link" UI and removes the bookmark descriptor in the datastore

Clients are listening for "deleted" events and reacting to this new event

----

I saw the "places" datastore code but when I read it I didn't see anything related to bookmarks and I thought that it stores history and frequencies (nothing related to bookmarks). IMHO I think that bookmarks management is huge and it should live in a separated app that will publish all inline activities that manages bookmarks (addition, deletion and update). Maybe in the future we will have to populate with bookmarks in build time and again, I think that it should live in this app instead of adding more and more functionality to system app. But this is just my personal opinion and I don't know if there is any problem because of having the datastore in "bookmarks" app.

Currently I am focus on this new feature and I think that I could have it landed and working in three weeks, ready to be used for the new vertical homescreen and, of course, aligned with current homescreen as you can see in my pull request.

Thanks in advance
(In reply to Dale Harvey (:daleharvey) from comment #6)
> This definitely needs coordinated with the places work (which is the same
> work), as gregor mentioned browser bookmarks are going to be replaced by
> homescreen bookmarks.
> 

Homescreen bookmarks needs to go away as it prevents third party homescreen. If it is just a terminology name, let's call then 'system bookmarks'.

> Currently we have the places datastore in the system app which currently
> only stores history, bookmarks will either need to go within the same store
> or ast least closely integrated, they have an effect on frecency and will
> likely need to share services like how we handle icons (which is non trivial)
> 

I'm curious to understand how you handle icons and what can not be a shared library that helps other apps to do the same ?

> I am gonna look around for previous discussion to link, the best start is
> probably the spec though -
> https://mozilla.app.box.com/s/0gm5nqtm6inkbjx6qfn8, also a bunch of
> discussion about it on https://etherpad.mozilla.org/RB-Eng-Review
> 
> It would be helpful to know your plans / background for this change, we are
> gonna have a meeting about this stuff on tuesday it might help us get the
> changes alighned
> 


Creating a dedicated datastore for bookmarks seems fine to me. It basically split responsibilities. People expect a lot from bookmarks and having less things to do on top of Places should help maintenance imho. Less things in the system app is always good to, and it may lead to the third party 'bookmarking' app.

In Firefox Places is a big name for multiples databases. This separate app can be considered as the moz_bookmarks and moz_bookmarks_root database (as there is a something similar with Collections). So in the future I can also see this app managing Collections.

That's already a lot of responsibilites while I see the Places work you are doing more like the moz_places and moz_historyvisits part of Places. 

In the future if we see some limitations about this approach we can fallback on moving it back to the system app but that sounds more a limitation.

I can see your Places work accessing the bookmarks datastore, and using it to tag particular entries. The work you are doing is also responsible of frecency as it lives in the system scope and can be informed when something is opened or not.

And because entries are tagged in Places you always know if an url is a bookmark or not and don't need to spend all your time reading the bookmarks_datastore. The only thing is that it's not the Places part of the system app that is directly responsible of managing the bookmarks database. But the system app can decide to add/remove a bookmark from the remote datastore if needed.

Basically screen 3 of the page 41 and the page 42 of the spec belongs to this app, while the 'star' of of screen 3 of page 43 is managed by the system app in order to add/remove a bookmark from the remote datastore and screen 4 belongs to the bookmarks app.

The star button may also offers a hook for some third party apps. If you try Manana from the market place and try to bookmark a web site you will see what I mean.

Hope it helps to answer some of the questions.

> As an aside, when vivien does give feedback, we would very much like to
> avoid these stores being owned by the system app, however being that it has
> to store history, thats the only place we can currently have it

We can probably imagine a kind of system messages that wake up a separate app when someone navigates somewhere. But that may create a unnecessary amount of work for now.

Why do you want the history part of the database to live somewhere else ? Is it related to memory ?
Comment on attachment 8397118 [details]
Patch v1

The approach sounds good to me. It's nice that the BookmarksDatabase helper lives into shared/ so the system app and Settings may be able to access it.

Dale/Ben/Gregor please let me know if after my explanation this is still an issue. I could have misunderstood the schema you are looking for the Places database in the system app, or I could have missed some parts of the spec that prevent this makes this approach unreliable.

But in any cases we need to move Bookmarks management (and Collections in the future) out of the homescreen as it prevent replaceable homescreen to work properly.
Attachment #8397118 - Flags: feedback?(21) → feedback+
Comment on attachment 8397118 [details]
Patch v1

All code ready to review
Attachment #8397118 - Flags: review?(21)
Comment on attachment 8397118 [details]
Patch v1

While I'm normally the homescreen reviewer, I think it would be better if one of the Gaia Places folk review this patch.

That would ensure that this can fit nicely into their plan, and also that they know exactly what's going on.
Attachment #8397118 - Flags: review?(21) → review?(dale)
ok, your feedback is there as well. OK for me
So that makes sense, in reality it isnt the System Places that need bookmark information, its the search app / rocketbar frontend which needs to build a fast index and it already deals with multiple sources, it add some complexity to for things to be seperate but its liveable.

Before reviewing properly, need to know the plan for migrating bookmarks? this is the main reason we hadnt already done a similiar bookmark migration before now
(In reply to Dale Harvey (:daleharvey) from comment #13)
> So that makes sense, in reality it isnt the System Places that need bookmark
> information, its the search app / rocketbar frontend which needs to build a
> fast index and it already deals with multiple sources, it add some
> complexity to for things to be seperate but its liveable.
> 
> Before reviewing properly, need to know the plan for migrating bookmarks?
> this is the main reason we hadnt already done a similiar bookmark migration
> before now

Hi Dale,

   How's it going? I hope very good. Quick question, do you know who decide/know the plan what you are talking about? I would like to advance here because after this migration I have to move the update/remove UIs to this new application.

Thanks a lot
I forgot to add the ni: sorry Dale
Flags: needinfo?(dale)
Blocks: 988174, 988177
(In reply to Dale Harvey (:daleharvey) from comment #13)
> So that makes sense, in reality it isnt the System Places that need bookmark
> information, its the search app / rocketbar frontend which needs to build a
> fast index and it already deals with multiple sources, it add some
> complexity to for things to be seperate but its liveable.
> 
Cool.

> Before reviewing properly, need to know the plan for migrating bookmarks?
> this is the main reason we hadnt already done a similiar bookmark migration
> before now

I was more or less thinking of a simple and dumb migration plan. The first time the homescreen will see the datastore it can simply copy it's local set of bookmarks to the datastore.

Maybe I'm missing something obvious that will prevent this plan to work ?
(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #16)
> (In reply to Dale Harvey (:daleharvey) from comment #13)
> > So that makes sense, in reality it isnt the System Places that need bookmark
> > information, its the search app / rocketbar frontend which needs to build a
> > fast index and it already deals with multiple sources, it add some
> > complexity to for things to be seperate but its liveable.
> > 
> Cool.
> 
> > Before reviewing properly, need to know the plan for migrating bookmarks?
> > this is the main reason we hadnt already done a similiar bookmark migration
> > before now
> 
> I was more or less thinking of a simple and dumb migration plan. The first
> time the homescreen will see the datastore it can simply copy it's local set
> of bookmarks to the datastore.
> 
> Maybe I'm missing something obvious that will prevent this plan to work ?

Also the homescreen, like the search app, will keep its own copy of bookmarks for fast display. So once the homescreen bookmarks has been copied into the datastore, it will start to use the datastore as a source of new bookmarks, and compare it's local copy versus the one from the datastore in order to new if he needs to update its own local database.
Completely agree with Vivien. Also I have almost implemented the update of bookmarks in the second bug 988174 and with that patch the homescreen is thiner XD
Comment on attachment 8397118 [details]
Patch v1

Putting off the review until solving this bug 990542
Attachment #8397118 - Flags: review?(dale)
(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #16)
> (In reply to Dale Harvey (:daleharvey) from comment #13)
> 
> > Before reviewing properly, need to know the plan for migrating bookmarks?
> > this is the main reason we hadnt already done a similiar bookmark migration
> > before now
> 
> I was more or less thinking of a simple and dumb migration plan. The first
> time the homescreen will see the datastore it can simply copy it's local set
> of bookmarks to the datastore.
> 
> Maybe I'm missing something obvious that will prevent this plan to work ?

I think the question was more about migrating existing bookmarks from the existing browser app. That's the hard part and I guess we will fix it in bug 990542?

One other concern I have is the fact that we are adding a new process just for bookmarks. We should make sure that works fine on devices like tarako. Maybe we need a preference where we can join the bookmark app with another app?
(In reply to Gregor Wagner [:gwagner] from comment #20)
> (In reply to Vivien Nicolas (:vingtetun) (:21) from comment #16)
> > (In reply to Dale Harvey (:daleharvey) from comment #13)
> > 
> > > Before reviewing properly, need to know the plan for migrating bookmarks?
> > > this is the main reason we hadnt already done a similiar bookmark migration
> > > before now
> > 
> > I was more or less thinking of a simple and dumb migration plan. The first
> > time the homescreen will see the datastore it can simply copy it's local set
> > of bookmarks to the datastore.
> > 
> > Maybe I'm missing something obvious that will prevent this plan to work ?
> 
> I think the question was more about migrating existing bookmarks from the
> existing browser app. That's the hard part and I guess we will fix it in bug
> 990542?
> 

Ah. Then why does the browser app can not do the same migration mechanism than the one described earlier ? Is there anything else that complicated this process ?

Again I may miss some informations and misunderstand things :)

> One other concern I have is the fact that we are adding a new process just
> for bookmarks. We should make sure that works fine on devices like tarako.
> Maybe we need a preference where we can join the bookmark app with another
> app?

I think it's OK for Tarako since the app is really just intended to show some UI in order to save/edit the bookmarks. Also let's say you are browsing the web (which is usually an expensive task when it comes to memory) and you want to bookmarks a particular web sites on your homescreen. As of today you need to have the Homescreen running, which is usually an expensive process. 

If the homescreen is killed you will end up starting a new process, with the save-bookmark UI which is the more or less the same thing than opening this bookmarks app.
I am working right now in bug 990542 that will implemented in this same patch to avoid migration problems in the Gaia's community 

How to migrate current bookmarks in the homescreen to datastore for people updating their devices from 1.4 to 2.0?

The first time that users turn on their device:

1) The homescreen will render as usual painting its apps/bookmarks
2) It will check the revisonId stored in the homescreen. At this point, the revisionId will be undefined
3) As the revisionId is undefined, it will add its bookmarks to the datastore and update the revisionId in the home

Rest of cases:

1) The homescreen will render its apps/bookmarks
2.1) If the homescreen's revisionId is equal to system's revisionId, nothing to do, local bookmarks are already updated and synchronized
2.2) If the homescreen's revisionId is different than system's revisionId (they were added/modified/deleted from another homescreen) it will update them and the revisionId flag in the home
Gregor,

   I don't know if I misunderstand things but IMHO another task will be migrate favorite sites from browser to bookmark's datastore. It should be another bug that will be implemented once I finishes with these set of three bugs

   Are there any problem/issue what I am not aware of?

   My concern here is another one very different, to be faster in the reviews when it will be ready because we have a lot of work to do here and we need to do it asap in order to discover any future problem

Thanks a lot in advance

(In reply to Gregor Wagner [:gwagner] from comment #20)
> (In reply to Vivien Nicolas (:vingtetun) (:21) from comment #16)
> > (In reply to Dale Harvey (:daleharvey) from comment #13)
> > 
> > > Before reviewing properly, need to know the plan for migrating bookmarks?
> > > this is the main reason we hadnt already done a similiar bookmark migration
> > > before now
> > 
> > I was more or less thinking of a simple and dumb migration plan. The first
> > time the homescreen will see the datastore it can simply copy it's local set
> > of bookmarks to the datastore.
> > 
> > Maybe I'm missing something obvious that will prevent this plan to work ?
> 
> I think the question was more about migrating existing bookmarks from the
> existing browser app. That's the hard part and I guess we will fix it in bug
> 990542?
> 
> One other concern I have is the fact that we are adding a new process just
> for bookmarks. We should make sure that works fine on devices like tarako.
> Maybe we need a preference where we can join the bookmark app with another
> app?
Flags: needinfo?(anygregor)
Gregor, I thought a bit about the particular case of the browser app. In order to migrate the bookmarks we should be able to do the following:

 - Update the browser code so the first time bookmarks datastore is seen, the browser bookmarks are added to the store
 - Update the FTU for 1.4 -> 2.0 migration so it display a screen explaining what happens with the bookmarks of the browser to the user. In the background, the FTU can open the browser app as a <iframe mozbrowser mozapp> so the browser code can runs and update the bookmarks.

 - If we decide to get rid of the browser app for 2.0, the FTU will uninstall the browser app from the mozapps database.

 - If we keep the browser app, then nothing else to do.
I am waiting for Travis, after getting a green, we will have the pull request ready to be reviewed. I added the migration of bookmarks already installed in the current homescreen to datastore
Comment on attachment 8397118 [details]
Patch v1

Thanks Dale
Attachment #8397118 - Flags: review?(dale)
Duplicate of this bug: 990542
Ok, I was actually unaware datastores could have multiple writers, that brings up its own issues but they dont apply here and make life easier, thats great. The migration plan sounds good given that however hugely worried given a glance over that we arent testing migrations at all, that is all mocked code, we dont have the ability to unit test datastore at the moment afaik and no reasonable way to test true version migrations.

I will make sure to review this asap, its my top priority right now
Flags: needinfo?(dale)
I have given it a test run and look through the code, mentioned a few spelling nits, sorry, just figured I should. It looks good, and it worked for the example use cases I had.

Since its a fairly huge commit I want to have time tomorrow to go more in depth, its the first thing I will do in the morning, so not delaying.

I am worried about how much mocked code we are using for the tests, migrations are notoriously tricky and without testing actual old code vs new code its stupidly easy to add regressions, but I think thats way out of scope for what we can fix now

I am kinda against code like |callbacks && dostuff| its smart but just early returning is clearer, but wouldnt take a stance against it.

Cheers, will get back further in the morning
Glad to hear that. I am addressing all comments on Github. Thanks a lot
Whiteboard: [systemsfe]
Target Milestone: --- → 1.4 S5 (11apr)
All comments were addressed
still testing, one issue, how are you testing this manually? if I do an old build, add bookmarks, then install-gaia over it we dont register permissions properly and the homescreen cant write to the datastore
Flags: needinfo?(crdlc)
You have can do (at least I don't know more options as developer):

1) Go to my branch or your own branch with my patch
2) Override with the homescreen's master but copy the datastore section to master homescreen's manifest
3) |make reset-gaia| 
4) Install a bookmark with homescreen's activity
5) Restore the homescreen of my patch
6) |make install-gaia|
7) When the homescreen starts, you should see the bookmark already installed and also if you try to install again the same bookmark (same url) with bookmark's activity, you should can read "Bookmark updated" instead of "Added to Home Screen"

-------------------

See the suite of unit tests which deals with this behavior

https://github.com/mozilla-b2g/gaia/pull/17642/files#diff-0ebc16573d2554f71ace7d46c403ca88R112
Flags: needinfo?(crdlc)
I am very happy with your review because it seems that we have the same opinion thought, I mean, the first of all is to test the patch solves the issue before reviewing the code. I am very pragmatic and the most important for me is what the patch works fine :)
Comment on attachment 8397118 [details]
Patch v1

This looks great, I tested it with a bunch of different scenarios of homescreen bookmarks, havent been able to find an issue, sorry for the delay, https://bugzilla.mozilla.org/show_bug.cgi?id=992466 ended up blocking me from reviewing. 

I think for a patch this size it would be sensible to do a tbpl run before landing (I wont land right now as its late and I cant watch the tree)

The lack of real migration tests (that generate an old profile) worries me, however that is a fairly huge task so I dont think it can block this
Attachment #8397118 - Flags: review?(dale) → review+
Thanks a lot for the review, I appreciate it a lot. I have pushed again my branch rebased from master in order to check Travis again before landing (just in case some test changed last weekend)

After this, I gonna continue working very hard to be ready the bug 988174 asap (maybe a couple of days)
Merged in master:

https://github.com/mozilla-b2g/gaia/commit/3796021a52948a2997d9e449ab2c83279d3e1074
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
(In reply to Vivien Nicolas (:vingtetun) (:21) (NOT reading bugmails, needinfo? please) from comment #24)
> Gregor, I thought a bit about the particular case of the browser app. In
> order to migrate the bookmarks we should be able to do the following:
> 
>  - Update the browser code so the first time bookmarks datastore is seen,
> the browser bookmarks are added to the store
>  - Update the FTU for 1.4 -> 2.0 migration so it display a screen explaining
> what happens with the bookmarks of the browser to the user. In the
> background, the FTU can open the browser app as a <iframe mozbrowser mozapp>
> so the browser code can runs and update the bookmarks.
> 
>  - If we decide to get rid of the browser app for 2.0, the FTU will
> uninstall the browser app from the mozapps database.
> 
>  - If we keep the browser app, then nothing else to do.

Yep that works for our next release but I am not sure if this covers the case where we upgrade from 1.4 to 2.1. There we need to remove the browser app in the upgrade step and even before we have to export the bookmarks to the new datastore.
Flags: needinfo?(anygregor)
You need to log in before you can comment on or make changes to this bug.