Bug 23587 - Eliminate boolean arguments in HitTestRequest ctor and replace with enum
Summary: Eliminate boolean arguments in HitTestRequest ctor and replace with enum
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other All
: P2 Normal
Assignee: Adam Treat
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-01-28 08:47 PST by Adam Treat
Modified: 2009-02-02 12:47 PST (History)
3 users (show)

See Also:


Attachments
Eliminate bools from HitTestRequest (20.82 KB, patch)
2009-02-01 15:48 PST, Adam Treat
no flags Details | Formatted Diff | Diff
New version to incorporate comments (20.94 KB, patch)
2009-02-01 17:45 PST, Adam Treat
no flags Details | Formatted Diff | Diff
Fixing oops and flip bool (22.97 KB, patch)
2009-02-02 07:13 PST, Adam Treat
zimmermann: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Treat 2009-01-28 08:47:26 PST
The WebCore::HitTestRequest class has too many boolean arguments in the constructor.  This should be refactored to replace the boolean arguments with an enum.
Comment 1 Adam Treat 2009-02-01 15:48:34 PST
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.
Comment 2 David Levin 2009-02-01 16:51:59 PST
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.)

Comment 3 Nikolas Zimmermann 2009-02-01 16:57:24 PST
(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
Comment 4 Adam Treat 2009-02-01 17:45:54 PST
Created attachment 27238 [details]
New version to incorporate comments

ClipToVisible -> IgnoreClipping

Fix the bitflags.
Comment 5 Adam Treat 2009-02-01 17:51:48 PST
Latest patch has teensy oops at EventHandler.cpp:733.  I've fixed it.
Comment 6 Holger Freyther 2009-02-02 06:43:14 PST
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);



Comment 7 Adam Treat 2009-02-02 07:13:12 PST
Created attachment 27246 [details]
Fixing oops and flip bool

New version flips the bool and updates the ChangeLog
Comment 8 Nikolas Zimmermann 2009-02-02 07:53:06 PST
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.
Comment 9 Adam Treat 2009-02-02 08:03:38 PST
Niko, unfortunately I can not verify as I don't own a mac.  Please apply the patch and let me know.
Comment 10 Nikolas Zimmermann 2009-02-02 08:06:00 PST
(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.

Comment 11 Adam Treat 2009-02-02 12:47:06 PST
Fixed and landed with r40486.