distribution.js should use the new Bookmarks.jsm API

RESOLVED FIXED in Firefox 40

Status

()

RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: mak, Assigned: ttaubert)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 40
Points:
5
Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify -

Firefox Tracking Flags

(firefox40 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

I'm not sure whether distribution is async-friendly...
Flags: qe-verify+
Flags: firefox-backlog+
(Assignee)

Updated

4 years ago
Blocks: 1141547
(Assignee)

Comment 1

4 years ago
Created attachment 8577986 [details] [diff] [review]
0001-Bug-1094886-Make-distribution.js-use-the-new-Bookmar.patch

Now that DistributionCustomizer.applyBookmarks() is async, should nsBrowserGlue.ensurePlacesDefaultQueriesInitialized() only be called after that is finished?
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
Attachment #8577986 - Flags: review?(mak77)
(Assignee)

Updated

4 years ago
Iteration: --- → 39.2 - 23 Mar
Comment on attachment 8577986 [details] [diff] [review]
0001-Bug-1094886-Make-distribution.js-use-the-new-Bookmar.patch

Review of attachment 8577986 [details] [diff] [review]:
-----------------------------------------------------------------

Yes, I think it's worth to yield in nsBrowserGlue where we have .applyBookmarks() calls.
Luckily I just changed that code to yield sequentially in the keywords patch and looks like it's working (or at least I hope so!).

please ensure to run the tests in browser/components/places, some of them go through this code path. better to get a green Try run.

::: browser/components/distribution.js
@@ +90,4 @@
>      let keys = [];
>      for (let i in enumerate(this._ini.getKeys(section)))
>        keys.push(i);
>      keys.sort();

while here, probably this could use a comprehension like
keys = [for (i in enumerate(this._ini.getKeys(section)))].sort();

@@ +93,5 @@
>      keys.sort();
>  
>      let items = {};
>      let defaultItemId = -1;
>      let maxItemId = -1;

I know it's out of scope for this bug, but this code is so unreadable that makes my eyes bleed.

Could you please rename iid to itemIndex, defaultItemId to defaultIndex and maxItemId to maxIndex?
I really can't read this code with such terrible naming. Anything would be an improvement here.

@@ +178,1 @@
>          PlacesUtils.livemarks.addLivemark({ title: items[iid]["title"]

I'll file a bug to add parentGuid support to livemarks and deprecate passing parentId (bug 1145653)
For now this is ok.

Annotations is a different story since there ideally we want a new implementation (bug 699844)
Attachment #8577986 - Flags: review?(mak77) → feedback+

Updated

3 years ago
Iteration: 39.2 - 23 Mar → 39.3 - 30 Mar

Updated

3 years ago
Iteration: 39.3 - 30 Mar → 40.1 - 13 Apr
(Assignee)

Comment 3

3 years ago
Created attachment 8586750 [details] [diff] [review]
0001-Bug-1094886-Make-distribution.js-use-the-new-Bookmar.patch, v2

Made all the requested changes. Tests are passing.
Attachment #8577986 - Attachment is obsolete: true
Attachment #8586750 - Flags: review?(mak77)
Comment on attachment 8586750 [details] [diff] [review]
0001-Bug-1094886-Make-distribution.js-use-the-new-Bookmar.patch, v2

Review of attachment 8586750 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/distribution.js
@@ +170,5 @@
>            index = prependIndex++;
>  
>          // Don't bother updating the livemark contents on creation.
> +        let parentId = yield PlacesUtils.promiseItemId(parentGuid);
> +        PlacesUtils.livemarks.addLivemark({ title: items[itemIndex]["title"]

rather than catching I'd probably yield now that we can.

@@ +374,5 @@
>  }
>  
>  function enumToObject(UTF8Enumerator) {
>    let ret = {};
> +  for (let i of enumerate(UTF8Enumerator))

I didn't verify if this is correct for whatever UTF8Enumerator is, I assume you did.
Attachment #8586750 - Flags: review?(mak77) → review+
(Assignee)

Comment 5

3 years ago
(In reply to Marco Bonardo [::mak] from comment #4)
> >          // Don't bother updating the livemark contents on creation.
> > +        let parentId = yield PlacesUtils.promiseItemId(parentGuid);
> > +        PlacesUtils.livemarks.addLivemark({ title: items[itemIndex]["title"]
> 
> rather than catching I'd probably yield now that we can.

Good point, totally missed that.

> >  function enumToObject(UTF8Enumerator) {
> >    let ret = {};
> > +  for (let i of enumerate(UTF8Enumerator))
> 
> I didn't verify if this is correct for whatever UTF8Enumerator is, I assume
> you did.

Well I changed enumerate() to be real generator now and so I had to change enumToObject() to use for...of with enumerate(). BUT I just realized I'll have to change all the other places where we still use for...in, will do before landing. Looks like we have no real tests for those parts.
https://hg.mozilla.org/mozilla-central/rev/3305f01851d5
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
QA Contact: andrei.vaida
Tim, please provide a bit of context for this fix, so that QA can verify it.
Flags: needinfo?(ttaubert)
(Assignee)

Comment 9

3 years ago
Marco, do we really need manual verification here? We have a few tests covering most of the code. If we do, how would we do that? Would we need to customize distribution.ini and package it with an official build?
Flags: needinfo?(ttaubert) → needinfo?(mak77)
bookmarks import in ditribution has a test, considered the difficulty in doing that manually we can avoid the qe. sorry for setting the flag wrongly.
Flags: qe-verify-
Flags: qe-verify+
Flags: needinfo?(mak77)
QA Contact: andrei.vaida
You need to log in before you can comment on or make changes to this bug.