Bug 138305

Summary: [EFL] Improve previous temporary fix against drawing (0,0) position before rendering content of next web page
Product: WebKit Reporter: Gyuyoung Kim <gyuyoung.kim>
Component: WebKit EFLAssignee: Gyuyoung Kim <gyuyoung.kim>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, gyuyoung.kim, lucas.de.marchi, ossy, ryuan.choi, yoon
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 133300    
Bug Blocks:    
Attachments:
Description Flags
WIP
none
Patch
none
another_approach
none
another_approach2
none
Patch none

Description Gyuyoung Kim 2014-11-03 03:32:20 PST
In EFL port, there was to draw current content at (0, 0) scroll position before drawing next page. r170243 fixed it using a flag for the occasion though, we should fix it with more correct way.
Comment 1 Gyuyoung Kim 2014-11-03 03:42:47 PST
Created attachment 240838 [details]
WIP
Comment 2 Gyuyoung Kim 2014-11-03 18:30:36 PST
Created attachment 240896 [details]
Patch
Comment 3 Gyuyoung Kim 2014-11-03 18:38:41 PST
CC'ing Ossy.
Comment 4 Ryuan Choi 2014-11-03 20:28:04 PST
Created attachment 240905 [details]
another_approach
Comment 5 Ryuan Choi 2014-11-03 20:30:22 PST
(In reply to comment #4)
> Created attachment 240905 [details]
> another_approach

Dear Gyuyoung,
As mentioned in IRC,

I uplodated my approach based on analysis.

Feel free to check this.
I hope that this is helpfull for you
Comment 6 Ryuan Choi 2014-11-03 20:47:50 PST
Created attachment 240907 [details]
another_approach2
Comment 7 Gyuyoung Kim 2014-11-03 23:49:21 PST
Comment on attachment 240907 [details]
another_approach2

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

> Source/WebKit2/ChangeLog:13
> +        rendered the results of first visually not empty layout.

I modify this description a little.

"WebKit2 has freezed layer tree until frame load completion since r101838. In EFL port, we unfreeze it when PageViewportController::pageTransitionViewportReady() is called though, UIProcess on EFL port updates cairo surface during the freezing time of layer tree. Thus UIProcess should not update the layer trees until the layerTreeState is unfrozen."

> Source/WebKit2/UIProcess/CoordinatedGraphics/PageViewportController.h:107
> +    enum class PageState {

I think that it would be nicer if we use variable or enum name based on "layerTreeStateIsFrozen" to improve our code readability. Under my understanding, this state is depended on m_layerTreeStateIsFrozen state.
Comment 8 Ryuan Choi 2014-11-04 02:38:16 PST
Created attachment 240919 [details]
Patch
Comment 9 Ryuan Choi 2014-11-04 02:50:19 PST
Comment on attachment 240907 [details]
another_approach2

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

>> Source/WebKit2/UIProcess/CoordinatedGraphics/PageViewportController.h:107
>> +    enum class PageState {
> 
> I think that it would be nicer if we use variable or enum name based on "layerTreeStateIsFrozen" to improve our code readability. Under my understanding, this state is depended on m_layerTreeStateIsFrozen state.

After considered more, boolean looks enough like your original approach.

I changed this as boolean member, m_layerTreeStateIsFrozen
Comment 10 Gyuyoung Kim 2014-11-05 00:08:53 PST
Comment on attachment 240919 [details]
Patch

LGTM, r=me.
Comment 11 Gyuyoung Kim 2014-11-05 00:56:22 PST
Before landing, could you check if there is any side effect ? It looks black screen is drawn as soon as the MiniBrowser is shown.
Comment 12 Gyuyoung Kim 2014-11-05 00:58:03 PST
Comment on attachment 240919 [details]
Patch

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

> Source/WebKit2/UIProcess/CoordinatedGraphics/PageViewportController.cpp:273
> +    if (!m_layerTreeStateIsFrozen)

I'm not sure if we can block during the m_layerTreeStateIsFrozen is true. If we block to update display, can black screen be shown during the period ?
Comment 13 Ryuan Choi 2014-11-05 05:54:05 PST
(In reply to comment #12)
> Comment on attachment 240919 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=240919&action=review
> 
> > Source/WebKit2/UIProcess/CoordinatedGraphics/PageViewportController.cpp:273
> > +    if (!m_layerTreeStateIsFrozen)
> 
> I'm not sure if we can block during the m_layerTreeStateIsFrozen is true. If
> we block to update display, can black screen be shown during the period ?

Well, I can't imagine that scenario.
Are you considering with direct rendering feature?

In that scenario, previous screen will be used because we didn't remove it.
Comment 14 Gyuyoung Kim 2014-11-05 20:48:44 PST
(In reply to comment #13)
> (In reply to comment #12)
> > Comment on attachment 240919 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=240919&action=review
> > 
> > > Source/WebKit2/UIProcess/CoordinatedGraphics/PageViewportController.cpp:273
> > > +    if (!m_layerTreeStateIsFrozen)
> > 
> > I'm not sure if we can block during the m_layerTreeStateIsFrozen is true. If
> > we block to update display, can black screen be shown during the period ?
> 
> Well, I can't imagine that scenario.
> Are you considering with direct rendering feature?
> 
> In that scenario, previous screen will be used because we didn't remove it.

This problem occurs on current WebKit EFL MiniBrowser. Other previous commit looks like to made this problem.
Comment 15 WebKit Commit Bot 2014-11-05 21:34:57 PST
Comment on attachment 240919 [details]
Patch

Clearing flags on attachment: 240919

Committed r175669: <http://trac.webkit.org/changeset/175669>
Comment 16 WebKit Commit Bot 2014-11-05 21:35:02 PST
All reviewed patches have been landed.  Closing bug.