Implements EditorClientEfl::respondToChangedContents by calling callback both from ewk_frame and ewk_view with frame and view objects respectively.
Created attachment 100260 [details] proposed patch
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.
Created attachment 100261 [details] fixed changelog
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.
(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
(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
Created attachment 100303 [details] changes according to Raphael's suggestions
LGTM.
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.
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?
Given he's already given an r+, I think you could also leave it as-is.
(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.
(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.
(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.
Created attachment 102353 [details] changed frame check in editorclientefl
Comment on attachment 102353 [details] changed frame check in editorclientefl Clearing flags on attachment: 102353 Committed r92039: <http://trac.webkit.org/changeset/92039>
All reviewed patches have been landed. Closing bug.