Bug 112450 – Clean up RenderFrameSet::nodeAtPoint

• Status: RESOLVED
• Resolution: FIXED
• Component: UI Events

[Allan Sandfeld Jensen]

As part of my work to remove side-effects in hit-testing I want to also eliminate any tests on the HitTestRequest::isReadOnly(), only one such remain and is in a cryptic test in RenderFrameSet::nodeAtPoint.

if (inside && frameSet()->noResize()
            && !request.readOnly() && !result.innerNode() && !request.touchMove())

Reseaching the history of the test shows that it has been invalid since r18333 in December 2006 where a part of the test was accidentally reversed from !element()->noResize to frameSet->noResize(). That has however had no effect since the check itself was dead code since r17770 in November 2006 where the newly introduced EventHandler started taking care of sending events the frameset being resized without needing a hit-test.

The check can therefore be removed and RenderFrameSet::nodeAtPoint made both cleaner and sane.

[Allan Sandfeld Jensen]

Created attachment 193328 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 193328 [details]
Patch

OK.  I assume we have some tests for this?  If not, we definitely need to add one.

[Darin Adler]

Comment on attachment 193328 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=193328&action=review

> Source/WebCore/rendering/RenderFrameSet.cpp:166
>      if (action != HitTestForeground)
>          return false;

Do we still need this part? What does this accomplish? I think we probably want to remove the entire function unless this has some benefit.

[Allan Sandfeld Jensen]

(In reply to comment #3)
> (From update of attachment 193328 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=193328&action=review
> 
> > Source/WebCore/rendering/RenderFrameSet.cpp:166
> >      if (action != HitTestForeground)
> >          return false;
> 
> Do we still need this part? What does this accomplish? I think we probably want to remove the entire function unless this has some benefit.

No, I guess not. RenderBox::nodeAtPoint also makes the check if (action == HitTestForeground).

[Allan Sandfeld Jensen]

Created attachment 193527 [details]
Patch for landing

[Allan Sandfeld Jensen]

Committed r146056: <http://trac.webkit.org/changeset/146056>