Bug 71564 – [EFL] Invalidation request for ewk_view can be discarded without rendering.

• Component: WebKit EFL
• Status: RESOLVED FIXED

[KwangHyuk]

As ewk_view_layout_if_needed_recursive API causes invalidation request for the web view via ChromeClient, some invalidation requests can be discarded without rendering as long as it would be called after ewk_view_repaints_get api.
As for the ewk_view_single, this matter was fixed. But ewk_view_tiled still has had same issues.
So, this issues must be solved in ewk_view_tiled too.

[KwangHyuk]

Created attachment 115130 [details]
Patch.

[WebKit Review Bot]

Attachment 115130 [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/ewk/ewk_view.cpp:299:  One line control clauses should not use braces.  [whitespace/braces] [4]
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 115137 [details]
Patch.

[KwangHyuk]

Review me please. :)

[Raphael Kubo da Costa (:rakuco)]

Do you have some test case for this?

[KwangHyuk]

(In reply to comment #5)
> Do you have some test case for this?

Hi, Kubo, 
First of all, thank you for your review. :)

As you remember, in fact, this bug was created because of your idea that you mentioned for bug 70170 before.

As I recovered the code before bug 70170 was applied, you can use same test site like daum.net and  http://ie.microsoft.com/testdrive/Graphics/RequestAnimationFrame/Default.html by use ewk_view_single.

And this patch is commonly working for both single and tiled view too.
I hope to get your idea for the code first whether this patch looks reasonable.

Finally, If you want to see more simple test page, I can try it.

[Raphael Kubo da Costa (:rakuco)]

Hi there.

I find rendering related code very tricky, be it in ewk or in (especially) WebCore itself. As I was not involved in the writing of this part of the code in the port, it is even more difficult to assess what is going on behind the scenes in order to review it well. This is why it would be really helpful if you could produce a small test case -- sites such as microsoft's or daum's have a lot of other, unrelated code which make it hard to isolate the problem. With a small test case whose only purpose is to trigger this problem, we can even integrate it into LayoutTests/ so that this problem does not show up again (besides also making it easy to review the patch as I then have a small file to make some tests).

[KwangHyuk]

(In reply to comment #7)
> Hi there.
> 
> I find rendering related code very tricky, be it in ewk or in (especially) WebCore itself. As I was not involved in the writing of this part of the code in the port, it is even more difficult to assess what is going on behind the scenes in order to review it well. This is why it would be really helpful if you could produce a small test case -- sites such as microsoft's or daum's have a lot of other, unrelated code which make it hard to isolate the problem. With a small test case whose only purpose is to trigger this problem, we can even integrate it into LayoutTests/ so that this problem does not show up again (besides also making it easy to review the patch as I then have a small file to make some tests).

Ok, Let me generate any simple test soon. :)

[KwangHyuk]

Created attachment 119962 [details]
Patch rebased.

[Gyuyoung Kim]

Comment on attachment 119962 [details]
Patch rebased.

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

[KwangHyuk]

Created attachment 119976 [details]
Patch fixed.

[KwangHyuk]

Created attachment 120527 [details]
Simple test page related with this issue.

Simple test page was generated.

[KwangHyuk]

Created attachment 120533 [details]
patch rebased.

[KwangHyuk]

Created attachment 120534 [details]
title updated.

[Gyuyoung Kim]

Comment on attachment 120534 [details]
title updated.

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

Though I'm not specialist for this area, Looks better than before.

> Source/WebKit/efl/ewk/ewk_view.cpp:295
>      Eina_Rectangle* rect;

Why don't you move above code to 312 line ? As you know, C++ declares it when it is used.

[KwangHyuk]

Created attachment 120566 [details]
Patch updated.

[Gyuyoung Kim]

Comment on attachment 120566 [details]
Patch updated.

Though I'm not specialist for this area, looks better before.

[KwangHyuk]

Created attachment 120858 [details]
Patch rebased.

[KwangHyuk]

Created attachment 121195 [details]
Patch rebased.

[Hajime Morrita]

Comment on attachment 121195 [details]
Patch rebased.

Rubberstamping.

[WebKit Review Bot]

Comment on attachment 121195 [details]
Patch rebased.

Clearing flags on attachment: 121195

Committed r104160: <http://trac.webkit.org/changeset/104160>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.