Bug 23587 - Eliminate boolean arguments in HitTestRequest ctor and replace with enum
: Eliminate boolean arguments in HitTestRequest ctor and replace with enum
Status: RESOLVED FIXED
: WebKit
Layout and Rendering
: 528+ (Nightly build)
: Other All
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2009-01-28 08:47 PST by
Modified: 2009-02-02 12:47 PST (History)


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


Note

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


Description From 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 From 2009-02-01 15:48:34 PST -------
Created an attachment (id=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 From 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 From 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 From 2009-02-01 17:45:54 PST -------
Created an attachment (id=27238) [details]
New version to incorporate comments

ClipToVisible -> IgnoreClipping

Fix the bitflags.
------- Comment #5 From 2009-02-01 17:51:48 PST -------
Latest patch has teensy oops at EventHandler.cpp:733.  I've fixed it.
------- Comment #6 From 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 From 2009-02-02 07:13:12 PST -------
Created an attachment (id=27246) [details]
Fixing oops and flip bool

New version flips the bool and updates the ChangeLog
------- Comment #8 From 2009-02-02 07:53:06 PST -------
(From update of attachment 27246 [details])
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 From 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 From 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 From 2009-02-02 12:47:06 PST -------
Fixed and landed with r40486.