Bug 71791 – [EFL] Add implementation considering ewk_tiled_backing_store's visible status.

• Component: WebKit EFL
• Status: RESOLVED FIXED

[KwangHyuk]

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.

[KwangHyuk]

Created attachment 115109 [details]
Patch.

[Gyuyoung Kim]

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.

[KwangHyuk]

Created attachment 115114 [details]
Patch.

[Gyuyoung Kim]

Comment on attachment 115114 [details]
Patch.

LGTM.

[KwangHyuk]

Created attachment 115126 [details]
Patch.

As I missed two lines for this patch, I newly added them into smart_show and smart_hie.

[KwangHyuk]

Created attachment 115128 [details]
Patch again.

[Ryuan Choi]

Looks good to me.

[Grzegorz Czajkowski]

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?

[KwangHyuk]

(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.

[KwangHyuk]

(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.

[KwangHyuk]

Created attachment 115344 [details]
Patch.

[Grzegorz Czajkowski]

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.

[KwangHyuk]

(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. :)

[Grzegorz Czajkowski]

(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,

[KwangHyuk]

Created attachment 116032 [details]
Patch rebased.

[Raphael Kubo da Costa (:rakuco)]

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?

[KwangHyuk]

(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.

[KwangHyuk]

Created attachment 116182 [details]
Patch updated.

[KwangHyuk]

Created attachment 116183 [details]
Patch.

[Raphael Kubo da Costa (:rakuco)]

Tiled backing store and rendering in general are not my specialties, but the code looks OK, and if you've verified it works, LGTM.

[KwangHyuk]

Created attachment 116477 [details]
Patch rebased.

[Gyuyoung Kim]

Comment on attachment 116477 [details]
Patch rebased.

LGTM too.

[Gustavo Noronha (kov)]

Comment on attachment 116477 [details]
Patch rebased.

OK

[WebKit Review Bot]

Comment on attachment 116477 [details]
Patch rebased.

Clearing flags on attachment: 116477

Committed r101135: <http://trac.webkit.org/changeset/101135>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.