Closed
Bug 901114
Opened 11 years ago
Closed 10 years ago
Convert Alarm API to WebIDL
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla33
People
(Reporter: nsm, Assigned: selin, Mentored)
References
Details
(Whiteboard: [good first bug])
Attachments
(1 file, 12 obsolete files)
12.00 KB,
patch
|
Details | Diff | Splinter Review |
dom/alarm/nsIDOMAlarmsManager.idl should be converted to WebIDL.
Guide:
https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings
Another simple & small example: 884897
Hi Nikhil
I am planning on working on this as my first bug.
I've already made some initial modification by referring to 884897.
Although getting a successful build with my initial patch, I still have a lot of uncertainty and questions.
Can you show me what I should do next?
Attachment #803910 -
Flags: review?(nsm.nikhil)
Reporter | ||
Comment 3•11 years ago
|
||
Comment on attachment 803910 [details] [diff] [review]
Bug 901114 Convert Alarm API to WebIDL(initial work)
Review of attachment 803910 [details] [diff] [review]:
-----------------------------------------------------------------
You'll want to do a few more things:
1) Move the navigator property declaration to the IDL file using [NavigatorProperty]
2) Use a custom function in Navigator.cpp and the [Func="..."] to write some C++ code that checks that the caller has permission to use the alarm API and that alarms are enabled.
3) Modify AlarmsManager.js init() to be __init() and other minor changes (see https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#Implementing_WebIDL_using_Javascript)
::: dom/alarm/AlarmsManager.js
@@ +18,5 @@
> Cu.import("resource://gre/modules/Services.jsm");
> Cu.import("resource://gre/modules/DOMRequestHelper.jsm");
> Cu.import("resource://gre/modules/ObjectWrapper.jsm");
>
> +const ALARMSMANAGER_CONTRACTID = "mozilla.org/alarm/AlarmsManager;1";
don't change this since other parts of Gecko rely on it and the old name is acceptable.
::: dom/alarm/AlarmsManager.manifest
@@ +1,3 @@
> component {fea1e884-9b05-11e1-9b64-87a7016c3860} AlarmsManager.js
> +contract @mozilla.org/alarm/AlarmsManager;1 {fea1e884-9b05-11e1-9b64-87a7016c3860}
> +category JavaScript-navigator-property mozAlarms @mozilla.org/alarm/AlarmsManager;1
you can drop line 3
Attachment #803910 -
Flags: review?(nsm.nikhil)
(In reply to Nikhil Marathe [:nsm] from comment #3)
> Comment on attachment 803910 [details] [diff] [review]
> Bug 901114 Convert Alarm API to WebIDL(initial work)
>
> Review of attachment 803910 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> You'll want to do a few more things:
> 1) Move the navigator property declaration to the IDL file using
> [NavigatorProperty]
> 2) Use a custom function in Navigator.cpp and the [Func="..."] to write some
> C++ code that checks that the caller has permission to use the alarm API and
> that alarms are enabled.
> 3) Modify AlarmsManager.js init() to be __init() and other minor changes
> (see
> https://developer.mozilla.org/en-US/docs/Mozilla/
> WebIDL_bindings#Implementing_WebIDL_using_Javascript)
>
> ::: dom/alarm/AlarmsManager.js
> @@ +18,5 @@
> > Cu.import("resource://gre/modules/Services.jsm");
> > Cu.import("resource://gre/modules/DOMRequestHelper.jsm");
> > Cu.import("resource://gre/modules/ObjectWrapper.jsm");
> >
> > +const ALARMSMANAGER_CONTRACTID = "mozilla.org/alarm/AlarmsManager;1";
>
> don't change this since other parts of Gecko rely on it and the old name is
> acceptable.
>
> ::: dom/alarm/AlarmsManager.manifest
> @@ +1,3 @@
> > component {fea1e884-9b05-11e1-9b64-87a7016c3860} AlarmsManager.js
> > +contract @mozilla.org/alarm/AlarmsManager;1 {fea1e884-9b05-11e1-9b64-87a7016c3860}
> > +category JavaScript-navigator-property mozAlarms @mozilla.org/alarm/AlarmsManager;1
>
> you can drop line 3
OK, got it :)
Updated•11 years ago
|
Assignee: nobody → i
OS: Linux → All
Hardware: x86_64 → All
Sorry I'm a little late, patch updated.
> 3) Modify AlarmsManager.js init() to be __init() and other minor changes
> (see
> https://developer.mozilla.org/en-US/docs/Mozilla/
> WebIDL_bindings#Implementing_WebIDL_using_Javascript)
I don't understand why AlarmsManager.js need a __init() method?
nsIDOMAlarmsManager.idl doesn't describe a constructor with any other constructor arguments.
Attachment #803910 -
Attachment is obsolete: true
Reporter | ||
Comment 7•11 years ago
|
||
(In reply to i from comment #5)
> Sorry I'm a little late, patch updated.
>
> > 3) Modify AlarmsManager.js init() to be __init() and other minor changes
> > (see
> > https://developer.mozilla.org/en-US/docs/Mozilla/
> > WebIDL_bindings#Implementing_WebIDL_using_Javascript)
>
> I don't understand why AlarmsManager.js need a __init() method?
> nsIDOMAlarmsManager.idl doesn't describe a constructor with any other
> constructor arguments.
Ignore this bit ;)
Reporter | ||
Comment 8•11 years ago
|
||
Comment on attachment 806023 [details] [diff] [review]
Bug 901114 Convert Alarm API to WebIDL r2
Review of attachment 806023 [details] [diff] [review]:
-----------------------------------------------------------------
r=me with comments addressed and as long as Alarms mochitests have been tested and are passing.
::: dom/alarm/AlarmsManager.js
@@ +38,3 @@
> classID : ALARMSMANAGER_CID,
>
> QueryInterface : XPCOMUtils.generateQI([nsIDOMMozAlarmsManager,
Remove nsIDOMMozAlarmsManager from the list.
@@ +40,5 @@
> QueryInterface : XPCOMUtils.generateQI([nsIDOMMozAlarmsManager,
> Ci.nsIDOMGlobalPropertyInitializer,
> Ci.nsISupportsWeakReference]),
>
> classInfo : XPCOMUtils.generateCI({ classID: ALARMSMANAGER_CID,
You can get rid of classInfo. After that, since ALARMSMANAGER_CID and ALARMSMANAGER_CONTRACTID are used only in one place, un const them and directly put in the string literals.
::: dom/base/Navigator.cpp
@@ +1785,1 @@
> bool Navigator::HasPushNotificationsSupport(JSContext* /* unused */,
please make this
bool
Navigator::HasPushNotificationsSupport()
while you are here. Thanks
@@ +1789,5 @@
> return win && Preferences::GetBool("services.push.enabled", false) && CheckPermission(win, "push");
> }
>
> /* static */
> +bool Navigator::HasAlarmsSupport(JSContext* /* unused */,
Coding style is
bool
Navigator::HasAlarmsSupport(...)
::: dom/webidl/AlarmsManager.webidl
@@ +3,5 @@
> +* License, v. 2.0. If a copy of the MPL was not distributed with this file,
> +* You can obtain one at http://mozilla.org/MPL/2.0/.
> +*/
> +
> +[NavigatorProperty="mozAlarms", JSImplementation="@mozilla.org/alarmsManager;1", Func="Navigator::HasAlarmsSupport"]
Check if it is possible to do 'new AlarmsManager()' from content JS, which I think it will be, that is AlarmsManager will be exported as a global. You want to use [NoInterfaceObject] to prevent that.
Attachment #806023 -
Flags: review+
Alarms mochitests cannot pass with my previous patch, so I made some modifications to test code based on the WebIDL convert,
Please take a look:
-----------------------------------------------------------------
diff --git a/dom/alarm/test/test_alarm_permitted_app.html b/dom/alarm/test/test_alarm_permitted_app.html
--- a/dom/alarm/test/test_alarm_permitted_app.html
+++ b/dom/alarm/test/test_alarm_permitted_app.html
@@ -19,18 +19,16 @@ SpecialPowers.pushPrefEnv({"set": [["dom
SpecialPowers.addPermission("alarms", true, document);
// mozAlarms is intalled on all platforms except Android for the moment.
if (navigator.appVersion.indexOf("Android") != -1) {
ok(!('mozAlarms' in navigator), "navigator.mozAlarms should not exist");
} else {
ok('mozAlarms' in navigator, "navigator.mozAlarms should exist");
ok(navigator.mozAlarms, "navigator.mozAlarms should return an object");
- ok(navigator.mozAlarms instanceof SpecialPowers.Ci.nsIDOMMozAlarmsManager,
- "navigator.mozAlarms should be an nsIDOMMozAlarmsManager object");
}
SimpleTest.finish();
});
-----------------------------------------------------------------
nsIDOMMozAlarmsManager doesn't exist any more, so I removed the third assert.(Am I right?)
-----------------------------------------------------------------
diff --git a/dom/alarm/test/test_alarm_non_permitted_app.html b/dom/alarm/test/test_alarm_non_permitted_app.html
--- a/dom/alarm/test/test_alarm_non_permitted_app.html
+++ b/dom/alarm/test/test_alarm_non_permitted_app.html
@@ -20,17 +20,18 @@ if (SpecialPowers.hasPermission("alarms"
} else {
SpecialPowers.pushPrefEnv({"set": [["dom.mozAlarms.enabled", true]]}, function() {
SpecialPowers.removePermission("alarms", document);
// mozAlarms is intalled on all platforms except Android for the moment.
if (navigator.appVersion.indexOf("Android") != -1) {
ok(!('mozAlarms' in navigator), "navigator.mozAlarms should not exist");
} else {
- ok('mozAlarms' in navigator, "navigator.mozAlarms should exist");
+ ok(!('mozAlarms' in navigator),
+ "navigator.mozAlarms should not exist if permission not set");
is(navigator.mozAlarms, null, "navigator.mozAlarms should return null");
}
SpecialPowers.addPermission("alarms", true, document);
SimpleTest.finish();
});
}
</script>
</pre>
-----------------------------------------------------------------
By move permission check code to Navigator.cpp then add Func="..." to WebIDL,
I don't think mozAlarms should still exist in navigator if app doesn't have permission
(based on https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#PrefControlled
and https://bugzilla.mozilla.org/show_bug.cgi?id=884897#c15)
So I negate the assert.(Am I right?)
...And there is more:
-----------------------------------------------------------------
diff --git a/dom/base/Navigator.cpp b/dom/base/Navigator.cpp
--- a/dom/base/Navigator.cpp
+++ b/dom/base/Navigator.cpp
@@ -1764,34 +1764,45 @@ Navigator::HasTimeSupport(JSContext* /*
{
nsCOMPtr<nsPIDOMWindow> win = GetWindowFromGlobal(aGlobal);
return win && CheckPermission(win, "time");
}
#endif // MOZ_TIME_MANAGER
#ifdef MOZ_MEDIA_NAVIGATOR
/* static */
-bool Navigator::HasUserMediaSupport(JSContext* /* unused */,
- JSObject* /* unused */)
+bool
+Navigator::HasUserMediaSupport(JSContext* /* unused */,
+ JSObject* /* unused */)
{
// Make enabling peerconnection enable getUserMedia() as well
return Preferences::GetBool("media.navigator.enabled", false) ||
Preferences::GetBool("media.peerconnection.enabled", false);
}
#endif // MOZ_MEDIA_NAVIGATOR
-----------------------------------------------------------------
I also rewrite Navigator::HasUserMediaSupport complying with coding style,
but it's not our code(by Boris Zbarsky <bzbarsky@mit.edu),
Is it appropriate if I do this?
Comment 10•11 years ago
|
||
Attachment #806023 -
Attachment is obsolete: true
Reporter | ||
Comment 11•11 years ago
|
||
(In reply to i from comment #9)
> Alarms mochitests cannot pass with my previous patch, so I made some
> modifications to test code based on the WebIDL convert,
> Please take a look:
>
> -----------------------------------------------------------------
> diff --git a/dom/alarm/test/test_alarm_permitted_app.html
> b/dom/alarm/test/test_alarm_permitted_app.html
> --- a/dom/alarm/test/test_alarm_permitted_app.html
> +++ b/dom/alarm/test/test_alarm_permitted_app.html
> @@ -19,18 +19,16 @@ SpecialPowers.pushPrefEnv({"set": [["dom
> SpecialPowers.addPermission("alarms", true, document);
>
> // mozAlarms is intalled on all platforms except Android for the moment.
> if (navigator.appVersion.indexOf("Android") != -1) {
> ok(!('mozAlarms' in navigator), "navigator.mozAlarms should not exist");
> } else {
> ok('mozAlarms' in navigator, "navigator.mozAlarms should exist");
> ok(navigator.mozAlarms, "navigator.mozAlarms should return an object");
> - ok(navigator.mozAlarms instanceof
> SpecialPowers.Ci.nsIDOMMozAlarmsManager,
> - "navigator.mozAlarms should be an nsIDOMMozAlarmsManager object");
> }
>
> SimpleTest.finish();
> });
> -----------------------------------------------------------------
>
> nsIDOMMozAlarmsManager doesn't exist any more, so I removed the third
> assert.(Am I right?)
But change the test case for "should return an object" to use typeof.
This is correct.
Please fix the spelling of "installed" in the comment.
>
>
> -----------------------------------------------------------------
> diff --git a/dom/alarm/test/test_alarm_non_permitted_app.html
> b/dom/alarm/test/test_alarm_non_permitted_app.html
> --- a/dom/alarm/test/test_alarm_non_permitted_app.html
> +++ b/dom/alarm/test/test_alarm_non_permitted_app.html
> @@ -20,17 +20,18 @@ if (SpecialPowers.hasPermission("alarms"
> } else {
> SpecialPowers.pushPrefEnv({"set": [["dom.mozAlarms.enabled", true]]},
> function() {
> SpecialPowers.removePermission("alarms", document);
>
> // mozAlarms is intalled on all platforms except Android for the moment.
> if (navigator.appVersion.indexOf("Android") != -1) {
> ok(!('mozAlarms' in navigator), "navigator.mozAlarms should not
> exist");
> } else {
> - ok('mozAlarms' in navigator, "navigator.mozAlarms should exist");
> + ok(!('mozAlarms' in navigator),
> + "navigator.mozAlarms should not exist if permission not set");
> is(navigator.mozAlarms, null, "navigator.mozAlarms should return
> null");
> }
> SpecialPowers.addPermission("alarms", true, document);
> SimpleTest.finish();
> });
> }
> </script>
> </pre>
> -----------------------------------------------------------------
>
> By move permission check code to Navigator.cpp then add Func="..." to WebIDL,
> I don't think mozAlarms should still exist in navigator if app doesn't have
> permission
> (based on
> https://developer.mozilla.org/en-US/docs/Mozilla/
> WebIDL_bindings#PrefControlled
> and https://bugzilla.mozilla.org/show_bug.cgi?id=884897#c15)
> So I negate the assert.(Am I right?)
>
Yes, also remove the line that checks for null.
>
>
> ...And there is more:
> -----------------------------------------------------------------
> diff --git a/dom/base/Navigator.cpp b/dom/base/Navigator.cpp
> --- a/dom/base/Navigator.cpp
> +++ b/dom/base/Navigator.cpp
> @@ -1764,34 +1764,45 @@ Navigator::HasTimeSupport(JSContext* /*
> {
> nsCOMPtr<nsPIDOMWindow> win = GetWindowFromGlobal(aGlobal);
> return win && CheckPermission(win, "time");
> }
> #endif // MOZ_TIME_MANAGER
>
> #ifdef MOZ_MEDIA_NAVIGATOR
> /* static */
> -bool Navigator::HasUserMediaSupport(JSContext* /* unused */,
> - JSObject* /* unused */)
> +bool
> +Navigator::HasUserMediaSupport(JSContext* /* unused */,
> + JSObject* /* unused */)
> {
> // Make enabling peerconnection enable getUserMedia() as well
> return Preferences::GetBool("media.navigator.enabled", false) ||
> Preferences::GetBool("media.peerconnection.enabled", false);
> }
> #endif // MOZ_MEDIA_NAVIGATOR
> -----------------------------------------------------------------
>
> I also rewrite Navigator::HasUserMediaSupport complying with coding style,
> but it's not our code(by Boris Zbarsky <bzbarsky@mit.edu),
> Is it appropriate if I do this?
That is fine.
Comment 12•11 years ago
|
||
Attachment #810517 -
Attachment is obsolete: true
Comment 13•11 years ago
|
||
Patch updated,
my modification this time:
-----------------------------------------------------------------
diff --git a/dom/alarm/test/test_alarm_non_permitted_app.html b/dom/alarm/test/test_alarm_non_permitted_app.html
--- a/dom/alarm/test/test_alarm_non_permitted_app.html
+++ b/dom/alarm/test/test_alarm_non_permitted_app.html
@@ -22,17 +22,16 @@ if (SpecialPowers.hasPermission("alarms"
SpecialPowers.removePermission("alarms", document);
// mozAlarms is intalled on all platforms except Android for the moment.
if (navigator.appVersion.indexOf("Android") != -1) {
ok(!('mozAlarms' in navigator), "navigator.mozAlarms should not exist");
} else {
ok(!('mozAlarms' in navigator),
"navigator.mozAlarms should not exist if permission not set");
- is(navigator.mozAlarms, null, "navigator.mozAlarms should return null");
}
SpecialPowers.addPermission("alarms", true, document);
SimpleTest.finish();
});
}
</script>
</pre>
</body>
diff --git a/dom/alarm/test/test_alarm_permitted_app.html b/dom/alarm/test/test_alarm_permitted_app.html
--- a/dom/alarm/test/test_alarm_permitted_app.html
+++ b/dom/alarm/test/test_alarm_permitted_app.html
@@ -13,22 +13,23 @@
"use strict";
SimpleTest.waitForExplicitFinish();
SpecialPowers.pushPrefEnv({"set": [["dom.mozAlarms.enabled", true]]}, function() {
SpecialPowers.addPermission("alarms", true, document);
- // mozAlarms is intalled on all platforms except Android for the moment.
+ // mozAlarms is installed on all platforms except Android for the moment.
if (navigator.appVersion.indexOf("Android") != -1) {
ok(!('mozAlarms' in navigator), "navigator.mozAlarms should not exist");
} else {
ok('mozAlarms' in navigator, "navigator.mozAlarms should exist");
- ok(navigator.mozAlarms, "navigator.mozAlarms should return an object");
+ ok(typeof navigator.mozAlarms === 'object',
+ "navigator.mozAlarms should return an object");
}
SimpleTest.finish();
});
</script>
</pre>
</body>
-----------------------------------------------------------------
What's my next step?
Comment 14•11 years ago
|
||
(In reply to i from comment #9)
> --- a/dom/alarm/test/test_alarm_permitted_app.html
> +++ b/dom/alarm/test/test_alarm_permitted_app.html
> @@ -19,18 +19,16 @@ SpecialPowers.pushPrefEnv({"set": [["dom
> - ok(navigator.mozAlarms instanceof
> SpecialPowers.Ci.nsIDOMMozAlarmsManager,
> - "navigator.mozAlarms should be an nsIDOMMozAlarmsManager object");
> nsIDOMMozAlarmsManager doesn't exist any more, so I removed the third
> assert.(Am I right?)
Doesn't |navigator.mozAlarms instanceof MozAlarmsManager| work? (MozAlarmsManager will exist on the global object iff the "alarms" permission is set.)
Comment 15•11 years ago
|
||
(In reply to Masatoshi Kimura [:emk] from comment #14)
> (In reply to i from comment #9)
> > --- a/dom/alarm/test/test_alarm_permitted_app.html
> > +++ b/dom/alarm/test/test_alarm_permitted_app.html
> > @@ -19,18 +19,16 @@ SpecialPowers.pushPrefEnv({"set": [["dom
> > - ok(navigator.mozAlarms instanceof
> > SpecialPowers.Ci.nsIDOMMozAlarmsManager,
> > - "navigator.mozAlarms should be an nsIDOMMozAlarmsManager object");
>
> > nsIDOMMozAlarmsManager doesn't exist any more, so I removed the third
> > assert.(Am I right?)
>
> Doesn't |navigator.mozAlarms instanceof MozAlarmsManager| work?
> (MozAlarmsManager will exist on the global object iff the "alarms"
> permission is set.)
Probably it will not work on the B2G mochitest. Please disregard the comment.
Comment 16•11 years ago
|
||
(In reply to Masatoshi Kimura [:emk] from comment #15)
> (In reply to Masatoshi Kimura [:emk] from comment #14)
> > (In reply to i from comment #9)
> > > --- a/dom/alarm/test/test_alarm_permitted_app.html
> > > +++ b/dom/alarm/test/test_alarm_permitted_app.html
> > > @@ -19,18 +19,16 @@ SpecialPowers.pushPrefEnv({"set": [["dom
> > > - ok(navigator.mozAlarms instanceof
> > > SpecialPowers.Ci.nsIDOMMozAlarmsManager,
> > > - "navigator.mozAlarms should be an nsIDOMMozAlarmsManager object");
> >
> > > nsIDOMMozAlarmsManager doesn't exist any more, so I removed the third
> > > assert.(Am I right?)
> >
> > Doesn't |navigator.mozAlarms instanceof MozAlarmsManager| work?
> > (MozAlarmsManager will exist on the global object iff the "alarms"
> > permission is set.)
>
> Probably it will not work on the B2G mochitest. Please disregard the comment.
MozAlarmsManager? Did you mean AlarmsManager?
AlarmsManager won't exist on the global object even if the "alarms" permission is set because [NoInterfaceObject] is set on AlarmsManager.webidl
Comment 17•11 years ago
|
||
(In reply to i from comment #16)
> MozAlarmsManager? Did you mean AlarmsManager?
> AlarmsManager won't exist on the global object even if the "alarms"
> permission is set because [NoInterfaceObject] is set on AlarmsManager.webidl
Oh, then |Func="Navigator::HasAlarmsSupport"| is redundant. The "Func" annotation is used to control when then name will be exposed on the global. Just remove it.
Comment 18•11 years ago
|
||
By the way, why the interface name takes plural forms? It should be "AlarmManager" per the spec.
http://www.w3.org/TR/web-alarms/#interface-alarmmanager
Even if the interface name is annotated with [NoInterfaceObject], it will be visible from content via the toString() method.
Comment 19•11 years ago
|
||
(In reply to Masatoshi Kimura [:emk] from comment #18)
> Even if the interface name is annotated with [NoInterfaceObject], it will be
> visible from content via the toString() method.
So
ok(navigator.mozAlarms.toString(), "[object AlarmManager]",
"navigator.mozAlarms should be an AlarmManager object");
will work.
Comment 20•11 years ago
|
||
> ok(navigator.mozAlarms.toString(), "[object AlarmManager]",
is(navigator.mozAlarms.toString(), "[object AlarmManager]",
, of course. Sorry for the bugspam.
Comment 21•11 years ago
|
||
(In reply to Nikhil Marathe [:nsm] from comment #3)
> 1) Move the navigator property declaration to the IDL file using
> [NavigatorProperty]
> 2) Use a custom function in Navigator.cpp and the [Func="..."] to write some
> C++ code that checks that the caller has permission to use the alarm API and
> that alarms are enabled.
Why? We can just add [Func="..."] to the navigator property.
Reporter | ||
Comment 22•11 years ago
|
||
(In reply to Masatoshi Kimura [:emk] from comment #21)
> (In reply to Nikhil Marathe [:nsm] from comment #3)
> > 1) Move the navigator property declaration to the IDL file using
> > [NavigatorProperty]
> > 2) Use a custom function in Navigator.cpp and the [Func="..."] to write some
> > C++ code that checks that the caller has permission to use the alarm API and
> > that alarms are enabled.
>
> Why? We can just add [Func="..."] to the navigator property.
Thanks for clearing that up, I didn't know this could be done on the Navigator WebIDL.
Reporter | ||
Updated•11 years ago
|
Assignee: i → nobody
Comment 23•11 years ago
|
||
Fixing this before bug 983502 means that MarketPlace's feature detection for this API will break.
Depends on: 983502
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → selin
Assignee | ||
Comment 24•11 years ago
|
||
I'd like to take over this bug as an initial practice for WebIDL.
Attachment #810912 -
Attachment is obsolete: true
Attachment #8427480 -
Flags: review?(nsm.nikhil)
Assignee | ||
Comment 25•11 years ago
|
||
Updated to fix some uncovered test cases found in
https://tbpl.mozilla.org/?tree=Try&rev=257a2230723b
Attachment #8427480 -
Attachment is obsolete: true
Attachment #8427480 -
Flags: review?(nsm.nikhil)
Attachment #8427657 -
Flags: review?(nsm.nikhil)
Reporter | ||
Comment 26•11 years ago
|
||
Comment on attachment 8427657 [details] [diff] [review]
New patch attempt 2
Review of attachment 8427657 [details] [diff] [review]:
-----------------------------------------------------------------
r=me with the comments fixed.
Since we expose new interfaces on Window to webapps, this will need DOM peer review.
Thanks for the patch!
::: dom/alarm/AlarmsManager.js
@@ +31,4 @@
>
> + classID : Components.ID("{fea1e884-9b05-11e1-9b64-87a7016c3860}"),
> +
> + QueryInterface : XPCOMUtils.generateQI([Ci.nsISupports,
nsISupports is not required.
@@ +152,2 @@
> if (!principal.URI) {
> return null;
WebIDL components cannot return null. You don't need this check anymore since the WebIDL and permission machinery should ensure you are in a valid document.
::: dom/alarm/test/test_alarm_permitted_app.html
@@ +23,5 @@
> ok(!('mozAlarms' in navigator), "navigator.mozAlarms should not exist");
> } else {
> ok('mozAlarms' in navigator, "navigator.mozAlarms should exist");
> + ok(typeof navigator.mozAlarms === 'object',
> + "navigator.mozAlarms should return an object");
The interface AlarmsManager will be exposed on window, so this test can become |navigator.mozAlarms instanceof AlarmsManager| with the text changed accordingly.
::: dom/webidl/AlarmsManager.webidl
@@ +1,5 @@
> +/* -*- Mode: IDL; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/.
> + */
Although we can't add a "Origin of this IDL file is" due to there being no spec IDL, please add a link to https://wiki.mozilla.org/WebAPI/AlarmAPI as is done in other WebIDL files.
Attachment #8427657 -
Flags: superreview?(jst)
Attachment #8427657 -
Flags: review?(nsm.nikhil)
Attachment #8427657 -
Flags: review+
Assignee | ||
Comment 27•11 years ago
|
||
Updated based on Nikhil's comments.
Attachment #8427657 -
Attachment is obsolete: true
Attachment #8427657 -
Flags: superreview?(jst)
Attachment #8428477 -
Flags: superreview?(jst)
Attachment #8428477 -
Flags: review?(nsm.nikhil)
Reporter | ||
Comment 28•11 years ago
|
||
Comment on attachment 8428477 [details] [diff] [review]
New patch attempt 3
Review of attachment 8428477 [details] [diff] [review]:
-----------------------------------------------------------------
Looks good. For future reference, when a review has been granted, and the new patch only makes changes requested by the reviewer, a re-review is not required unless you've made other major changes.
Attachment #8428477 -
Flags: review?(nsm.nikhil) → review+
Updated•11 years ago
|
Attachment #8428477 -
Flags: superreview?(jst) → superreview+
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Comment 29•11 years ago
|
||
Your last Try run is showing failures in test_alarms.html. Please provide a Try link showing them to be resolved. Also, it would be nice if you tested test_alarms.html across all platforms (as we've certainly had our fair-share of platform specific bustage for these kinds of tests).
Keywords: checkin-needed
Assignee | ||
Comment 30•11 years ago
|
||
Replacing the former patch, which has passed review and super review, to resolve potential merge conflicts with the latest code base.
Attachment #8428477 -
Attachment is obsolete: true
Assignee | ||
Comment 31•11 years ago
|
||
The latest try run for this patch. FYI.
https://tbpl.mozilla.org/?tree=Try&rev=203a003c9aaa
Keywords: checkin-needed
Comment 32•11 years ago
|
||
We're not concerned with the Gaia test failure on that Try push?
https://tbpl.mozilla.org/php/getParsedLog.php?id=41022156&tree=Try
Keywords: checkin-needed
Comment 33•11 years ago
|
||
Or B2G test_interfaces.html?
https://tbpl.mozilla.org/php/getParsedLog.php?id=41022711&tree=Try
Reporter | ||
Comment 34•11 years ago
|
||
Sean, please don't set the checkin needed flag until the try run has passed. Needinfo me after the next try run before that.
Assignee | ||
Comment 35•11 years ago
|
||
Fixing the failed test case mentioned in comment 33.
Besides, for another failed Gaia unit test case mentioned in comment 32, I've filed bug 1021592 and made a correspondent patch for review. The final try-run might need to wait until the patch for that bug lands.
Attachment #8434606 -
Attachment is obsolete: true
Attachment #8435692 -
Flags: superreview?(jst)
Attachment #8435692 -
Flags: review?(nsm.nikhil)
Reporter | ||
Comment 36•11 years ago
|
||
Comment on attachment 8435692 [details] [diff] [review]
New patch attempt 5
Review of attachment 8435692 [details] [diff] [review]:
-----------------------------------------------------------------
I'm clearing jst's review request until the patch is updated.
::: dom/alarm/test/test_alarm_non_permitted_app.html
@@ +25,4 @@
> if (navigator.appVersion.indexOf("Android") != -1) {
> ok(!('mozAlarms' in navigator), "navigator.mozAlarms should not exist");
> } else {
> + ok(!('mozAlarms' in navigator),
Also add a test for interface non-exposure.
::: dom/alarm/test/test_alarm_permitted_app.html
@@ +22,2 @@
> if (navigator.appVersion.indexOf("Android") != -1) {
> + ok(true, "mozAlarms is not allowed on Android for now. TODO Bug 863557.");
Can you explicitly check |!(AlarmsManager in window)| instead of true? What is the true behaviour of alarms on Android when both the pref and the permission are set? Does add() throw or something? Whatever it is, please test for that.
That way we'll get a failing test when it's enabled for Android and people won't forget to refactor this.
::: dom/base/Navigator.cpp
@@ +2222,5 @@
> + win && CheckPermission(win, "alarms");
> +}
> +
> +/* static */
> +bool
Bug 952486 obsoleted this by just adding a webidl attribute. please see the patches there for examples of how to change the webidl.
::: dom/webidl/AlarmsManager.webidl
@@ +7,5 @@
> + */
> +
> +[NavigatorProperty="mozAlarms",
> + JSImplementation="@mozilla.org/alarmsManager;1",
> + Func="Navigator::HasAlarmsSupport"]
You'll have to change this to add CheckPermission="alarms" and remove the Func=
Attachment #8435692 -
Flags: superreview?(jst)
Attachment #8435692 -
Flags: review?(nsm.nikhil)
Attachment #8435692 -
Flags: review-
Assignee | ||
Comment 37•10 years ago
|
||
Updating based on Nikhil's comments.
And for test_alarm_permitted_app.html, |'mozAlarms' in navigator| throws a 0x80004005 (NS_ERROR_FAILURE) exception. So I tried to catch it and leave a todo message without making the test case failed.
Attachment #8435692 -
Attachment is obsolete: true
Attachment #8437499 -
Flags: review?(nsm.nikhil)
Reporter | ||
Comment 38•10 years ago
|
||
Comment on attachment 8437499 [details] [diff] [review]
New patch attempt 6
Review of attachment 8437499 [details] [diff] [review]:
-----------------------------------------------------------------
Can you post another try run with the following fixed?
::: dom/alarm/test/test_alarm_non_permitted_app.html
@@ +25,4 @@
> if (navigator.appVersion.indexOf("Android") != -1) {
> ok(!('mozAlarms' in navigator), "navigator.mozAlarms should not exist");
> + ok(!('AlarmsManager' in window),
> + "Interface AlarmsManager should not exist without permission");
you can remove the without permission bit.
::: dom/alarm/test/test_alarm_permitted_app.html
@@ +22,2 @@
> if (navigator.appVersion.indexOf("Android") != -1) {
> + ok('AlarmsManager' in window, "Interface AlarmsManager should exist");
The interface shouldn't be exposed if the API isn't. The Pref= will ensure that.
Trailing whitespace.
Attachment #8437499 -
Flags: review?(nsm.nikhil) → review+
Assignee | ||
Comment 39•10 years ago
|
||
(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #38)
> Comment on attachment 8437499 [details] [diff] [review]
> New patch attempt 6
>
> Review of attachment 8437499 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: dom/alarm/test/test_alarm_permitted_app.html
> @@ +22,2 @@
> > if (navigator.appVersion.indexOf("Android") != -1) {
> > + ok('AlarmsManager' in window, "Interface AlarmsManager should exist");
>
> The interface shouldn't be exposed if the API isn't. The Pref= will ensure
> that.
Actually the interface is exposed in this test case since |dom.mozAlarms.enabled| is explicitly set to true at the beginning, even though |Pref="dom.mozAlarms.enabled"| check has already been added to the correspondent webidl. And the following try-run across all platforms reflects the behavior stated above.
https://tbpl.mozilla.org/?tree=Try&rev=2767bbb066ff
Thoughts?!
Flags: needinfo?(nsm.nikhil)
Reporter | ||
Comment 40•10 years ago
|
||
Oh right, then only the trailing whitespace. r=me. Please ask for superreview.
Flags: needinfo?(nsm.nikhil)
Assignee | ||
Comment 41•10 years ago
|
||
Refining based on Nikhil's r+ comments. Pending for super review.
Attachment #8437499 -
Attachment is obsolete: true
Attachment #8439725 -
Flags: superreview?(jst)
Comment 42•10 years ago
|
||
Comment on attachment 8439725 [details] [diff] [review]
New patch attempt 7
diff --git a/dom/alarm/AlarmsManager.manifest b/dom/alarm/AlarmsManager.manifest
--- a/dom/alarm/AlarmsManager.manifest
+++ b/dom/alarm/AlarmsManager.manifest
@@ -1,3 +1,3 @@
component {fea1e884-9b05-11e1-9b64-87a7016c3860} AlarmsManager.js
contract @mozilla.org/alarmsManager;1 {fea1e884-9b05-11e1-9b64-87a7016c3860}
-category JavaScript-navigator-property mozAlarms @mozilla.org/alarmsManager;1
+
Remove the empty line here?
sr=jst
Attachment #8439725 -
Flags: superreview?(jst) → superreview+
Assignee | ||
Comment 43•10 years ago
|
||
Minor updates based on Johnny's sr+ comments.
Attachment #8439725 -
Attachment is obsolete: true
Assignee | ||
Comment 44•10 years ago
|
||
The latest try run FYI. https://tbpl.mozilla.org/?tree=Try&rev=240191af7812
Need-infoing Nikhil to get his thoughts on this.
Flags: needinfo?(nsm.nikhil)
Updated•10 years ago
|
Mentor: nsm.nikhil
Whiteboard: [good first bug][mentor=nsm] → [good first bug]
Reporter | ||
Comment 45•10 years ago
|
||
can you do another try push? Just to make sure some of those reds and oranges aren't due to this patch.
Flags: needinfo?(nsm.nikhil)
Assignee | ||
Comment 46•10 years ago
|
||
Replacing the tabs I misplaced in my code with white spaces.
The latest try run FYI. https://tbpl.mozilla.org/?tree=Try&rev=5fe19662192d
All the failed items were rerun and then succeeded. (Please note that test_networkstats_alarms.html might intermittently fail. It's a known testing issue on mozNetworkStats tracked in bug 958689.)
Attachment #8440600 -
Attachment is obsolete: true
Assignee | ||
Comment 47•10 years ago
|
||
Since it has passed all the items in try run, I'm inclined to directly go for 'checkin-needed'. Yet please still feel free to share your concerns if there's any.
Flags: needinfo?(nsm.nikhil)
Keywords: checkin-needed
Comment 48•10 years ago
|
||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 49•10 years ago
|
||
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•