1015100 - Polish the Alarm API Mochitest

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Comment 1

[Gene Lian (I already quit Mozilla)]

Attachment: Part 1, check manifestURL for getAll/remove (won't land)

Since we already did this for add(), there is no reason for not doing so for remove() and getAll(). The Alarm API logic is relying on the valid manifest URL to manage alarms, so |manifestURL| has to be valid.

Comment 2

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2, polish tests to avoid accessing null request

Just polished the logic of failed cases to avoid accessing the null domRequest:

TypeError: domRequest is undefined at http://mochi.test:8888/tests/dom/alarm/test/test_alarm_add_date.html:62

Comment 3

[Gene Lian (I already quit Mozilla)]

Attachment: Part 3, polish tests to stop running invalid builds

In bug 876980 and bug 867868, we already supported Alarm API for installed apps on Firefox desktop, so the check for b2g only here doesn't make sense. Polish the tests by introducing a more reasonable checking condition.

Comment 4

[Gene Lian (I already quit Mozilla)]

Full try: https://tbpl.mozilla.org/?tree=Try&rev=00a5c26a0898

Comment 5

[Gene Lian (I already quit Mozilla)]

Attachment: Part 3, polish tests to not run on invalid builds, V2

Comment 6

[Gene Lian (I already quit Mozilla)]

Comment on attachment 8427663 [details] [diff] [review]
Part 1, check manifestURL for getAll/remove (won't land)

Please see comment #1 for the summary.

Comment 7

[Gene Lian (I already quit Mozilla)]

Comment on attachment 8427664 [details] [diff] [review]
Part 2, polish tests to avoid accessing null request

Please see comment #2 for the summary.

Comment 8

[Gene Lian (I already quit Mozilla)]

Comment on attachment 8427821 [details] [diff] [review]
Part 3, polish tests to not run on invalid builds, V2

Please see comment #3 for the summary.

Comment 9

[Gene Lian (I already quit Mozilla)]

Attachment: Part 3, polish tests to not run on invalid builds, V2.1

Please comment #3 for the summary.

Comment 10

[Gene Lian (I already quit Mozilla)]

Try: https://tbpl.mozilla.org/?tree=Try&rev=7a0be2b67f29

Comment 11

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8427663 [details] [diff] [review]
Part 1, check manifestURL for getAll/remove (won't land)

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

r+ for the code, but while this does make the API uniform, it breaks the API contract. remove() and getAll() initially promised not to throw. :)
Do we have a policy in b2g for when we break APIs? Which release will this land in?

Please ask a b2g/webapi owner/peer about this.

Comment 12

[Gene Lian (I already quit Mozilla)]

(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #11)
> Comment on attachment 8427663 [details] [diff] [review]
> Part 1, check manifestURL for getAll/remove
> 
> Review of attachment 8427663 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ for the code, but while this does make the API uniform, it breaks the API
> contract. remove() and getAll() initially promised not to throw. :)
> Do we have a policy in b2g for when we break APIs? Which release will this
> land in?
> 
> Please ask a b2g/webapi owner/peer about this.

Yeap, after some thoughts, I decide to not to land the part-1 patch, because:

1. If users already failed to call add() to add an alarm, they wouldn't attempt to call getAll() or remove() to manage an alarm that has been added.

2. If users want to call getAll()/remove() without calling add() first, in the original codes it would just do nothing. Adding a throw might break the apps in the field.

To avoid breaking the compatibility in the field, we'd better not to do this change.

Comment 13

[Gene Lian (I already quit Mozilla)]

Btw, if we still hope to throw for getAll()/remove(), I tend to fire another bug for it. Let this bug just polish the tests only.

Comment 14

[Gene Lian (I already quit Mozilla)]

https://hg.mozilla.org/integration/b2g-inbound/rev/c1cc2ffe8bfd
https://hg.mozilla.org/integration/b2g-inbound/rev/9aa3042c0023

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c1cc2ffe8bfd
https://hg.mozilla.org/mozilla-central/rev/9aa3042c0023