Bug 86076 – [EFL] The view is not invalidated after going back

• Status: RESOLVED
• Resolution: WONTFIX
• Component: WebKit EFL

[Mikhail Pozdnyakov]

The view is not invalidated after going back. Started to happen after bug 85620 was landed. 
Probably because of unimplemented FrameLoaderClientEfl::transitionToCommittedFromCachedFrame().

[Mikhail Pozdnyakov]

Created attachment 141373 [details]
fix

[Gyuyoung Kim]

Comment on attachment 141373 [details]
fix

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

> Source/WebKit/efl/WebCoreSupport/FrameLoaderClientEfl.cpp:945
> +

Don't you need to consider viewport meta tag for cached frame. GTK port supports viewport meta tag in this function.

[Mikhail Pozdnyakov]

(In reply to comment #2)
> (From update of attachment 141373 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=141373&action=review
> 
> > Source/WebKit/efl/WebCoreSupport/FrameLoaderClientEfl.cpp:945
> > +
> 
> Don't you need to consider viewport meta tag for cached frame. GTK port supports viewport meta tag in this function.

Think viewport parameters invalidation is done already each time a frame is restored from cache. Here is how it happens

546	    ewk_view_viewport_attributes_set(m_view, arguments);
(gdb) bt
#0  WebCore::ChromeClientEfl::dispatchViewportPropertiesDidChange (this=0x80b85d8, arguments=...) at /usr/share/WebKit/Source/WebKit/efl/WebCoreSupport/ChromeClientEfl.cpp:546
#1  0xb43c87ed in WebCore::Chrome::dispatchViewportPropertiesDidChange (this=0x80582c8, arguments=...) at /usr/share/WebKit/Source/WebCore/page/Chrome.cpp:468
#2  0xb3fba604 in WebCore::Document::updateViewportArguments (this=0xaec4c218) at /usr/share/WebKit/Source/WebCore/dom/Document.cpp:3037
#3  0xb43fc2c8 in WebCore::Frame::setDocument (this=0x80be908, newDoc=...) at /usr/share/WebKit/Source/WebCore/page/Frame.cpp:309
#4  0xb43345ad in WebCore::FrameLoader::open (this=0x80be964, cachedFrame=...) at /usr/share/WebKit/Source/WebCore/loader/FrameLoader.cpp:1926
#5  0xb414c8c5 in WebCore::CachedFrame::open (this=0xad0681a0) at /usr/share/WebKit/Source/WebCore/history/CachedFrame.cpp:212
#6  0xb414e37a in WebCore::CachedPage::restore (this=0xad073588, page=0x80b8658) at /usr/share/WebKit/Source/WebCore/history/CachedPage.cpp:79

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 141373 [details]
fix

Is this covered by any test?

[Mikhail Pozdnyakov]

(In reply to comment #4)
> (From update of attachment 141373 [details])
> Is this covered by any test?

I guess I should write one..

[Thiago Marcos P. Santos]

What' the status of this bug? This is a critical issue IMO.

[Mikhail Pozdnyakov]

(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 141373 [details] [details])
> > Is this covered by any test?
> 
> I guess I should write one..

The problem I faced writing a pixel test for this issue is that even not-invalidated view generates image having an absolutely right content. So cannot test it with DRT yet..

[Mikhail Pozdnyakov]

(In reply to comment #7)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > (From update of attachment 141373 [details] [details] [details])
> > > Is this covered by any test?
> > 
> > I guess I should write one..
> 
> The problem I faced writing a pixel test for this issue is that even not-invalidated view generates image having an absolutely right content. So cannot test it with DRT yet..

And now I know the reason of the described problem:
createBitmapContextFromWebView in EFL port is using ecore_evas_buffer_pixels_get() to get the content of the pixel test result.

From ecore_evas_buffer_pixels_get() documentation at http://docs.enlightenment.org/auto/ecore/group__Ecore__Evas__Group.html:

"Besides returning a pointer to the actual pixel array of the given canvas, this call will force a rendering update on ee, first"

So EFL DRT invalidates the view itself when generating pixel test result.

[Gyuyoung Kim]

Comment on attachment 141373 [details]
fix

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

> Source/WebKit/efl/WebCoreSupport/FrameLoaderClientEfl.cpp:954
> +

Don't you need to check if CachedFrame has view object like gtk port ?

http://trac.webkit.org/browser/trunk/Source/WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp#L1209

[Mikhail Pozdnyakov]

(In reply to comment #9)
> (From update of attachment 141373 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=141373&action=review
> 
> > Source/WebKit/efl/WebCoreSupport/FrameLoaderClientEfl.cpp:954
> > +
> 
> Don't you need to check if CachedFrame has view object like gtk port ?
> 
> http://trac.webkit.org/browser/trunk/Source/WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp#L1209

I think it is checked here already

http://trac.webkit.org/browser/trunk/Source/WebCore/history/CachedFrame.cpp#L152

But I can add an extra assertion..

[Mikhail Pozdnyakov]

Created attachment 143272 [details]
patch v2 (added assertion)

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 143272 [details]
patch v2 (added assertion)

I was finally able to reproduce it with the steps described on IRC with EWebLauncher. So one thing that should be done here is to describe in the ChangeLog how the issue can be triggered, otherwise people won't know what this was supposed to fix.

So this seems to be an issue indeed, but I'm not sure calling invalidateRect() in that place is the right solution -- for one, the issue does not appear to affect the tiled backing store (I couldn't reproduce it by passing "-b tiled" to EWebLauncher), so it might be an issue in the single backing store code.

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 143272 [details]
patch v2 (added assertion)

Clearing flags after my last comment.

[Michael Catanzaro]

Closing this bug because the EFL port has been removed from trunk.

If you feel this bug applies to a different upstream WebKit port and was closed in error, please either update the title and reopen the bug, or leave a comment to request this.