Bug 95720

Summary: Simplify hitTestResultAtPoint and nodesFromRect APIs
Product: WebKit Reporter: Allan Sandfeld Jensen <allan.jensen>
Component: UI EventsAssignee: Allan Sandfeld Jensen <allan.jensen>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, eric, gyuyoung.kim, jchaffraix, kenneth, mifenton, ojan.autocc, rakuco, rwlbuis, tonikitoo, webkit.review.bot, yong.li.webkit, yosin
Priority: P2    
Version: 420+   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 96600, 96612    
Bug Blocks: 95204    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch jchaffraix: review+, jchaffraix: commit-queue-

Description Allan Sandfeld Jensen 2012-09-04 01:50:15 PDT 
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
Comment 1 Allan Sandfeld Jensen 2012-09-04 03:55:25 PDT 
Created attachment 162001 [details]
Patch
Comment 2 Allan Sandfeld Jensen 2012-09-04 05:14:53 PDT 
Created attachment 162019 [details]
Patch

Reupload to let EWS try again now that its depending patch has landed.
Comment 3 Build Bot 2012-09-04 05:59:21 PDT 
Comment on attachment 162019 [details]
Patch

Attachment 162019 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13739527
Comment 4 Gyuyoung Kim 2012-09-04 06:11:15 PDT 
Comment on attachment 162019 [details]
Patch

Attachment 162019 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13746453
Comment 5 Allan Sandfeld Jensen 2012-09-04 07:30:51 PDT 
Comment on attachment 162019 [details]
Patch

Forgot to update one symbol for Mac, and some calls for EFL.
Comment 6 Allan Sandfeld Jensen 2012-09-05 01:47:12 PDT 
Created attachment 162186 [details]
Patch
Comment 7 Gyuyoung Kim 2012-09-05 02:15:13 PDT 
Comment on attachment 162186 [details]
Patch

Attachment 162186 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13746793
Comment 8 Allan Sandfeld Jensen 2012-09-05 02:42:51 PDT 
Created attachment 162198 [details]
Patch

Fix EFL build
Comment 9 Kenneth Rohde Christiansen 2012-09-05 02:45:20 PDT 
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
Comment 10 Allan Sandfeld Jensen 2012-09-05 02:58:20 PDT 
(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.
Comment 11 Antonio Gomes 2012-09-05 08:03:16 PDT 
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.
Comment 12 Allan Sandfeld Jensen 2012-09-05 08:21:01 PDT 
(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 ?
Comment 13 Antonio Gomes 2012-09-05 09:09:31 PDT 
> 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?
Comment 14 Allan Sandfeld Jensen 2012-09-05 09:10:33 PDT 
Created attachment 162262 [details]
Patch

Eliminated the use of boolean arguments in the redefinition
Comment 15 Allan Sandfeld Jensen 2012-09-05 09:12:03 PDT 
(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.
Comment 16 Allan Sandfeld Jensen 2012-09-05 09:13:52 PDT 
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.
Comment 17 Antonio Gomes 2012-09-05 12:45:24 PDT 
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
Comment 18 Allan Sandfeld Jensen 2012-09-05 13:49:37 PDT 
(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.
Comment 19 Antonio Gomes 2012-09-05 14:19:54 PDT 
(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.
Comment 20 Allan Sandfeld Jensen 2012-09-06 02:13:00 PDT 
(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.
Comment 21 Antonio Gomes 2012-09-06 07:22:35 PDT 
> 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?
Comment 22 Kenneth Rohde Christiansen 2012-09-06 07:26:14 PDT 
(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
Comment 23 WebKit Review Bot 2012-09-06 23:27:16 PDT 
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
Comment 24 Allan Sandfeld Jensen 2012-09-07 02:18:04 PDT 
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
Comment 25 Allan Sandfeld Jensen 2012-09-07 02:57:03 PDT 
Created attachment 162720 [details]
Patch
Comment 26 Gyuyoung Kim 2012-09-07 04:49:22 PDT 
Comment on attachment 162720 [details]
Patch

Attachment 162720 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13772710
Comment 27 Gyuyoung Kim 2012-09-07 04:54:16 PDT 
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
Comment 28 Gyuyoung Kim 2012-09-07 05:07:20 PDT 
Comment on attachment 162720 [details]
Patch

Attachment 162720 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13768764
Comment 29 Allan Sandfeld Jensen 2012-09-07 06:04:31 PDT 
Created attachment 162752 [details]
Patch

Fix EFL build
Comment 30 Antonio Gomes 2012-09-07 07:40:36 PDT 
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?
Comment 31 Allan Sandfeld Jensen 2012-09-07 08:03:36 PDT 
(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.
Comment 32 WebKit Review Bot 2012-09-07 09:08:54 PDT 
Comment on attachment 162752 [details]
Patch

Clearing flags on attachment: 162752

Committed r127876: <http://trac.webkit.org/changeset/127876>
Comment 33 WebKit Review Bot 2012-09-07 09:09:02 PDT 
All reviewed patches have been landed.  Closing bug.
Comment 34 WebKit Review Bot 2012-09-12 22:01:38 PDT 
Re-opened since this is blocked by 96600
Comment 35 yosin 2012-09-12 22:49:53 PDT 
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.
Comment 36 yosin 2012-09-13 00:45:44 PDT 
I'm trying to add tests for clicking spin button in iframe:
https://bugs.webkit.org/show_bug.cgi?id=96612
Comment 37 yosin 2012-09-13 01:38:59 PDT 
(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)
Comment 38 Allan Sandfeld Jensen 2012-09-13 03:00:49 PDT 
(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.
Comment 39 Allan Sandfeld Jensen 2013-02-13 06:03:21 PST 
Created attachment 188065 [details]
Patch

Rebase and update to apply before patch for bug #95204
Comment 40 WebKit Review Bot 2013-02-13 06:07:21 PST 
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.
Comment 41 Allan Sandfeld Jensen 2013-02-13 06:10:59 PST 
Created attachment 188066 [details]
Patch

Fix a three missed rebase failures
Comment 42 WebKit Review Bot 2013-02-13 06:13:07 PST 
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.
Comment 43 Julien Chaffraix 2013-02-14 10:21:04 PST 
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
Comment 44 Maciej Stachowiak 2013-02-14 10:24:12 PST 
Comment on attachment 188066 [details]
Patch

r=me on the WebKit2 part
Comment 45 Allan Sandfeld Jensen 2013-02-15 02:50:50 PST 
Committed r142977: <http://trac.webkit.org/changeset/142977>
Comment 46 Antonio Gomes 2013-02-15 06:07:03 PST 
Comment on attachment 188066 [details]
Patch

Very nice clean up. Thanks for doing this!