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
Created attachment 237824 [details] Patch for feedback
Seeking feedback on the approach.
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.
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.
I null-check the RenderView if the existing code did, but maybe should do it all the time.
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.
> Not a big fan of these default values. Specifically with the default values there is nothing indicating this type actually updates the layout.
(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.
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.
(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.
Other places that need this class (and may not have it in the patch) RenderEmbeddedObject::isReplacementObscured() RenderWidget::nodeAtPoint() FrameSelection::contains() ViewGestureGeometryCollector::collectGeometryForSmartMagnificationGesture() WebPage::dynamicViewportSizeUpdate()
Created attachment 238505 [details] Patch
I went with a watered-down version after chatting with hyatt.
*** Bug 136265 has been marked as a duplicate of this bug. ***
(In reply to comment #13) > I went with a watered-down version after chatting with hyatt. What arguments were made?
He thought that the first patch was ugly, and it was just much simpler to update layout inside RenderView::hitTest.
Comment on attachment 238505 [details] Patch r=me
Comment on attachment 238505 [details] Patch Clearing flags on attachment: 238505 Committed r173865: <http://trac.webkit.org/changeset/173865>
All reviewed patches have been landed. Closing bug.