1014657 - Port DOMStorageEvent to WebIDL and remove nsIDOMStorageEvent

Status: RESOLVED FIXED

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

Description

[Andrea Marchesini [:baku]]

This is useful for porting DOM Storage to WebIDL.

Comment 1

[Andrea Marchesini [:baku]]

Attachment: storageEvent.patch

Comment 2

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

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 4

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

(and please use Bubbles(), Cancelable() )

Comment 6

[Andrea Marchesini [:baku]]

Attachment: storageEvent.patch

https://tbpl.mozilla.org/?tree=Try&rev=d909a5893202

Comment 7

[Andrea Marchesini [:baku]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f2b9e7e71113

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f2b9e7e71113

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

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.