Bug 136651 – Ensure that layout is up-to-date before hit-testing via RenderView

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Simon Fraser (smfr)]

Add a stack-based helper to ensure that layout is up-to-date before getting the RenderView, and use it in places that hit-test

[Simon Fraser (smfr)]

Created attachment 237824 [details]
Patch for feedback

[Simon Fraser (smfr)]

Seeking feedback on the approach.

[WebKit Commit Bot]

Attachment 237824 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/DragImage.cpp:137:  The parameter name "renderView" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Sam Weinig]

I'm not completely clear on the nullity of things?  It seems like you sometimes need to null check the RenderView, and sometimes don't.

[Simon Fraser (smfr)]

I null-check the RenderView if the existing code did, but maybe should do it all the time.

[Antti Koivisto]

Comment on attachment 237824 [details]
Patch for feedback

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

Where are you planning to go with this type? The current patch essentially just adds some scattered assertions so it is not clear from it. 

I think this is a great idea but the goal should be to expand it everywhere. That is, you shouldn't be able to get a renderer pointer (from Element for example) without first requesting access. Also we should differentiate between read and write access with the former giving back const renderer reference.

> Source/WebCore/rendering/RenderTreeAccess.h:38
> +class RenderTreeAccess {

What sort of access? Read? Read-write?

> Source/WebCore/rendering/RenderTreeAccess.h:46
> +    RenderTreeAccess(Frame&, LayoutUpdateType = UpdateLayout);
> +    RenderTreeAccess(Document&, LayoutUpdateType = UpdateLayout);

Not a big fan of these default values.

> Source/WebCore/rendering/RenderTreeAccess.h:50
> +    RenderView* renderView();

I think this should be a reference. Constructors should assert document.hasLivingRenderTree(). Clients should not use this class without a living tree and the class should prevent render tree teardown.

> Source/WebCore/rendering/RenderTreeAccess.h:52
> +    void ensureLayoutUpdated(LayoutUpdateType = UpdateLayout);

This is not used externally so should be private. Any client doing tree mutating things should drop and reacquire the render tree (read) access instead of using this.

[Antti Koivisto]

> Not a big fan of these default values.

Specifically with the default values there is nothing indicating this type actually updates the layout.

[Darin Adler]

(In reply to comment #6)
> That is, you shouldn't be able to get a renderer pointer (from Element for example) without first requesting access.

If practical, to go along with this the renderer pointers should be smart pointers that, in debug builds, assert if used after additional DOM mutation. Much like hash table iterators.

[Simon Fraser (smfr)]

All good ideas, thanks!

> I think this should be a reference. Constructors should assert document.hasLivingRenderTree(). Clients should not use this class without a living tree and the class should prevent render tree teardown.

That's an interesting thought.

There are lots of access of RenderView which don't need to force layout; they just get the RenderView's RenderLayer, for example. So I don't think we can say that EVERY access of the render tree should force layout to be updated.

[Antti Koivisto]

(In reply to comment #9)
> There are lots of access of RenderView which don't need to force layout; they just get the RenderView's RenderLayer, for example. So I don't think we can say that EVERY access of the render tree should force layout to be updated.

Right, there should be a way to request access without updating the layout. In that case we can still assert for example that no mutations happens until the access is dropped.

[Simon Fraser (smfr)]

Other places that need this class (and may not have it in the patch)

RenderEmbeddedObject::isReplacementObscured()
RenderWidget::nodeAtPoint()
FrameSelection::contains()
ViewGestureGeometryCollector::collectGeometryForSmartMagnificationGesture()
WebPage::dynamicViewportSizeUpdate()

[Simon Fraser (smfr)]

Created attachment 238505 [details]
Patch

[Simon Fraser (smfr)]

I went with a watered-down version after chatting with hyatt.

[Simon Fraser (smfr)]

*** Bug 136265 has been marked as a duplicate of this bug. ***

[Sam Weinig]

(In reply to comment #13)
> I went with a watered-down version after chatting with hyatt.

What arguments were made?

[Simon Fraser (smfr)]

He thought that the first patch was ugly, and it was just much simpler to update layout inside RenderView::hitTest.

[Dave Hyatt]

Comment on attachment 238505 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 238505 [details]
Patch

Clearing flags on attachment: 238505

Committed r173865: <http://trac.webkit.org/changeset/173865>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.