Closed Bug 1148466 Opened 5 years ago Closed 5 years ago

Use new keywords API in BookmarkHTMLUtils and BookmarkJSONUtils

Categories

(Toolkit :: Places, defect)

defect
Not set
Points:
3

Tracking

()

RESOLVED FIXED
mozilla40
Iteration:
40.2 - 27 Apr
Tracking Status
firefox39 --- fixed
firefox40 --- fixed

People

(Reporter: mak, Assigned: mak)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

It's possible some of this will already happen with the bookmarks conversion, but still better to track it.
Blocks: 1147891
we need this to properly support backup/restore of POST data for keywords, otherwise we are trying to set/read annos that are no more used.
We likely need to uplift the fix to Aurora.
see also http://mxr.mozilla.org/mozilla-central/ident?i=POST_DATA_ANNO
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Points: 2 → 3
Flags: in-testsuite?
Iteration: --- → 40.1 - 13 Apr
Flags: qe-verify?
Flags: firefox-backlog+
Depends on: 1149488
Changing bug dependencies based on Bug 1095426 comment 6
Blocks: 1095426, 1095427
No longer depends on: 1095426, 1095427
Attached patch patch v1Splinter Review
I'm sorry for the huge tests changes, they were terrible, this makes them nicer, even if not yet as good as I'd want them.
Btw, what matters is the code changes
Attachment #8590827 - Flags: review?(ttaubert)
Blocks: 1148467
No longer blocks: 1148467
Duplicate of this bug: 1148467
note we should uplift this.
Iteration: 40.1 - 13 Apr → 40.2 - 27 Apr
Comment on attachment 8590827 [details] [diff] [review]
patch v1

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

::: toolkit/components/places/BookmarkJSONUtils.jsm
@@ +190,5 @@
>     */
> +  importFromURL(spec) {
> +    return new Promise((resolve, reject) => {
> +      let streamObserver = {
> +        onStreamComplete: (aLoader, aContext, aStatus, aLength, aResult) => {

Too bad nsIStreamLoaderObserver doesn't have the function annotation.

::: toolkit/components/places/PlacesUtils.jsm
@@ +1811,5 @@
>        }
>        return exclude;
>      };
>  
>      let rootItem = null, rootItemCreationEx = null;

Can remove |rootItemCreationEx|.

::: toolkit/components/places/tests/unit/test_bookmarks_html_corrupt.js
@@ +79,5 @@
>  
> +  let entry = yield PlacesUtils.keywords.fetch({ url: bookmarkNode.uri });
> +  Assert.equal("test", entry.keyword);
> +  Assert.equal("hidden1%3Dbar&text1%3D%25s", entry.postData);
> +  

Nit: stray white space
Attachment #8590827 - Flags: review?(ttaubert) → review+
Flags: qe-verify? → qe-verify-
Flags: in-testsuite? → in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/437533dd34b9
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Attached patch coalesced patchSplinter Review
this includes the dependency bug (that is a low risk method addition used only by the patch in this bug)

Approval Request Comment
[Feature/regressing bug #]: new bookmarks keywords API
[User impact if declined]: POST data of keywords is lost on restoring a backup or importing a bookmarks.html file
[Describe test coverage new/current, TreeHerder]: unit test
[Risks and why]: contained changes, has unit tests
[String/UUID change made/needed]: none
Attachment #8593950 - Flags: approval-mozilla-aurora?
Comment on attachment 8593950 [details] [diff] [review]
coalesced patch

Approving for uplift to aurora since it looks like we have good test coverage of this fix, and can possibly avoid people losing their keyword data on bookmarks.
Attachment #8593950 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Blocks: 993314
You need to log in before you can comment on or make changes to this bug.