Bug 150662

Summary: [CoordinatedGraphics] invisible webview should not paint the content
Product: WebKit Reporter: Ryuan Choi <ryuan.choi>
Component: WebKit EFLAssignee: Ryuan Choi <ryuan.choi>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, lucas.de.marchi
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Ryuan Choi 2015-10-28 21:29:19 PDT
CoordinatedDrawingArea can suspend or resume the painting when visibility of webpage is changed.
But, CoordinatedDrawingArea does not suspend the painting when invisible webview is created.
Comment 1 Ryuan Choi 2015-10-28 21:35:18 PDT
Created attachment 264296 [details]
Patch
Comment 2 Darin Adler 2015-10-31 14:32:03 PDT
Comment on attachment 264296 [details]
Patch

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

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedDrawingArea.cpp:-72
> -    , m_isPaintingSuspended(!(parameters.viewState & ViewState::IsVisible))

Not sure why we are changing the code to not initialize this. We need to give it some value at this point.

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedDrawingArea.cpp:79
> +    m_isPaintingSuspended = parameters.viewState & ViewState::IsVisible;

This doesn’t look right for the visible case. It’s going to set m_isPaintingSuspended to true in that case.

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedDrawingArea.cpp:81
> +    if (!m_isPaintingSuspended)
> +        suspendPainting();

Seems like what we actually want to do is to initialize m_isPaintingSuspended to false, and then write:

    if (!(parameters.viewState & ViewState::IsVisible))
        suspendPainting();

Also would be good to come up with a way to do a regression test.
Comment 3 Ryuan Choi 2015-11-01 03:54:25 PST
Created attachment 264513 [details]
Patch
Comment 4 Ryuan Choi 2015-11-01 03:58:52 PST
Comment on attachment 264296 [details]
Patch

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

>> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedDrawingArea.cpp:-72
>> -    , m_isPaintingSuspended(!(parameters.viewState & ViewState::IsVisible))
> 
> Not sure why we are changing the code to not initialize this. We need to give it some value at this point.

OK, I tried as another approach.

>> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedDrawingArea.cpp:81
>> +        suspendPainting();
> 
> Seems like what we actually want to do is to initialize m_isPaintingSuspended to false, and then write:
> 
>     if (!(parameters.viewState & ViewState::IsVisible))
>         suspendPainting();
> 
> Also would be good to come up with a way to do a regression test.

I followed like you mentioned.

But, I don't have idea to test this.
This is not to draw anything when view is created without visibility.
Comment 5 Darin Adler 2015-11-01 12:47:33 PST
Comment on attachment 264513 [details]
Patch

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

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedDrawingArea.cpp:80
> +    if (!(parameters.viewState & ViewState::IsVisible))
> +        suspendPainting();

I suggest initializing m_isPaintingSuspended to false, instead of initializing it based on the IsVisible flag.

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedDrawingArea.cpp:-408
> -    ASSERT(!m_isPaintingSuspended);

I suggest leaving this assertion in.
Comment 6 Ryuan Choi 2015-11-03 02:04:39 PST
Created attachment 264677 [details]
Patch
Comment 7 WebKit Commit Bot 2015-11-03 15:40:30 PST
Comment on attachment 264677 [details]
Patch

Clearing flags on attachment: 264677

Committed r191987: <http://trac.webkit.org/changeset/191987>
Comment 8 WebKit Commit Bot 2015-11-03 15:40:35 PST
All reviewed patches have been landed.  Closing bug.