Bug 123379

Summary: Change WebPage, WebPageProxy, WebPageCreationParameters to use ViewState
Product: WebKit Reporter: Gavin Barraclough <barraclough>
Component: WebKit2Assignee: Gavin Barraclough <barraclough>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, buildbot, commit-queue, eflews.bot, ggaren, gyuyoung.kim, philn, rego+ews, rniwa, sam, sergio, simon.fraser, thorton, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Fix
none
style fix
eflews.bot: commit-queue-
Fix
darin: review+, buildbot: commit-queue-
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2 none

Description Gavin Barraclough 2013-10-25 18:01:15 PDT
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).
Comment 1 Gavin Barraclough 2013-10-25 18:12:27 PDT
Created attachment 215233 [details]
Fix
Comment 2 WebKit Commit Bot 2013-10-25 18:14:00 PDT
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.
Comment 3 Gavin Barraclough 2013-10-25 18:16:06 PDT
Created attachment 215234 [details]
style fix
Comment 4 EFL EWS Bot 2013-10-25 18:23:56 PDT
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
Comment 5 Gavin Barraclough 2013-10-28 09:01:23 PDT
Created attachment 215311 [details]
Fix
Comment 6 WebKit Commit Bot 2013-10-28 09:02:51 PDT
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.
Comment 7 Darin Adler 2013-10-28 09:18:06 PDT
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.
Comment 8 Build Bot 2013-10-28 09:51:59 PDT
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
Comment 9 Build Bot 2013-10-28 09:52:01 PDT
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
Comment 10 Geoffrey Garen 2013-10-28 09:56:23 PDT
> 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.
Comment 11 Sergio Correia (qrwteyrutiyoup) 2013-10-28 10:13:52 PDT
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.
Comment 12 EFL EWS Bot 2013-10-28 10:30:39 PDT
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
Comment 13 Gavin Barraclough 2013-10-28 10:38:37 PDT
(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. :-/
Comment 14 Gavin Barraclough 2013-10-28 10:40:38 PDT
(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!
Comment 15 Gavin Barraclough 2013-10-31 11:05:31 PDT
Fixed in r158372