[OS.File] Remove outer try/catch in osfile_async_worker.js

RESOLVED FIXED in mozilla27

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Yoric, Assigned: errietta)

Tracking

unspecified
mozilla27
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mentor=Yoric][lang=js])

Attachments

(2 attachments, 2 obsolete attachments)

Now that worker errors are displayed, we should get rid of the outer try/catch in osfile_async_worker.js.
(Assignee)

Comment 1

5 years ago
Created attachment 808522 [details] [diff] [review]
Proposed patch
Attachment #808522 - Flags: review?(dteller)
Comment on attachment 808522 [details] [diff] [review]
Proposed patch

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

Looks good, but I'll wait until I have a version of this patch that ignores whitespace before finishing the review.
Also, you'll need to add ";r=yoric" at the end of your commit message.

::: toolkit/components/osfile/modules/osfile_async_worker.js
@@ +18,2 @@
>  
> +  /**

Nit: Could you keep the alignment of /** and the following * as it was before your patch?

@@ +94,2 @@
>  
> +  /**

Nit: Same thing her and in other uses of /**
Attachment #808522 - Flags: review?(dteller) → feedback+
(Assignee)

Comment 3

5 years ago
Created attachment 808534 [details] [diff] [review]
Proposed patch
Attachment #808534 - Flags: review?(dteller)
(Assignee)

Comment 4

5 years ago
Created attachment 808535 [details] [diff] [review]
whitespace-insensitive patch (do not land)
Attachment #808535 - Flags: review?(dteller)
Comment on attachment 808535 [details] [diff] [review]
whitespace-insensitive patch (do not land)

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

Looks good, thanks.
Attachment #808535 - Flags: review?(dteller)
(Assignee)

Updated

5 years ago
Attachment #808522 - Attachment is obsolete: true
Comment on attachment 808534 [details] [diff] [review]
Proposed patch

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

Looks good. I forgot to ask you to add the bug# in the commit message. So, something like "Bug 918354 - Remove outer try/catch in osfile_async_worker.js; r=yoric"
Attachment #808534 - Flags: review?(dteller) → review+
Assignee: nobody → errietta
Attachment #808535 - Attachment description: whitespace-insensitive patch → whitespace-insensitive patch (do not land)
(Assignee)

Comment 7

5 years ago
Created attachment 808564 [details] [diff] [review]
Patch
Attachment #808534 - Attachment is obsolete: true
Attachment #808564 - Flags: review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/5d4752319dd6
Keywords: checkin-needed
Whiteboard: [mentor=Yoric][lang=js] → [mentor=Yoric][lang=js][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/5d4752319dd6
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [mentor=Yoric][lang=js][fixed-in-fx-team] → [mentor=Yoric][lang=js]
Target Milestone: --- → mozilla27
You need to log in before you can comment on or make changes to this bug.