Closed Bug 1014657 Opened 6 years ago Closed 6 years ago

Port DOMStorageEvent to WebIDL and remove nsIDOMStorageEvent

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: baku, Assigned: baku)

References

Details

Attachments

(1 file, 1 obsolete file)

This is useful for porting DOM Storage to WebIDL.
Attached patch storageEvent.patch (obsolete) — Splinter Review
Attachment #8427101 - Flags: review?(Ms2ger)
Blocks: 660237
OS: Linux → All
Hardware: x86_64 → All
Comment on attachment 8427101 [details] [diff] [review]
storageEvent.patch

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

::: dom/base/nsGlobalWindow.cpp
@@ +11321,5 @@
>  }
>  
>  nsresult
>  nsGlobalWindow::CloneStorageEvent(const nsAString& aType,
> +                                  nsRefPtr<StorageEvent>& aEvent)

Weird API... I think you could take StorageEvent* and return already_AddRefed<StorageEvent>, and that'd be clearer

@@ +11330,3 @@
>  
>    nsCOMPtr<nsIDOMEvent> domEvent = do_QueryInterface(aEvent, &rv);
>    NS_ENSURE_SUCCESS(rv, rv);

I don't think you need this

@@ +11330,5 @@
>  
>    nsCOMPtr<nsIDOMEvent> domEvent = do_QueryInterface(aEvent, &rv);
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> +  domEvent->GetBubbles(&dict.mBubbles);

Bubbles(), Cancelable()

@@ +11339,5 @@
> +  aEvent->GetNewValue(dict.mNewValue);
> +  aEvent->GetUrl(dict.mUrl);
> +  dict.mStorageArea = aEvent->GetStorageArea();
> +
> +  aEvent = StorageEvent::Constructor(this, aType, dict);

Can't this throw?

::: dom/events/EventDispatcher.cpp
@@ -831,5 @@
>    if (aEventType.LowerCaseEqualsLiteral("mozsmsevent"))
>      return NS_NewDOMMozSmsEvent(aDOMEvent, aOwner, aPresContext, nullptr);
>    if (aEventType.LowerCaseEqualsLiteral("mozmmsevent"))
>      return NS_NewDOMMozMmsEvent(aDOMEvent, aOwner, aPresContext, nullptr);
> -  if (aEventType.LowerCaseEqualsLiteral("storageevent")) {

Need to ask smaug about this
Attachment #8427101 - Flags: review?(bugs)
Attachment #8427101 - Flags: review?(Ms2ger)
Attachment #8427101 - Flags: feedback+
So this will break at least https://mxr.mozilla.org/addons/source/162068/chrome/dataman/content/dataman/dataman.js#2602
And I wouldn't be surprised if some binary addons use the .idl interface too.
Comment on attachment 8427101 [details] [diff] [review]
storageEvent.patch

But I think this should be ok. Binary addons just need to use JS for this stuff if needed.

blink doesn't seem to support createEvent("storageevent") either, so removing
that is hopefully ok.

This is risky, but not sure what we could do to make it less risky.
Attachment #8427101 - Flags: review?(bugs) → review+
(and please use Bubbles(), Cancelable() )
https://hg.mozilla.org/mozilla-central/rev/f2b9e7e71113
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Depends on: 1016053
Could we back this out and have a patch which supports createEvent. Apparently it is needed after all -
at least for now.
Perhaps worth to re-check whether other engines have init*Event.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.