Bug 200609 – Disable ContentChangeObserver on the mobile version of youtube.com

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Alex Christensen]

Disable ContentChangeObserver on youtube.com

[Alex Christensen]

Created attachment 376000 [details]
Patch

[Alex Christensen]

<rdar://problem/54015403>

[Alex Christensen]

Created attachment 376001 [details]
Patch

[Maciej Stachowiak]

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

[Alex Christensen]

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.

[Alex Christensen]

I'm going to bed.  If someone wants to take this one home, be my guest.

[Tim Horton]

(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

[Maciej Stachowiak]

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

[Tim Horton]

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

[Simon Fraser (smfr)]

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.

[Alex Christensen]

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.

[Alex Christensen]

Created attachment 376013 [details]
Patch

[Alex Christensen]

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.

[Maciej Stachowiak]

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

[zalan]

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.

[Alex Christensen]

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

[zalan]

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