Bug 215132 - Update event regions only once per frame
Summary: Update event regions only once per frame
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Compositing (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Simon Fraser (smfr)
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-08-04 11:36 PDT by Simon Fraser (smfr)
Modified: 2020-08-05 09:37 PDT (History)
14 users (show)

See Also:


Attachments
Patch (10.38 KB, patch)
2020-08-04 11:42 PDT, Simon Fraser (smfr)
darin: review+
Details | Formatted Diff | Diff
For EWS (20.89 KB, patch)
2020-08-04 21:08 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
For EWS (21.93 KB, patch)
2020-08-04 22:34 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2020-08-04 11:36:19 PDT
Update event regions only once per frame
Comment 1 Simon Fraser (smfr) 2020-08-04 11:42:42 PDT
Created attachment 405933 [details]
Patch
Comment 2 Simon Fraser (smfr) 2020-08-04 11:42:45 PDT
<rdar://problem/66533779>
Comment 3 Darin Adler 2020-08-04 12:46:05 PDT
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.
Comment 4 Simon Fraser (smfr) 2020-08-04 14:32:59 PDT
(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 5 Darin Adler 2020-08-04 14:34:50 PDT
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 6 Darin Adler 2020-08-04 14:35:24 PDT
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.
Comment 7 Simon Fraser (smfr) 2020-08-04 15:15:43 PDT
Because forEachDocument() will also hit the main document.
Comment 8 Simon Fraser (smfr) 2020-08-04 15:16:25 PDT
... and I want Document::updateEventRegions() to update touch event regions. That code will go away at some point.
Comment 9 Simon Fraser (smfr) 2020-08-04 15:17:56 PDT
Eh, I can revert that bit.
Comment 10 Simon Fraser (smfr) 2020-08-04 21:08:47 PDT
Created attachment 405982 [details]
For EWS
Comment 11 Simon Fraser (smfr) 2020-08-04 22:34:17 PDT
Created attachment 405987 [details]
For EWS
Comment 12 Simon Fraser (smfr) 2020-08-05 09:37:18 PDT
https://trac.webkit.org/changeset/265289/webkit