Bug 200609

Summary: Disable ContentChangeObserver on the mobile version of youtube.com
Product: WebKit Reporter: Alex Christensen <achristensen>
Component: New BugsAssignee: Alex Christensen <achristensen>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, dbates, ews, japhet, koivisto, mjs, simon.fraser, thorton, webkit-bug-importer, wenson_hsieh, zalan
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch mjs: review+

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!