Closed
Bug 1016053
Opened 10 years ago
Closed 10 years ago
document.createEvent("StorageEvent") is not supported
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla32
People
(Reporter: markus.popp, Assigned: baku)
References
Details
(Keywords: regression)
Attachments
(2 files, 2 obsolete files)
123.30 KB,
image/png
|
Details | |
13.87 KB,
patch
|
Details | Diff | Splinter Review |
In the latest Firefox Nightly (2014-05-26), one of the Web Storage Ring 0 Ringmark tests fails, see screenshot. No error in Aurora, Beta & Release.
Updated•10 years ago
|
Keywords: regressionwindow-wanted
Comment 1•10 years ago
|
||
Regression window(m-i) Good: https://hg.mozilla.org/integration/mozilla-inbound/rev/9c9833f16a62 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:32.0) Gecko/20100101 Firefox/32.0 ID:20140522233819 Bad: https://hg.mozilla.org/integration/mozilla-inbound/rev/f2b9e7e71113 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:32.0) Gecko/20100101 Firefox/32.0 ID:20140523003424 Pushlog: http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=9c9833f16a62&tochange=f2b9e7e71113 Regressed by: f2b9e7e71113 Andrea Marchesini — Bug 1014657 - Port DOMStorageEvent to WebIDL and remove nsIDOMStorageEvent, r=smaug, f=ms2ger
Blocks: 1014657
Keywords: regressionwindow-wanted → regression
Updated•10 years ago
|
tracking-firefox32:
--- → ?
The relevant code is at https://github.com/facebook/rng.io/blob/master/tests/webstorage/test.js#L51, which tries to do XPConnect specific hacks that no longer apply.
Summary: Ringmark test (Ring 0, Web Storage) fails → document.createEvent("StorageEvent") is not supported
Comment 3•10 years ago
|
||
Hmm. document.createEvent("StorageEvent") used to work. Why does it not now? Note also that per DOM spec at http://dom.spec.whatwg.org/#dom-document-createevent it shouldn't work. Presumably that spec needs to be fixed.
Flags: needinfo?(bugs)
Flags: needinfo?(annevk)
Comment 4•10 years ago
|
||
But yes, the hackery in that test is a bit confusing to me...
Comment 5•10 years ago
|
||
We should fix it because a test relies on it? I guess we could just add anything we come across and vow not to add anything more... All the additional init*Event() methods kinda suck.
Flags: needinfo?(annevk)
Comment 6•10 years ago
|
||
Ringmark being crap as usual... Rick, you've got blame for this, please fix the test.
Flags: needinfo?(waldron.rick)
Comment 7•10 years ago
|
||
> We should fix it because a test relies on it?
This test _claims_ to be only testing things people actually care about in real life, last I checked.
Comment 8•10 years ago
|
||
Note that StorageEvent in HTML does not define an inti*Event() method.
Comment 9•10 years ago
|
||
This test needs a `storageevent` instance to verify some of the event's properties. The way it's generated is sketchy (and possibly not spec-compliant) and is the part of the code that is erroring out now. I would suggest modifying the test so that it generates a `storageevent` through actual localstorage changes.
Comment 10•10 years ago
|
||
Test could also use new StorageEvent().
Comment 11•10 years ago
|
||
Hmm perhaps it was too risky to remove createEvent support for StorageEvent. Apparently blink does have it after all, it is just strict with case handling, unlink gecko or trident. I think we should backout bug 1014657 for now, and then have a bit different patch for it.
Flags: needinfo?(bugs)
Assignee | ||
Comment 12•10 years ago
|
||
We can introduce it quickly without backout that bug. How does it sound?
Comment 13•10 years ago
|
||
Sounds fine too, but please re-test whether other engines have initStorageEvent.
Assignee | ||
Comment 14•10 years ago
|
||
Attachment #8429207 -
Flags: review?(bugs)
Comment 15•10 years ago
|
||
> This test needs a `storageevent` instance
Why? The tests it performs could just as well be performed on StorageEvent.prototype per spec. Of course WebKit doesn't follow the spec...
Comment 16•10 years ago
|
||
It seems this test is verifying that the event object passed to the event handler contains a number of properties. To the end developer, what generally matters is whether these props are available on the event object and have correct values. Whether these are set by the constructor or are already present on the prototype is usually not particularly important for common use cases.
Comment 17•10 years ago
|
||
(In reply to :Ms2ger from comment #6) > Ringmark being crap as usual... Rick, you've got blame for this, please fix > the test. Wow, aren't you pleasant—a true representative of practitioners.
Flags: needinfo?(waldron.rick)
Comment 18•10 years ago
|
||
To make sure I get this right, I'll explain what the test is looking for and then this thread can advise the best way to do that test. The "Storage Events in Practice" test simply wants to confirm that StorageEvent objects contain these three properties: "key", "oldValue", "newValue". We didn't want to take the path of actually storing something, as that may have unwanted side effects*, which is why we create an object or look at the StorageEvent.prototype. When these were written, the following was true: - Chrome WebKit has a StorageEvent constructor - Mobile Safari WebKit has a StorageEvent object that needs to be created - Firefox has a StorageEvent prototype object * Originally we had a test that confirmed that those properties behaved correctly by actually storing something, but we discarded this for the same reason.
Comment 19•10 years ago
|
||
>- Firefox has a StorageEvent prototype object
What does that mean, exactly?
Current Firefox has a StorageEvent constructor (in the sense that you can |new StorageEvent|). There is also a StorageEvent.prototype, obviously. Very old Firefox didn't have a constructible StorageEvent; I agree you needed createEvent there.
What I'm not sure about is why the test insists on creating StorageEvent objects via createEvent no matter what. Seems to me like the right thing to do would be:
try {
event = new StorageEvent("something");
} catch (e) {
// Fall back on createEvent for browsers that don't follow the current spec, like
// really old Firefox
}
or something. You could check typeof StorageEvent instead of try/catch, I guess; certainly in Firefox if that's "function" then you can construct with new.
Comment 20•10 years ago
|
||
And in particular, in old Firefox the test ended up doing event = StorageEvent.prototype; and leaving it like that, afaict. So it wasn't even testing StorageEvent instances there...
Comment 21•10 years ago
|
||
Comment on attachment 8429207 [details] [diff] [review] se.patch > if (aEventType.LowerCaseEqualsLiteral("mozmmsevent")) > return NS_NewDOMMozMmsEvent(aDOMEvent, aOwner, aPresContext, nullptr); >+ if (aEventType.LowerCaseEqualsLiteral("storageevent")) { >+ StorageEventInit dict; >+ nsRefPtr<StorageEvent> event = >+ StorageEvent::Constructor(aOwner, NS_LITERAL_STRING("storage"), dict); >+ event.forget(aDOMEvent); >+ return NS_OK; >+ } Event created using .createEvent shouldn't have .type set. Could you perhaps add similar old style NS_New* method what those other classes have.
Attachment #8429207 -
Flags: review?(bugs) → review-
Assignee | ||
Comment 22•10 years ago
|
||
Attachment #8429207 -
Attachment is obsolete: true
Attachment #8429421 -
Flags: review?(bugs)
Comment 23•10 years ago
|
||
(In reply to Boris Zbarsky [:bz] from comment #19) > >- Firefox has a StorageEvent prototype object > > What does that mean, exactly? > > Current Firefox has a StorageEvent constructor (in the sense that you can > |new StorageEvent|). There is also a StorageEvent.prototype, obviously. > Very old Firefox didn't have a constructible StorageEvent; I agree you > needed createEvent there. > > What I'm not sure about is why the test insists on creating StorageEvent > objects via createEvent no matter what. No one is "insisting" on anything. The code in that test is written around the three items I included in my previous message—that was also in early 2012. > Seems to me like the right thing to > do would be: > > try { > event = new StorageEvent("something"); > } catch (e) { > // Fall back on createEvent for browsers that don't follow the current > spec, like > // really old Firefox > } I can assure you this was the _first_ thing we tried for _every_ constructor. Please, give me the benefit of the doubt here. > > or something. You could check typeof StorageEvent instead of try/catch, I > guess; certainly in Firefox if that's "function" then you can construct with > new. That's great news.
Comment 24•10 years ago
|
||
> I can assure you this was the _first_ thing we tried for _every_ constructor.
Can you explain how it fails in the case of this test, exactly? Presumably the UAs where it fails have (typeof StorageEvent != "function") test true and either (typeof StorageEvent != "object") or ("key" in StorageEvent.prototype) test true. Were there UAs like that in 2012? Certainly Firefox wasn't such a UA back then (and incidentally we've had a constructible StorageEvent since January 2012 on trunk, June 2012 in releases...).
Comment 25•10 years ago
|
||
Also, all that is somewhat beside the point. The real questions here are: 1) Is the test going to continue requiring that createEvent work for this type of event in UAs where StorageEvent is a function? 2) What should the DOM spec say here. That is, what are the actual web compat requirements if we ignore the test?
Comment 26•10 years ago
|
||
(In reply to Boris Zbarsky [:bz] from comment #25) > 2) What should the DOM spec say here. That is, what are the actual web > compat requirements if we ignore the test? Because the answer to this is unclear and we'll have the next uplift pretty soon, I'd like to take baku's patch.
Comment 27•10 years ago
|
||
Comment on attachment 8429421 [details] [diff] [review] se.patch >+StorageEvent::Constructor(EventTarget* aOwner, >+ nsIDOMEvent** aDOMEvent) >+{ Please add NS_New*Event, and not this kind of ctor - for the sake of consistency with other events.
Attachment #8429421 -
Flags: review?(bugs) → review+
Comment 28•10 years ago
|
||
(In reply to Boris Zbarsky [:bz] from comment #25) > Also, all that is somewhat beside the point. The real questions here are: > > 1) Is the test going to continue requiring that createEvent work for this > type of event in UAs where StorageEvent is a function? Here's the real, unfiltered truth: we were instructed to take whatever steps were necessary to make Firefox and Opera look as good as possible in those tests so that some kind of partnership might blossom. It's not important _who_ gave that instruction, just accept that it happened. I'm probably out of line saying this, but 2 years later it's embarrassing that we even bothered to jump through these silly hoops; but those are hoops that web devs generally have to jump through (not that specifically, but hopefully you understand). All browsers should've been made to fail if `new StorageEvent()` didn't produce a storage event object. To answer your question: no, it should not require createEvent to work in UAs where StorageEvent is a function. > > 2) What should the DOM spec say here. That is, what are the actual web > compat requirements if we ignore the test? The test is going to be corrected. I don't think Firefox should make any changes if it's doing the correct thing.
Comment 29•10 years ago
|
||
Rick, I'm not trying to attack you here. I'm trying to figure out whether there are compat constraints other than "old Firefox" on the test code at this point. If there aren't and the test is going to get changed, so much the better. ;)
Assignee | ||
Comment 30•10 years ago
|
||
> Please add NS_New*Event, and not this kind of ctor -
> for the sake of consistency with other events.
Bz, the patch is almost ready, do we want to land it or we want to keep the current behaviour?
Flags: needinfo?(bzbarsky)
Assignee | ||
Comment 31•10 years ago
|
||
Attachment #8429421 -
Attachment is obsolete: true
Comment 32•10 years ago
|
||
I think smaug is right and we want to restore the createEvent bit for now.
Flags: needinfo?(bzbarsky)
Assignee | ||
Comment 33•10 years ago
|
||
Green on try: https://tbpl.mozilla.org/?tree=Try&rev=bfb9639fe1f1 m-i tree is closed: checkin-needed.
Keywords: checkin-needed
Comment 35•10 years ago
|
||
Backed out for debug bustage. https://hg.mozilla.org/integration/mozilla-inbound/rev/450ca9dc47e6 https://tbpl.mozilla.org/php/getParsedLog.php?id=40570913&tree=Mozilla-Inbound
Assignee | ||
Comment 36•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=81c164ccb54d I try again on try.
Assignee | ||
Comment 37•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=79a7198ce7d7 https://hg.mozilla.org/integration/mozilla-inbound/rev/498162161226
Updated•10 years ago
|
https://hg.mozilla.org/mozilla-central/rev/498162161226
Status: NEW → RESOLVED
Closed: 10 years ago
status-firefox32:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•