Update event regions only once per frame

• Component: Compositing
• Status: RESOLVED FIXED

[Simon Fraser (smfr)]

Update event regions only once per frame

[Simon Fraser (smfr)]

Created attachment 405933 [details]
Patch

[Simon Fraser (smfr)]

<rdar://problem/66533779>

[Darin Adler]

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.

[Simon Fraser (smfr)]

(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.

[Darin Adler]

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?

[Darin Adler]

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.

[Simon Fraser (smfr)]

Because forEachDocument() will also hit the main document.

[Simon Fraser (smfr)]

... and I want Document::updateEventRegions() to update touch event regions. That code will go away at some point.

[Simon Fraser (smfr)]

Eh, I can revert that bit.

[Simon Fraser (smfr)]

Created attachment 405982 [details]
For EWS

[Simon Fraser (smfr)]

Created attachment 405987 [details]
For EWS

[Simon Fraser (smfr)]

https://trac.webkit.org/changeset/265289/webkit