Comment on attachment 734210 [details] [diff] [review] part 1 - renaming Review of attachment 734210 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/interfaces/notification/nsIDOMNavigatorDesktopNotification.idl @@ +9,5 @@ > */ > [scriptable, uuid(EC2E6E4F-2F65-439C-B6C6-27E89B03B348)] > interface nsIDOMNavigatorDesktopNotification : nsISupports > { > + readonly attribute nsISupports mozNotification; Move this to the next patch.

Comment on attachment 734211 [details] [diff] [review] part 2 - webidl Review of attachment 734211 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, but I'd like to see another version ::: dom/src/notification/DesktopNotification.cpp @@ +78,4 @@ > NS_INTERFACE_MAP_END_INHERITING(nsDOMEventTargetHelper) > > NS_IMPL_ADDREF_INHERITED(DesktopNotification, nsDOMEventTargetHelper) > NS_IMPL_RELEASE_INHERITED(DesktopNotification, nsDOMEventTargetHelper) Drop the QI/AddRef/Release implementation. @@ +199,5 @@ > { > mShowHasBeenCalled = true; > > if (!mAllow) > + return; {} @@ +232,4 @@ > > + nsString iconURL; > + if (aIconURL.WasPassed()) { > + iconURL.Assign(aIconURL.Value()); So not passing the argument is equivalent to passing the empty string? Move that into the IDL, please. ::: dom/src/notification/DesktopNotification.h @@ +8,3 @@ > #include "PCOMContentPermissionRequestChild.h" > > #include "nsDOMClassInfoID.h" Can you drop this? @@ +56,5 @@ > // Grab the uri of the document > nsCOMPtr<nsIDOMDocument> domdoc; > mOwner->GetDocument(getter_AddRefs(domdoc)); > nsCOMPtr<nsIDocument> doc = do_QueryInterface(domdoc); > mPrincipal = doc->NodePrincipal(); Bonus points for replacing these four lines by mPrincipal = mOwner->GetDoc()->NodePrincipal(); @@ +93,5 @@ > { > friend class DesktopNotificationRequest; > > public: > NS_DECL_ISUPPORTS_INHERITED Drop this @@ +94,5 @@ > friend class DesktopNotificationRequest; > > public: > NS_DECL_ISUPPORTS_INHERITED > + NS_FORWARD_NSIDOMEVENTTARGET(nsDOMEventTargetHelper::) And this @@ +96,5 @@ > public: > NS_DECL_ISUPPORTS_INHERITED > + NS_FORWARD_NSIDOMEVENTTARGET(nsDOMEventTargetHelper::) > + NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED(DesktopNotification, > + nsDOMEventTargetHelper) And this @@ +128,5 @@ > + // WebIDL > + > + nsPIDOMWindow* GetParentObject() const > + { > + return mOwner; Return GetOwner() @@ +145,5 @@ > nsString mDescription; > nsString mIconURL; > > nsRefPtr<AlertServiceObserver> mObserver; > + nsCOMPtr<nsPIDOMWindow> mOwner; You shouldn't add this, nsDOMEventTargetHelper already has an owner. ::: dom/webidl/DesktopNotification.webidl @@ +7,5 @@ > +interface MozObserver; > + > +interface DesktopNotificationCenter > +{ > + DesktopNotification createNotification(DOMString title, [Creator]

Comment on attachment 734271 [details] [diff] [review] part 2 - webidl Review of attachment 734271 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/interfaces/notification/nsIDOMNavigatorDesktopNotification.idl @@ +9,5 @@ > */ > [scriptable, uuid(EC2E6E4F-2F65-439C-B6C6-27E89B03B348)] > interface nsIDOMNavigatorDesktopNotification : nsISupports > { > + readonly attribute nsISupports mozNotification; Nit: add a /* DesktopNotificationCenter */ around here ::: dom/src/notification/DesktopNotification.cpp @@ +229,3 @@ > > + nsString iconURL; > + if (aIconURL.WasPassed()) { 'optional iconURL = ""' in the IDL... @@ +244,5 @@ > > +JSObject* > +DesktopNotificationCenter::WrapObject(JSContext* aCx, JSObject* aScope) > +{ > + return DesktopNotificationCenterBinding::Wrap(aCx, aScope, this); You're not including anything for this?

> You're not including anything for this? No. DesktopNotificationCenterBinding is defined in DesktopNotificationBinding.h

Comment on attachment 734280 [details] [diff] [review] part 2 - webidl Review of attachment 734280 [details] [diff] [review]: ----------------------------------------------------------------- lgtm ::: dom/src/notification/DesktopNotification.cpp @@ +271,4 @@ > > nsCOMPtr<nsIDOMWindow> window = > do_QueryInterface(mDesktopNotification->GetOwner()); > NS_IF_ADDREF(*aRequestingWindow = window); While you're here, this could just be NS_IF_ADDREF(*aRequestingWindow = mDesktopNotification->GetOwner()); ::: dom/src/notification/DesktopNotification.h @@ +72,5 @@ > + } > + > + virtual JSObject* WrapObject(JSContext* aCx, JSObject* aScope) MOZ_OVERRIDE; > + > + // Mark this as resultNotAddRefed to return raw pointers Drop this comment.

https://hg.mozilla.org/integration/mozilla-inbound/rev/64d2859ca6b1 https://hg.mozilla.org/integration/mozilla-inbound/rev/9f92f6c2d0de

Follow-up for B2G bustage. http://hg.mozilla.org/integration/mozilla-inbound/rev/78e13e815d62

https://hg.mozilla.org/mozilla-central/rev/64d2859ca6b1 https://hg.mozilla.org/mozilla-central/rev/9f92f6c2d0de https://hg.mozilla.org/mozilla-central/rev/78e13e815d62