Closed Bug 879308 Opened 6 years ago Closed 6 years ago

null date to mozAlarms.add results in callback error rather than exception.

Categories

(Core :: DOM: Device Interfaces, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: nsm, Assigned: almasry.mina)

References

Details

(Whiteboard: [good first bug][mentor=nsm])

Attachments

(1 file, 4 obsolete files)

mozAlarms.add(null, ...)

The check currently happens in AlarmService and bubbles up into the onerror callback, whereas

mozAlarms.add(null, "footimezone" /* invalid timezone value */)

results in an immediate NOT_IMPLEMENTED error. This should be standardized. Convention seems to be that invalid arguments which can be 'validated' at point of call throw exceptions rather than perform any async error notification.

Fix should:
1) The date typecheck should be moved to AlarmsManager and fail with NS_ERROR_INVALID_ARG.
2) bad timezone value should fail in INVALID_ARG and not in NOT_IMPLEMENTED.
Hi this sounds like something I can do. I'd like to work on it. Can you please point out where in the source tree are the methods you want me to change. Where is AlarmsManager defined?

Assign?
Mina,

https://mxr.mozilla.org is a good way to find where code is located. In this case its in dom/alarm.

Good luck!
Assignee: nobody → almasry.mina
Attached patch Patch (obsolete) — Splinter Review
Here is a patch. I've also changed the tests to reflect the new behavior. But I couldn't run the tests though as I don't have Firefox OS. Not sure how to do that. If you want me to, just let me know how to do it.

I also have a couple of lines with a whitespace diff :( I think it's because my editor autodeletes trailing whitespace and it's in the same hunk as actual changes so I didn't want to change it in the patch.
Attachment #771636 - Flags: review?(nsm.nikhil)
Comment on attachment 771636 [details] [diff] [review]
Patch

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

Can you attach the patch again, the review tool isn't processing it correctly. Most things look good, but there seems to be some trailing whitespace in AlarmsManager.
Attachment #771636 - Flags: review?(nsm.nikhil) → review-
Attached patch Patch v2 (obsolete) — Splinter Review
Yes, I was getting fancy editing my patches and they seem to be broken. I'm trying again.
Attachment #771636 - Attachment is obsolete: true
Attachment #771937 - Flags: review?(nsm.nikhil)
Comment on attachment 771937 [details] [diff] [review]
Patch v2

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

The core logic is fine, but tests are broken and some formatting can be improved.

::: dom/alarm/AlarmService.jsm
@@ -403,5 @@
> -    if (!aNewAlarm.date) {
> -      aErrorCb("alarm.date is null");
> -      return;
> -    }
> -

I think you still want to keep this, since direct users of AlarmService from chrome code might make a mistake.

::: dom/alarm/AlarmsManager.js
@@ +67,5 @@
>      }
>  
> +    if (!aDate) {
> +      throw Components.results.NS_ERROR_INVALID_ARG;
> +    }

Move this check above the timezone check to keep it in line with the order of arguments.

@@ +73,3 @@
>      let request = this.createRequest();
>      this._cpmm.sendAsyncMessage(
>        "AlarmsManager:Add", 

Could you also remove this trailing whitespace?

::: dom/alarm/test/test_alarm_add_date.html
@@ +68,1 @@
>      var domRequest;

There is no need to introduce the domRequest variable, since it isn't used anywhere.

@@ +77,3 @@
>  
> +  function testInvalidTimeZone() {
> +    var domRequest;

You can get rid of this one too.

@@ +95,5 @@
>            navigator.appVersion.indexOf("Android") == -1) {
>  
>          testFutureDate();
> +        testNullDate();
> +        testInvaldTimeZone()

typo

@@ +96,5 @@
>  
>          testFutureDate();
> +        testNullDate();
> +        testInvaldTimeZone()
> +        SimpleTest.finish();

This won't work. The tests are async, which means SimpleTest.finish() will be called before the 3 test functions DOMRequests finish. You'll have to go back to the old style of calling the next function in the onsuccess of the first.
Attachment #771937 - Flags: review?(nsm.nikhil) → review-
Attached patch patch v3 (obsolete) — Splinter Review
Yeah it's hard to figure out your tests work just by staring at them :( Anyway here is a patch with the changes you asked for.
Attachment #771937 - Attachment is obsolete: true
Attachment #772189 - Flags: review?(nsm.nikhil)
(In reply to Mina Almasry from comment #7)
> Created attachment 772189 [details] [diff] [review]
> patch v3
> 
> Yeah it's hard to figure out your tests work just by staring at them :(
> Anyway here is a patch with the changes you asked for.

You can run the tests locally using |./mach mochitest-plain dom/alarm/test|
But I don't have firefox OS so when I do that the tests don't run, I only get "mozalarms on firefox OS only". I'm not sure how to run those tests from my mozilla-central build.
Comment on attachment 772189 [details] [diff] [review]
patch v3

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

r=nsm with following changes.

::: dom/alarm/test/test_alarm_add_date.html
@@ +57,5 @@
>      try {
>        domRequest = navigator.mozAlarms.add(yesterday, "honorTimezone", {});
>      } catch (e) {
>        ok(false,
>           "Unexpected exception trying to add alarm for yesterday.");

Should go to the next test even if this test fails.

@@ +84,2 @@
>      } catch(e) {
> +      // Exception thrown. Test passes

Remove this comment.

@@ +105,5 @@
>        if (navigator.userAgent.indexOf("Mobile") != -1 &&
> +          navigator.appVersion.indexOf("Android") == -1)
> +      {
> +        testDate();
> +        testInvalidTimeZone()

testInvalidTimeZone() should be called by testNullDate()
Attachment #772189 - Flags: review?(nsm.nikhil) → review+
Attached patch patch v4 (obsolete) — Splinter Review
- The current set of tests work fine, because that last testNullDate isn't async.
- The comment you wanted removed: I added it because there was one just like it in the same file.

Anyway, here is another patch.
Attachment #772189 - Attachment is obsolete: true
Attachment #772278 - Flags: review?(nsm.nikhil)
Comment on attachment 772278 [details] [diff] [review]
patch v4

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

Almost there.

::: dom/alarm/test/test_alarm_add_date.html
@@ +15,5 @@
>    "use strict";
>  
> +  // Verify passing future, past, null date works correctly, and that passing an
> +  // invalid time zone raises an exception.
> +  function testDate() {

Nit: Keep the function name and description as before, since this test itself only tests future date.

@@ +48,5 @@
>      };
>  
>    }
>  
> +  // Verify passing a Date that's already past doesn't fail (it should fire immediately).

Nit: Please wrap to 80 lines.

@@ +109,5 @@
>        if (navigator.userAgent.indexOf("Mobile") != -1 &&
> +          navigator.appVersion.indexOf("Android") == -1)
> +      {
> +        testDate();
> +        SimpleTest.finish();

Bug: SimpleTest.finish() should be called by testInvalidTimezone()
Attachment #772278 - Flags: review?(nsm.nikhil) → review-
Attached patch patchSplinter Review
No problem. I'm right behind you chief.
Attachment #772278 - Attachment is obsolete: true
Attachment #772881 - Flags: review?(nsm.nikhil)
Attachment #772881 - Attachment is patch: true
Attachment #772881 - Attachment mime type: text/x-patch → text/plain
Attachment #772881 - Flags: review?(nsm.nikhil) → review+
Comment on attachment 772881 [details] [diff] [review]
patch

Jonas,

This changes API behaviour (although undocumented). sr please.
Attachment #772881 - Flags: superreview?(jonas)
Keywords: checkin-needed
Keywords: checkin-needed
Jonas, 

Just a friendly reminder this is still waiting for super review. Thanks!
Attachment #772881 - Flags: superreview?(jonas) → superreview?(mounir)
Comment on attachment 772881 [details] [diff] [review]
patch

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

The API change looks good. Thanks Mina and sorry for the late review, I am a bit behind my reviews lately.
Attachment #772881 - Flags: superreview?(mounir) → superreview+
https://hg.mozilla.org/mozilla-central/rev/49939eb7372e
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.