Bug 23587 – Eliminate boolean arguments in HitTestRequest ctor and replace with enum

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Adam Treat]

The WebCore::HitTestRequest class has too many boolean arguments in the constructor.  This should be refactored to replace the boolean arguments with an enum.

[Adam Treat]

Created attachment 27236 [details]
Eliminate bools from HitTestRequest

Patch to eliminate ugly boolean arguments to HitTestRequest ctor.  Refactor and cleanup all the code that constructs and uses HitTestRequest.

[David Levin]

enum RequestType {
    ReadOnly = 0x0,
    Active = 0x1,
    MouseMove = 0x2,
    MouseUp = 0x4,
    ClipToVisible = 0x8
 };

I think it should be

enum RequestType {
    ReadOnly = 0x1,
    Active = 0x2,
    MouseMove = 0x4,
    MouseUp = 0x8,
    ClipToVisible = 0x10
 };

(right now readOnly() is never true.)

[Nikolas Zimmermann]

(In reply to comment #2)
> (right now readOnly() is never true.)

Yes, and I also think ClipToVisible obfuscates the code a bit. How about calling it 'IgnoreClipping' or sth. along the lines, to not force most call-sites to add the HitTestRequest::ClipToVisible flag. It's also a dangerous default behaviour to force callers to enable it explicitely. There is only a Qt platform specific codepath, that actually needs ClipToVisible.

Niko

[Adam Treat]

Created attachment 27238 [details]
New version to incorporate comments

ClipToVisible -> IgnoreClipping

Fix the bitflags.

[Adam Treat]

Latest patch has teensy oops at EventHandler.cpp:733.  I've fixed it.

[Holger Freyther]

Do you need to update the EventHandler.h hitTestResultAtPoint to change clipToVisible and flip the bool?

WebCore/page/EventHandler.cpp:727:        result = mainFrame->eventHandler()->hitTestResultAtPoint(mainFramePoint, allowShadowContent, clipToVisible);

[Adam Treat]

Created attachment 27246 [details]
Fixing oops and flip bool

New version flips the bool and updates the ChangeLog

[Nikolas Zimmermann]

Comment on attachment 27246 [details]
Fixing oops and flip bool

Looks great, r=me, in case no layout test broke.
Adam, can you verify that it doesn't break any mac layout test? If not, I can try the patch first, just mail me.

[Adam Treat]

Niko, unfortunately I can not verify as I don't own a mac.  Please apply the patch and let me know.

[Nikolas Zimmermann]

(In reply to comment #9)
> Niko, unfortunately I can not verify as I don't own a mac.  Please apply the
> patch and let me know.
We really need a trybot :-) Applying your patch as we speak, going to post results soon.

[Adam Treat]

Fixed and landed with r40486.