Closed Bug 1308515 Opened 8 years ago Closed 8 years ago

Update version of Kinto.js client to 5.0.0

Categories

(WebExtensions :: Untriaged, defect)

Product:
WebExtensions
Component:
Untriaged
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox52 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox52 --- fixed

People

(Reporter: glasserc, Assigned: glasserc)

Details

(Whiteboard: triaged)

Attachments

(2 files)

Bug 1308515: Update version of kinto.js client to 5.0.0,
58 bytes, text/x-review-board-request
MattN
: review+
Details
Bug 1308515: Fix blocklist-clients.js so that it can work with new kinto.js,
58 bytes, text/x-review-board-request
leplatrem
: review+
MattN
: review+
Details 
This version of Kinto.js has a `resetSyncStorage` function in FirefoxStorage, which I will need in my chrome.storage.sync work (bug 1253740).
Hmm, seems like this broke on some stuff. I guess I'll have to look more at this..
Whiteboard: triaged
I looked at this more today in between meetings. It seems like https://github.com/Kinto/kinto.js/pull/553#pullrequestreview-3556048 is the change that broke blocklist_signatures. It seems like a member variable on `Collection` used to be populated and is now no longer being populated at all. My question is really: do we even need this member variable? Why are we populating it at all? Why are we populating it at the beginning of sync?

:mgoodwin -- do you think it would be OK for me to change blocklist-clients.js to refer to `yield collection.db.getLastModified()` instead of `collection.lastModified`? You already use this in one place so I'm hoping I can use it here too.

:leplatrem -- do you think it would make sense to get rid of `lastModified` as a property of `Collection`? Alternately, would it make sense to load it during/after construction?
Flags: needinfo?(mgoodwin)
Flags: needinfo?(mathieu)
Attachment #8798880 - Flags: review?(MattN+bmo)
Clearing review until the test failure is sorted out
(In reply to Ethan Glasser-Camp (:glasserc) from comment #4)
> I looked at this more today in between meetings. It seems like
> https://github.com/Kinto/kinto.js/pull/553#pullrequestreview-3556048 is the
> change that broke blocklist_signatures. It seems like a member variable on
> `Collection` used to be populated and is now no longer being populated at
> all. My question is really: do we even need this member variable? Why are we
> populating it at all? Why are we populating it at the beginning of sync?
> 
> :mgoodwin -- do you think it would be OK for me to change
> blocklist-clients.js to refer to `yield collection.db.getLastModified()`
> instead of `collection.lastModified`? You already use this in one place so
> I'm hoping I can use it here too.

My recollection is that there was some difference between these. I don't recall exactly what. Maybe leplatrem remembers?
Flags: needinfo?(mgoodwin)
>  It seems like a member variable on `Collection` used to be populated and is now no longer being populated at all

The previous implementation in `kinto.js` was not good, it was using this attribute to store some intermediate state during the sync process. 

Now the value should be set at the end of a successful sync process only. 

In our case, here:
https://dxr.mozilla.org/mozilla-central/rev/7452437b3ab571b1d60aed4e973d82a1471f72b2/services/common/blocklist-clients.js#205
we were relying on a value that was not consistently set, because it was assigned in the middle of failing sync process.

Hence, here we want `collection.db.getLastModified()` which is the timestamp of the last successful sync effectively stored in the local DB.
Flags: needinfo?(mathieu)
Comment on attachment 8801362 [details]
Bug 1308515: Fix blocklist-clients.js so that it can work with new kinto.js,

https://reviewboard.mozilla.org/r/86134/#review85032

::: services/common/blocklist-clients.js:206
(Diff revision 1)
>              yield this.validateCollectionSignature(payload, collection, true);
>              // if the signature is good (we haven't thrown), and the remote
>              // last_modified is newer than the local last_modified, replace the
>              // local data
> -            if (payload.last_modified >= collection.lastModified) {
> +            const newLastModified = yield collection.db.getLastModified();
> +            if (payload.last_modified >= newLastModified) {

µnit: instead `newLastModified` I would have put `localLastModified` to match the comment
Oops, looks like I failed to post the comment I wanted to post the first time around. From :leplatrem's comments it sounds like I always want to get the last-modified date from the DB, so I've taken the liberty of making that change as a standalone commit and putting it before the update of the kinto.js library.

The first try builds failed for reasons that don't seem related to what I was working on, so I've rebased everything onto the most up-to-date version of the code and repushed to try.
Comment on attachment 8798880 [details]
Bug 1308515: Update version of kinto.js client to 5.0.0,

https://reviewboard.mozilla.org/r/84266/#review85274
Attachment #8798880 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8801362 [details]
Bug 1308515: Fix blocklist-clients.js so that it can work with new kinto.js,

https://reviewboard.mozilla.org/r/86134/#review85448
Attachment #8801362 - Flags: review?(mathieu) → review+
Keywords: checkin-needed
MozReview says that the first patch doesn't have sufficient reviews to be autolanded. Please resolve that.
Keywords: checkin-needed
Assignee: nobody → eglassercamp
Status: NEW → ASSIGNED
Attachment #8801362 - Flags: review?(MattN+bmo)
Comment on attachment 8801362 [details]
Bug 1308515: Fix blocklist-clients.js so that it can work with new kinto.js,

https://reviewboard.mozilla.org/r/86134/#review85670

I didn't realize MozReview enforced this.
Attachment #8801362 - Flags: review?(MattN+bmo) → review+
Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/autoland/rev/2e4a60eeb1f0
Fix blocklist-clients.js so that it can work with new kinto.js, r=leplatrem,MattN
https://hg.mozilla.org/integration/autoland/rev/a33f036e3496
Update version of kinto.js client to 5.0.0, r=MattN
Oops! I forgot that :leplatrem doesn't have high enough privileges to r+ a commit by himself. Thanks :MattN for reviewing the other patch.
https://hg.mozilla.org/mozilla-central/rev/2e4a60eeb1f0
https://hg.mozilla.org/mozilla-central/rev/a33f036e3496
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: