[WK2][Mac] WebPageProxy::supressVisibilityUpdates() should suppress visibility updates.
Created attachment 247118 [details] Patch
Created attachment 247136 [details] Patch
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.
(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.
(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.
Created attachment 247343 [details] Patch
Comment on attachment 247343 [details] Patch Clearing flags on attachment: 247343 Committed r180886: <http://trac.webkit.org/changeset/180886>
All reviewed patches have been landed. Closing bug.