Bug 179591 – Event improvements

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Darin Adler]

Event improvements

[Darin Adler]

Created attachment 326711 [details]
Patch

[Darin Adler]

Created attachment 326724 [details]
Patch

[Build Bot]

Attachment 326724 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/KeyboardEvent.h:40:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/dom/KeyboardEvent.h:41:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/dom/KeyboardEvent.h:42:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/dom/KeyboardEvent.h:43:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 4 in 99 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 326724 [details]
Patch

Attachment 326724 [details] did not pass bindings-ews (mac):
Output: http://webkit-queues.webkit.org/results/5203664

New failing tests:
(JS) JSTestGlobalObject.cpp
(JS) JSTestNamedGetterCallWith.cpp
(JS) JSTestObj.cpp

[Build Bot]

Comment on attachment 326724 [details]
Patch

Attachment 326724 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/5203807

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 326726 [details]
Archive of layout-test-results from ews101 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 326724 [details]
Patch

Attachment 326724 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/5203817

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 326727 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 326724 [details]
Patch

Attachment 326724 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/5203820

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 326728 [details]
Archive of layout-test-results from ews114 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Darin Adler]

Created attachment 331285 [details]
Patch

[EWS Watchlist]

Attachment 331285 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:829:  out_wasThrown is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:829:  out_savedResultIndex is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/dom/KeyboardEvent.h:40:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/dom/KeyboardEvent.h:41:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/dom/KeyboardEvent.h:42:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/dom/KeyboardEvent.h:43:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 6 in 106 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EWS Watchlist]

Comment on attachment 331285 [details]
Patch

Attachment 331285 [details] did not pass bindings-ews (mac):
Output: http://webkit-queues.webkit.org/results/6062987

New failing tests:
(JS) JSTestGlobalObject.cpp
(JS) JSTestNamedGetterCallWith.cpp
(JS) JSTestObj.cpp

[Darin Adler]

Created attachment 331286 [details]
Patch

[EWS Watchlist]

Attachment 331286 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:829:  out_wasThrown is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:829:  out_savedResultIndex is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/dom/KeyboardEvent.h:40:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/dom/KeyboardEvent.h:41:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/dom/KeyboardEvent.h:42:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/dom/KeyboardEvent.h:43:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 6 in 107 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EWS Watchlist]

Comment on attachment 331286 [details]
Patch

Attachment 331286 [details] did not pass bindings-ews (mac):
Output: http://webkit-queues.webkit.org/results/6063091

New failing tests:
(JS) JSTestGlobalObject.cpp
(JS) JSTestNamedGetterCallWith.cpp
(JS) JSTestObj.cpp

[Darin Adler]

Created attachment 331287 [details]
Patch

[EWS Watchlist]

Attachment 331287 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:829:  out_wasThrown is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:829:  out_savedResultIndex is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/dom/KeyboardEvent.h:40:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/dom/KeyboardEvent.h:41:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/dom/KeyboardEvent.h:42:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/dom/KeyboardEvent.h:43:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 6 in 112 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Created attachment 331288 [details]
Patch

[EWS Watchlist]

Attachment 331288 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:829:  out_wasThrown is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:829:  out_savedResultIndex is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
WARNING: File exempt from style guide. Skipping: "Source/WebKit/WebProcess/InjectedBundle/API/gtk/DOM/WebKitDOMWheelEvent.cpp"
ERROR: Source/WebCore/dom/KeyboardEvent.h:40:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/dom/KeyboardEvent.h:41:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/dom/KeyboardEvent.h:42:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/dom/KeyboardEvent.h:43:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 6 in 114 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Created attachment 331289 [details]
Patch

[EWS Watchlist]

Attachment 331289 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:829:  out_wasThrown is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:829:  out_savedResultIndex is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
WARNING: File exempt from style guide. Skipping: "Source/WebKit/WebProcess/InjectedBundle/API/gtk/DOM/WebKitDOMWheelEvent.cpp"
ERROR: Source/WebCore/dom/KeyboardEvent.h:40:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/dom/KeyboardEvent.h:41:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/dom/KeyboardEvent.h:42:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/dom/KeyboardEvent.h:43:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 6 in 115 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EWS Watchlist]

Comment on attachment 331289 [details]
Patch

Attachment 331289 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/6063907

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 331290 [details]
Archive of layout-test-results from ews101 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 331289 [details]
Patch

Attachment 331289 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/6063917

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 331291 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 331289 [details]
Patch

Attachment 331289 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/6063936

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 331292 [details]
Archive of layout-test-results from ews117 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Darin Adler]

Created attachment 331294 [details]
Patch

[EWS Watchlist]

Comment on attachment 331294 [details]
Patch

Attachment 331294 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/6064822

New failing tests:
fast/events/constructors/message-event-constructor.html
imported/w3c/web-platform-tests/html/webappapis/scripting/events/messageevent-constructor.https.html

[EWS Watchlist]

Created attachment 331295 [details]
Archive of layout-test-results from ews100 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 331294 [details]
Patch

Attachment 331294 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/6064870

New failing tests:
fast/events/constructors/message-event-constructor.html
imported/w3c/web-platform-tests/html/webappapis/scripting/events/messageevent-constructor.https.html

[EWS Watchlist]

Created attachment 331297 [details]
Archive of layout-test-results from ews115 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 331294 [details]
Patch

Attachment 331294 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/6065416

New failing tests:
fast/events/constructors/message-event-constructor.html
imported/w3c/web-platform-tests/html/webappapis/scripting/events/messageevent-constructor.https.html

[EWS Watchlist]

Created attachment 331300 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[Darin Adler]

Created attachment 331305 [details]
Patch

[EWS Watchlist]

Attachment 331305 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:829:  out_wasThrown is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:829:  out_savedResultIndex is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
WARNING: File exempt from style guide. Skipping: "Source/WebKit/WebProcess/InjectedBundle/API/gtk/DOM/WebKitDOMWheelEvent.cpp"
ERROR: Source/WebCore/dom/KeyboardEvent.h:40:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/dom/KeyboardEvent.h:41:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/dom/KeyboardEvent.h:42:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/dom/KeyboardEvent.h:43:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 6 in 120 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Hooray, all platforms are compiling now and all tests are passing.

I hope that someone will review now and either ask me to cut this down and land it in smaller pieces, or approve this for landing.

[Darin Adler]

Chris, you might be the best reviewer for this. When you have a chance I’d love to hear your thoughts. No real rush, but I did get this working well three weeks ago, so it’s ready for you when you have some time.

[Chris Dumez]

Comment on attachment 331305 [details]
Patch

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

r=me

> Source/WebCore/bindings/js/JSValueInWrappedObject.h:36
> +// FIXME: The following uses of JSC::Strong are incorrect, can lead to storage leaks

We may want those comments to be in those classes. Otherwise, it is easy to forget removing them when those classes get updated (since we presumably wouldn't have to update this file).

> Source/WebCore/bindings/js/JSValueInWrappedObject.h:48
> +// FIXME: Better name for this?

I don't have a better idea.

> Source/WebCore/dom/CustomEvent.h:49
> +    const JSValueInWrappedObject& detail() const { return m_detail; }

I do not quite understand why this class needs both a detail and a cachedDetail? What's the difference between the 2? Why cannot we rely solely on detail()? It seems cachedDetail is basically always initialized from detail.

> Source/WebCore/dom/Event.h:163
> +    bool m_canceled { false };

nice. I know it is called canceled in the spec, but WebKit coding would recommend a prefix here. Maybe m_wasCanceled.

[Darin Adler]

Comment on attachment 331305 [details]
Patch

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

>> Source/WebCore/dom/CustomEvent.h:49
>> +    const JSValueInWrappedObject& detail() const { return m_detail; }
> 
> I do not quite understand why this class needs both a detail and a cachedDetail? What's the difference between the 2? Why cannot we rely solely on detail()? It seems cachedDetail is basically always initialized from detail.

m_cachedDetail can be m_detail cloned across worlds

[Chris Dumez]

(In reply to Darin Adler from comment #41)
> Comment on attachment 331305 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=331305&action=review
> 
> >> Source/WebCore/dom/CustomEvent.h:49
> >> +    const JSValueInWrappedObject& detail() const { return m_detail; }
> > 
> > I do not quite understand why this class needs both a detail and a cachedDetail? What's the difference between the 2? Why cannot we rely solely on detail()? It seems cachedDetail is basically always initialized from detail.
> 
> m_cachedDetail can be m_detail cloned across worlds

But it sounds like an optimization, right? We could purely rely on m_detail and clone on demand when needed. I am not sure it is worth optimizing this. I do not even know how a CustomEvent could be passed across world.

[Darin Adler]

Comment on attachment 331305 [details]
Patch

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

>> Source/WebCore/bindings/js/JSValueInWrappedObject.h:36
>> +// FIXME: The following uses of JSC::Strong are incorrect, can lead to storage leaks
> 
> We may want those comments to be in those classes. Otherwise, it is easy to forget removing them when those classes get updated (since we presumably wouldn't have to update this file).

OK, I will do that.

>>>> Source/WebCore/dom/CustomEvent.h:49
>>>> +    const JSValueInWrappedObject& detail() const { return m_detail; }
>>> 
>>> I do not quite understand why this class needs both a detail and a cachedDetail? What's the difference between the 2? Why cannot we rely solely on detail()? It seems cachedDetail is basically always initialized from detail.
>> 
>> m_cachedDetail can be m_detail cloned across worlds
> 
> But it sounds like an optimization, right? We could purely rely on m_detail and clone on demand when needed. I am not sure it is worth optimizing this. I do not even know how a CustomEvent could be passed across world.

The helper that handles the lifetime across worlds called cachedPropertyValue is used two places: JSCustomEvent::detail and JSMessageEvent::data. In the case of JSMessageEvent, I was preserving the existing caching that has reasons to be there besides the "cloned across worlds" issue. I guess you are saying that here in JSCustomEvent the caching isn’t needed or helpful. It’s easy for me to remove it, I suppose. I think I was hoping that the pattern would be more similar in all these classes. I can eliminate this caching.

>> Source/WebCore/dom/Event.h:163
>> +    bool m_canceled { false };
> 
> nice. I know it is called canceled in the spec, but WebKit coding would recommend a prefix here. Maybe m_wasCanceled.

OK, yes I will use m_wasCanceled.

[Darin Adler]

Comment on attachment 331305 [details]
Patch

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

>>>>> Source/WebCore/dom/CustomEvent.h:49
>>>>> +    const JSValueInWrappedObject& detail() const { return m_detail; }
>>>> 
>>>> I do not quite understand why this class needs both a detail and a cachedDetail? What's the difference between the 2? Why cannot we rely solely on detail()? It seems cachedDetail is basically always initialized from detail.
>>> 
>>> m_cachedDetail can be m_detail cloned across worlds
>> 
>> But it sounds like an optimization, right? We could purely rely on m_detail and clone on demand when needed. I am not sure it is worth optimizing this. I do not even know how a CustomEvent could be passed across world.
> 
> The helper that handles the lifetime across worlds called cachedPropertyValue is used two places: JSCustomEvent::detail and JSMessageEvent::data. In the case of JSMessageEvent, I was preserving the existing caching that has reasons to be there besides the "cloned across worlds" issue. I guess you are saying that here in JSCustomEvent the caching isn’t needed or helpful. It’s easy for me to remove it, I suppose. I think I was hoping that the pattern would be more similar in all these classes. I can eliminate this caching.

OK, I figured more about why I had m_cachedDetail:

The old version of CustomEvent cached the serialized form of detail; that’s what m_serializedDetail was about. So if we repeatedly used the detail in a world other than the one where the CustomEvent was created, the detail would be serialized only once, although it would be deserialized over and over again. Using m_cachedDetail I preserved a similar optimization, slightly different because it optimizes things even more if only two worlds are involved and does re-serialize when a third world is involved.

One other factor is whether the detail attribute needs to act like [SameObject]. I didn't fix that or think it through.

I wasn’t sure it was OK to remove the optimization the old code had to avoid serializing and deserializing over and over again, so I decided to put the cache in; also was similar to JSMessageEvent::data and I was trying to come up with a pattern I could reuse.

I honestly have no idea if this optimization is needed or not. I was trying not to change the performance characteristics too much in this patch.

Please let me know if any of that changes your mind about whether I should keep m_cachedDetail or get rid of it. Or what you think about the [SameObject] issue.

[Chris Dumez]

(In reply to Darin Adler from comment #44)
> Comment on attachment 331305 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=331305&action=review
> 
> >>>>> Source/WebCore/dom/CustomEvent.h:49
> >>>>> +    const JSValueInWrappedObject& detail() const { return m_detail; }
> >>>> 
> >>>> I do not quite understand why this class needs both a detail and a cachedDetail? What's the difference between the 2? Why cannot we rely solely on detail()? It seems cachedDetail is basically always initialized from detail.
> >>> 
> >>> m_cachedDetail can be m_detail cloned across worlds
> >> 
> >> But it sounds like an optimization, right? We could purely rely on m_detail and clone on demand when needed. I am not sure it is worth optimizing this. I do not even know how a CustomEvent could be passed across world.
> > 
> > The helper that handles the lifetime across worlds called cachedPropertyValue is used two places: JSCustomEvent::detail and JSMessageEvent::data. In the case of JSMessageEvent, I was preserving the existing caching that has reasons to be there besides the "cloned across worlds" issue. I guess you are saying that here in JSCustomEvent the caching isn’t needed or helpful. It’s easy for me to remove it, I suppose. I think I was hoping that the pattern would be more similar in all these classes. I can eliminate this caching.
> 
> OK, I figured more about why I had m_cachedDetail:
> 
> The old version of CustomEvent cached the serialized form of detail; that’s
> what m_serializedDetail was about. So if we repeatedly used the detail in a
> world other than the one where the CustomEvent was created, the detail would
> be serialized only once, although it would be deserialized over and over
> again. Using m_cachedDetail I preserved a similar optimization, slightly
> different because it optimizes things even more if only two worlds are
> involved and does re-serialize when a third world is involved.
> 
> One other factor is whether the detail attribute needs to act like
> [SameObject]. I didn't fix that or think it through.
> 
> I wasn’t sure it was OK to remove the optimization the old code had to avoid
> serializing and deserializing over and over again, so I decided to put the
> cache in; also was similar to JSMessageEvent::data and I was trying to come
> up with a pattern I could reuse.
> 
> I honestly have no idea if this optimization is needed or not. I was trying
> not to change the performance characteristics too much in this patch.
> 
> Please let me know if any of that changes your mind about whether I should
> keep m_cachedDetail or get rid of it. Or what you think about the
> [SameObject] issue.

Seems fine to keep cachedDetail for now to maintain existing behavior.

[SameObject] is only used for object or interface types, which is why the spec does not says [SameObject] for the detail attribute (which is of type 'any').

What the spec says is that the detail attribute must return the value it was initialized to. I do not believe we construct CustomEvents from inside the engine, they are always constructed by JS using a JSValue. We should always return that JSValue that was provided by JS when constructing the CustomEvent, as long as detail is accessed from the same world of course.

When detail is accessed from another world, I do not think the behavior is specified. In such case, we serialize the JSValue and deserialize it for use in another world. I personally wouldn't worry about returning the same value every time in this case.

[Darin Adler]

(In reply to Chris Dumez from comment #45)
> When detail is accessed from another world, I do not think the behavior is
> specified. In such case, we serialize the JSValue and deserialize it for use
> in another world. I personally wouldn't worry about returning the same value
> every time in this case.

OK, I’ll go with that.

We might want to return to this and change that some day. I probably would want to return the same object every time. It seems strange to deserialize it every time it’s accessed rather than just once, and returning the same value every time seems to better match the behavior you get in the original world.

[Darin Adler]

Committed r228260: <https://trac.webkit.org/changeset/228260>

[Radar WebKit Bug Importer]

<rdar://problem/37341103>

[Darin Adler]

I see error event tests failing here that did not fail when I tested before landing. Unfortunately I don’t have time to fix the problem right now, so if someone would either skip those tests or roll out my patch, that would be fine.

I should be able to resolve this soon once I have a little free time. But not this morning.

[Chris Dumez]

(In reply to Darin Adler from comment #49)
> I see error event tests failing here that did not fail when I tested before
> landing. Unfortunately I don’t have time to fix the problem right now, so if
> someone would either skip those tests or roll out my patch, that would be
> fine.
> 
> I should be able to resolve this soon once I have a little free time. But
> not this morning.

I am looking into it. This change looks bad:
72	 	    JSDOMGlobalObject* globalObject = toJSDOMGlobalObject(&scriptExecutionContext, isolatedWorld());
 	74	    auto* globalObject = toJSDOMWindow(downcast<Document>(scriptExecutionContext).frame(), isolatedWorld());

What if the global object is a workerglobalscope and not a window? In JSErrorHandler.cpp.

[Chris Dumez]

(In reply to Chris Dumez from comment #50)
> (In reply to Darin Adler from comment #49)
> > I see error event tests failing here that did not fail when I tested before
> > landing. Unfortunately I don’t have time to fix the problem right now, so if
> > someone would either skip those tests or roll out my patch, that would be
> > fine.
> > 
> > I should be able to resolve this soon once I have a little free time. But
> > not this morning.
> 
> I am looking into it. This change looks bad:
> 72	 	    JSDOMGlobalObject* globalObject =
> toJSDOMGlobalObject(&scriptExecutionContext, isolatedWorld());
>  	74	    auto* globalObject =
> toJSDOMWindow(downcast<Document>(scriptExecutionContext).frame(),
> isolatedWorld());
> 
> What if the global object is a workerglobalscope and not a window? In
> JSErrorHandler.cpp.

Follow-up fix in <https://trac.webkit.org/changeset/228276>. Please use commit queue for such big patches, especially if they contain last minutes changes like this one.

I'll monitor the bots to make sure we're back to green but locally, the workers WPT were passing again.

[Antoine Quint]

My patch for https://bugs.webkit.org/show_bug.cgi?id=182608 stopped building after updating to ToT, which was caused by having jsNull() instead of JSC::jsNull() in JSCustomEvent::detail(). This will be fixed when that patch lands.

[Ross Kirsling]

Follow-up patch related to the keyEvent -> underlyingPlatformEvent change:
https://bugs.webkit.org/show_bug.cgi?id=182663