Bug 100370 – [EFL][WK2] Remove some C'ism from EwkView

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

[Chris Dumez]

EwkView still contains quite a bit of C'ism. We should gradually get rid of it.

[Chris Dumez]

Created attachment 170623 [details]
Patch

[Mikhail Pozdnyakov]

Comment on attachment 170623 [details]
Patch

looks good

[Gyuyoung Kim]

Comment on attachment 170623 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:112
> +PassOwnPtr<Ecore_IMF_Context> EwkViewImpl::createIMFContext(Ewk_View_Smart_Data* sd)

Why not using smartData instead of sd ?

[Chris Dumez]

(In reply to comment #3)
> (From update of attachment 170623 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=170623&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:112
> > +PassOwnPtr<Ecore_IMF_Context> EwkViewImpl::createIMFContext(Ewk_View_Smart_Data* sd)
> 
> Why not using smartData instead of sd ?

Because it conflicts with the method name smartData() in the same class.

[Chris Dumez]

Created attachment 170636 [details]
Patch

Requesting r? again because I had to move the mouse / touch callbacks to EwkViewImpl to address an API tests regression.

[Kenneth Rohde Christiansen]

Comment on attachment 170636 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:225
> +    data = edje_object_data_get(m_cursorObject.get(), "hot.y");

This is getting quite hot :-)

> Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:559
> +bool EwkViewImpl::focused() const

why not isFocused() that is like webkit style

> Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:576
> +Ecore_IMF_Context* EwkViewImpl::imfContext()

i really dislike the imf abbrivation

[Antonio Gomes]

Comment on attachment 170636 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:106
> +    free(buffer);

Unrelated?

> Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:588
> -    _ewk_view_imf_context_destroy(imfContext);
> -
>      void* item;
>      EINA_LIST_FREE(popupMenuItems, item)
>          delete static_cast<Ewk_Popup_Menu_Item*>(item);
> +    if (m_theme != theme) {

Do you really want to compare the memory addresses here?

[Chris Dumez]

Comment on attachment 170636 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:106
>> +    free(buffer);
> 
> Unrelated?

Hmm. Yes, this is a memory leak fix. Do you really want me to move it to a separate patch?

>> Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:559
>> +bool EwkViewImpl::focused() const
> 
> why not isFocused() that is like webkit style

Ok.

>> Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:588
>> +    if (m_theme != theme) {
> 
> Do you really want to compare the memory addresses here?

I am not :) m_theme is a WKEinaSharedString. Therefore it is safe.

[Chris Dumez]

Comment on attachment 170636 [details]
Patch

Will take feedback into consideration and reupload.

[Chris Dumez]

Created attachment 170657 [details]
Patch

Take feedback from Kenneth and Antonio into consideration:
- Renamed visible() / focused() to isVisible() / isFocused()
- Moved memory leak fix to another patch (Bug 100380)
- renamed "imf" to "inputMethod"
- Rebase on master since some AC fixes landed

[Chris Dumez]

Comment on attachment 170657 [details]
Patch

Could someone please cq+ ?

[WebKit Review Bot]

Comment on attachment 170657 [details]
Patch

Clearing flags on attachment: 170657

Committed r132498: <http://trac.webkit.org/changeset/132498>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.