Marco Bonardo [:mak] Assignee
	Description • 9 years ago

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

	Marco Bonardo [:mak] Assignee
	Comment 1 • 9 years ago

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

	Steven MacLeod [:smacleod]
	Comment 2 • 9 years ago

Changing bug dependencies based on Bug 1095426 comment 6

	Marco Bonardo [:mak] Assignee
	Comment 3 • 9 years ago

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

	Marco Bonardo [:mak] Assignee
	Comment 5 • 9 years ago

note we should uplift this.

	Tim Taubert [:ttaubert] (inactive)
	Comment 6 • 9 years ago

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

	Pulsebot
	Comment 7 • 9 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 8 • 9 years ago

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

	Marco Bonardo [:mak] Assignee
	Comment 9 • 9 years ago

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

	Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)
	Comment 10 • 9 years ago

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.

	Ryan VanderMeulen [:RyanVM]
	Comment 11 • 9 years ago

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