Last Comment Bug 855190 - Use new async getCharsetForURI in /toolkit/components/places/tests/unit/test_bookmarks_json.js
: Use new async getCharsetForURI in /toolkit/components/places/tests/unit/test_...
Status: RESOLVED FIXED
:
Product: Toolkit
Classification: Components
Component: Places (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla23
Assigned To: Marcos Aruj
:
Mentors:
Depends on: 834543
Blocks:
  Show dependency treegraph
 
Reported: 2013-03-26 21:54 PDT by Raymond Lee [:raymondlee]
Modified: 2013-04-16 09:05 PDT (History)
3 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Use new getCharsetForURI. (8.77 KB, patch)
2013-03-27 12:14 PDT, Marcos Aruj
mak77: feedback+
Details | Diff | Splinter Review
Use new getCharsetForURI, return Task.spawn directly. (10.27 KB, patch)
2013-04-11 09:30 PDT, Marcos Aruj
mak77: review+
Details | Diff | Splinter Review
Patch for check-in (9.58 KB, patch)
2013-04-14 21:22 PDT, Raymond Lee [:raymondlee]
no flags Details | Diff | Splinter Review

Description Raymond Lee [:raymondlee] 2013-03-26 21:54:01 PDT
Use new async PlacesUtils.getCharsetForURI in this test
/toolkit/components/places/tests/unit/test_bookmarks_json.js
Comment 1 Raymond Lee [:raymondlee] 2013-03-26 21:56:15 PDT
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 2013-03-27 12:14:32 PDT
Created attachment 730275 [details] [diff] [review]
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] (Away 6-20 Aug) 2013-04-05 08:01:45 PDT
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 2013-04-11 09:30:09 PDT
Created attachment 736336 [details] [diff] [review]
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] (Away 6-20 Aug) 2013-04-11 11:10:48 PDT
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] 2013-04-11 20:39:49 PDT
(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] 2013-04-14 21:22:09 PDT
Created attachment 737351 [details] [diff] [review]
Patch for check-in
Comment 8 Ryan VanderMeulen [:RyanVM] 2013-04-15 05:46:30 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e93879f1adb
Comment 9 Ryan VanderMeulen [:RyanVM] 2013-04-15 19:04:05 PDT
https://hg.mozilla.org/mozilla-central/rev/2e93879f1adb

Note You need to log in before you can comment on or make changes to this bug.