Bug 174681 – Use more references in event dispatch code

• Component: UI Events
• Status: RESOLVED FIXED

[Andreas Kling]

Guess what this patch will be about

[Andreas Kling]

Created attachment 316003 [details]
Patch

[Build Bot]

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.

[Jeremy Jones]

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.

[Andreas Kling]

(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!

[Andreas Kling]

Created attachment 316015 [details]
Patch II

Added assertion per Jeremy's comment. Try to fix up Windows and Gtk+ builds.

[Build Bot]

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.

[Geoffrey Garen]

Comment on attachment 316015 [details]
Patch II

r=me

Is Windows broken?

[Jeremy Jones]

(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+

[Andreas Kling]

Created attachment 316104 [details]
Patch for landing

[Build Bot]

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.

[WebKit Commit Bot]

Comment on attachment 316104 [details]
Patch for landing

Clearing flags on attachment: 316104

Committed r219743: <http://trac.webkit.org/changeset/219743>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.