Bug 64259 – [EFL] Implement EditorClientEfl::respondToChangedContents

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Michal Pakula vel Rutka]

Implements EditorClientEfl::respondToChangedContents by calling callback both from ewk_frame and ewk_view with frame and view objects respectively.

[Michal Pakula vel Rutka]

Created attachment 100260 [details]
proposed patch

[WebKit Review Bot]

Attachment 100260 [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/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Michal Pakula vel Rutka]

Created attachment 100261 [details]
fixed changelog

[Raphael Kubo da Costa (:rakuco)]

Informal r- to address these small issues:

LGTM in general, however I think these new signals should be documented in ewk_frame.h and ewk_view.h even though the functions which emit them are marked as internal, as the signals are emitted publicly anyway.

> Source/WebKit/efl/WebCoreSupport/EditorClientEfl.cpp:125
> +    Evas_Object* frame = ewk_view_frame_focused_get(m_view);

Doing it like this should make the code a bit shorter:

  Evas_Object* frame = ewk_view_frame_focused_get(m_view) ? ewk_view_frame_focused_get(m_view) : ewk_view_frame_main_get(m_view);

> Source/WebKit/efl/ewk/ewk_private.h:115
> +void ewk_view_editor_client_contents_changed(Evas_Object* o);

Nitpick: this declaration could come before the one above it so that they are sorted alphabetically.

> Source/WebKit/efl/ewk/ewk_private.h:184
> +void ewk_frame_editor_client_contents_changed(Evas_Object* o);

Nitpick: this declaration could come before the one above it so that they are sorted alphabetically.

[Michal Pakula vel Rutka]

(In reply to comment #4)
> Informal r- to address these small issues:
> 
> LGTM in general, however I think these new signals should be documented in ewk_frame.h and ewk_view.h even though the functions which emit them are marked as internal, as the signals are emitted publicly anyway.

OK I see, should I add documentation for 'editorclient,selection,changed' in this or another patch, as I haven't added it before?

> 
> > Source/WebKit/efl/WebCoreSupport/EditorClientEfl.cpp:125
> > +    Evas_Object* frame = ewk_view_frame_focused_get(m_view);
> 
> Doing it like this should make the code a bit shorter:
> 
>   Evas_Object* frame = ewk_view_frame_focused_get(m_view) ? ewk_view_frame_focused_get(m_view) : ewk_view_frame_main_get(m_view);
> 

OK

> > Source/WebKit/efl/ewk/ewk_private.h:115
> > +void ewk_view_editor_client_contents_changed(Evas_Object* o);
> 
> Nitpick: this declaration could come before the one above it so that they are sorted alphabetically.
> 
> > Source/WebKit/efl/ewk/ewk_private.h:184
> > +void ewk_frame_editor_client_contents_changed(Evas_Object* o);
> 
> Nitpick: this declaration could come before the one above it so that they are sorted alphabetically.

OK

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #5)
> OK I see, should I add documentation for 'editorclient,selection,changed' in this or another patch, as I haven't added it before?

Sorry, I meant the signals you added in this patch ("editorclient,contents,changed"). As for the "editorclient,selection,changed" signal, I suspect there are more undocumented signals being emitted, and they should go in a separate patch.

BTW, I forgot to mention one thing:

> Source/WebKit/efl/ewk/ewk_view.cpp:4593
> + * Reports the view that editor client contents were changed.

the view -> to the view
editor client contents -> the editor client's contents

[Michal Pakula vel Rutka]

Created attachment 100303 [details]
changes according to Raphael's suggestions

[Raphael Kubo da Costa (:rakuco)]

LGTM.

[Antonio Gomes]

Comment on attachment 100303 [details]
changes according to Raphael's suggestions

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

> Source/WebKit/efl/WebCoreSupport/EditorClientEfl.cpp:125
> +    Evas_Object* frame = ewk_view_frame_focused_get(m_view) ? ewk_view_frame_focused_get(m_view) : ewk_view_frame_main_get(m_view);

I would not call the getter twice here.

[Michal Pakula vel Rutka]

Comment on attachment 100303 [details]
changes according to Raphael's suggestions

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

>> Source/WebKit/efl/WebCoreSupport/EditorClientEfl.cpp:125
>> +    Evas_Object* frame = ewk_view_frame_focused_get(m_view) ? ewk_view_frame_focused_get(m_view) : ewk_view_frame_main_get(m_view);
> 
> I would not call the getter twice here.

OK, should I do it this way:

Evas_Object* frame = ewk_view_frame_focused_get(m_view);
frame = frame ? frame : ewk_view_frame_main_get(m_view);

or get back to if?

[Raphael Kubo da Costa (:rakuco)]

Given he's already given an r+, I think you could also leave it as-is.

[Gyuyoung Kim]

(In reply to comment #11)
> Given he's already given an r+, I think you could also leave it as-is.

I think this is not big work. Why don't we fix antonio's comment before landing this patch ? 

Michal,

Could you update this patch based on comment #10 ? Please fill review field in ChangeLog as below,

Reviewed by NOBODY (OOPS!). => Reviewed by Antonio Gomes.

Then, please request only cq.

[Michal Pakula vel Rutka]

(In reply to comment #12)
> (In reply to comment #11)
> > Given he's already given an r+, I think you could also leave it as-is.
> 
> I think this is not big work. Why don't we fix antonio's comment before landing this patch ? 
> 
> Michal,
> 
> Could you update this patch based on comment #10 ? Please fill review field in ChangeLog as below,
> 
> Reviewed by NOBODY (OOPS!). => Reviewed by Antonio Gomes.
> 
> Then, please request only cq.

I am bit confused... I know that is not a big work, but I want to get Antonio's confirmation first. Anyway until then I am leaving it as it is.

[Gyuyoung Kim]

(In reply to comment #13)
> (In reply to comment #12)
> > (In reply to comment #11)
> > > Given he's already given an r+, I think you could also leave it as-is.
> > 
> > I think this is not big work. Why don't we fix antonio's comment before landing this patch ? 
> > 
> > Michal,
> > 
> > Could you update this patch based on comment #10 ? Please fill review field in ChangeLog as below,
> > 
> > Reviewed by NOBODY (OOPS!). => Reviewed by Antonio Gomes.
> > 
> > Then, please request only cq.
> 
> I am bit confused... I know that is not a big work, but I want to get Antonio's confirmation first. Anyway until then I am leaving it as it is.

Antonio already gave r+. But, he pointed minor problem in this patch. So, if you fix it, AFAIK, you don't need to get r+ again. But, frankly, I am not sure if this fix is minor problem. If you think it is better to get r+ from Antonio again, please request r?/cq? again when you submit updated patch.

[Michal Pakula vel Rutka]

Created attachment 102353 [details]
changed frame check in editorclientefl

[WebKit Review Bot]

Comment on attachment 102353 [details]
changed frame check in editorclientefl

Clearing flags on attachment: 102353

Committed r92039: <http://trac.webkit.org/changeset/92039>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.