This is different way of pre-rendering. Main difference is the way of queuing the request. The request which is close to view port queues in spiral order so that backing store render the nearest request to viewport first. And other difference is calculating the pre-rendering region. EFL port calculates the region of pre-rendering itself. so that the region is as big as the memory can support, and while scrolling, no white tiles can be shown.
Created attachment 117171 [details] proposal patch proposal patch
Attachment 117171 [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_tiled_backing_store.cpp:22: Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Source/WebKit/efl/ewk/ewk_view.h:318: Missing space after , [whitespace/comma] [3] Total errors found: 2 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 117177 [details] fix style check
Comment on attachment 117177 [details] fix style check View in context: https://bugs.webkit.org/attachment.cgi?id=117177&action=review > Source/WebKit/efl/ewk/ewk_private.h:51 > +#define ARGB_BYTES_SIZE 4 I think it is better to use EWK_ prefix to recognize that this macro is defined by ewebkit. > Source/WebKit/efl/ewk/ewk_util.h:34 > +int ewk_util_rect_collision_check(int destinationX, int destinationY, int destinationWidth, int destinationHeight, int sourceX, int sourceY, int sourceWidth, int sourceHeight); I'm not sure if we have to put this function on ewk_util.cpp. Will this function be able to be used by single backing store ?
(In reply to comment #4) > (From update of attachment 117177 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=117177&action=review > > > Source/WebKit/efl/ewk/ewk_private.h:51 > > +#define ARGB_BYTES_SIZE 4 OK, I will fix it. > > I think it is better to use EWK_ prefix to recognize that this macro is defined by ewebkit. > > > Source/WebKit/efl/ewk/ewk_util.h:34 > > +int ewk_util_rect_collision_check(int destinationX, int destinationY, int destinationWidth, int destinationHeight, int sourceX, int sourceY, int sourceWidth, int sourceHeight); > > I'm not sure if we have to put this function on ewk_util.cpp. Will this function be able to be used by single backing store ? the original intention was making a common basic API like eina_rectangle_interection. so I put them in ewk_util. I am also not sure it will be used in other function. So how do you think it should be moved?
(In reply to comment #5) > (In reply to comment #4) > > (From update of attachment 117177 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=117177&action=review > > > > > Source/WebKit/efl/ewk/ewk_private.h:51 > > > +#define ARGB_BYTES_SIZE 4 > OK, I will fix it. > > > > I think it is better to use EWK_ prefix to recognize that this macro is defined by ewebkit. > > > > > Source/WebKit/efl/ewk/ewk_util.h:34 > > > +int ewk_util_rect_collision_check(int destinationX, int destinationY, int destinationWidth, int destinationHeight, int sourceX, int sourceY, int sourceWidth, int sourceHeight); > > > > I'm not sure if we have to put this function on ewk_util.cpp. Will this function be able to be used by single backing store ? > > the original intention was making a common basic API like eina_rectangle_interection. so I put them in ewk_util. I am also not sure it will be used in other function. So how do you think it should be moved? If these functions will not be used by other functions, IMO, it is better to move them to ewk_tiled_xxx files. For example, ewk_tiled_private.h and so on. When other functions need to use them, I think we can move them to ewk_util.
(In reply to comment #6) > (In reply to comment #5) > > (In reply to comment #4) > > > (From update of attachment 117177 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=117177&action=review > > > > > > > Source/WebKit/efl/ewk/ewk_private.h:51 > > > > +#define ARGB_BYTES_SIZE 4 > > OK, I will fix it. > > > > > > I think it is better to use EWK_ prefix to recognize that this macro is defined by ewebkit. > > > > > > > Source/WebKit/efl/ewk/ewk_util.h:34 > > > > +int ewk_util_rect_collision_check(int destinationX, int destinationY, int destinationWidth, int destinationHeight, int sourceX, int sourceY, int sourceWidth, int sourceHeight); > > > > > > I'm not sure if we have to put this function on ewk_util.cpp. Will this function be able to be used by single backing store ? > > > > the original intention was making a common basic API like eina_rectangle_interection. so I put them in ewk_util. I am also not sure it will be used in other function. So how do you think it should be moved? > > If these functions will not be used by other functions, IMO, it is better to move them to ewk_tiled_xxx files. For example, ewk_tiled_private.h and so on. When other functions need to use them, I think we can move them to ewk_util. Ok, then I will move APIs to ewk_view_tiled_XXX and change them to static inline functions.
Created attachment 117417 [details] edited by comment. ARGB_BYTES_SIZE rename to EWK_ARGB_BYTES_SIZE and ewk_util_XX functions are moved to ewk_view_tiled_XXX.
> Source/WebKit/efl/ewk/ewk_private.h:51 > +#define EWK_ARGB_BYTES_SIZE 4 I suggest you to put this into ewk_tiled_backing_store.h since only backing store is using it. > Source/WebKit/efl/ewk/ewk_tiled_backing_store.cpp:24 > +#include "ewk_private.h" I think that this can be removed. > Source/WebKit/efl/ewk/ewk_tiled_backing_store.cpp:1868 > + viewRect->x, viewRect->y, viewRect->w, viewRect->h, tileWidth, tileHeight)) { Do you have any reason these must have two lines ? > Source/WebKit/efl/ewk/ewk_tiled_backing_store.cpp:1874 > + renderRect->x, renderRect->y, renderRect->w, renderRect->h, tileWidth, tileHeight)) { Ditto. > Source/WebKit/efl/ewk/ewk_view.h:1257 > + * This is an alternative method to pre-render the view area. may you mean "pre-render the view area" -> "pre-render around the view area". > Source/WebKit/efl/ewk/ewk_view_tiled.cpp:237 > +static inline void _ewk_view_tiled_rect_collision_resolve(int direction, int* destinationX, int* destinationY, int* destinationWidth, int* destinationHeight, int sourceX, int sourceY, int sourceWidth, int sourceHeight) As no new assignment for destinationWidth and destinationHeight was found, why should it be pointer ? > Source/WebKit/efl/ewk/ewk_view_tiled.cpp:277 > + && smartData->previousView.zoom == currentViewZoom) { Do you have any reason these must have three lines ? currentViewZoom is just available from this line, but you put the code to bring it from the beginning of the function. I suggest you to move calling of it into somewhere near the this condition block. > Source/WebKit/efl/ewk/ewk_view_tiled.cpp:292
> Source/WebKit/efl/ewk/ewk_view.h:182 > +#define EWK_VIEW_SMART_CLASS_INIT(smart_class_init) {smart_class_init, EWK_VIEW_SMART_CLASS_VERSION, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0} I have got one more idea from Kubo. :) You need to update EWK_VIEW_SMART_CLASS_VERSION.
(In reply to comment #9) > > Source/WebKit/efl/ewk/ewk_private.h:51 > > +#define EWK_ARGB_BYTES_SIZE 4 > > I suggest you to put this into ewk_tiled_backing_store.h since only backing store is using it. This value is using in view and backing store both, so it should be in ewk_private.h > > > Source/WebKit/efl/ewk/ewk_tiled_backing_store.cpp:24 > > +#include "ewk_private.h" > > I think that this can be removed. > Due to EWK_ARGB_BYTES_SIZE, it is necessary. > > Source/WebKit/efl/ewk/ewk_tiled_backing_store.cpp:1868 > > + viewRect->x, viewRect->y, viewRect->w, viewRect->h, tileWidth, tileHeight)) { > > Do you have any reason these must have two lines ? > > > Source/WebKit/efl/ewk/ewk_tiled_backing_store.cpp:1874 > > + renderRect->x, renderRect->y, renderRect->w, renderRect->h, tileWidth, tileHeight)) { > > Ditto. > It looks too long so I split it up to two lines. > > Source/WebKit/efl/ewk/ewk_view.h:1257 > > + * This is an alternative method to pre-render the view area. > > may you mean "pre-render the view area" -> "pre-render around the view area". > OK, I will change the comment. > > Source/WebKit/efl/ewk/ewk_view_tiled.cpp:237 > > +static inline void _ewk_view_tiled_rect_collision_resolve(int direction, int* destinationX, int* destinationY, int* destinationWidth, int* destinationHeight, int sourceX, int sourceY, int sourceWidth, int sourceHeight) > > As no new assignment for destinationWidth and destinationHeight was found, why should it be pointer ? > No necessary to be a pointer, I will change them. > > Source/WebKit/efl/ewk/ewk_view_tiled.cpp:277 > > + && smartData->previousView.zoom == currentViewZoom) { > > Do you have any reason these must have three lines ? > currentViewZoom is just available from this line, but you put the code to bring it from the beginning of the function. > I suggest you to move calling of it into somewhere near the this condition block. > > > Source/WebKit/efl/ewk/ewk_view_tiled.cpp:292 Looks too long, same reason. and I will move the currentViewZoom near by condition block.
(In reply to comment #10) > > Source/WebKit/efl/ewk/ewk_view.h:182 > > +#define EWK_VIEW_SMART_CLASS_INIT(smart_class_init) {smart_class_init, EWK_VIEW_SMART_CLASS_VERSION, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0} > > I have got one more idea from Kubo. :) > You need to update EWK_VIEW_SMART_CLASS_VERSION. I will up the version number.
Created attachment 120430 [details] edited patch fixed by following the comments.
Comment on attachment 120430 [details] edited patch View in context: https://bugs.webkit.org/attachment.cgi?id=120430&action=review > Source/WebKit/efl/ewk/ewk_view_tiled.cpp:236 > +static inline void _ewk_view_tiled_rect_collision_resolve(int direction, int* destinationX, int* destinationY, int destinationWidth, int destinationHeight, int sourceX, int sourceY, int sourceWidth, int sourceHeight) IMO, how about using IntRect or Eina_Rectangle for internal functions ? IntRect is better for me. > Source/WebKit/efl/ewk/ewk_view_tiled.cpp:239 > + *destinationX = *destinationX + (sourceX - *destinationX); I don't know what you want, but it looks strange. *destinationX will be sourceX. In addition 1, 2, 4, 8 can be enumaration.
(In reply to comment #14) > (From update of attachment 120430 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=120430&action=review > > > Source/WebKit/efl/ewk/ewk_view_tiled.cpp:236 > > +static inline void _ewk_view_tiled_rect_collision_resolve(int direction, int* destinationX, int* destinationY, int destinationWidth, int destinationHeight, int sourceX, int sourceY, int sourceWidth, int sourceHeight) > > IMO, how about using IntRect or Eina_Rectangle for internal functions ? > > IntRect is better for me. > IntRect is big for this, I will change it with using Eina_Rectangle. > > Source/WebKit/efl/ewk/ewk_view_tiled.cpp:239 > > + *destinationX = *destinationX + (sourceX - *destinationX); > > I don't know what you want, but it looks strange. > *destinationX will be sourceX. > > In addition 1, 2, 4, 8 can be enumaration. I totally agree that it means nothing. I will change them again! Enumeration is considerable.
Few small comments: > Source/WebKit/efl/ewk/ewk_tiled_backing_store.cpp:1881 > + const int maxViewSideLength = std::max(viewSlicer.col2 - viewSlicer.col1 , viewSlicer.row2 - viewSlicer.row1); Codint style: ' ,'. And line below this same. > Source/WebKit/efl/ewk/ewk_tiled_backing_store.cpp:1892 > + for (int i = 1; i < step * squareSide + 1; i++) { "step * squareSide + 1" this could be moved outside the loop. > Source/WebKit/efl/ewk/ewk_tiled_backing_store.cpp:1919 > + if (static_cast<int>(viewSlicer.col1) < centerColumn It may look better when these static_cast operations will be done once before this two ifs
(In reply to comment #16) > Few small comments: > > > Source/WebKit/efl/ewk/ewk_tiled_backing_store.cpp:1881 > > + const int maxViewSideLength = std::max(viewSlicer.col2 - viewSlicer.col1 , viewSlicer.row2 - viewSlicer.row1); > > Codint style: ' ,'. And line below this same. > I will fix the coding style. > > Source/WebKit/efl/ewk/ewk_tiled_backing_store.cpp:1892 > > + for (int i = 1; i < step * squareSide + 1; i++) { > > "step * squareSide + 1" this could be moved outside the loop. > why should it be outside the loop? what makes difference? > > Source/WebKit/efl/ewk/ewk_tiled_backing_store.cpp:1919 > > + if (static_cast<int>(viewSlicer.col1) < centerColumn > > It may look better when these static_cast operations will be done once before this two ifs To do so I have to assign the values to new values. Does it need?
Created attachment 120544 [details] fourth edition patch coding style and minor fix
Comment on attachment 120544 [details] fourth edition patch It seems there are no critical problems. LGTM.
Created attachment 121045 [details] rebased_patch I got an informal review so file the rebased patch.
Created attachment 121092 [details] minor fix a patch fixed by following webkit style coding rules and integer calculation.
Comment on attachment 121092 [details] minor fix r=me
Comment on attachment 121092 [details] minor fix Attachment 121092 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11084443 New failing tests: http/tests/inspector/resource-tree/resource-tree-document-url.html
Created attachment 121192 [details] rebase patch due to a failure in chrome test, I filed new rebased patch.
Comment on attachment 121192 [details] rebase patch Rejecting attachment 121192 [details] from commit-queue. New failing tests: http/tests/inspector/network/download.html Full output: http://queues.webkit.org/results/11126312
Created attachment 121391 [details] upload again due to cr-linux failure. it is the same with the previous patch.
Comment on attachment 121391 [details] upload again due to cr-linux failure. Clearing flags on attachment: 121391 Committed r104282: <http://trac.webkit.org/changeset/104282>
All reviewed patches have been landed. Closing bug.