Bug 174681

Summary: Use more references in event dispatch code
Product: WebKit Reporter: Andreas Kling <kling>
Component: UI EventsAssignee: Andreas Kling <kling>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, commit-queue, ggaren, jeremyj-wk, kling
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch II
ggaren: review+
Patch for landing none

Description Andreas Kling 2017-07-20 11:08:36 PDT
Guess what this patch will be about
Comment 1 Andreas Kling 2017-07-20 11:09:33 PDT
Created attachment 316003 [details]
Patch
Comment 2 Build Bot 2017-07-20 11:11:04 PDT
Attachment 316003 [details] did not pass style-queue:


ERROR: Source/WebKit/WebProcess/Plugins/PluginView.cpp:944:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 35 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Jeremy Jones 2017-07-20 11:31:18 PDT
Comment on attachment 316003 [details]
Patch

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

> Source/WebCore/dom/EventTarget.cpp:236
> +    auto& context = *scriptExecutionContext();

Is scriptExectutionContext() guaranteed to never return nullptr?
Either it should be changed to return a ref, or there should be a null check, or there should be an assert that it is not null.
Comment 4 Andreas Kling 2017-07-20 12:12:55 PDT
(In reply to Jeremy Jones from comment #3)
> Comment on attachment 316003 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=316003&action=review
> 
> > Source/WebCore/dom/EventTarget.cpp:236
> > +    auto& context = *scriptExecutionContext();
> 
> Is scriptExectutionContext() guaranteed to never return nullptr?
> Either it should be changed to return a ref, or there should be a null
> check, or there should be an assert that it is not null.

In this case it's guaranteed (event listeners shouldn't be firing outside of a script execution context) and we were already dereferencing the pointer a few lines below.

But you are right, let's add an assertion for this!
Comment 5 Andreas Kling 2017-07-20 12:49:24 PDT
Created attachment 316015 [details]
Patch II

Added assertion per Jeremy's comment. Try to fix up Windows and Gtk+ builds.
Comment 6 Build Bot 2017-07-20 12:52:15 PDT
Attachment 316015 [details] did not pass style-queue:


WARNING: File exempt from style guide. Skipping: "Source/WebKit/WebProcess/InjectedBundle/API/gtk/DOM/GObjectEventListener.cpp"
WARNING: File exempt from style guide. Skipping: "Source/WebKit/WebProcess/InjectedBundle/API/gtk/DOM/GObjectEventListener.h"
ERROR: Source/WebKit/WebProcess/Plugins/PluginView.cpp:944:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKitLegacy/win/DOMEventsClasses.cpp:79:  'ePtr' is incorrectly named. It should be named 'protector' or 'protectedE'.  [readability/naming/protected] [4]
Total errors found: 2 in 42 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Geoffrey Garen 2017-07-20 13:52:50 PDT
Comment on attachment 316015 [details]
Patch II

r=me

Is Windows broken?
Comment 8 Jeremy Jones 2017-07-20 15:55:16 PDT
(In reply to Andreas Kling from comment #4)
> (In reply to Jeremy Jones from comment #3)
> > Comment on attachment 316003 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=316003&action=review
> > 
> > > Source/WebCore/dom/EventTarget.cpp:236
> > > +    auto& context = *scriptExecutionContext();
> > 
> > Is scriptExectutionContext() guaranteed to never return nullptr?
> > Either it should be changed to return a ref, or there should be a null
> > check, or there should be an assert that it is not null.
> 
> In this case it's guaranteed (event listeners shouldn't be firing outside of
> a script execution context) and we were already dereferencing the pointer a
> few lines below.
> 
> But you are right, let's add an assertion for this!

provisional r+
Comment 9 Andreas Kling 2017-07-21 11:02:11 PDT
Created attachment 316104 [details]
Patch for landing
Comment 10 Build Bot 2017-07-21 11:14:18 PDT
Attachment 316104 [details] did not pass style-queue:


WARNING: File exempt from style guide. Skipping: "Source/WebKit/WebProcess/InjectedBundle/API/gtk/DOM/GObjectEventListener.cpp"
ERROR: Source/WebKitLegacy/win/Plugins/PluginViewWin.cpp:626:  Multi line control clauses should use braces.  [whitespace/braces] [4]
WARNING: File exempt from style guide. Skipping: "Source/WebKit/WebProcess/InjectedBundle/API/gtk/DOM/GObjectEventListener.h"
ERROR: Source/WebKit/WebProcess/Plugins/PluginView.cpp:944:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKitLegacy/win/DOMEventsClasses.cpp:79:  'ePtr' is incorrectly named. It should be named 'protector' or 'protectedE'.  [readability/naming/protected] [4]
Total errors found: 3 in 43 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 11 WebKit Commit Bot 2017-07-21 14:16:33 PDT
Comment on attachment 316104 [details]
Patch for landing

Clearing flags on attachment: 316104

Committed r219743: <http://trac.webkit.org/changeset/219743>
Comment 12 WebKit Commit Bot 2017-07-21 14:16:35 PDT
All reviewed patches have been landed.  Closing bug.