Bug 200609 - Disable ContentChangeObserver on the mobile version of youtube.com
Summary: Disable ContentChangeObserver on the mobile version of youtube.com
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alex Christensen
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-08-09 23:58 PDT by Alex Christensen
Modified: 2019-08-10 15:49 PDT (History)
11 users (show)

See Also:


Attachments
Patch (10.68 KB, patch)
2019-08-10 00:04 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (10.60 KB, patch)
2019-08-10 00:09 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (11.98 KB, patch)
2019-08-10 10:02 PDT, Alex Christensen
mjs: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Christensen 2019-08-09 23:58:25 PDT
Disable ContentChangeObserver on youtube.com
Comment 1 Alex Christensen 2019-08-10 00:04:24 PDT
Created attachment 376000 [details]
Patch
Comment 2 Alex Christensen 2019-08-10 00:04:27 PDT
<rdar://problem/54015403>
Comment 3 Alex Christensen 2019-08-10 00:09:46 PDT
Created attachment 376001 [details]
Patch
Comment 4 Maciej Stachowiak 2019-08-10 00:11:33 PDT
Comment on attachment 376001 [details]
Patch

We should almost certainly do this only in mobile content mode. Otherwise, this change is likely to break YouTube on iPad as it was necessary for reasonable hover interaction. (ContentChangeObserver isn't a web platform feature, it's just part of the new hover heuristic for desktop websites on iPad.)
Comment 5 Alex Christensen 2019-08-10 00:16:24 PDT
Comment on attachment 376001 [details]
Patch

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

> Source/WebCore/page/Quirks.cpp:141
> +bool Quirks::shouldDisableContentChangeObserver() const

I'm not sure where or how to check if it's in mobile content mode.  I have a Document here.  Any tips would be appreciated.
Comment 6 Alex Christensen 2019-08-10 00:24:02 PDT
I'm going to bed.  If someone wants to take this one home, be my guest.
Comment 7 Tim Horton 2019-08-10 00:41:34 PDT
(In reply to Alex Christensen from comment #5)
> Comment on attachment 376001 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=376001&action=review
> 
> > Source/WebCore/page/Quirks.cpp:141
> > +bool Quirks::shouldDisableContentChangeObserver() const
> 
> I'm not sure where or how to check if it's in mobile content mode.  I have a
> Document here.  Any tips would be appreciated.

You want WebsitePoliciesData
Comment 8 Maciej Stachowiak 2019-08-10 00:51:52 PDT
(In reply to Tim Horton from comment #7)
> (In reply to Alex Christensen from comment #5)
> > Comment on attachment 376001 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=376001&action=review
> > 
> > > Source/WebCore/page/Quirks.cpp:141
> > > +bool Quirks::shouldDisableContentChangeObserver() const
> > 
> > I'm not sure where or how to check if it's in mobile content mode.  I have a
> > Document here.  Any tips would be appreciated.
> 
> You want WebsitePoliciesData

That doesn't seem to exist anywhere in WebCore, and it also doesn't seem to directly contain the content mode (though it does contain some things controlled by it, like meta viewport mode). In fact I don't think any explicit knowledge of the content mode exists in the WebContent process. 

Could you be a bit more explicit in your suggestion? Does WebsitePoliciesData contain something that can be used to infer the content mode, and if so how does one get to it from WebCore?
Comment 9 Tim Horton 2019-08-10 00:55:32 PDT
(In reply to Maciej Stachowiak from comment #8)
> (In reply to Tim Horton from comment #7)
> > (In reply to Alex Christensen from comment #5)
> > > Comment on attachment 376001 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=376001&action=review
> > > 
> > > > Source/WebCore/page/Quirks.cpp:141
> > > > +bool Quirks::shouldDisableContentChangeObserver() const
> > > 
> > > I'm not sure where or how to check if it's in mobile content mode.  I have a
> > > Document here.  Any tips would be appreciated.
> > 
> > You want WebsitePoliciesData
> 
> That doesn't seem to exist anywhere in WebCore, and it also doesn't seem to
> directly contain the content mode (though it does contain some things
> controlled by it, like meta viewport mode). In fact I don't think any
> explicit knowledge of the content mode exists in the WebContent process. 

Agreed, but it's where you would go with such a bit if you were to add one.

> Could you be a bit more explicit in your suggestion? Does
> WebsitePoliciesData contain something that can be used to infer the content
> mode, and if so how does one get to it from WebCore?

You add a bit to WebsitePoliciesData, a bit to DocumentLoader, propagate it in WebsitePoliciesData::applyToDocumentLoader, and then you can get to it from WebCore.

Not beautiful, but it's how all the others work.
Comment 10 Simon Fraser (smfr) 2019-08-10 08:26:04 PDT
Comment on attachment 376001 [details]
Patch

Rather than disable all content observation, i think it would be better to figure out which specific behavior causes the bug. Content Observation is about whether to simulate hover or just send simulated clicks. There's a log channel you can turn on to figure out what it's doing.
Comment 11 Alex Christensen 2019-08-10 08:58:35 PDT
Good morning!  I'm back on this.  I'll add a mobile content only check and bisect this change to find out which behavior change is needed.
Comment 12 Alex Christensen 2019-08-10 10:02:30 PDT
Created attachment 376013 [details]
Patch
Comment 13 Alex Christensen 2019-08-10 12:46:58 PDT
Alan is looking into whether the quirk check should be moved to ContentChangeObserver::shouldObserveVisibilityChangeForElement (which I verified also fixes the problem) or an even less invasive change that has lower risk.
Comment 14 Maciej Stachowiak 2019-08-10 14:58:43 PDT
Comment on attachment 376013 [details]
Patch

This change seems to address previous comments and looks like it will work. I'm gonna guess quirks are untestable, but if not I'd suggest a test case. I think it would be good to land some working version ASAP, and consider refining further from there.

r=me
Comment 15 zalan 2019-08-10 15:37:22 PDT
the mobile version of youtube.com 
1. constructs a visible and actionable content
2. attaches it to the tree
3. and later destroys it completely
Content change observer (historically) stops at the first visible change.

I've seen this type of behavior on another page and was planning to
1. not stop at the first visible change but instead collect all the qualified nodes.
2. check if the collected nodes are still considered visible right at the end of the observation window.
Comment 16 Alex Christensen 2019-08-10 15:46:36 PDT
Alan's plan sounds like a good plan longer term, but too risky for this particular change.  I committed the patch to http://trac.webkit.org/r248502
Comment 17 zalan 2019-08-10 15:49:06 PDT
(In reply to Alex Christensen from comment #16)
> Alan's plan sounds like a good plan longer term, but too risky for this
> particular change.  I committed the patch to http://trac.webkit.org/r248502
Agree!