Closed Bug 901114 Opened 11 years ago Closed 10 years ago

Convert Alarm API to WebIDL

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla33

People

(Reporter: nsm, Assigned: selin, Mentored)

References

Details

(Whiteboard: [good first bug])

Attachments

(1 file, 12 obsolete files)

New patch attempt 8 (no tab)
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)

    
    

    
  
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 :)
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

    
    

    
  
(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 ;)

    
    

    
  
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?

    
    

    
  


  

        Attachment #806023 -
        Attachment is obsolete: true

    
    

    
  
(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.

    
    

    
  


  

        Attachment #810517 -
        Attachment is obsolete: true

    
    

    
  
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?

    
    

    
  
(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.)

    
    

    
  
(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.

    
    

    
  
(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

    
    

    
  
(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.

    
    

    
  
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.

    
    

    
  
(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.

    
    

    
  
>  ok(navigator.mozAlarms.toString(), "[object AlarmManager]",
   is(navigator.mozAlarms.toString(), "[object AlarmManager]",
, of course. Sorry for the bugspam.

    
    

    
  
(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.

    
    

    
  
(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.
Assignee: i → nobody

    
    

    
  
Fixing this before bug 983502 means that MarketPlace's feature detection for this API will break.
Depends on: 983502

    
  
Blocks: 981845

    
  
Depends on: 1009645

    
  
No longer depends on: 983502
Assignee: nobody → selin

    
    

    
  

      
      
      
      

        Attached patch
          
          
        New patch attempt 1 (obsolete)
        — 
          — Splinter Review
      

    


  
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)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        New patch attempt 2 (obsolete)
        — 
          — Splinter Review
      

    


  
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)

    
    

    
  
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+

    
    

    
  

      
      
      
      

        Attached patch
          
          
        New patch attempt 3 (obsolete)
        — 
          — Splinter Review
      

    


  
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)

    
    

    
  
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+

        Attachment #8428477 -
        Flags: superreview?(jst) → superreview+

    
    

    
  
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

    
    

    
  

      
      
      
      

        Attached patch
          
          
        New patch attempt 4 (obsolete)
        — 
          — Splinter Review
      

    


  
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

    
    

    
  
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

    
    

    
  
Sean, please don't set the checkin needed flag until the try run has passed. Needinfo me after the next try run before that.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        New patch attempt 5 (obsolete)
        — 
          — Splinter Review
      

    


  
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)

    
    

    
  
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-
Depends on: 1021592

    
    

    
  

      
      
      
      

        Attached patch
          
          
        New patch attempt 6 (obsolete)
        — 
          — Splinter Review
      

    


  
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)

    
    

    
  
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+

    
    

    
  
(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)

    
    

    
  
Oh right, then only the trailing whitespace. r=me. Please ask for superreview.
Flags: needinfo?(nsm.nikhil)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        New patch attempt 7 (obsolete)
        — 
          — Splinter Review
      

    


  
Refining based on Nikhil's r+ comments. Pending for super review.

        Attachment #8437499 -
        Attachment is obsolete: true

        Attachment #8439725 -
        Flags: superreview?(jst)

    
    

    
  
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+

    
    

    
  

      
      
      
      

        Attached patch
          
          
        New patch attempt 8 (obsolete)
        — 
          — Splinter Review
      

    


  
Minor updates based on Johnny's sr+ comments.

        Attachment #8439725 -
        Attachment is obsolete: true

    
    

    
  
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)
Mentor: nsm.nikhil
Whiteboard: [good first bug][mentor=nsm] → [good first bug]

    
    

    
  
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)

    
    

    
  


  
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

    
    

    
  
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

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/9acf8e23414d
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla33

    
    

    
  
Nice job!
Flags: needinfo?(nsm.nikhil)
Component: DOM → DOM: Core & HTML

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: