Bug 71791 - [EFL] Add implementation considering ewk_tiled_backing_store's visible status.
Summary: [EFL] Add implementation considering ewk_tiled_backing_store's visible status.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-11-08 03:14 PST by KwangHyuk
Modified: 2011-11-24 05:35 PST (History)
8 users (show)

See Also:


Attachments
Patch. (4.52 KB, patch)
2011-11-15 00:13 PST, KwangHyuk
no flags Details | Formatted Diff | Diff
Patch. (4.52 KB, patch)
2011-11-15 00:48 PST, KwangHyuk
no flags Details | Formatted Diff | Diff
Patch. (1.13 KB, patch)
2011-11-15 01:54 PST, KwangHyuk
no flags Details | Formatted Diff | Diff
Patch again. (4.59 KB, patch)
2011-11-15 01:56 PST, KwangHyuk
no flags Details | Formatted Diff | Diff
Patch. (4.63 KB, patch)
2011-11-16 00:51 PST, KwangHyuk
no flags Details | Formatted Diff | Diff
Patch rebased. (4.62 KB, patch)
2011-11-20 23:04 PST, KwangHyuk
no flags Details | Formatted Diff | Diff
Patch updated. (1.01 KB, patch)
2011-11-21 23:33 PST, KwangHyuk
no flags Details | Formatted Diff | Diff
Patch. (3.47 KB, patch)
2011-11-21 23:34 PST, KwangHyuk
no flags Details | Formatted Diff | Diff
Patch rebased. (3.48 KB, patch)
2011-11-23 20:47 PST, KwangHyuk
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description KwangHyuk 2011-11-08 03:14:08 PST
The ewk's tiled backing store also disappear whenever ewk_view disappear caused by evas_object_hide call.
But, rendering is still allowed although it's not visible and in inactive status.
Therefore, ewk's tiled backing store should consider status whether it's active or inactive.
Comment 1 KwangHyuk 2011-11-15 00:13:31 PST
Created attachment 115109 [details]
Patch.
Comment 2 Gyuyoung Kim 2011-11-15 00:22:32 PST
Comment on attachment 115109 [details]
Patch.

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

> Source/WebKit/efl/ewk/ewk_tiled_backing_store.cpp:74
> +        Eina_Bool visible:1;

We don't use Eina_Bool in internal implementation anymore.
Comment 3 KwangHyuk 2011-11-15 00:48:48 PST
Created attachment 115114 [details]
Patch.
Comment 4 Gyuyoung Kim 2011-11-15 01:00:05 PST
Comment on attachment 115114 [details]
Patch.

LGTM.
Comment 5 KwangHyuk 2011-11-15 01:54:07 PST
Created attachment 115126 [details]
Patch.

As I missed two lines for this patch, I newly added them into smart_show and smart_hie.
Comment 6 KwangHyuk 2011-11-15 01:56:02 PST
Created attachment 115128 [details]
Patch again.
Comment 7 Ryuan Choi 2011-11-15 17:36:26 PST
Looks good to me.
Comment 8 Grzegorz Czajkowski 2011-11-15 23:35:58 PST
View in context: https://bugs.webkit.org/attachment.cgi?id=115128&action=review

> Source/WebKit/efl/ewk/ewk_tiled_backing_store.cpp:419
> +        return false;

Do we need above calculations (currentColumn, currentRow, lastUsed) in case of !visible mode?
Comment 9 KwangHyuk 2011-11-15 23:55:51 PST
(In reply to comment #8)
> View in context: https://bugs.webkit.org/attachment.cgi?id=115128&action=review
> 
> > Source/WebKit/efl/ewk/ewk_tiled_backing_store.cpp:419
> > +        return false;
> 
> Do we need above calculations (currentColumn, currentRow, lastUsed) in case of !visible mode?

Yes, it can be.
For example, cases like when content size is changed or load finished, they can cause it.
Comment 10 KwangHyuk 2011-11-16 00:41:03 PST
(In reply to comment #9)
> (In reply to comment #8)
> > View in context: https://bugs.webkit.org/attachment.cgi?id=115128&action=review
> > 
> > > Source/WebKit/efl/ewk/ewk_tiled_backing_store.cpp:419
> > > +        return false;
> > 
> > Do we need above calculations (currentColumn, currentRow, lastUsed) in case of !visible mode?
> Yes, it can be.
> For example, cases like when content size is changed or load finished, they can cause it.

Ah, I have misunderstood your comment.
I will consider those variables soon.
Comment 11 KwangHyuk 2011-11-16 00:51:11 PST
Created attachment 115344 [details]
Patch.
Comment 12 Grzegorz Czajkowski 2011-11-16 02:31:30 PST
View in context: https://bugs.webkit.org/attachment.cgi?id=115344&action=review

> Source/WebKit/efl/ewk/ewk_tiled_backing_store.cpp:415
> +        return false;

I am wondering if we really have to return false in this case. The function tends to return false only in case of failures like:
if (!tile)
   return false;

In case of:
else if (old->row == currentRow && old->col == currentColumn && old->zoom == zoom)
   goto end; // true is returned

I would rather hear specialists of backing store about this.
Comment 13 KwangHyuk 2011-11-16 18:19:42 PST
(In reply to comment #12)
> View in context: https://bugs.webkit.org/attachment.cgi?id=115344&action=review
> 
> > Source/WebKit/efl/ewk/ewk_tiled_backing_store.cpp:415
> > +        return false;
> 
> I am wondering if we really have to return false in this case. The function tends to return false only in case of failures like:
> if (!tile)
>    return false;
> 
> In case of:
> else if (old->row == currentRow && old->col == currentColumn && old->zoom == zoom)
>    goto end; // true is returned
> 
> I would rather hear specialists of backing store about this.

From _ewk_tiled_backing_store_item_add api, you can notice the reason why this must be false. :)
Comment 14 Grzegorz Czajkowski 2011-11-16 23:05:12 PST
(In reply to comment #13)
> (In reply to comment #12)
> > View in context: https://bugs.webkit.org/attachment.cgi?id=115344&action=review
> > 
> > > Source/WebKit/efl/ewk/ewk_tiled_backing_store.cpp:415
> > > +        return false;
> > 
> > I am wondering if we really have to return false in this case. The function tends to return false only in case of failures like:
> > if (!tile)
> >    return false;
> > 
> > In case of:
> > else if (old->row == currentRow && old->col == currentColumn && old->zoom == zoom)
> >    goto end; // true is returned
> > 
> > I would rather hear specialists of backing store about this.
> From _ewk_tiled_backing_store_item_add api, you can notice the reason why this must be false. :)

In thie case LGTM,
Comment 15 KwangHyuk 2011-11-20 23:04:21 PST
Created attachment 116032 [details]
Patch rebased.
Comment 16 Raphael Kubo da Costa (:rakuco) 2011-11-21 05:18:24 PST
Comment on attachment 116032 [details]
Patch rebased.

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

> Source/WebKit/efl/ChangeLog:8
> +        In order to prevent unexpected rendering and pre-rendering while backing store is hidden,

"Unexpected" in what sense? Is anything rendered wrong?

> Source/WebKit/efl/ChangeLog:9
> +        its visibility is checked and pre-rendering is handled whenever backing store is shown or hidden.

Is there a risk of some events being lost with this change?

> Source/WebKit/efl/ewk/ewk_tiled_backing_store.cpp:74
> +        bool visible:1;

Isn't it possible to use evas_object_visible_get/set and drop this flag?
Comment 17 KwangHyuk 2011-11-21 17:54:16 PST
(In reply to comment #16)
> (From update of attachment 116032 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=116032&action=review
> 
> > Source/WebKit/efl/ChangeLog:8
> > +        In order to prevent unexpected rendering and pre-rendering while backing store is hidden,
> 
> "Unexpected" in what sense? Is anything rendered wrong?
> 
It's not about wrong and right thing.
As webkit1 is just based on single thread, any rendering on background can hurt user interaction for the visible view in front.
As a result, I better change title one more time to avoid misunderstood of this patch.

> > Source/WebKit/efl/ChangeLog:9
> > +        its visibility is checked and pre-rendering is handled whenever backing store is shown or hidden.
> 
> Is there a risk of some events being lost with this change?
> 
According to the patch, pre-render request won't be flushed.


> > Source/WebKit/efl/ewk/ewk_tiled_backing_store.cpp:74
> > +        bool visible:1;
> 
> Isn't it possible to use evas_object_visible_get/set and drop this flag?
Good point,
In fact, I have thought that flag check cost is lighter than api call.
But, I will reconsider this one more time.
Comment 18 KwangHyuk 2011-11-21 23:33:22 PST
Created attachment 116182 [details]
Patch updated.
Comment 19 KwangHyuk 2011-11-21 23:34:49 PST
Created attachment 116183 [details]
Patch.
Comment 20 Raphael Kubo da Costa (:rakuco) 2011-11-23 18:42:24 PST
Tiled backing store and rendering in general are not my specialties, but the code looks OK, and if you've verified it works, LGTM.
Comment 21 KwangHyuk 2011-11-23 20:47:23 PST
Created attachment 116477 [details]
Patch rebased.
Comment 22 Gyuyoung Kim 2011-11-23 21:37:24 PST
Comment on attachment 116477 [details]
Patch rebased.

LGTM too.
Comment 23 Gustavo Noronha (kov) 2011-11-24 03:09:02 PST
Comment on attachment 116477 [details]
Patch rebased.

OK
Comment 24 WebKit Review Bot 2011-11-24 05:35:25 PST
Comment on attachment 116477 [details]
Patch rebased.

Clearing flags on attachment: 116477

Committed r101135: <http://trac.webkit.org/changeset/101135>
Comment 25 WebKit Review Bot 2011-11-24 05:35:31 PST
All reviewed patches have been landed.  Closing bug.