Bug 123379 – Change WebPage, WebPageProxy, WebPageCreationParameters to use ViewState

• Component: WebKit2
• Status: RESOLVED FIXED

[Gavin Barraclough]

Remove individual visibility flags from WebPageProxy, WebPageCreationParameters, and replace them with a ViewState.  This way we can pass a consistent visibility state from the UIProcess to the WebProcess.  Restructure WebPageProxy::viewStateDidChange to break the operation down into three steps:
    1) Update the m_viewState member in the WebPageProxy
    2) Send any messages to the WebPage to communicate state changes
    3) Perform any further work necessary in the UIProcess.
(In a following patch we'll merge the messages in step 2).

[Gavin Barraclough]

Created attachment 215233 [details]
Fix

[WebKit Commit Bot]

Attachment 215233 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/Shared/ViewState.h', u'Source/WebKit2/Shared/WebPageCreationParameters.cpp', u'Source/WebKit2/Shared/WebPageCreationParameters.h', u'Source/WebKit2/UIProcess/WebPageProxy.cpp', u'Source/WebKit2/UIProcess/WebPageProxy.h', u'Source/WebKit2/WebProcess/WebPage/WebPage.cpp', u'Source/WebKit2/WebProcess/WebPage/mac/TiledCoreAnimationDrawingArea.mm']" exit_code: 1
Source/WebKit2/Shared/WebPageCreationParameters.h:31:  "SessionState.h" already included at Source/WebKit2/Shared/WebPageCreationParameters.h:30  [build/include] [4]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Gavin Barraclough]

Created attachment 215234 [details]
style fix

[EFL EWS Bot]

Comment on attachment 215234 [details]
style fix

Attachment 215234 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/13328004

[Gavin Barraclough]

Created attachment 215311 [details]
Fix

[WebKit Commit Bot]

Attachment 215311 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/Shared/ViewState.h', u'Source/WebKit2/Shared/WebPageCreationParameters.cpp', u'Source/WebKit2/Shared/WebPageCreationParameters.h', u'Source/WebKit2/UIProcess/WebPageProxy.cpp', u'Source/WebKit2/UIProcess/WebPageProxy.h', u'Source/WebKit2/WebProcess/WebPage/WebPage.cpp', u'Source/WebKit2/WebProcess/WebPage/WebPage.h', u'Source/WebKit2/WebProcess/WebPage/WebPage.messages.in', u'Source/WebKit2/WebProcess/WebPage/mac/TiledCoreAnimationDrawingArea.mm']" exit_code: 1
Source/WebKit2/WebProcess/WebPage/WebPage.h:687:  The parameter name "isInWindow" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 215311 [details]
Fix

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

> Source/WebKit2/Shared/ViewState.h:42
> +    static const Flags Reset = 0;

Maybe NoFlags would be a better name than Reset?

> Source/WebKit2/UIProcess/WebPageProxy.h:755
> +    void updateViewState(ViewState::Flags = ViewState::AllFlags);

The purpose of this argument is not entirely clear. I suggest naming it something like flagsToUpdate.

>> Source/WebKit2/WebProcess/WebPage/WebPage.h:687
>> +    void setIsInWindow(bool isInWindow);
> 
> The parameter name "isInWindow" adds no information, so it should be removed.  [readability/parameter_name] [5]

I agree with the style bot; better to omit this name.

[Build Bot]

Comment on attachment 215311 [details]
Fix

Attachment 215311 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/12448066

New failing tests:
fast/dom/Window/mozilla-focus-blur.html

[Build Bot]

Created attachment 215315 [details]
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-13  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Geoffrey Garen]

> New failing tests:
> fast/dom/Window/mozilla-focus-blur.html

Is this test failure real? Seems like blur could be related to changes in view state.

[Sergio Correia (qrwteyrutiyoup)]

Hi Gavin,

If you wish to fix the GTK/EFK build prior to landing, just replace m_isPaintingSuspended(!parameters.isVisible) with m_isPaintingSuspended(!(parameters.viewState & ViewState::IsVisible)) in both

WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedDrawingArea.cpp:63 and
WebKit2/WebProcess/WebPage/DrawingAreaImpl.cpp:60, 

similar to your change in TiledCoreAnimationDrawingArea.mm.

[EFL EWS Bot]

Comment on attachment 215311 [details]
Fix

Attachment 215311 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/15288009

[Gavin Barraclough]

(In reply to comment #10)
> > New failing tests:
> > fast/dom/Window/mozilla-focus-blur.html
> 
> Is this test failure real? Seems like blur could be related to changes in view state.

This test is passing for me locally. :-/

[Gavin Barraclough]

(In reply to comment #11)
> Hi Gavin,
> 
> If you wish to fix the GTK/EFK build prior to landing, just replace m_isPaintingSuspended(!parameters.isVisible) with m_isPaintingSuspended(!(parameters.viewState & ViewState::IsVisible)) in both
> 
> WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedDrawingArea.cpp:63 and
> WebKit2/WebProcess/WebPage/DrawingAreaImpl.cpp:60, 
> 
> similar to your change in TiledCoreAnimationDrawingArea.mm.

Done!

[Gavin Barraclough]

Fixed in r158372