Bug 95720 – Simplify hitTestResultAtPoint and nodesFromRect APIs

• Status: RESOLVED
• Resolution: FIXED
• Component: UI Events

[Allan Sandfeld Jensen]

EventHandler::hitTestResultAtPoint and Document::nodesFromRect both have a list a boolean arguments that could be expressed as a HitTestRequest::Type instead. EventHandler::hitTestResultAtPoint already takes a HitTestRequest as an argument and chould just lose the unnecessary boolean arguments, while Document::nodesFromRect should be converted to taking HitTestRequest arguments instead.

Bug based on review suggestions in https://bugs.webkit.org/show_bug.cgi?id=95204#c16

[Allan Sandfeld Jensen]

Created attachment 162001 [details]
Patch

[Allan Sandfeld Jensen]

Created attachment 162019 [details]
Patch

Reupload to let EWS try again now that its depending patch has landed.

[Build Bot]

Comment on attachment 162019 [details]
Patch

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

[Gyuyoung Kim]

Comment on attachment 162019 [details]
Patch

Attachment 162019 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13746453

[Allan Sandfeld Jensen]

Comment on attachment 162019 [details]
Patch

Forgot to update one symbol for Mac, and some calls for EFL.

[Allan Sandfeld Jensen]

Created attachment 162186 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 162186 [details]
Patch

Attachment 162186 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13746793

[Allan Sandfeld Jensen]

Created attachment 162198 [details]
Patch

Fix EFL build

[Kenneth Rohde Christiansen]

Comment on attachment 162198 [details]
Patch

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

> Source/WebCore/rendering/HitTestRequest.h:51
> +    static HitTestRequestType Default(bool allowShadowContent, bool ignoreClipping = false)

What about using flags?

HitTestResult::Default(ALLOW_SHADOW_CONTENT | CLIP)

That is more clear

[Allan Sandfeld Jensen]

(In reply to comment #9)
> (From update of attachment 162198 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=162198&action=review
> 
> > Source/WebCore/rendering/HitTestRequest.h:51
> > +    static HitTestRequestType Default(bool allowShadowContent, bool ignoreClipping = false)
> 
> What about using flags?
> 
> HitTestRequest::Default(ALLOW_SHADOW_CONTENT | CLIP)
> 
> That is more clear

True, but if you want refactor it like that, you just stop using the arguments to HitTestRequest::Default and write it like HitTestRequest::Default() | HitTestRequest::AllowChildContent | HitTestRequest::IgnoreClipping. The reason the function exists and takes boolean arguments is to make it easy and safe to upgrade existing calls.

[Antonio Gomes]

Comment on attachment 162198 [details]
Patch

Idea is good, but I am really not a big fan of the HitTestRequest::Default(true|false) new idiom.

While looking over the patch, the calls Default(true|false) were making things equally non-obvious than before.

IMO, the APIs we improved were degraded by the suggested alternative.

[Allan Sandfeld Jensen]

(In reply to comment #11)
> (From update of attachment 162198 [details])
> Idea is good, but I am really not a big fan of the HitTestRequest::Default(true|false) new idiom.
> 
> While looking over the patch, the calls Default(true|false) were making things equally non-obvious than before.
> 
> IMO, the APIs we improved were degraded by the suggested alternative.

What about removing the boolean arguments from Default and calling something like HitTestRequest::Default() | HitTestRequest::AllowChildContent | HitTestRequest::IgnoreClipping ?

[Antonio Gomes]

> What about removing the boolean arguments from Default and calling something like HitTestRequest::Default() | HitTestRequest::AllowChildContent | HitTestRequest::IgnoreClipping ?

Looks cleaner to me. I think kenneth would be happier as well.

Would "default" be empty?

[Allan Sandfeld Jensen]

Created attachment 162262 [details]
Patch

Eliminated the use of boolean arguments in the redefinition

[Allan Sandfeld Jensen]

(In reply to comment #13)
> > What about removing the boolean arguments from Default and calling something like HitTestRequest::Default() | HitTestRequest::AllowChildContent | HitTestRequest::IgnoreClipping ?
> 
> Looks cleaner to me. I think kenneth would be happier as well.
> 
> Would "default" be empty?

No it is ReadOnly | Active currently. If I manage to get rid of side-effects from hit-testing, Default can be redefined to 0, as ReadOnly is no longer needed to make the hit-test just hit-test.

[Allan Sandfeld Jensen]

Comment on attachment 162262 [details]
Patch

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

> Source/WebCore/ChangeLog:13
> +        To allow for simple conversion a new static function has been  added to HitTestRequest to build
> +        a default type based on the old boolean arguments.

I forgot to remove this from the ChangeLog. Will remove before landing.

[Antonio Gomes]

Comment on attachment 162262 [details]
Patch

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

Getting close...

Last question is: is that clear that Default means Active + ReadOnly, nomenclature-wise or API-wise?

> Source/WebCore/page/EventHandler.cpp:1071
> -    if (!allowShadowContent)
> +    if (!request.allowsShadowContent())
>          result.setToNonShadowAncestor();

From what I remember, setToNonShadowAncestor and the HitTestRequest::allowShadowContent were not working together: the first were about point-based hit test, the latter, about rect-based.

Mind to confirm?

> Source/WebCore/rendering/HitTestRequest.h:45
> +    static const HitTestRequestType Default = ReadOnly | Active;

That is  the key point.

> Source/WebKit/blackberry/WebKitSupport/FatFingers.cpp:499
> +    HitTestResult result(contentPos, topPadding, rightPadding, bottomPadding, leftPadding);
> +    if (m_targetType != Text)
> +        request |= HitTestRequest::AllowShadowContent;

nit: swap these two lines, so the request gets constructed altogether

[Allan Sandfeld Jensen]

(In reply to comment #17)
> (From update of attachment 162262 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=162262&action=review
> 
> Getting close...
> 
> Last question is: is that clear that Default means Active + ReadOnly, nomenclature-wise or API-wise?
> 
I guess not, but I was hoping to redefine it later, and that makes a generic name more suitable. 

> > Source/WebCore/page/EventHandler.cpp:1071
> > -    if (!allowShadowContent)
> > +    if (!request.allowsShadowContent())
> >          result.setToNonShadowAncestor();
> 
> From what I remember, setToNonShadowAncestor and the HitTestRequest::allowShadowContent were not working together: the first were about point-based hit test, the latter, about rect-based.
> 
> Mind to confirm?
> 
You are right. What this does is to ensure that the returned innerNode is not a shadow node, if the caller has requested no shadow nodes.

[Antonio Gomes]

(In reply to comment #18)
> (In reply to comment #17)
> > (From update of attachment 162262 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=162262&action=review
> > 
> > Getting close...
> > 
> > Last question is: is that clear that Default means Active + ReadOnly, nomenclature-wise or API-wise?
> > 
> I guess not, but I was hoping to redefine it later, and that makes a generic name more suitable. 
> 

We could have s/Default/ActiveAndReadOnly for convenience? 


> > > Source/WebCore/page/EventHandler.cpp:1071
> > > -    if (!allowShadowContent)
> > > +    if (!request.allowsShadowContent())
> > >          result.setToNonShadowAncestor();
> > 
> > From what I remember, setToNonShadowAncestor and the HitTestRequest::allowShadowContent were not working together: the first were about point-based hit test, the latter, about rect-based.
> > 
> > Mind to confirm?
> > 
> You are right. What this does is to ensure that the returned innerNode is not a shadow node, if the caller has requested no shadow nodes.

[Allan Sandfeld Jensen]

(In reply to comment #19)
> (In reply to comment #18)
> > (In reply to comment #17)
> > > (From update of attachment 162262 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=162262&action=review
> > > 
> > > Getting close...
> > > 
> > > Last question is: is that clear that Default means Active + ReadOnly, nomenclature-wise or API-wise?
> > > 
> > I guess not, but I was hoping to redefine it later, and that makes a generic name more suitable. 
> > 
> 
> We could have s/Default/ActiveAndReadOnly for convenience? 
> 
There is another point to this. Active shouldn't really have any effect. It only does two things in hit-testing. If readonly is not set, it is used to update hover/active state, with ReadOnly set and it is only used in RenderLayer to make root-layers claim all tests hit it. Most of the ReadOnly hit-tests probably shouldn't even have Active set. One of the things I am hoping to do later, is find the ones that need Active, and set it on them and let the rest use only ReadOnly.

If you don't like the concept of Default here, I think the alternative is to get rid of it and expand the values in full everywhere. It is pretty much what you suggest anyway with ActiveAndReadOnly, but it means any future improvements to what default is, would require changing all these ports again to update the calls.

[Antonio Gomes]

> If you don't like the concept of Default here, I think the alternative is to get rid of it and expand the values in full everywhere. It is pretty much what you suggest anyway with ActiveAndReadOnly, but it means any future improvements to what default is, would require changing all these ports again to update the calls.

I think that is preferable. Kenneth?

[Kenneth Rohde Christiansen]

(In reply to comment #21)
> > If you don't like the concept of Default here, I think the alternative is to get rid of it and expand the values in full everywhere. It is pretty much what you suggest anyway with ActiveAndReadOnly, but it means any future improvements to what default is, would require changing all these ports again to update the calls.
> 
> I think that is preferable. Kenneth?

Me too

[WebKit Review Bot]

Comment on attachment 162262 [details]
Patch

Attachment 162262 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13766734

New failing tests:
fast/forms/drag-out-of-textarea.html
touchadjustment/nested-shadow-node.html
editing/pasteboard/drop-text-without-selection.html
touchadjustment/search-cancel.html
editing/pasteboard/drop-text-events.html
editing/pasteboard/drag-drop-input-textarea.html
editing/selection/drag-text-delay.html
fast/events/5056619.html
fast/forms/drag-into-textarea.html
editing/pasteboard/drag-drop-url-text.html
fast/events/content-changed-during-drop.html

[Allan Sandfeld Jensen]

Comment on attachment 162262 [details]
Patch

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

I seem to have managed to mix up the parameters in a few places which is why the tests fail now.

> Source/WebCore/page/EventHandler.cpp:1010
> -    HitTestResult hitTest = hitTestResultAtPoint(m_panScrollStartPos, true);
> +    HitTestResult hitTest = hitTestResultAtPoint(m_panScrollStartPos, HitTestRequest::Default | HitTestRequest::AllowChildFrameContent);

Wrong autocomplete, should AllowShadowContent.

> Source/WebCore/page/EventHandler.cpp:2483
> -    HitTestResult result = hitTestResultAtPoint(hitTestPoint, /*allowShadowContent*/ true, /*ignoreClipping*/ false, DontHitTestScrollbars, hitType, touchRadius);
> +    HitTestResult result = hitTestResultAtPoint(hitTestPoint, HitTestRequest::Default | HitTestRequest::AllowChildFrameContent, touchRadius);

Ditto

> Source/WebCore/page/EventHandler.cpp:2501
> -    HitTestResult result = hitTestResultAtPoint(hitTestPoint, /*allowShadowContent*/ true, /*ignoreClipping*/ false, DontHitTestScrollbars, hitType, touchRadius);
> +    HitTestResult result = hitTestResultAtPoint(hitTestPoint, HitTestRequest::Default | HitTestRequest::AllowChildFrameContent, touchRadius);

Ditto

> Source/WebCore/page/Frame.cpp:727
> -    HitTestResult result = eventHandler()->hitTestResultAtPoint(framePoint, true);
> +    HitTestResult result = eventHandler()->hitTestResultAtPoint(framePoint, HitTestRequest::Default | HitTestRequest::AllowChildFrameContent);

Ditto

[Allan Sandfeld Jensen]

Created attachment 162720 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 162720 [details]
Patch

Attachment 162720 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13772710

[Gyuyoung Kim]

Comment on attachment 162720 [details]
Patch

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

> Source/WebKit/efl/ewk/ewk_frame.cpp:698
> +                                        WebCore::HitTestRequest::ReadOnly | HitTestRequest::Active | WebCore::HitTestRequest::IgnoreClipping);

It looks "WebCore::" is missing. s/HitTestRequest::Active/WebCore::HitTestRequest::Active/g

[Gyuyoung Kim]

Comment on attachment 162720 [details]
Patch

Attachment 162720 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13768764

[Allan Sandfeld Jensen]

Created attachment 162752 [details]
Patch

Fix EFL build

[Antonio Gomes]

Comment on attachment 162752 [details]
Patch

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

> Source/WebCore/page/EventHandler.cpp:1070
> -    if (!allowShadowContent)
> +    if (!request.allowsShadowContent())
>          result.setToNonShadowAncestor();

so is this ok?

[Allan Sandfeld Jensen]

(In reply to comment #30)
> (From update of attachment 162752 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=162752&action=review
> 
> > Source/WebCore/page/EventHandler.cpp:1070
> > -    if (!allowShadowContent)
> > +    if (!request.allowsShadowContent())
> >          result.setToNonShadowAncestor();
> 
> so is this ok?

It duplicates what the code did before. Removing it would be changing functionality.

Essentially the old allowShadowContent argument applied to both point based and area based tests, but since the HitTestRequest flag only applies automatically to area-based tests, this code ensures it is also applied to point-based tests.

[WebKit Review Bot]

Comment on attachment 162752 [details]
Patch

Clearing flags on attachment: 162752

Committed r127876: <http://trac.webkit.org/changeset/127876>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by 96600

I'm going rollout this patch, bug 96600. Sorry about this.

It seems this patch changed calculation of event position of mouse click on iframe.

Spin button, input type "number", doesn't work since this change.
See http://crbug.com/148842 for bug report.

Please try following URI for regression checking.
http://jsfiddle.net/EdP8j/

Thanks for your understanding.

I'm trying to add tests for clicking spin button in iframe:
https://bugs.webkit.org/show_bug.cgi?id=96612

(In reply to comment #36)
> I'm trying to add tests for clicking spin button in iframe:
> https://bugs.webkit.org/show_bug.cgi?id=96612

Landed: http://trac.webkit.org/changeset/128423

New tests are:
* fast/forms/number/number-spinbutton-click-in-iframe.html
* fast/forms/time-multiple-fields/time-multiple-fields-spinbutton-click-in-iframe.html (for Chromium port only, or ENABLE_INPUT_TYPE_TIME_MULTIPLE_FIELDS enabled)

[Allan Sandfeld Jensen]

(In reply to comment #35)
> I'm going rollout this patch, bug 96600. Sorry about this.
> 
> It seems this patch changed calculation of event position of mouse click on iframe.
> 
This particular patch shouldn't have any functional effect. It seems more like that a call wasn't converted correctly to the new API. Since these form-elements use shadow-dom, I suspect a call missing AllowShadowContent.

[Allan Sandfeld Jensen]

Created attachment 188065 [details]
Patch

Rebase and update to apply before patch for bug #95204

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/WebCore/ChangeLog', u'Source/WebCore/WebCore.exp.in', u'Source/WebCore/WebCore.order', u'Source/WebCore/dom/Document.cpp', u'Source/WebCore/dom/Document.h', u'Source/WebCore/page/ContextMenuController.cpp', u'Source/WebCore/page/DragController.cpp', u'Source/WebCore/page/EventHandler.cpp', u'Source/WebCore/page/EventHandler.h', u'Source/WebCore/page/FocusController.cpp', u'Source/WebCore/page/Frame.cpp', u'Source/WebCore/page/TouchDisambiguation.cpp', u'Source/WebCore/rendering/HitTestRequest.h', u'Source/WebCore/testing/Internals.cpp', u'Source/WebKit/blackberry/Api/WebPage.cpp', u'Source/WebKit/blackberry/ChangeLog', u'Source/WebKit/blackberry/WebKitSupport/SelectionHandler.cpp', u'Source/WebKit/chromium/ChangeLog', u'Source/WebKit/chromium/src/ContextMenuClientImpl.cpp', u'Source/WebKit/chromium/src/FrameLoaderClientImpl.cpp', u'Source/WebKit/chromium/src/WebFrameImpl.cpp', u'Source/WebKit/chromium/src/WebPluginContainerImpl.cpp', u'Source/WebKit/chromium/src/WebViewImpl.cpp', u'Source/WebKit/efl/ChangeLog', u'Source/WebKit/efl/ewk/ewk_frame.cpp', u'Source/WebKit/mac/ChangeLog', u'Source/WebKit/mac/WebCoreSupport/WebFrameLoaderClient.mm', u'Source/WebKit/mac/WebView/WebHTMLView.mm', u'Source/WebKit/qt/ChangeLog', u'Source/WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp', u'Source/WebKit/qt/WebCoreSupport/QWebFrameAdapter.cpp', u'Source/WebKit/qt/WebCoreSupport/QWebPageAdapter.cpp', u'Source/WebKit/win/ChangeLog', u'Source/WebKit/win/WebActionPropertyBag.cpp', u'Source/WebKit/win/WebKit.vcproj/WebKitExports.def.in', u'Source/WebKit/win/WebView.cpp', u'Source/WebKit/wx/ChangeLog', u'Source/WebKit/wx/WebFrame.cpp', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/WebProcess/InjectedBundle/InjectedBundleNavigationAction.cpp', u'Source/WebKit2/WebProcess/WebPage/WebFrame.cpp', u'Source/WebKit2/WebProcess/WebPage/WebPage.cpp', u'Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm', u'Source/autotools/symbols.filter']" exit_code: 1
Source/WebCore/dom/Document.h:37:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/dom/Document.h:386:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/Document.h:387:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/efl/ewk/ewk_frame.cpp:695:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Total errors found: 4 in 45 files


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

[Allan Sandfeld Jensen]

Created attachment 188066 [details]
Patch

Fix a three missed rebase failures

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/WebCore/ChangeLog', u'Source/WebCore/WebCore.exp.in', u'Source/WebCore/WebCore.order', u'Source/WebCore/dom/Document.cpp', u'Source/WebCore/dom/Document.h', u'Source/WebCore/page/ContextMenuController.cpp', u'Source/WebCore/page/DragController.cpp', u'Source/WebCore/page/EventHandler.cpp', u'Source/WebCore/page/EventHandler.h', u'Source/WebCore/page/FocusController.cpp', u'Source/WebCore/page/Frame.cpp', u'Source/WebCore/page/TouchDisambiguation.cpp', u'Source/WebCore/rendering/HitTestRequest.h', u'Source/WebCore/testing/Internals.cpp', u'Source/WebKit/blackberry/Api/WebPage.cpp', u'Source/WebKit/blackberry/ChangeLog', u'Source/WebKit/blackberry/WebKitSupport/SelectionHandler.cpp', u'Source/WebKit/chromium/ChangeLog', u'Source/WebKit/chromium/src/ContextMenuClientImpl.cpp', u'Source/WebKit/chromium/src/FrameLoaderClientImpl.cpp', u'Source/WebKit/chromium/src/WebFrameImpl.cpp', u'Source/WebKit/chromium/src/WebPluginContainerImpl.cpp', u'Source/WebKit/chromium/src/WebViewImpl.cpp', u'Source/WebKit/efl/ChangeLog', u'Source/WebKit/efl/ewk/ewk_frame.cpp', u'Source/WebKit/mac/ChangeLog', u'Source/WebKit/mac/WebCoreSupport/WebFrameLoaderClient.mm', u'Source/WebKit/mac/WebView/WebFrame.mm', u'Source/WebKit/mac/WebView/WebHTMLView.mm', u'Source/WebKit/qt/ChangeLog', u'Source/WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp', u'Source/WebKit/qt/WebCoreSupport/QWebFrameAdapter.cpp', u'Source/WebKit/qt/WebCoreSupport/QWebPageAdapter.cpp', u'Source/WebKit/win/ChangeLog', u'Source/WebKit/win/WebActionPropertyBag.cpp', u'Source/WebKit/win/WebKit.vcproj/WebKitExports.def.in', u'Source/WebKit/win/WebView.cpp', u'Source/WebKit/wx/ChangeLog', u'Source/WebKit/wx/WebFrame.cpp', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/WebProcess/InjectedBundle/InjectedBundleNavigationAction.cpp', u'Source/WebKit2/WebProcess/WebPage/WebFrame.cpp', u'Source/WebKit2/WebProcess/WebPage/WebPage.cpp', u'Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm', u'Source/autotools/symbols.filter']" exit_code: 1
Source/WebCore/dom/Document.h:386:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/Document.h:387:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/efl/ewk/ewk_frame.cpp:695:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Total errors found: 3 in 46 files


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

[Julien Chaffraix]

Comment on attachment 188066 [details]
Patch

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

r=me, but fix the bad change.

> Source/WebKit/chromium/src/WebViewImpl.cpp:1094
> +    HitTestRequest::HitTestRequestType hitType = HitTestRequest::ReadOnly | HitTestRequest::Active | ((zoomType == FindInPage) ? HitTestRequest::IgnoreClipping : 0);

Nit: I would put the zoomType checks to the next line for readibility.

> Source/WebKit/mac/WebView/WebHTMLView.mm:6205
> +    HitTestRequest::HitTestRequestType hitType = HitTestRequest::ReadOnly | HitTestRequest::Active | (allow ? HitTestRequest::AllowShadowContent : 0);

Same nit.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1766
> +        HitTestResult result = mainframe->eventHandler()->hitTestResultAtPoint(mainframe->view()->windowToContents(point), HitTestRequest::ReadOnly | HitTestRequest::Active | HitTestRequest::AllowShadowContent);

This is wrong, it should be:  HitTestRequest::ReadOnly | HitTestRequest::Active | HitTestRequest::IgnoreClipping

[Maciej Stachowiak]

Comment on attachment 188066 [details]
Patch

r=me on the WebKit2 part

[Allan Sandfeld Jensen]

Committed r142977: <http://trac.webkit.org/changeset/142977>

[Antonio Gomes]

Comment on attachment 188066 [details]
Patch

Very nice clean up. Thanks for doing this!