Bug 90927

Created attachment 152115 [details] Add ewk_view_find_client.h / cpp for wrapping WKPageSetPageFindClient.

Comment on attachment 152115 [details] Add ewk_view_find_client.h / cpp for wrapping WKPageSetPageFindClient. View in context: https://bugs.webkit.org/attachment.cgi?id=152115&action=review I don't see the point of this patch if you don't define any callback. In itself, this patch provides no additional functionality, it is just more code to maintain for no good reason. > Source/WebKit2/ChangeLog:3 > + Need a short description and bug URL (OOPS!) Please remove this line. > Source/WebKit2/ChangeLog:4 > + Ditto. > Source/WebKit2/ChangeLog:10 > + Reviewed by NOBODY (OOPS!). The "Reviewed by" line is supposed to be above your changelog text, not under.

Comment on attachment 152115 [details] Add ewk_view_find_client.h / cpp for wrapping WKPageSetPageFindClient. Please fix changelog nits first.

Created attachment 152469 [details] Add ewk_view_find_client.h / cpp for wrapping WKPageSetPageFindClient. Apply chris's comments.

Comment on attachment 152469 [details] Add ewk_view_find_client.h / cpp for wrapping WKPageSetPageFindClient. View in context: https://bugs.webkit.org/attachment.cgi?id=152469&action=review > Source/WebKit2/UIProcess/API/efl/ewk_view_private.h:55 > +void ewk_view_string_found(Evas_Object* ewkView, unsigned matchCount); I think ewk_view_string_found is too ambiguous. GTK port also uses "FOUND_TEXT" enum type for this. So, ewk_view_text_found or ewk_view_text_matched is better I think.

Created attachment 152484 [details] Add ewk_view_find_client.h / cpp for wrapping WKPageSetPageFindClient. Apply gyoyoung's comments.

Comment on attachment 152484 [details] Add ewk_view_find_client.h / cpp for wrapping WKPageSetPageFindClient. Looks good to me.

Comment on attachment 152484 [details] Add ewk_view_find_client.h / cpp for wrapping WKPageSetPageFindClient. View in context: https://bugs.webkit.org/attachment.cgi?id=152484&action=review Do we have an API to find text matches somewhere? If not, the client does not bring anything in terms of functionality, does it? > Source/WebKit2/ChangeLog:2 > + Reviewed by NOBODY (OOPS!). Misunderstanding here :) The "Reviewed By" needs to be under the bug URL and before your changelog explanation. > Source/WebKit2/ChangeLog:6 > + Missing changelog explanation. > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:685 > + * The text was changed. This documentation seems very imprecise. > Source/WebKit2/UIProcess/API/efl/ewk_view.h:44 > + * - "text,found", unsigned*: text was found. You need to explain what the unsigned* argument is here.

Oops. I missed to check ChangeLog. Sorry.

Created attachment 152491 [details] Patch

Comment on attachment 152491 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152491&action=review Could you please point me to the Ewk API for finding text? if we don't have one yet, I think it needs to be included in this patch. > Source/WebKit2/UIProcess/API/efl/ewk_view.h:44 > + * - "text,found", unsigned*: text was found by find text functionality(value of number of found text). instead of the "(value of number of found text)", I would say "with the number of matches".

Created attachment 152499 [details] Patch

Comment on attachment 152499 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152499&action=review > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:954 > + Please use COMPILE_ASSERT_MATCHING_ENUM() from ewk_private.h to make sure EwkFindOptions and WKFindOptions enumerations match. > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:963 > + WKPageFindString(toAPI(priv->pageClient->page()), findText.get(), options, maxMatchCount); Here you will need to cast the EwkFindOptions into WKFindOptions. > Source/WebKit2/UIProcess/API/efl/ewk_view.h:194 > + EWK_FIND_OPTIONS_CASE_INSENSITIVE = 1 << 0, Would be nice to have per-value documentation as some of them are not obvious. WebKitFindOptions has the documented, you can use it as reference. > Source/WebKit2/UIProcess/API/efl/ewk_view.h:203 > +typedef uint32_t EwkFindOptions; In other files we use: typedef _EwkFindOptions EwkFindOptions; > Source/WebKit2/UIProcess/API/efl/ewk_view.h:420 > +* @param max_match_count max count to find Does 0 mean unlimited? If so, it should be documented. > Source/WebKit2/UIProcess/API/efl/ewk_view.h:424 > +EAPI Eina_Bool ewk_view_text_find(Evas_Object *o, const char *text, EwkFindOptions options, unsigned max_match_count); Nice to introduce this API. However, this will highlight all the matches in the page. I think we need another API to unhighlight the search terms when the user closes the search UI. Basically, I think we need a wrapper around WKPageHideFindUI().

Comment on attachment 152499 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152499&action=review >> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:954 >> + > > Please use COMPILE_ASSERT_MATCHING_ENUM() from ewk_private.h to make sure EwkFindOptions and WKFindOptions enumerations match. Do you mean that make a new file(ewk_private.h) and defining the enums using COMPILE_ASSERT_MATCHING_ENUM?

Comment on attachment 152499 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152499&action=review >>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:954 >>> + >> >> Please use COMPILE_ASSERT_MATCHING_ENUM() from ewk_private.h to make sure EwkFindOptions and WKFindOptions enumerations match. > > Do you mean that make a new file(ewk_private.h) and defining the enums using COMPILE_ASSERT_MATCHING_ENUM? I understand what you say by referring gtk port. I'll make another patch for this. Do you agree with me? I'll make ewk_private.h and define it.

(In reply to comment #15) > (From update of attachment 152499 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=152499&action=review > > >>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:954 > >>> + > >> > >> Please use COMPILE_ASSERT_MATCHING_ENUM() from ewk_private.h to make sure EwkFindOptions and WKFindOptions enumerations match. > > > > Do you mean that make a new file(ewk_private.h) and defining the enums using COMPILE_ASSERT_MATCHING_ENUM? > > I understand what you say by referring gtk port. > I'll make another patch for this. Do you agree with me? > > I'll make ewk_private.h and define it. ewk_private.h and the macro are already defined. You just need to use them :)

(In reply to comment #16) > (In reply to comment #15) > > (From update of attachment 152499 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=152499&action=review > > > > >>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:954 > > >>> + > > >> > > >> Please use COMPILE_ASSERT_MATCHING_ENUM() from ewk_private.h to make sure EwkFindOptions and WKFindOptions enumerations match. > > > > > > Do you mean that make a new file(ewk_private.h) and defining the enums using COMPILE_ASSERT_MATCHING_ENUM? > > > > I understand what you say by referring gtk port. > > I'll make another patch for this. Do you agree with me? > > > > I'll make ewk_private.h and define it. > > ewk_private.h and the macro are already defined. You just need to use them :) You can use ewk_navigation_policy_decision.cpp as example, it already uses the macro.

Created attachment 152668 [details] Patch

Comment on attachment 152668 [details] Patch LGTM.

If there is no comments from Grzegorz, looks fine. CC'ing Grzegorz.

Comment on attachment 152668 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152668&action=review > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:688 > + * The text which is used by WKPageSetPageFindClient was found. Reports that the requested text was found. > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:690 > + * Emits signal: "text,found" with count. ... with the number of matches. > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:957 > +COMPILE_ASSERT_MATCHING_ENUM(EWK_FIND_OPTIONS_CASE_INSENSITIVE, kWKFindOptionsCaseInsensitive); Please add short comment why those asserts are needed. > Source/WebKit2/UIProcess/API/efl/ewk_view.h:48 > + * - "text,found", unsigned*: text was found by ewk_view_text_find with the number of matches. what do you say about: the requested text was found and it gives the number of matches. ? > Source/WebKit2/UIProcess/API/efl/ewk_view.h:194 > + * @EWK_FIND_OPTIONS_NONE: no search flags, this means a case I think those detailed descriptions should be moved next to their declarations as it has been done in wk1. > Source/WebKit2/UIProcess/API/efl/ewk_view.h:208 > + * @info Keep this in sync with WKFindOptions.h This line should be moved below. > Source/WebKit2/UIProcess/API/efl/ewk_view.h:209 > + * Enum values used to specify search options. I'd add @brief tag here. > Source/WebKit2/UIProcess/API/efl/ewk_view.h:433 > +* Queries to find text Searches the given string in the document. > Source/WebKit2/UIProcess/API/efl/ewk_view.h:438 > +* @param max_match_count max count to find, unlimited if 0 please remove max_match_count > Source/WebKit2/UIProcess/API/efl/ewk_view.h:440 > +* @return @c EINA_TRUE on successful request, @c EINA_FALSE on errors What is returned if string wasn't found? :) > Source/WebKit2/UIProcess/API/efl/ewk_view.h:445 > +* Queries to clear highlight of searched text. Clears the highlight ... > Source/WebKit2/UIProcess/API/efl/ewk_view.h:449 > +* @return @c EINA_TRUE on successful request, @c EINA_FALSE on errors in wk1 we just use "@c EINA_TRUE on success"

Comment on attachment 152668 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152668&action=review >> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:957 >> +COMPILE_ASSERT_MATCHING_ENUM(EWK_FIND_OPTIONS_CASE_INSENSITIVE, kWKFindOptionsCaseInsensitive); > > Please add short comment why those asserts are needed. Because the EkWKFindOptions should sync with kWKFindOptions. Plz refer above christophe's comments.

Comment on attachment 152668 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152668&action=review >> Source/WebKit2/UIProcess/API/efl/ewk_view.h:48 >> + * - "text,found", unsigned*: text was found by ewk_view_text_find with the number of matches. > > what do you say about: the requested text was found and it gives the number of matches. ? Yes. the unsigned * is the number of matches.

Comment on attachment 152668 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152668&action=review >> Source/WebKit2/UIProcess/API/efl/ewk_view.h:440 >> +* @return @c EINA_TRUE on successful request, @c EINA_FALSE on errors > > What is returned if string wasn't found? :) There is another WKPageFindClient callback. I'll make another patch for it later.

Created attachment 152739 [details] Patch

(In reply to comment #22) > (From update of attachment 152668 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=152668&action=review > > >> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:957 > >> +COMPILE_ASSERT_MATCHING_ENUM(EWK_FIND_OPTIONS_CASE_INSENSITIVE, kWKFindOptionsCaseInsensitive); > > > > Please add short comment why those asserts are needed. > > Because the EkWKFindOptions should sync with kWKFindOptions. > Plz refer above christophe's comments. I meant to add this explanation to the source file see other WebKit-EFL files.

(In reply to comment #23) > (From update of attachment 152668 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=152668&action=review > > >> Source/WebKit2/UIProcess/API/efl/ewk_view.h:48 > >> + * - "text,found", unsigned*: text was found by ewk_view_text_find with the number of matches. > > > > what do you say about: the requested text was found and it gives the number of matches. ? > > Yes. the unsigned * is the number of matches. I've suggested to change the description, imo "text was found by ewk_view_text_find with the number of matches" doesn't sound well.

Created attachment 152878 [details] Patch

Comment on attachment 152878 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152878&action=review > Source/WebKit2/UIProcess/API/efl/ewk_view.h:194 > + * @EWK_FIND_OPTIONS_CASE_INSENSITIVE: case insensitive search. As I mentioned before I'd prefer to keep those descriptions next to declarations for example typedef enum _EwkFindOptions { ... EWK_FIND_OPTIONS_CASE_INSENSITIVE = 1 << 0, /**< search text only at the begining of the words*/ ... } EwkFindOptions; > Source/WebKit2/UIProcess/API/efl/ewk_view.h:196 > + * begining of the words. beginning > Source/WebKit2/UIProcess/API/efl/ewk_view.h:469 > +* Clear the highlight of searched text. Clears

Created attachment 152944 [details] Patch

LGTM.

Comment on attachment 152944 [details] Patch LGTM too.

Comment on attachment 152944 [details] Patch rs=me, based on LGTMs from EFL guys.

Comment on attachment 152944 [details] Patch Rejecting attachment 152944 [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: (offset 10 lines). Hunk #3 succeeded at 471 with fuzz 2 (offset 33 lines). patching file Source/WebKit2/UIProcess/API/efl/ewk_view_find_client.cpp patching file Source/WebKit2/UIProcess/API/efl/ewk_view_find_client_private.h patching file Source/WebKit2/UIProcess/API/efl/ewk_view_private.h Hunk #1 succeeded at 62 (offset 4 lines). Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Kentaro Ha..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue/ Full output: http://queues.webkit.org/results/13367727

Created attachment 155996 [details] Patch

Comment on attachment 155996 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=155996&action=review > Source/WebKit2/UIProcess/API/efl/ewk_view.h:62 > + * - "text,found", unsigned*: the requested text was found and it gives the number of matches. Using only "unsigned" without an actual type after that is a WebKit convention not followed by the EFL themselves. You should either report that it's an unsigned int, unsigned long or whatever or, if you think it makes sense, change that to an actual struct, which could contain the text searched and the match count. > Source/WebKit2/UIProcess/API/efl/ewk_view.h:224 > +typedef enum _EwkFindOptions { Although it does not make much difference, in WK1 we have been declaring enums this way: enum _Foo_Bar { }; typedef enum _Foo_Bar Foo_Bar; The EFL themselves are not very consistent in this regard, so I recommend following our usual conventions here. > Source/WebKit2/UIProcess/API/efl/ewk_view.h:234 > +} EwkFindOptions; structs and enums have each word separated by an underscore, so this should actually be called Ewk_Find_Options. > Source/WebKit2/UIProcess/API/efl/ewk_view.h:528 > +* Clears the highlight of searched text. If "WKPageHideFindUI" is only responsible for clearing the search highlights in the page, first of all ewk_view_text_find should document that the highlight happens at all; if that API call is responsible for more than that, the documentation here should be updated accordingly. > Source/WebKit2/UIProcess/API/efl/ewk_view.h:534 > +EAPI Eina_Bool ewk_view_text_find_finish(Evas_Object *o); "finish" sounds vague in this context. A good name would really depend on what this call really does; if it only clears the search highlights, "ewk_view_text_find_highlight_clear()" would be more self-explanatory.

Comment on attachment 155996 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=155996&action=review >> Source/WebKit2/UIProcess/API/efl/ewk_view.h:528 >> +* Clears the highlight of searched text. > > If "WKPageHideFindUI" is only responsible for clearing the search highlights in the page, first of all ewk_view_text_find should document that the highlight happens at all; if that API call is responsible for more than that, the documentation here should be updated accordingly. The highlight is only done if EWK_FIND_OPTIONS_SHOW_HIGHLIGHT is set, could you plz guide me how to specify the documentation in ewk_view_text_find?

Created attachment 156875 [details] Patch

You already get a r+, please request cq? with reviewer name in ChangeLog. When there is no problem, committers will set cq+.

Created attachment 156886 [details] Patch

Comment on attachment 156886 [details] Patch Clearing flags on attachment: 156886 Committed r124864: <http://trac.webkit.org/changeset/124864>

All reviewed patches have been landed. Closing bug.