Closed Bug 865555 Opened 7 years ago Closed 7 years ago

Remove synchronous setCharsetForURI in BI_importJSONNode

Categories

(Toolkit :: Places, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: raymondlee, Assigned: raymondlee)

References

Details

Attachments

(1 file, 2 obsolete files)

At the moment, BI_importJSONNode contains a synchronous setCharsetForURI and we should change it to asynchronous so that the caller BJU_importJSONNode can be real async instead of a fake one (mainThread.dispatch).
(In reply to Raymond Lee [:raymondlee] from comment #0)
> At the moment, BI_importJSONNode contains a synchronous setCharsetForURI and
> we should change it to asynchronous so that the caller BJU_importJSONNode
> can be real async instead of a fake one (mainThread.dispatch).

For reference: bug 855842 comment 6 suggested to use the fake one and this bug is to make it real async
Blocks: 852030
Assignee: nobody → raymond
Attached patch v1 (obsolete) — Splinter Review
Attachment #758477 - Flags: review?(mak77)
Attached patch v1 -w (obsolete) — Splinter Review
This is a -w version so you can review more easily.
Comment on attachment 758478 [details] [diff] [review]
v1 -w

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

It looks fine to me, thanks!

::: toolkit/components/places/BookmarkJSONUtils.jsm
@@ +323,5 @@
>  
>        PlacesUtils.bookmarks.runInBatchMode(batch, null);
>      }
>  
>      return deferred.promise;

alternatively you may wrap everything in importFromJSON into the Task and return its promise. Not important, see what looks better.
Attachment #758478 - Flags: review+
Attachment #758477 - Flags: review?(mak77) → review+
Attachment #758478 - Attachment is obsolete: true
(In reply to Marco Bonardo [:mak] from comment #4)
> Comment on attachment 758478 [details] [diff] [review]
> v1 -w
> 
> Review of attachment 758478 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It looks fine to me, thanks!
> 
> ::: toolkit/components/places/BookmarkJSONUtils.jsm
> @@ +323,5 @@
> >  
> >        PlacesUtils.bookmarks.runInBatchMode(batch, null);
> >      }
> >  
> >      return deferred.promise;
> 
> alternatively you may wrap everything in importFromJSON into the Task and
> return its promise. Not important, see what looks better.

I have given it a try, however, the |batch| we pass into  PlacesUtils.bookmarks.runInBatchMode(batch, null); is an object which contains a method to be executed and we need the deferred.resolve() inside that method, otherwise, the code wouldn't return early and tests fail.  Therefore, I stick with the current patch.

Pushed to try and waiting for results
https://tbpl.mozilla.org/?tree=Try&rev=49ba36477408
Attachment #758477 - Attachment is obsolete: true
Passed try.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7d5c7429fe03
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Depends on: 885205
You need to log in before you can comment on or make changes to this bug.