[webkit-reviews] review granted: [Bug 195256] [ContentChangeObserver] Content observation should be limited to the current document. : [Attachment 363459] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sun Mar 3 19:31:44 PST 2019


Simon Fraser (smfr) <simon.fraser at apple.com> has granted zalan
<zalan at apple.com>'s request for review:
Bug 195256: [ContentChangeObserver] Content observation should be limited to
the current document.
https://bugs.webkit.org/show_bug.cgi?id=195256

Attachment 363459: Patch

https://bugs.webkit.org/attachment.cgi?id=363459&action=review




--- Comment #8 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 363459
  --> https://bugs.webkit.org/attachment.cgi?id=363459
Patch

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

>>>>>> Source/WebCore/ChangeLog:8
>>>>>> +	It limits content observation to the target node's owner
document.
>>>>> 
>>>>> To other reviewers: I am not for or against this patch.
>>>>> 
>>>>> Why do we want to do make content change observers document-specific?
Interactions with an element can do anything including causing a content change
in a subframe (same-origin iframe is easy, cross-origin is still possible via
postMessage()). We know Microsoft Word online does the crazy with a subframe
that effectively follows the cursor. So there is craziness on the web. Like you
needed proof, right :)
>>>> 
>>>> :) yeah, that is very true. Do you think if that Microsoft Word use case
is a "menu hovering"-like case that we try to cover here? I still yet to see a
page where the hover triggers a "bring this menu-pane up in a different frame"
type of content change.
>>> 
>>> I don’t know. I could swear that I recall that Microsoft Word has their own
callout bar that they show when you tap or tap and hold, but whether that is in
a subframe or would be affected by this patch is something I don’t know off the
top of my head. There was a time, a long time ago when everyone was using a a
frameset and it was absolutely common to click something and for another frame
to change. We flatten frames on iOS, for now, but just pointing it out that it
was common for different frames to change. Unless you inspect source it will be
hard to tell what technique a site is using. Could use the prostenmez :)
approach and just “do it” and see what breaks. I personally prefer to the
Feynman algorithm though I’ve been known to gamble with “see what breaks” when
I like the odds.
>> 
>> I'm not sure if I'd call it a "callout bar", but it is true that Word shows
a contextual menu upon tap/long press; this contextual menu is contained in the
same subframe as the one that contains the actual selection ("sdx_ow_iframe").
> 
> Right and that's fine since the change in triggered by a click (and not by
mouse-move). I've seen many pages like that, but again not when the content
change is triggered by only hovering over an element.

Googling for "prostenmez" has no hits.

> Source/WebCore/dom/Document.cpp:539
> +    ,
m_contentChangeObserver(std::make_unique<ContentChangeObserver>(*this))

Maybe make one the first time we need to? Many documents are probably never
interacted with.

> Source/WebCore/page/ios/ContentChangeObserver.cpp:46
> -    if (!m_page.mainFrame().document())
> -	   return;
> -    if (m_page.mainFrame().document()->activeDOMObjectsAreSuspended())
> +    if (m_document.activeDOMObjectsAreSuspended())

This is so much nicer.


More information about the webkit-reviews mailing list