[WK2][Mac] WebPageProxy::supressVisibilityUpdates() should suppress visibility updates.

• Component: New Bugs
• Status: RESOLVED FIXED

[Jer Noble]

[WK2][Mac] WebPageProxy::supressVisibilityUpdates() should suppress visibility updates.

[Jer Noble]

Created attachment 247118 [details]
Patch

[Jer Noble]

Created attachment 247136 [details]
Patch

[Tim Horton]

Comment on attachment 247136 [details]
Patch

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

> Source/WebKit2/UIProcess/WebPageProxy.cpp:1231
> +        m_viewStateChangeDispatcher->schedule();

If one of the suppressed changes was a view-in-window change or any of the requests requested immediate dispatch, you should probably do that here instead of waiting for the next runloop.

> Source/WebKit2/UIProcess/WebPageProxy.cpp:1262
> +    if (m_suppressVisibilityUpdates)

It is super dangerous for any viewStateDidChange call that wantsSynchronousReply or immediate dispatch to bail like this, because often the caller will be blocking waiting for a reply that won't come.

You should add an assert or logging or something here. Or something? It's just scary and has caused lots of trouble in the past.

[Jer Noble]

(In reply to comment #3)
> Comment on attachment 247136 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=247136&action=review
> 
> > Source/WebKit2/UIProcess/WebPageProxy.cpp:1231
> > +        m_viewStateChangeDispatcher->schedule();
> 
> If one of the suppressed changes was a view-in-window change or any of the
> requests requested immediate dispatch, you should probably do that here
> instead of waiting for the next runloop.

Well, the immediate dispatch thing should never happen if we add an ASSERT, as you suggest below.  But why should we do the view-in-window change immediately? It's already been delayed; what's wrong with delaying a bit longer?

> > Source/WebKit2/UIProcess/WebPageProxy.cpp:1262
> > +    if (m_suppressVisibilityUpdates)
> 
> It is super dangerous for any viewStateDidChange call that
> wantsSynchronousReply or immediate dispatch to bail like this, because often
> the caller will be blocking waiting for a reply that won't come.
> 
> You should add an assert or logging or something here. Or something? It's
> just scary and has caused lots of trouble in the past.

Ok, lets add an ASSERT. From looking at all the call points, this should never get hit, but better safe than sorry.

[Jer Noble]

(In reply to comment #4)
> (In reply to comment #3)
> > Comment on attachment 247136 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=247136&action=review
> > 
> > > Source/WebKit2/UIProcess/WebPageProxy.cpp:1231
> > > +        m_viewStateChangeDispatcher->schedule();
> > 
> > If one of the suppressed changes was a view-in-window change or any of the
> > requests requested immediate dispatch, you should probably do that here
> > instead of waiting for the next runloop.
> 
> Well, the immediate dispatch thing should never happen if we add an ASSERT,
> as you suggest below.  But why should we do the view-in-window change
> immediately? It's already been delayed; what's wrong with delaying a bit
> longer?
> 
> > > Source/WebKit2/UIProcess/WebPageProxy.cpp:1262
> > > +    if (m_suppressVisibilityUpdates)
> > 
> > It is super dangerous for any viewStateDidChange call that
> > wantsSynchronousReply or immediate dispatch to bail like this, because often
> > the caller will be blocking waiting for a reply that won't come.
> > 
> > You should add an assert or logging or something here. Or something? It's
> > just scary and has caused lots of trouble in the past.
> 
> Ok, lets add an ASSERT. From looking at all the call points, this should
> never get hit, but better safe than sorry.

Actually, looking at all the call sites, I think it would be safe here to just ignore the m_suppressVisibilityUpdates flag if immediate dispatch is requested.

[Jer Noble]

Created attachment 247343 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 247343 [details]
Patch

Clearing flags on attachment: 247343

Committed r180886: <http://trac.webkit.org/changeset/180886>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.