Update event regions only once per frame
Created attachment 405933 [details] Patch
<rdar://problem/66533779>
Comment on attachment 405933 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=405933&action=review > Source/WebCore/dom/Document.cpp:4296 > +#if ENABLE(IOS_TOUCH_EVENTS) > + // updateTouchEventRegions() needs to be called only on the top document. > + if (this == &topDocument()) > + updateTouchEventRegions(); > +#endif Seems wrong to iterate all documents and call this, and then have each document check if it’s the top one. Could we just move this back outside the iteration? > Source/WebCore/dom/Document.h:1134 > + void updateEventRegions(); Does this really need to be a member function? Seems like we could put this in the Page and maybe in RenderView and not add a function here.
(In reply to Darin Adler from comment #3) > Comment on attachment 405933 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=405933&action=review > > > Source/WebCore/dom/Document.cpp:4296 > > +#if ENABLE(IOS_TOUCH_EVENTS) > > + // updateTouchEventRegions() needs to be called only on the top document. > > + if (this == &topDocument()) > > + updateTouchEventRegions(); > > +#endif > > Seems wrong to iterate all documents and call this, and then have each > document check if it’s the top one. Could we just move this back outside the > iteration? That looks much messier at the call site; I'd have to add a "if (this == &topDocument()) return" there. > > Source/WebCore/dom/Document.h:1134 > > + void updateEventRegions(); > > Does this really need to be a member function? Seems like we could put this > in the Page and maybe in RenderView and not add a function here. The pattern in Page::updateRendering is to talk to each document. I think it's worthwhile for this function to exist here. Also, touch event code stores data on the document.
Comment on attachment 405933 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=405933&action=review >>> Source/WebCore/dom/Document.cpp:4296 >>> +#endif >> >> Seems wrong to iterate all documents and call this, and then have each document check if it’s the top one. Could we just move this back outside the iteration? > > That looks much messier at the call site; I'd have to add a "if (this == &topDocument()) return" there. Why would you?
Comment on attachment 405933 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=405933&action=review > Source/WebCore/page/Page.cpp:1549 > -#if ENABLE(IOS_TOUCH_EVENTS) > - // updateTouchEventRegions() needs to be called only on the top document. > - if (RefPtr<Document> document = mainFrame().document()) > - document->updateTouchEventRegions(); > -#endif > + forEachDocument([] (Document& document) { > + document.updateEventRegions(); > + }); Just leave the old code and add the new code here.
Because forEachDocument() will also hit the main document.
... and I want Document::updateEventRegions() to update touch event regions. That code will go away at some point.
Eh, I can revert that bit.
Created attachment 405982 [details] For EWS
Created attachment 405987 [details] For EWS
https://trac.webkit.org/changeset/265289/webkit