Summary: | [EFL] Add implementation considering ewk_tiled_backing_store's visible status. | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | KwangHyuk <hyuki.kim> | ||||||||||||||||||||
Component: | WebKit EFL | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||
Severity: | Normal | CC: | g.czajkowski, gyuyoung.kim, l.slachciak, lucas.de.marchi, rakuco, ryuan.choi, t.morawski, webkit.review.bot | ||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||
OS: | Linux | ||||||||||||||||||||||
Attachments: |
|
Description
KwangHyuk
2011-11-08 03:14:08 PST
Created attachment 115109 [details]
Patch.
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. Created attachment 115114 [details]
Patch.
Comment on attachment 115114 [details]
Patch.
LGTM.
Created attachment 115126 [details]
Patch.
As I missed two lines for this patch, I newly added them into smart_show and smart_hie.
Created attachment 115128 [details]
Patch again.
Looks good to me. 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? (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. (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. Created attachment 115344 [details]
Patch.
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. (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 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, Created attachment 116032 [details]
Patch rebased.
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? (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. Created attachment 116182 [details]
Patch updated.
Created attachment 116183 [details]
Patch.
Tiled backing store and rendering in general are not my specialties, but the code looks OK, and if you've verified it works, LGTM. Created attachment 116477 [details]
Patch rebased.
Comment on attachment 116477 [details]
Patch rebased.
LGTM too.
Comment on attachment 116477 [details]
Patch rebased.
OK
Comment on attachment 116477 [details] Patch rebased. Clearing flags on attachment: 116477 Committed r101135: <http://trac.webkit.org/changeset/101135> All reviewed patches have been landed. Closing bug. |