855190 - Use new async getCharsetForURI in /toolkit/components/places/tests/unit/test_bookmarks_json.js

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Raymond Lee [:raymondlee]]

Use new async PlacesUtils.getCharsetForURI in this test
/toolkit/components/places/tests/unit/test_bookmarks_json.js

Comment 1

[Raymond Lee [:raymondlee]]

Have a look at this as a reference
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/tests/unit/test_bookmarks_html.js#374

Comment 2

[Marcos Aruj]

Attachment: Use new getCharsetForURI.

Hi. Here's a patch to close this bug. Let me know if it's OK. Thanks.

Comment 3

[Marco Bonardo [:mak]]

Comment on attachment 730275 [details] [diff] [review]
Use new getCharsetForURI.

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

::: toolkit/components/places/tests/unit/test_bookmarks_json.js
@@ +232,4 @@
>      }
> +  }).then(deferred.resolve);
> +
> +  return deferred.promise;

rather than defining a deferred and returning its promise, directly
return Task.spawn(...

so basically return the Task promise.
Please also do the same here:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/tests/unit/test_bookmarks_html.js#406

Comment 4

[Marcos Aruj]

Attachment: Use new getCharsetForURI, return Task.spawn directly.

Hi Marco,

I've changed the patch so it directly returns Task.spawn instead of the deferred's promise. Let me know if it's ok. 

Cheers,

Marcos.

Comment 5

[Marco Bonardo [:mak]]

Comment on attachment 736336 [details] [diff] [review]
Use new getCharsetForURI, return Task.spawn directly.

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

it look ok!

::: toolkit/components/places/tests/unit/test_bookmarks_json.js
@@ +121,4 @@
>          break;
>        case "unfiled":
>          root =
> +            PlacesUtils.getFolderContents(PlacesUtils.unfiledBookmarksFolderId).root;

these 3 indentation changes could be avoided

Comment 6

[Raymond Lee [:raymondlee]]

(In reply to Marco Bonardo [:mak] from comment #5)
> Comment on attachment 736336 [details] [diff] [review]
> Use new getCharsetForURI, return Task.spawn directly.
> 
> Review of attachment 736336 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> it look ok!
> 
> ::: toolkit/components/places/tests/unit/test_bookmarks_json.js
> @@ +121,4 @@
> >          break;
> >        case "unfiled":
> >          root =
> > +            PlacesUtils.getFolderContents(PlacesUtils.unfiledBookmarksFolderId).root;
> 
> these 3 indentation changes could be avoided

Pushed to try and waiting for results
https://tbpl.mozilla.org/?tree=Try&rev=fc7165d459f5

Please fix the indentation changes and mark this bug for check-in if it passes try tests.

Comment 7

[Raymond Lee [:raymondlee]]

Attachment: Patch for check-in

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2e93879f1adb

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2e93879f1adb