Bug 73430 – [EFL] Add pre-rendering implementation.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[JungJik Lee]

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.

[JungJik Lee]

Created attachment 117171 [details]
proposal patch

proposal patch

[WebKit Review Bot]

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.

[JungJik Lee]

Created attachment 117177 [details]
fix style check

[Gyuyoung Kim]

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 ?

[JungJik Lee]

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

[Gyuyoung Kim]

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

[JungJik Lee]

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

[JungJik Lee]

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.

[KwangHyuk]

> 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

[KwangHyuk]

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

[JungJik Lee]

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

[JungJik Lee]

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

[JungJik Lee]

Created attachment 120430 [details]
edited patch

fixed by following the comments.

[Ryuan Choi]

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.

[JungJik Lee]

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

[Tomasz Morawski]

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

[JungJik Lee]

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

[JungJik Lee]

Created attachment 120544 [details]
fourth edition patch

coding style and minor fix

[Gyuyoung Kim]

Comment on attachment 120544 [details]
fourth edition patch

It seems there are no critical problems. LGTM.

[JungJik Lee]

Created attachment 121045 [details]
rebased_patch

I got an informal review so file the rebased patch.

[JungJik Lee]

Created attachment 121092 [details]
minor fix

a patch fixed by following webkit style coding rules and integer calculation.

[Zoltan Herczeg]

Comment on attachment 121092 [details]
minor fix

r=me

[WebKit Review Bot]

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

[JungJik Lee]

Created attachment 121192 [details]
rebase patch

due to a failure in chrome test, I filed new rebased patch.

[WebKit Review Bot]

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

[JungJik Lee]

Created attachment 121391 [details]
upload again due to cr-linux failure.

it is the same with the previous patch.

[WebKit Review Bot]

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>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.