Bug 163967 – Regression(r203848): 百度糯米 app fails to load content due to a JavaScript error

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Chris Dumez]

Making all Event.initEvent() parameters mandatory caused some breakage. There are some iOS apps passing only the first parameter (event type). Since this is a legacy API, I don't think it is worth the trouble and I propose we scale back the change to mark only the first parameter as mandatory.

[Chris Dumez]

<rdar://problem/28707838>

[Chris Dumez]

Created attachment 292808 [details]
Patch

[Darin Adler]

Comment on attachment 292808 [details]
Patch

OK. Don’t want to use quirk for old apps that we can remove some day?

[Chris Dumez]

(In reply to comment #3)
> Comment on attachment 292808 [details]
> Patch
> 
> OK. Don’t want to use quirk for old apps that we can remove some day?

Hmm, I personally think making the 2 booleans mandatory does not result in a nicer API. Those 2 boolean parameters are expected to be false by default when using the constructor. The one that was bothering me was the first one because it did not make sense to use "undefined" as default event type.

[Darin Adler]

Can we get the specification changed?

[Chris Dumez]

(In reply to comment #5)
> Can we get the specification changed?

We could if it broke actual sites. I don't think breaking engine-specific apps or extensions would be enough to get the spec changed, especially considering Blink and Gecko were able to do this.

[Chris Dumez]

(In reply to comment #6)
> (In reply to comment #5)
> > Can we get the specification changed?
> 
> We could if it broke actual sites. I don't think breaking engine-specific
> apps or extensions would be enough to get the spec changed, especially
> considering Blink and Gecko were able to do this.

One thing we could do to help steer in the right direction would be to log a console warning when calling initEvent() with only 1 parameter so that app developers are more likely to update their code. thoughts?

[Darin Adler]

I just hate having our engine fail a test suite. A solution that would cause us to pass the test suite and still keep the apps working would make me happy.

[Chris Dumez]

(In reply to comment #8)
> I just hate having our engine fail a test suite. A solution that would cause
> us to pass the test suite and still keep the apps working would make me
> happy.

So, then I think we would have to:
- Mark the 2 last parameters as optional in the IDL (or add an overload taking only 1 parameter)
- Update the implementation to detect broken apps, and use default parameter values if a broken app is detected. Otherwise, throw a TypeError from the implementation.

We would still fail some W3C checks because Event.prototype.initEvent.length would be 1 instead of 3. I guess we could add an IDL extended attribute (or special casing for initEvent in the bindings generator) so that length keeps returning 3.

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

As long as your implementation doesn't match the spec, I'll ensure it fails wpt tests, so don't bother adding workarounds for the existing tests :)

[Chris Dumez]

(In reply to comment #10)
> As long as your implementation doesn't match the spec, I'll ensure it fails
> wpt tests, so don't bother adding workarounds for the existing tests :)

Not sure I understand your comment. Our implementation matches the specification and we are talking about maintaining our standard behavior for the Web, merely introducing a quirk for a specific iOS app until its developers have a chance to fix their bug.

[Chris Dumez]

Created attachment 292942 [details]
Patch

[Chris Dumez]

Created attachment 292948 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 292948 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=292948&action=review

I've got a feeling that we might need to make the spec change to make this parameter optional instead.

> Source/WebCore/dom/Event.cpp:96
> +    // FIXME: Temporary quirk for ç¾åº¦ç³¯ç±³ App which calls initEvent() with too few parameters (rdar://problem/28707838).

Can we just use ASCII names like Baidu Nuomi App in the comment?

> Source/WebCore/platform/RuntimeApplicationChecks.mm:258
> +    // ç¾åº¦ç³¯ç±³ app.

Ditto.

[Chris Dumez]

Committed r207908: <http://trac.webkit.org/changeset/207908>

[Darin Adler]

Another part of our standard practice for this is to use "linked on or after" to make sure this quirk only works for versions of apps not yet updated with the SDK for a new release of iOS. Should we do that?