Bug 113171 - Allow ShadowContents in HitTests by default.
Summary: Allow ShadowContents in HitTests by default.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Hayato Ito
URL:
Keywords:
Depends on:
Blocks: 112096
  Show dependency treegraph
 
Reported: 2013-03-24 22:18 PDT by Hayato Ito
Modified: 2013-03-26 19:42 PDT (History)
20 users (show)

See Also:


Attachments
Refactoring to prevent further HitTests which disallow ShadowContents (35.69 KB, patch)
2013-03-24 23:21 PDT, Hayato Ito
no flags Details | Formatted Diff | Diff
Update also Source/WebKit and Source/WebKit2. Let me watch the result of ews. (60.77 KB, patch)
2013-03-25 00:59 PDT, Hayato Ito
no flags Details | Formatted Diff | Diff
Fix a build hopefully (61.32 KB, patch)
2013-03-26 01:03 PDT, Hayato Ito
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hayato Ito 2013-03-24 22:18:14 PDT
HitTestRequest is widely used in WebCore, but AllowShadowContents flag is not turned on by default.
In most of such places, we can set the flag on.

To track all existing HitTests which does not allow Shadow Contents, and discourage further HitTests which does now allow Shadow Contents in the future, we should turn AllowShadowContents flag on by default.

Instead of AllowShadowContent flag, we should introduce disallowShadowContenet flag so that callers must set this flag on explicitly if they want to disallow Shadow Trees in HitTests,.

This change should be just refactoring and should not include any behavior changes. 
After this change, we'll investigate each place where disallowShadowContents is used and get rid of the flag if it is okay to remove.
Comment 1 Hayato Ito 2013-03-24 23:21:25 PDT
Created attachment 194790 [details]
Refactoring to prevent further HitTests which disallow ShadowContents
Comment 2 Build Bot 2013-03-25 00:17:14 PDT
Comment on attachment 194790 [details]
Refactoring to prevent further HitTests which disallow ShadowContents

Attachment 194790 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17242359
Comment 3 Hayato Ito 2013-03-25 00:21:30 PDT
Let me fix the build of mac-wk2.

(In reply to comment #2)
> (From update of attachment 194790 [details])
> Attachment 194790 [details] did not pass mac-wk2-ews (mac-wk2):
> Output: http://webkit-commit-queue.appspot.com/results/17242359
Comment 4 Peter Beverloo (cr-android ews) 2013-03-25 00:24:50 PDT
Comment on attachment 194790 [details]
Refactoring to prevent further HitTests which disallow ShadowContents

Attachment 194790 [details] did not pass cr-android-ews (chromium-android):
Output: http://webkit-commit-queue.appspot.com/results/17312203
Comment 5 Hayato Ito 2013-03-25 00:28:42 PDT
Looks like I have to update Source/WebKit, Source/WebKit2 in addition to Source/WebCore.
HitTests are also used there.
Comment 6 Hayato Ito 2013-03-25 00:59:49 PDT
Created attachment 194797 [details]
Update also Source/WebKit and Source/WebKit2. Let me watch the result of ews.
Comment 7 Hayato Ito 2013-03-25 01:14:08 PDT
I am wondering how I should test this kind of change, which updated files in a lot of ports.
I've tested the patch on chromium-linux, but looks like it is not enough.
Is there any idea? Can I trust the test coverage of ews?
Comment 8 EFL EWS Bot 2013-03-25 01:15:24 PDT
Comment on attachment 194797 [details]
Update also Source/WebKit and Source/WebKit2. Let me watch the result of ews.

Attachment 194797 [details] did not pass efl-ews (efl):
Output: http://webkit-commit-queue.appspot.com/results/17322067
Comment 9 Peter Beverloo (cr-android ews) 2013-03-25 02:14:21 PDT
Comment on attachment 194797 [details]
Update also Source/WebKit and Source/WebKit2. Let me watch the result of ews.

Attachment 194797 [details] did not pass cr-android-ews (chromium-android):
Output: http://webkit-commit-queue.appspot.com/results/17302179
Comment 10 Dimitri Glazkov (Google) 2013-03-25 10:09:51 PDT
(In reply to comment #5)
> Looks like I have to update Source/WebKit, Source/WebKit2 in addition to Source/WebCore.
> HitTests are also used there.

I'd say that if all bubbles are green, we're good to go. This is great, thank you for making this change.
Comment 11 Hayato Ito 2013-03-26 01:03:37 PDT
Created attachment 195024 [details]
Fix a build hopefully
Comment 12 WebKit Review Bot 2013-03-26 19:42:33 PDT
Comment on attachment 195024 [details]
Fix a build hopefully

Clearing flags on attachment: 195024

Committed r146961: <http://trac.webkit.org/changeset/146961>
Comment 13 WebKit Review Bot 2013-03-26 19:42:38 PDT
All reviewed patches have been landed.  Closing bug.