Bug 95720

Summary:

Simplify hitTestResultAtPoint and nodesFromRect APIs

Product:

WebKit

Reporter:

Allan Sandfeld Jensen <allan.jensen>

Component:

UI Events

Assignee:

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:

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-

Allan Sandfeld Jensen

Reported 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

Attachments
Patch (55.45 KB, patch) 2012-09-04 03:55 PDT, Allan Sandfeld Jensen	no flags	Details Formatted Diff Diff
Patch (55.45 KB, patch) 2012-09-04 05:14 PDT, Allan Sandfeld Jensen	no flags	Details Formatted Diff Diff
Patch (61.57 KB, patch) 2012-09-05 01:47 PDT, Allan Sandfeld Jensen	no flags	Details Formatted Diff Diff
Patch (61.78 KB, patch) 2012-09-05 02:42 PDT, Allan Sandfeld Jensen	no flags	Details Formatted Diff Diff
Patch (62.20 KB, patch) 2012-09-05 09:10 PDT, Allan Sandfeld Jensen	no flags	Details Formatted Diff Diff
Patch (62.81 KB, patch) 2012-09-07 02:57 PDT, Allan Sandfeld Jensen	no flags	Details Formatted Diff Diff
Patch (62.82 KB, patch) 2012-09-07 06:04 PDT, Allan Sandfeld Jensen	no flags	Details Formatted Diff Diff
Patch (62.87 KB, patch) 2013-02-13 06:03 PST, Allan Sandfeld Jensen	no flags	Details Formatted Diff Diff
Patch (64.11 KB, patch) 2013-02-13 06:10 PST, Allan Sandfeld Jensen	jchaffraix: review+ jchaffraix: commit-queue-	Details Formatted Diff Diff
Show Obsolete (8) View All Add attachment proposed patch, testcase, etc.

Allan Sandfeld Jensen

Comment 1 2012-09-04 03:55:25 PDT

Created attachment 162001 [details] Patch

Allan Sandfeld Jensen

Comment 2 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.

Build Bot

Comment 3 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

Gyuyoung Kim

Comment 4 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

Allan Sandfeld Jensen

Comment 5 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.

Allan Sandfeld Jensen

Comment 6 2012-09-05 01:47:12 PDT

Created attachment 162186 [details] Patch

Gyuyoung Kim

Comment 7 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

Allan Sandfeld Jensen

Comment 8 2012-09-05 02:42:51 PDT

Created attachment 162198 [details] Patch Fix EFL build

Kenneth Rohde Christiansen

Comment 9 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

Allan Sandfeld Jensen

Comment 10 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.

Antonio Gomes

Comment 11 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.

Allan Sandfeld Jensen

Comment 12 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 ?

Antonio Gomes

Comment 13 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?

Allan Sandfeld Jensen

Comment 14 2012-09-05 09:10:33 PDT

Created attachment 162262 [details] Patch Eliminated the use of boolean arguments in the redefinition

Allan Sandfeld Jensen

Comment 15 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.

Allan Sandfeld Jensen

Comment 16 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.

Antonio Gomes

Comment 17 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

Allan Sandfeld Jensen

Comment 18 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.

Antonio Gomes

Comment 19 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.

Allan Sandfeld Jensen

Comment 20 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.

Antonio Gomes

Comment 21 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?

Kenneth Rohde Christiansen

Comment 22 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

WebKit Review Bot

Comment 23 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

Allan Sandfeld Jensen

Comment 24 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

Allan Sandfeld Jensen

Comment 25 2012-09-07 02:57:03 PDT

Created attachment 162720 [details] Patch

Gyuyoung Kim

Comment 26 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

Gyuyoung Kim

Comment 27 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

Gyuyoung Kim

Comment 28 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

Allan Sandfeld Jensen

Comment 29 2012-09-07 06:04:31 PDT

Created attachment 162752 [details] Patch Fix EFL build

Antonio Gomes

Comment 30 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?

Allan Sandfeld Jensen

Comment 31 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.

WebKit Review Bot

Comment 32 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>

WebKit Review Bot

Comment 33 2012-09-07 09:09:02 PDT

All reviewed patches have been landed. Closing bug.

WebKit Review Bot

Comment 34 2012-09-12 22:01:38 PDT

Re-opened since this is blocked by 96600

yosin

Comment 35 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.

yosin

Comment 36 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

yosin

Comment 37 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)

Allan Sandfeld Jensen

Comment 38 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.

Allan Sandfeld Jensen

Comment 39 2013-02-13 06:03:21 PST

Created attachment 188065 [details] Patch Rebase and update to apply before patch for bug #95204

WebKit Review Bot

Comment 40 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.

Allan Sandfeld Jensen

Comment 41 2013-02-13 06:10:59 PST

Created attachment 188066 [details] Patch Fix a three missed rebase failures

WebKit Review Bot

Comment 42 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.

Julien Chaffraix

Comment 43 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

Maciej Stachowiak

Comment 44 2013-02-14 10:24:12 PST

Comment on attachment 188066 [details] Patch r=me on the WebKit2 part

Allan Sandfeld Jensen

Comment 45 2013-02-15 02:50:50 PST

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

Antonio Gomes

Comment 46 2013-02-15 06:07:03 PST

Comment on attachment 188066 [details] Patch Very nice clean up. Thanks for doing this!

Note You need to log in before you can comment on or make changes to this bug.