Closed Bug 1208352 Opened 9 years ago Closed 6 years ago

Refactor places.js and history.js for extracting the similar logic of Places DataStore


(Firefox OS Graveyard :: Sync, defect, P1)

Gonk (Firefox OS)


(Not tracked)

2.6 S7 - 2/12


(Reporter: selee, Assigned: mbdejong)




(1 file)

At bug 1191773 comment 51, PlacesDS accessing code should be extracted to a shared code to prevent drifting out of sync from different piece codes.
In my basic idea, the extraction work for editPlace#places.js and addPlace#history.js is the first aim.
Assignee: nobody → ferjmoreno
Target Milestone: --- → FxOS-S8 (02Oct)
Target Milestone: FxOS-S8 (02Oct) → FxOS-S9 (16Oct)
Assignee: ferjmoreno → mbdejong
Target Milestone: FxOS-S9 (16Oct) → FxOS-S11 (13Nov)
Depends on: 1217340
Priority: -- → P1
Comment on attachment 8683644 [details] [review]
[gaia] michielbdejong:1208352-shared-places-code > mozilla-b2g:master

This is getting quite complex... Search, Homescreen and System all have their own funny way of accessing the places DataStore.

Also, the runtime 'verify' check is not what we want of course, we want the four apps to all use the same shared code.

Just wanted to ping you in case you have any advice on this. Should I make this a big refactor of all four apps or do you see an easier way out?
Attachment #8683644 - Flags: feedback?(ferjmoreno)
Yes, the access to this DataStore is quite spread, that's why we need this refactor :) And if there's a moment to do this kind of refactors is now, right after branching 2.5.

I believe Dale worked on this, so he is the best one to give you feedback and guidance here.
Flags: needinfo?(dale)
Attachment #8683644 - Flags: feedback?(ferjmoreno) → feedback+
Yeh this is looking mostly good to to me, I added a few notes on the PR, mostly aesthetic / minor things. and yeh the entire point was that if we have 4 apps touching that datastore, I would like them to do so in a consistent way (particularly as you put in there, the validation etc). I feel like the 'edit' logic should likely go in there as well at some point, but whats there looks like it would be a good start
Flags: needinfo?(dale)
Depends on: 1225809
Depends on: 1225812
Depends on: 1226166
Moving from apps/sync/js/adapters/history.js to shared/js/places_model.js:

addPlace (except for setDataStoreId)
Comment on attachment 8683644 [details] [review]
[gaia] michielbdejong:1208352-shared-places-code > mozilla-b2g:master

Hi Sean,

I'm waiting for the other two commits to land before requesting review, but can you maybe give me some early feedback on (third commit only)?
Attachment #8683644 - Flags: feedback?(selee)
Target Milestone: FxOS-S11 (13Nov) → 2.6 S2 - 12/4
Attachment #8683644 - Flags: feedback?(selee) → feedback+
Target Milestone: 2.6 S2 - 12/4 → 2.6 S4 - 1/1
Target Milestone: 2.6 S4 - 1/1 → 2.6 S6 - 1/29
Target Milestone: 2.6 S6 - 1/29 → 2.6 S7 - 2/12
Firefox OS is not being worked on
Closed: 6 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.