Porting DesktopNotification to WebIDL

RESOLVED FIXED in mozilla23

Status

()

defect
RESOLVED FIXED
7 years ago
5 months ago

People

(Reporter: baku, Assigned: baku)

Tracking

(Blocks 1 bug)

Trunk
mozilla23
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

No description provided.
Posted patch part 1 - renaming (obsolete) — Splinter Review
Attachment #734210 - Flags: review?(Ms2ger)
Posted patch part 2 - webidl (obsolete) — Splinter Review
Attachment #734211 - Flags: review?(Ms2ger)
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.
Attachment #734210 - Flags: review?(Ms2ger) → review+
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]
Attachment #734211 - Flags: review?(Ms2ger)
Attachment #734210 - Attachment is obsolete: true
Attachment #734270 - Flags: review+
Posted patch part 2 - webidl (obsolete) — Splinter Review
Attachment #734211 - Attachment is obsolete: true
Attachment #734271 - Flags: review?(Ms2ger)
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
Posted patch part 2 - webidl (obsolete) — Splinter Review
Attachment #734271 - Attachment is obsolete: true
Attachment #734271 - Flags: review?(Ms2ger)
Attachment #734280 - Flags: review?(Ms2ger)
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.
Attachment #734280 - Flags: review?(Ms2ger) → review+
Attachment #734280 - Attachment is obsolete: true
Attachment #735717 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/64d2859ca6b1
https://hg.mozilla.org/mozilla-central/rev/9f92f6c2d0de
https://hg.mozilla.org/mozilla-central/rev/78e13e815d62
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.