Bug 75848 – [EFL] Replace malloc/calloc/free to new/delete

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

[Tomasz Morawski]

Replace malloc/calloc/free functions calls to new/delete operator calls

[Alexey Proskuryakov]

We shouldn't just blindly do that, see e.g. bug 45735. What specific instances of malloc/free would you like to change?

[Alexey Proskuryakov]

Or is this only about WebKit EFL layer, not WebCore?

[Gyuyoung Kim]

(In reply to comment #2)
> Or is this only about WebKit EFL layer, not WebCore?

AFAIK, this patch will focus on EFL layer. He will add [EFL] to Bug title.

[Tomasz Morawski]

(In reply to comment #2)
> Or is this only about WebKit EFL layer, not WebCore?
Yes, it is EFL only. I am sorry for confusion. :)

[Gyuyoung Kim]

Any update ?

[Tomasz Morawski]

(In reply to comment #5)
> Any update ?
I am working on it.

[Tomasz Morawski]

Created attachment 124320 [details]
First patch

This patch:
- does not change ewk_js file due to that changes for this file are very big. I would like to show this changes in separate patch for this bug or open a new bug.
- does not change realloc operations (separate patch will be prepared).

[Gyuyoung Kim]

Comment on attachment 124320 [details]
First patch

View in context: https://bugs.webkit.org/attachment.cgi?id=124320&action=review

> Source/WebKit/efl/ewk/ewk_tiled_model.cpp:675
> +            tileUnusedCache->entries.count--;

I think this code should be reviewed by backing store guys.

[Tomasz Morawski]

(In reply to comment #8)
> (From update of attachment 124320 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=124320&action=review
> 
> > Source/WebKit/efl/ewk/ewk_tiled_model.cpp:675
> > +            tileUnusedCache->entries.count--;
> 
> I think this code should be reviewed by backing store guys.
It is a very simple change. Patch looks very bad only.

[Gyuyoung Kim]

Comment on attachment 124320 [details]
First patch

View in context: https://bugs.webkit.org/attachment.cgi?id=124320&action=review

>>> Source/WebKit/efl/ewk/ewk_tiled_model.cpp:675
>>> +            tileUnusedCache->entries.count--;
>> 
>> I think this code should be reviewed by backing store guys.
> 
> It is a very simple change. Patch looks very bad only.

Ok, it looks this patch almost good. If jungjik and Kwanghyuk agree with this patch, this patch is r+ informally.
I think we need to land this patch for EFL port's code quality.

Jungjik and Kwanghyuk, could you review this patch?

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 124320 [details]
First patch

View in context: https://bugs.webkit.org/attachment.cgi?id=124320&action=review

Looks good overall, but there are some minor things to work out and you need to rebase it.

> Source/WebKit/efl/ewk/ewk_history.cpp:251
> +    item->title = 0;
> +    item->alternateTitle = 0;
> +    item->uri = 0;
> +    item->originalUri = 0;

How about memset(item, 0, sizeof(*item)) before setting core?

> Source/WebKit/efl/ewk/ewk_tiled_backing_store.cpp:478
> +    OwnPtr<Ewk_Tiled_Backing_Store_Item> item = adoptPtr(new Ewk_Tiled_Backing_Store_Item);

It's safer to #include <OwnPtr.h> in this file.

> Source/WebKit/efl/ewk/ewk_tiled_matrix.cpp:202
> +    OwnPtr<Ewk_Tile_Matrix> tileMatrix = WTF::adoptPtr(new Ewk_Tile_Matrix);

PassOwnPtr.h adds a "using WTF::adoptPtr()" clause, so you can use just adoptPtr the same way you use just OwnPtr.

> Source/WebKit/efl/ewk/ewk_view.cpp:641
> +    priv->settings.encodingDefault = eina_stringshare_add(priv->pageSettings->defaultTextEncodingName().utf8().data());

I'd rather not commit unrelated changes in this patch.

[JungJik Lee]

(In reply to comment #10)
> (From update of attachment 124320 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=124320&action=review
> 
> >>> Source/WebKit/efl/ewk/ewk_tiled_model.cpp:675
> >>> +            tileUnusedCache->entries.count--;
> >> 
> >> I think this code should be reviewed by backing store guys.
> > 
> > It is a very simple change. Patch looks very bad only.
> 
> Ok, it looks this patch almost good. If jungjik and Kwanghyuk agree with this patch, this patch is r+ informally.
> I think we need to land this patch for EFL port's code quality.
> 
> Jungjik and Kwanghyuk, could you review this patch?

This part of the code is okay. I think the logic is the same.

[Tomasz Morawski]

Created attachment 129031 [details]
Next patch

Fixed Raphael's comments.

Additional change:
I had to left malloc for Ewk_Tile object due to compilation error when "new" was used:

Source/WebKit/efl/ewk/ewk_tiled_model.cpp:189:16: error: uninitialized const member in ‘Ewk_Tile {aka struct _Ewk_Tile}’ using ‘new’ without new-initializer [-fpermissive]
Source/WebKit/efl/ewk/ewk_tiled_backing_store.h:71:22: note: ‘_Ewk_Tile::width’ should be initialized

Anyway, initialization of const fileds in Ewk_Tile looks very ugly. Do you have any good idea to fix this without use of base C++ features like constructor or private section?

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 129031 [details]
Next patch

View in context: https://bugs.webkit.org/attachment.cgi?id=129031&action=review

> Source/WebKit/efl/ewk/ewk_tiled_backing_store.cpp:29
> +#include <OwnPtr.h>

You also need PassOwnPtr.h for the implementation of adoptPtr().

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #13)
> Anyway, initialization of const fileds in Ewk_Tile looks very ugly. Do you have any good idea to fix this without use of base C++ features like constructor or private section?

Nope.

[Tomasz Morawski]

Created attachment 129201 [details]
Fixed Raphael's comment

[Tomasz Morawski]

(In reply to comment #15)
> (In reply to comment #13)
> > Anyway, initialization of const fileds in Ewk_Tile looks very ugly. Do you have any good idea to fix this without use of base C++ features like constructor or private section?
> 
> Nope.
Maybe it is a good point to find a good solution for this and future similar "issues"?

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 129201 [details]
Fixed Raphael's comment

It finally looks OK to me :)

[Tomasz Morawski]

(In reply to comment #18)
> (From update of attachment 129201 [details])
> It finally looks OK to me :)
Thanks for infomal review. :)

[Hajime Morrita]

Comment on attachment 129201 [details]
Fixed Raphael's comment

I hope these new-allocated structs have a constructor instead of memeset()-ed...

[Tomasz Morawski]

(In reply to comment #20)
> (From update of attachment 129201 [details])
> I hope these new-allocated structs have a constructor instead of memeset()-ed...

Currently, fileds are initialized when it is needed in functions that creates object and this patch doesn't changed this way. IMO, missing simple constructor that initialize structure fields could be an issue in EFL port.

[WebKit Review Bot]

Comment on attachment 129201 [details]
Fixed Raphael's comment

Clearing flags on attachment: 129201

Committed r109205: <http://trac.webkit.org/changeset/109205>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.