Bug 69988

Created attachment 110975 [details] Prototype I make a prototype for this bug. Though I know well this patch is also very giant, I think it is better to change coding style as soon as possible. Major changes are as below, - Use ewkFrame instead of o parameter in ewk_frame.cpp. - Use ewkView instead of o parameter in ewk_view.cpp. - Use ewkTile instead of o parameter in ewk_tile_xxx.cpp. - Use smartData instead of sd parameter for Ewk_XXXX_Smart_Data struct. - Use width, height instead of w, h parameter. - Use scrollX, scrollY, scrollWidth, scrollHeight, centerX, centerY, deltaX, deltaY instead of sx, xy, sw, sh, cx, cy, dx, dy. - Use red, green, blue and alpha instead of r,g,b,a. - Remove "_" from parameter variable. However, backing store's variables is almost encrypt. Backing Store guys should review this patch. Because, this patch is initial prototype, I will modify this patch according to efl guys's comments.

CC'ing Ryuan, Kwang, Grzegorz, Lukasz, antognolli and Antonio.

Oops, there were wrong comments in #1. So, I list major changes up again. - Use *menu* instead of *o* in comment of ewk_contextmenu.cpp. - Use *list* instead of *l* local variable - Use *ewkPolicy* instead of *ewk_policy* in ewk_cookies.cpp - Use *smartData* instead of *sd* local variable for Ewk_XXXX_Smart_Data struct. - Use *width*, *height* instead of *w*, *h* local variables. - Use scrollX, scrollY, scrollWidth, scrollHeight, centerX, centerY, deltaX, deltaY, baseX, baseY instead of sx, xy, sw, sh, cx, cy, dx, dy, bx, by. - Use red, green, blue, alpha, contentRed, contentGreen, contentBlue and contentAlpha instead of r,g,b,a, cr, cg, cb, ca. - Use *object* instead of *obj* - Use *length* instead of *len* - Use *coreFrame* instead of *cf* - Use *buffer* instead of *buf* - Use *item* instead of *i* - Remove "_" in local variable. - And so on.

It's impossible to review this patch, so informal rs+ from my side.

Created attachment 111422 [details] Patch

Created attachment 111423 [details] Patch

I update patch with below changes. Though I know very well it is difficult to review this patch, I hope to help to review this patch. - Use *menu* instead of *o* in comment of ewk_contextmenu.cpp. - Use *list* instead of *l* local variable - Use *ewkPolicy* instead of *ewk_policy* in ewk_cookies.cpp - Use *smartData* instead of *sd* local variable for Ewk_XXXX_Smart_Data struct. - Use *width*, *height* instead of *w*, *h* local variables. - Use *scrollX*, *scrollY*, *scrollWidth*, *scrollHeight*, *centerX*, *centerY*, *deltaX*, *deltaY*, *baseX*, *baseY* instead of *sx*, *sy*, *sw*, *sh*, *cx*, *cy*, *dx*, *dy*, *bx*, *by*. - Use *red*, *green*, *blue*, *alpha*, *contentRed*, *contentGreen*, *contentBlue* and *contentAlpha* instead of *r*,*g*,*b*,*a*,*cr*,*cg*,*cb*,*ca*. - Use *tilePositionX*, *tilePositionY* instead of *ox*,*oy* in tiled backingstore files. - Use *object* instead of *obj* - Use *length* instead of *len* - Use *coreFrame* instead of *cf* - Use *buffer* instead of *buf* - Use *item* instead of *i* - Remove "_" in local variable. - And so on.

Created attachment 111425 [details] Patch

patch is re-based with latest trunk.

Comment on attachment 111425 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=111425&action=review > Source/WebKit/efl/ewk/ewk_auth_soup.cpp:82 > + Ewk_Auth_Data* authData = static_cast<Ewk_Auth_Data*>(data); authenticationData ? > Source/WebKit/efl/ewk/ewk_auth_soup.cpp:92 > + const char* realMessage; what is a real message? > Source/WebKit/efl/ewk/ewk_cookies.cpp:93 > + Ewk_Cookie* ewkCookie = static_cast<Ewk_Cookie*>(malloc(sizeof(*ewkCookie))); Why don't you do something like #define e_new(struct_type) (static_cast<struct_type*>(malloc(sizeof(struct_type))) similar to glib. then Ewk_Cookie* ewkCookie = cnew(Ewk_Cookie). Using templates it might be able to do something like cnew<Ewk_Cookie>(4) as well. template <typename struct_type> struct_type* cnew(size_t amount = 1) { return static_cast<struct_type*>malloc(amount * sizeof(struct_type)); } Ewk_Cookie* cookies = cnew<Ewk_Cookie>(10);

Created attachment 111546 [details] Patch

(In reply to comment #10) > (From update of attachment 111425 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=111425&action=review > > > Source/WebKit/efl/ewk/ewk_auth_soup.cpp:82 > > + Ewk_Auth_Data* authData = static_cast<Ewk_Auth_Data*>(data); > > authenticationData ? Fixed. > > Source/WebKit/efl/ewk/ewk_auth_soup.cpp:92 > > + const char* realMessage; > > what is a real message? Oops, I knew realm is abbreviation for realMessage. This is fixed. > > Source/WebKit/efl/ewk/ewk_cookies.cpp:93 > > + Ewk_Cookie* ewkCookie = static_cast<Ewk_Cookie*>(malloc(sizeof(*ewkCookie))); > > Why don't you do something like > > #define e_new(struct_type) (static_cast<struct_type*>(malloc(sizeof(struct_type))) > > similar to glib. > > then Ewk_Cookie* ewkCookie = cnew(Ewk_Cookie). > > Using templates it might be able to do something like cnew<Ewk_Cookie>(4) as well. > > template <typename struct_type> > struct_type* cnew(size_t amount = 1) { > return static_cast<struct_type*>malloc(amount * sizeof(struct_type)); > } > > Ewk_Cookie* cookies = cnew<Ewk_Cookie>(10); Looks good for me this template for memory allocation until using smart pointer. However, other ewk files also need to be adjusted. So, I would like to adjust this template into all ewk files after fixing this bug. If you agree with my opinion, I will file a new bug as soon as this bug is closed.

> > template <typename struct_type> > > struct_type* cnew(size_t amount = 1) { > > return static_cast<struct_type*>malloc(amount * sizeof(struct_type)); > > } > > > > Ewk_Cookie* cookies = cnew<Ewk_Cookie>(10); > > Looks good for me this template for memory allocation until using smart pointer. However, other ewk files also need to be adjusted. So, I would like to adjust this template into all ewk files after fixing this bug. If you agree with my opinion, I will file a new bug as soon as this bug is closed. You could add it for the EFL port in Platform.h or similar, I guess.

(In reply to comment #13) > You could add it for the EFL port in Platform.h or similar, I guess. Do you mean it is better to add your suggestion to this patch together ?

separate is better

Created attachment 111737 [details] Patch

There are local variables looks like member variables. So, I fix it with currentColumn and currentRow thanks to Kwang. static inline Eina_Bool _ewk_tiled_backing_store_item_fill(Ewk_Tiled_Backing_Store_Data* priv, Ewk_Tiled_Backing_Store_Item* item, unsigned long column, unsigned long row) { unsigned long m_col = priv->model.base.column + column; unsigned long m_row = priv->model.base.row + row;

Created attachment 111738 [details] Patch

Patch is re-based. This patch should be re-based whenever modified because of too giant.

Comment on attachment 111738 [details] Patch feel free to land it when it is agreed it is in good shape by others.

(In reply to comment #20) > (From update of attachment 111738 [details]) > feel free to land it when it is agreed it is in good shape by others. Antonio, Thank you. Hello CC'ing guys, could you please review this patch?

I can't review the patch, but I guess the changes are OK. Go ahead and cq+ it.

Comment on attachment 111738 [details] Patch Ok, let's go ahead. If there are problems, let's reopen this bug or file a new bug.

Comment on attachment 111738 [details] Patch Rejecting attachment 111738 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: patching file Source/WebKit/efl/ewk/ewk_tiled_matrix.cpp patching file Source/WebKit/efl/ewk/ewk_tiled_model.cpp patching file Source/WebKit/efl/ewk/ewk_view.cpp Hunk #96 FAILED at 3789. Hunk #97 succeeded at 3815 (offset -5 lines). Hunk #98 succeeded at 3828 (offset -5 lines). 1 out of 98 hunks FAILED -- saving rejects to file Source/WebKit/efl/ewk/ewk_view.cpp.rej Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Antonio Gomes', u'--fo..." exit_code: 1 Full output: http://queues.webkit.org/results/10183936

Created attachment 111975 [details] Patch

Comment on attachment 111975 [details] Patch Clearing flags on attachment: 111975 Committed r98109: <http://trac.webkit.org/changeset/98109>

All reviewed patches have been landed. Closing bug.

Created attachment 112069 [details] Patch

Comment on attachment 112069 [details] Patch *sd* variables were changed by this bug. So, I fix them.

As mentioned Comment #29, this patch need to be re-opened.

Comment on attachment 112069 [details] Patch Clearing flags on attachment: 112069 Committed r98190: <http://trac.webkit.org/changeset/98190>

All reviewed patches have been landed. Closing bug.