Bug 70873 – [EFL] Invalidation request outside of visible area doesn't seem to occur when tiled view is used.

• Component: WebKit EFL
• Status: RESOLVED FIXED

[KwangHyuk]

Created attachment 112454 [details]
Refer the right edge side of attached image.

In order to allow pre-render or keeping of dirty rect outside of visible area, ewk_tiled_view is now using ewk_frame_paint_full_set api for the main_frame.
But, this doesn't seem effective for some sites like naver.com.
After the scroll, there was no dirty rect for the area outside of viewport. so unpainted area seems to occur when page tries to scroll by itself.
In order to help you be understood this issue, I attached two screenshots for naver.com.

To fix this, I think that ewk_frame_paint_full_set is better to be called from ewk_frame_view_create_for_view as soon as frame would be created.
In addition, single view doesn't need to call this api, view type check routine would be required to fix this.

[KwangHyuk]

Created attachment 114874 [details]
patch.

[Gyuyoung Kim]

Comment on attachment 114874 [details]
patch.

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

> Source/WebKit/efl/ewk/ewk_view.cpp:3932
> +

Is it better to add documentation for this new internal function ?

[KwangHyuk]

Created attachment 115129 [details]
Patch.

Apply documentation comment for new internal api.

[Grzegorz Czajkowski]

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

> Source/WebKit/efl/ewk/ewk_view.cpp:3927
> +#define EWK_VIEW_TILED_TYPE_CHECK_OR_RETURN(ewkView, ...) \

Can you move this to the define section, please?

> Source/WebKit/efl/ewk/ewk_view.cpp:3933
> +/**

Please move this function where internal functions are defined.

> Source/WebKit/efl/ewk/ewk_view.cpp:3935
> + * Reports it to him that FrameView is created.

I'd rather write: Reports that FrameView object has been created.
Please add empty line before detailed descriptions (started with *).

> Source/WebKit/efl/ewk/ewk_view.cpp:3936
> + * Paint area is set for full contents rect if view is an instance of ewk_view_tiled

Please describe it similar to ewk_frame_paint_full_set's doc.

> Source/WebKit/efl/ewk/ewk_view.cpp:3939
> + * @param ewkView View.

I would rather use "view object" instead of View (as we are using it in ewk_view's doc).
And please do not use dot at the end of params' doc.

> Source/WebKit/efl/ewk/ewk_view.cpp:3940
> + *

Please remove unneeded line.

> Source/WebKit/efl/ewk/ewk_view.cpp:3946
> +    ewk_frame_paint_full_set(smartData->main_frame, true);

Do we have a guarantee that for single backing store paintsEntireContents flag is set to false before creating FrameView as you wrote in doc? I am sorry for asking this I am not familiar with backing store. Or should we set it on false in case of single backing store.

Do we need to call ewk_frame_paint_full_set(smartData->main_frame, true) in _ewk_view_tiled_smart_add() (ewk_view_tiled.cpp) too ?

[KwangHyuk]

(In reply to comment #4)
> View in context: https://bugs.webkit.org/attachment.cgi?id=115129&action=review
> 
> > Source/WebKit/efl/ewk/ewk_view.cpp:3927
> > +#define EWK_VIEW_TILED_TYPE_CHECK_OR_RETURN(ewkView, ...) \
> 
> Can you move this to the define section, please?
> 
OK,

> > Source/WebKit/efl/ewk/ewk_view.cpp:3933
> > +/**
> 
> Please move this function where internal functions are defined.
> 
Ok too.

> > Source/WebKit/efl/ewk/ewk_view.cpp:3935
> > + * Reports it to him that FrameView is created.
> 
> I'd rather write: Reports that FrameView object has been created.
> Please add empty line before detailed descriptions (started with *).
> 
> > Source/WebKit/efl/ewk/ewk_view.cpp:3936
> > + * Paint area is set for full contents rect if view is an instance of ewk_view_tiled
> 
> Please describe it similar to ewk_frame_paint_full_set's doc.
> 
> > Source/WebKit/efl/ewk/ewk_view.cpp:3939
> > + * @param ewkView View.
> 
> I would rather use "view object" instead of View (as we are using it in ewk_view's doc).
> And please do not use dot at the end of params' doc.
> 
> > Source/WebKit/efl/ewk/ewk_view.cpp:3940
> > + *
> 
> Please remove unneeded line.
> 
Ok, I will.

> > Source/WebKit/efl/ewk/ewk_view.cpp:3946
> > +    ewk_frame_paint_full_set(smartData->main_frame, true);
> 
> Do we have a guarantee that for single backing store paintsEntireContents flag is set to false before creating FrameView as you wrote in doc? I am sorry for asking this I am not familiar with backing store. Or should we set it on false in case of single backing store.
> 
Sure, parent of FrameView (ScrollView) set it as false when it created and nobody calls it.
But if TILED_BACKING_STORE is enabled, it can be enabled although ewk_view_single is used.
TILED_BACKING_STORE is not enabled for webkit-efl yet.

> Do we need to call ewk_frame_paint_full_set(smartData->main_frame, true) in _ewk_view_tiled_smart_add() (ewk_view_tiled.cpp) too ?
Good point, let me check it too.

[KwangHyuk]

Created attachment 115321 [details]
patch.

[Grzegorz Czajkowski]

Thanks for your changes. 

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

> Source/WebKit/efl/ewk/ewk_view.cpp:3203
> + * Function sets if dirty areas should be repainted even if they are out of the screen

I would rather write: 
 * Allows to repaint the frame completely even if the areas are out of the screen
 * when @ewkView is an instance of ewk_view_tiled.

> Source/WebKit/efl/ewk/ewk_view.cpp:3206
> + * @param ewkView view object.

Please do not use dot at the end of parameters' description.

> Source/WebKit/efl/ewk/ewk_view.cpp:3208
> +void ewk_view_frame_view_creation_notify(const Evas_Object* ewkView)

In my opinion ewkView shouldn't have const modifier here because ewkView will be changed by ewk_frame_paint_full_set. As you can see this function doesn't have const.
I should mention about this in my previous review. I am sorry for this.

[KwangHyuk]

(In reply to comment #7)
> Thanks for your changes. 
> 
> View in context: https://bugs.webkit.org/attachment.cgi?id=115321&action=review
> 
> > Source/WebKit/efl/ewk/ewk_view.cpp:3203
> > + * Function sets if dirty areas should be repainted even if they are out of the screen
> 
> I would rather write: 
>  * Allows to repaint the frame completely even if the areas are out of the screen
>  * when @ewkView is an instance of ewk_view_tiled.
> 
> > Source/WebKit/efl/ewk/ewk_view.cpp:3206
> > + * @param ewkView view object.
> 
> Please do not use dot at the end of parameters' description.
Ok, it looks fine.

> 
> > Source/WebKit/efl/ewk/ewk_view.cpp:3208
> > +void ewk_view_frame_view_creation_notify(const Evas_Object* ewkView)
> 
> In my opinion ewkView shouldn't have const modifier here because ewkView will be changed by ewk_frame_paint_full_set. As you can see this function doesn't have const.
> I should mention about this in my previous review. I am sorry for this.
Looks reasonable.

[KwangHyuk]

Created attachment 115341 [details]
Patch updated.

[Grzegorz Czajkowski]

LGTM

[Gyuyoung Kim]

Comment on attachment 115341 [details]
Patch updated.

Attachment 115341 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10486688

[KwangHyuk]

Created attachment 115480 [details]
Patch updated.

[Raphael Kubo da Costa (:rakuco)]

I don't like the approach taken by the patch -- it makes ewk_frame access ewk_view and also makes ewk_view aware of a specific subclass of itself. This is really wrong in terms of OOP.

Comment #0 mentions two screenshots were going to be attached, but I can only see one. It's hard for me to understand the problem, is it the two white blocks on the right side which seem to be shown over some other content? Comment #0 also mentions this happens when a "page tries to scroll by itself". Is this the only way to trigger the issue? Is it possible to come up with a reduced test case?

[KwangHyuk]

Created attachment 115494 [details]
Refer the right edge side of attached image.

This is the screenshot after applying this patch.

[KwangHyuk]

(In reply to comment #13)
> I don't like the approach taken by the patch -- it makes ewk_frame access ewk_view and also makes ewk_view aware of a specific subclass of itself. This is really wrong in terms of OOP.
> 
Ok, I will try to find other way, may be, do you have any idea for this ?

> Comment #0 mentions two screenshots were going to be attached, but I can only see one. It's hard for me to understand the problem, is it the two white blocks on the right side which seem to be shown over some other content? Comment #0 also mentions this happens when a "page tries to scroll by itself". Is this the only way to trigger the issue? Is it possible to come up with a reduced test case?

I have attached 2nd screenshot which the problem is fixed by this patch.
I think that you can compare both of two.
And there is one more site that you can check.
Would you visit www.daum.net and then scroll down as soon as page load is finished you can see some blank area which must be filled with images.
(I will attach new screenshot for it soon)

And of course, need to use -b=tiled option for this test.

[KwangHyuk]

Created attachment 115505 [details]
Unpainted area is shown at daum.net after the scroll.

[KwangHyuk]

Created attachment 115506 [details]
There is no unpainted area after the patch.

[KwangHyuk]

(In reply to comment #15)
> (In reply to comment #13)
> > I don't like the approach taken by the patch -- it makes ewk_frame access ewk_view and also makes ewk_view aware of a specific subclass of itself. This is really wrong in terms of OOP.
> > 
> Ok, I will try to find other way, may be, do you have any idea for this ?
> 

Hi, Kubo,

As I think that view type must be checked to solve this matter properly, it must be notified to ewk_view when frame view is created.
It's the reason why ewk_frame calls ewk_view's api.

As a result, I would like to get your idea in order to remove ewk_view access from ewk_frame.

[KwangHyuk]

Created attachment 115520 [details]
Patch.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/efl/ChangeLog', u'Source/Web..." exit_code: 1

Source/WebKit/efl/WebCoreSupport/FrameLoaderClientEfl.cpp:946:  Tab found; better to use spaces  [whitespace/tab] [1]
Total errors found: 1 in 5 files


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

[KwangHyuk]

Created attachment 115522 [details]
Patch updated.

[Raphael Kubo da Costa (:rakuco)]

OK, I think I got what the patch is supposed to fix. ewk_frame_paint_full_set is only called once when the frame is created by ewk_view's smart add, however FrameLoaderClient::transitionToCommittedForNewPage can be called more than once with the same frame, but since each time it is called a new view is created.

This should be as easy as calling ewk_frame_paint_full_set with the boolean parameter depending on some ewk_frame or ewk_view (or even FrameLoaderClientEfl, I haven't thought of it a lot) that is persistent across the many FrameViews created. There are many ways to solve this, among them having subclasses of ewk_view setting some flag for this. My point is that it can be done in a better way, OOP-wise.

But even more important than this is a reduced test case. It is impossible to properly debug this kind of thing with a full-fledged, huge site such as naver or daum. We just need a small html page which triggers the same behavior so we can always make sure this never regresses.

[KwangHyuk]

(In reply to comment #22)
> OK, I think I got what the patch is supposed to fix. ewk_frame_paint_full_set is only called once when the frame is created by ewk_view's smart add, however FrameLoaderClient::transitionToCommittedForNewPage can be called more than once with the same frame, but since each time it is called a new view is created.
> 
> This should be as easy as calling ewk_frame_paint_full_set with the boolean parameter depending on some ewk_frame or ewk_view (or even FrameLoaderClientEfl, I haven't thought of it a lot) that is persistent across the many FrameViews created. There are many ways to solve this, among them having subclasses of ewk_view setting some flag for this. My point is that it can be done in a better way, OOP-wise.
> 
> But even more important than this is a reduced test case. It is impossible to properly debug this kind of thing with a full-fledged, huge site such as naver or daum. We just need a small html page which triggers the same behavior so we can always make sure this never regresses.

(In reply to comment #22)
> OK, I think I got what the patch is supposed to fix. ewk_frame_paint_full_set is only called once when the frame is created by ewk_view's smart add, however FrameLoaderClient::transitionToCommittedForNewPage can be called more than once with the same frame, but since each time it is called a new view is created.
> 
> This should be as easy as calling ewk_frame_paint_full_set with the boolean parameter depending on some ewk_frame or ewk_view (or even FrameLoaderClientEfl, I haven't thought of it a lot) that is persistent across the many FrameViews created. There are many ways to solve this, among them having subclasses of ewk_view setting some flag for this. My point is that it can be done in a better way, OOP-wise.
> 
> But even more important than this is a reduced test case. It is impossible to properly debug this kind of thing with a full-fledged, huge site such as naver or daum. We just need a small html page which triggers the same behavior so we can always make sure this never regresses.

Ok, I see, let me generate some simple test site sooner or later. :)

[KwangHyuk]

Created attachment 116568 [details]
Simple html file for the test.

- Just decompress test.tgz
- run EWebLauncher by use  -b=tiled file:///home/test/testpage/index.html.
- As soon as EWebLauncher displays the page, move horizontal scrollbar to the right direction.
You will see unpainted area out of viewport without this patch.
After applying this patch, it will make sure the well rendered page for the same step.

[KwangHyuk]

Created attachment 116931 [details]
Patch rebased.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/efl/ChangeLog', u'Source/Web..." exit_code: 1

Source/WebKit/efl/WebCoreSupport/FrameLoaderClientEfl.cpp:946:  Tab found; better to use spaces  [whitespace/tab] [1]
Total errors found: 1 in 5 files


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

[KwangHyuk]

Created attachment 116932 [details]
Patch updated.

[KwangHyuk]

Created attachment 118329 [details]
Patch rebased.

[Gyuyoung Kim]

Comment on attachment 118329 [details]
Patch rebased.

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

> Source/WebKit/efl/ewk/ewk_view_tiled.cpp:244
> +        api->sc.parent = (Evas_Smart_Class *)&_parent_sc;

Do not use C casting. Use static_cast<...>.

[KwangHyuk]

Created attachment 118358 [details]
Patch updated.

[KwangHyuk]

(In reply to comment #30)
> Created an attachment (id=118358) [details]
> Patch updated.

Done. :)

[Gyuyoung Kim]

Comment on attachment 118358 [details]
Patch updated.

Attachment 118358 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10781249

[KwangHyuk]

Created attachment 118361 [details]
Patch updated.

[Gyuyoung Kim]

Comment on attachment 118361 [details]
Patch updated.

I'm not a specialist for tiled backing store. But, LGTM.

[KwangHyuk]

Created attachment 119349 [details]
Patch rebased.

[KwangHyuk]

Created attachment 119960 [details]
Patch rebased.

[WebKit Review Bot]

Comment on attachment 119960 [details]
Patch rebased.

Rejecting attachment 119960 [details] from commit-queue.

New failing tests:
platform/chromium-linux/fast/text/international/complex-joining-using-gpos.html
media/track/track-cue-rendering.html
Full output: http://queues.webkit.org/results/10996365

[KwangHyuk]

Created attachment 120285 [details]
Patch rebased.

[KwangHyuk]

Created attachment 120286 [details]
Patch rebased.

[WebKit Review Bot]

Comment on attachment 120286 [details]
Patch rebased.

Clearing flags on attachment: 120286

Committed r103520: <http://trac.webkit.org/changeset/103520>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.