Description
Darin Adler
2017-11-11 22:32:40 PST
Created attachment 326711 [details]
Patch
Created attachment 326724 [details]
Patch
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.
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 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. 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
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. 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
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. 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
Created attachment 331285 [details]
Patch
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.
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 Created attachment 331286 [details]
Patch
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.
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 Created attachment 331287 [details]
Patch
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.
Created attachment 331288 [details]
Patch
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.
Created attachment 331289 [details]
Patch
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.
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. 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
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. 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
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. 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
Created attachment 331294 [details]
Patch
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 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
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 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
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 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
Created attachment 331305 [details]
Patch
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.
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. 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. 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. 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 (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. 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. 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. (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. (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. Committed r228260: <https://trac.webkit.org/changeset/228260> 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. (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. (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. 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. Follow-up patch related to the keyEvent -> underlyingPlatformEvent change: https://bugs.webkit.org/show_bug.cgi?id=182663 |