If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

[OS.File] Clean up code by using default arguments

RESOLVED FIXED in mozilla23

Status

()

Toolkit
OS.File
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Yoric, Assigned: darkowlzz)

Tracking

Trunk
mozilla23
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(2 attachments, 2 obsolete attachments)

In OS.File, we often normalize options as follows:

function foo(bar, options) {
  options = options || noOptions;
  // ...
}

Instead, now that default arguments are available, we should be doing

function foo(bar, options = noOptions) {
  // ...
}
(Assignee)

Comment 1

5 years ago
Created attachment 730016 [details] [diff] [review]
Inserted default argument for |options| in OS.File

Hi,
I would like to work on this bug.

Submitting this patch. Please check if I missed anything.
Attachment #730016 - Flags: feedback?(dteller)
Comment on attachment 730016 [details] [diff] [review]
Inserted default argument for |options| in OS.File

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

That looks good, thank you very much.

::: toolkit/components/osfile/osfile_async_front.jsm
@@ +279,5 @@
>        options.bytes = buffer.byteLength;
>      }
>      // Note: Type.void_t.out_ptr.toMsg ensures that
>      // - the buffer is effectively shared (not neutered) between both
> +

Nit: Please remove that empty line.
Attachment #730016 - Flags: feedback?(dteller) → feedback+
(Assignee)

Comment 3

5 years ago
Created attachment 730077 [details] [diff] [review]
Inserted default argument for |options| in OS.File

Removed! :)
Attachment #730016 - Attachment is obsolete: true
Attachment #730077 - Flags: review?(dteller)
Comment on attachment 730077 [details] [diff] [review]
Inserted default argument for |options| in OS.File

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

Thanks for the patch :)
Attachment #730077 - Flags: review?(dteller) → review+
(Assignee)

Comment 5

5 years ago
Created attachment 730089 [details] [diff] [review]
2. Inserted default argument for |options| in OS.File

Added |r=Yoric|.

Can someone add |checkin-needed| in |keyword|?
Keywords: checkin-needed
(Assignee)

Comment 6

5 years ago
Thanks! Yoric :)
Attachment #730077 - Attachment is obsolete: true
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed90c16f9b22
Assignee: nobody → indiasuny000
Keywords: checkin-needed
Is it bad that I even said to myself "No Try push, what could possibly go wrong?" before landing this and bug 852143? Backed out for mochitest-other failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/70e442054249

https://tbpl.mozilla.org/php/getParsedLog.php?id=21161749&tree=Mozilla-Inbound

08:39:57     INFO -  14278 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/toolkit/components/osfile/tests/mochi/test_osfile_async.xul | read_write: Uncaught error [object WorkerErrorEvent]
08:39:58     INFO -  14528 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/toolkit/components/osfile/tests/mochi/test_osfile_async.xul | position: Uncaught error [object WorkerErrorEvent]
(Assignee)

Comment 9

5 years ago
Created attachment 730649 [details] [diff] [review]
1. Fixing a syntax error

This patch fixes the mochi-test failures which were happening due to the changes in the previous patch.
Attachment #730649 - Flags: review+
Attachment #730089 - Attachment description: Inserted default argument for |options| in OS.File → 2. Inserted default argument for |options| in OS.File
Attachment #730089 - Flags: review+
Attachment #730649 - Attachment description: Cleanup code → 1. Fixing a syntax error
Try: https://tbpl.mozilla.org/?tree=Try&rev=6e5790e9907c
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/c3afbf5e444c
https://hg.mozilla.org/integration/mozilla-inbound/rev/e53044984f1b
Flags: in-testsuite+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c3afbf5e444c
https://hg.mozilla.org/mozilla-central/rev/e53044984f1b
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in before you can comment on or make changes to this bug.