1148466 - Use new keywords API in BookmarkHTMLUtils and BookmarkJSONUtils

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Marco Bonardo [:mak]]

It's possible some of this will already happen with the bookmarks conversion, but still better to track it.

Comment 1

[Marco Bonardo [:mak]]

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

Comment 2

[Steven MacLeod [:smacleod]]

Changing bug dependencies based on Bug 1095426 comment 6

Comment 3

[Marco Bonardo [:mak]]

Attachment: patch v1

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

Comment 5

[Marco Bonardo [:mak]]

note we should uplift this.

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/437533dd34b9

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/437533dd34b9

Comment 9

[Marco Bonardo [:mak]]

Attachment: coalesced patch

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

Comment 10

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c705cd9ab5a2