1042116 - Add bookmarks provider

Status: RESOLVED FIXED

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

Description

[Dale Harvey (:daleharvey)]

The spec for this is @ https://mozilla.app.box.com/s/lbw2wzw3p4jvxs24k4sg/1/2099951272/19056548695/1

Page 17/19

Comment 1

[Dale Harvey (:daleharvey)]

Just to comment on the implementation, we need to sync both places and bookmarks into a local database for fast access, currently places syncs the entire history on app startup which cannot land. I havent implemented persistence of that data because of possible changing requirements.

I think we should implement bookmark syncing in a similiar way, sync the bookmarks into memory, but at the same time try to create an in memory localResults module with an API that works for bookmarks and hopefully for places as well. Then we should be able to add persistence to that localResults module and get stop the in memory syncing, that can be 3 bugs (1, implement bookmarks 2, create common api for storing and accessing results from places + bookmarks 3, make that api persist the data and not resync into memory)

Comment 2

[Francisco Jordano [:arcturus] [:francisco]]

Will happily take this one.

And folow Date advice, so this bug will be just for implementing the bookmarks search provider.

Comment 3

[Francisco Jordano [:arcturus] [:francisco]]

Attachment: Pointer to pr 22152

Hi folks,

sorry for the delay, found a problem when having the two providers working at the same time (sharing the same results array).

Now fixed and unit tests up and running.

Thanks for your feedback!

Comment 4

[Kevin Grandon :kgrandon]

Comment on attachment 8462442 [details] [review]
Pointer to pr 22152

Looks like we crossed patches and there's a few conflicts. Could you rebase and address my comments? I will also take another look and would like Dale to take a look. (Dale feel free to leave an R? if you'd like). Thanks!

Comment 5

[Kevin Grandon :kgrandon]

Comment on attachment 8462442 [details] [review]
Pointer to pr 22152

Hi Francisco,

Great work. I would like to see what Dale has to say.

I have one main question right now - what is the benefit of making a provider to inherit from, instead of making a utility? Sorry for not bringing this up earlier, but thinking about it a bit more I'm not sure if this is the right pattern to follow.

Generally I think providers make sense when there is certain logic you need to override in a base class that does not exist in the parent. I'm just not sure how I feel about having another layer of classes in between Provider and Bookmark/Place providers. I think I would prefer to use a utility class, but I could still be convinced otherwise. Here is something along the lines of what I was thinking. Let me know what you think.

function Bookmarks() {
  this.store = new SyncDatastore(STORE_NAME);
  this.store.sync().then(this.populate.bind(this));
}

Bookmarks.prototype = {
 __proto__: Provider.prototype,
 populate: function(results) {
 }
 // ...
};

Comment 6

[Francisco Jordano [:arcturus] [:francisco]]

Hi Kevin,

thanks a lot for taking a look to this and rise your questions.

At the beginning I oriented this more like a generic datastore provider. So if in the future we add anything else based on DS (sms, contacts, etc.) it could be reused.

But after syncing with Dale we decided to create something closer to Places, as Bookmarks could be consider a generalisation of Places.

The whole idea behind this was reuse the logic for syncing the data from the datastore, and reuse this work in future bugs like persisting the data and so on.

If you guys consider that is better not to add this extra level of complexity, let's move to what you propose, so will wait for Dale here, and continue with what is decided.

Thanks again!

Comment 7

[Dale Harvey (:daleharvey)]

Sorry didnt get to this over the weekend, but top of my list in the morning.

In general I avoid inheritance over more functional approaches, but I think considering how places is setup this seems like a good way to do it, will do a proper review in the morning.

Comment 8

[Dale Harvey (:daleharvey)]

Comment on attachment 8462442 [details] [review]
Pointer to pr 22152

As mentioned I generally always go for a more functional approach, but I dont think this adds much complexity, the contacts / messages etc will likely have very similiar properties (syncing from ds, persisting data), I think this abstracts fairly cleanly and the bookmarks + places implementation are very simple following this refactor, it looks great.

If the complexity goes up when we start adding contacts etc can look at splitting some of the functionality into more isolated functions, I have left one nit to remove itemClicked if we can, that behaviour should be baked into the gaia grid component.

But looking awesome, thanks :)

Comment 9

[Dale Harvey (:daleharvey)]

(also only land on green obvs :))

Comment 10

[Francisco Jordano [:arcturus] [:francisco]]

Thanks Dale,

Kevin just waiting for your last thoughs here, if you still prefer going for a dependency injection here will move the code to the approach you commented.

Thanks!
F.

Comment 11

[Kevin Grandon :kgrandon]

Currently the inheritance is starting to look like this:
Bookmarks -> SyncProvider -> DataGridProvider -> Provider

I think we need to rely much less on inheritance for features, and essentially get to a point where we can very quickly script functionality from within the provider. The problem is that scope starts to creep in and we quickly get into a place where the code is extremely tangled. I strongly suggest that we refactor this out into a utility that both bookmarks and places can utilize.

Francisco - I think it would be really good to compare this approach to something like the following. Would you be open to exploring this approach in another pull request so we can compare the approaches? I know this is some extra work, but I think it would be really valuable to get the high-level view of both approaches so we will have the best solution going forward.

- Bookmarks extends DataGridProvider extends Provider
- Places extends DataGridProvider extends Provider
- Both bookmarks and places leverage some sync utility class to populate data.

Comment 12

[Francisco Jordano [:arcturus] [:francisco]]

Hei Kevin,

Happy to do that, comparisson and meassurements are the way to go :)

Will try to provide it ASAP.

Comment 13

[Francisco Jordano [:arcturus] [:francisco]]

Attachment: Branch with functional approach

Hei folks,

I'm attaching a branch following the functional approach we were commenting before.

Didn't add unit tests yet, just to verify I'm in the same page than you and will add later.

Ended up adding to additional methods for making it a bit (but just a bit I promise ;)) more generic.

If you ok with this, I'll continue tomorrow with the unit test and documentation of the file.

Thanks!!

Comment 14

[Kevin Grandon :kgrandon]

Comment on attachment 8463620 [details]
Branch with functional approach

I like this a lot, thank you for taking the time to implement it.

My preference is to write some tests and land this one. I think over time it will be more maintainable than the sync provider. If you guys do both end up preferring the other one, I could potentially be convinced as well though as you have experience implementing both.

Comment 15

[Dale Harvey (:daleharvey)]

Comment on attachment 8463620 [details]
Branch with functional approach

So good call on this, I agree now, when implementing https://bugzilla.mozilla.org/show_bug.cgi?id=1041620 I realised I will likely need a module that syncs the data without providing it as a grid compontent and it makes much more sense to this functionality to be decoupled.

Comment 16

[Francisco Jordano [:arcturus] [:francisco]]

Ok folks, will proceed to refine the code, with comments and do the unit tests :)

Thanks for the feedback!

Comment 17

[Francisco Jordano [:arcturus] [:francisco]]

Attachment: Pointer to pr 22260

Hi folks,

there it goes, final version with tests and avoiding the sync parameter.

Comment 18

[Dale Harvey (:daleharvey)]

Comment on attachment 8463997 [details] [review]
Pointer to pr 22260

The code looks good, however I dont think we can maintain the syncStore.results API

The next step is for bookmarks / places to persist the results, we arent going to be able to keep the entire history in memory and we wont be doing syncs from scratch so I think this should just expose an onChange API, where bookmarks / places maintain their own 'results', we follow up so it stores its lastRevision.

I will also be using this as in the topSites functionality where we have a different persistence mechanism, so I dont think we want to tie persistence into this directly, it should just manage lastRevision and the boilerplate code for datastore.


Sorry for the churn on this patch

Comment 19

[Francisco Jordano [:arcturus] [:francisco]]

Comment on attachment 8463997 [details] [review]
Pointer to pr 22260

Hei Dale,

second version now separating where do we proper store the data into something that will be customisable in the future :)

Thanks for the feedback!

Comment 20

[Kevin Grandon :kgrandon]

Comment on attachment 8463997 [details] [review]
Pointer to pr 22260

I am happy with the direction and am basically an R+ here. I would like to leave the final R+ up to dale though as he's left some comments and is looking at the newtab page which might leverage this. I can always re-visit if he doesn't get to it in the next day or so. Thanks for the work on this.

Comment 21

[Dale Harvey (:daleharvey)]

Comment on attachment 8463997 [details] [review]
Pointer to pr 22260

This looks awesome, I think we need to wait for Gij to go green then LGTM :)

Comment 22

[Dale Harvey (:daleharvey)]

landed in: https://github.com/mozilla-b2g/gaia/commit/fe12967fcb0f09f50f36bd2d33c3bc5f7db87ef6