Closed Bug 1317841 Opened 6 years ago Closed 6 years ago

Update version of Kinto.js client to 6.0.0

Categories

(WebExtensions :: Untriaged, defect)

defect
Not set
normal

Tracking

(firefox53 fixed)

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: glasserc, Assigned: glasserc)

Details

Attachments

(2 files, 1 obsolete file)

kinto.js released a major version, v6.0.0, which removes FirefoxStorage from the repo. This blob of code which provides a Sqlite.jsm adapter for Kinto is really Gecko-specific so it's intended for it to migrate into the Gecko codebase. See https://github.com/Kinto/kinto.js/pull/562 for more information.

Because this release is a bit tricky, I'm hoping to get it out of the way sooner rather than later.
Assignee: nobody → eglassercamp
(In reply to Matthew N. [:MattN] from comment #3)
> Created attachment 8811092 [details]
> Bug 1317841 - Update version of kinto.js to v6.0.0,

This is an version of the patch using `hg cp --after services/common/kinto-offline-client.js services/common/kinto-storage-adapter.js` so that the history of the firefox adapter is preserved and so I can see the interdiff between the implementations. I will review this version instead. 

Ethan, can you update your patch to preserve the history? If not, I can land it for you with this change.
Flags: needinfo?(eglassercamp)
Attachment #8811083 - Flags: review?(MattN+bmo)
Comment on attachment 8811092 [details]
Bug 1317841 - Update version of kinto.js to v6.0.0,

https://reviewboard.mozilla.org/r/93308/#review93310

r=me with questions about why the style is diverging from the usual browser/toolkit style and also with a preference to have an `hg copy`.

::: services/common/kinto-storage-adapter.js:193
(Diff revision 1)
> -  constructor(collection, options = {}) {
> +  constructor(collection, options={}) {
>      super();
> -    const { sqliteHandle = null } = options;
> +    const {sqliteHandle=null} = options;

Why the change in style? Mozilla-central style guide is to have spaces around operators.

::: services/common/kinto-storage-adapter.js:223
(Diff revision 1)
>  
> -  _executeStatement(statement, params) {
> +  _executeStatement(statement, params){
>      if (!this._connection) {
>        throw new Error("The storage adapter is not open");
>      }
>      return this._connection.executeCached(statement, params);
>    }
>  
> +
>    open() {
>      const self = this;
> -    return Task.spawn(function* () {
> +    return Task.spawn(function* (){

Ditto for the lack of space before `{`.

::: services/common/kinto-storage-adapter.js:270
(Diff revision 1)
> +        ...options.preload
> +      ];

Nit: Add a trailing comma here? That's the usual style in m-c (to prevent the change in blame when another item is added to the array) and I think I saw it was used elsewhere in this file.
Attachment #8811092 - Flags: review?(MattN+bmo) → review+
I believe I've updated my patch to maintain the history using `hg cp --after`. (I've also rebased it.)

The first `try` build was one that I launched using `--artifact`, just as an experiment. It seems to have broken in a lot of ways. I tried again just to see, but it looks like that's going to break too, so I'm going to launch another one without `--artifact`.

The stylistic changes were because the original FirefoxStorage.js (which I copied) was a source file, and what we had in kinto-offline-client.js was the browserify'd output. Stylistic stuff got cleaned up automatically as part of the browserify step. I've tried to maintain the style (of the browserify'd output, which seems to match m-c house style) in the first commit, and then did some explicit cleanups as part of a second one. If there are other stylistic things you'd like me to clean up while I'm here, I'd be happy to do that. It doesn't seem like there's an eslint file that applies to this directory, since everything is eslint clean no matter what I do.
Flags: needinfo?(eglassercamp)
I think the failed tests are unrelated intermittents: browser/base/content/test/general/browser_save_private_link_perwindowpb.js, devtools/client/debugger/test/mochitest/browser_dbg_pretty-print-04.js, browser/components/originattributes/test/browser/browser_cache.js, browser/components/sessionstore/test/browser_windowStateContainer.js, browser/base/content/test/newtab/browser_newtab_bug722273.js, and devtools/client/netmonitor/test/browser_net_statistics-01.js.
Flags: needinfo?(MattN+bmo)
Attachment #8811092 - Attachment is obsolete: true
Comment on attachment 8811328 [details]
Bug 1317841 - Stylistic pass on new kinto-storage-adapter,

https://reviewboard.mozilla.org/r/93482/#review94432

Thanks
Attachment #8811328 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8811083 [details]
Bug 1317841 - Update version of kinto.js to v6.0.0,

https://reviewboard.mozilla.org/r/93300/#review94434
Attachment #8811083 - Flags: review?(MattN+bmo) → review+
LGTM, the one leak failure doesn't seem like an exact intermittent failure bug match but it's unlikely related to this.
Flags: needinfo?(MattN+bmo)
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/ea5ef470629e
Update version of kinto.js to v6.0.0, r=MattN
https://hg.mozilla.org/integration/autoland/rev/cb29d98401b6
Stylistic pass on new kinto-storage-adapter, r=MattN
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ea5ef470629e
https://hg.mozilla.org/mozilla-central/rev/cb29d98401b6
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.