Bug 90927 – [EFL][WK2] Add ewk_view_find_client.h / cpp for wrapping WKPageSetPageFindClient.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Hyerim Bae]

Define ewkViewFindClientAttatchClient in ewk_view_find_client for WKPageSetPageFindClient.

[Hyerim Bae]

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

[Chris Dumez]

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.

[Gyuyoung Kim]

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

Please fix changelog nits first.

[Hyerim Bae]

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

Apply chris's comments.

[Gyuyoung Kim]

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.

[Hyerim Bae]

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

Apply gyoyoung's comments.

[Gyuyoung Kim]

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

Looks good to me.

[Chris Dumez]

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.

[Gyuyoung Kim]

Oops. I missed to check ChangeLog. Sorry.

[Hyerim Bae]

Created attachment 152491 [details]
Patch

[Chris Dumez]

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".

[Hyerim Bae]

Created attachment 152499 [details]
Patch

[Chris Dumez]

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().

[Hyerim Bae]

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?

[Hyerim Bae]

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.

[Chris Dumez]

(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 :)

[Chris Dumez]

(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.

[Hyerim Bae]

Created attachment 152668 [details]
Patch

[Chris Dumez]

Comment on attachment 152668 [details]
Patch

LGTM.

[Gyuyoung Kim]

If there is no comments from Grzegorz, looks fine. 

CC'ing Grzegorz.

[Grzegorz Czajkowski]

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"

[Hyerim Bae]

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.

[Hyerim Bae]

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.

[Hyerim Bae]

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.

[Hyerim Bae]

Created attachment 152739 [details]
Patch

[Grzegorz Czajkowski]

(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.

[Grzegorz Czajkowski]

(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.

[Hyerim Bae]

Created attachment 152878 [details]
Patch

[Grzegorz Czajkowski]

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

[Hyerim Bae]

Created attachment 152944 [details]
Patch

[Grzegorz Czajkowski]

LGTM.

[Gyuyoung Kim]

Comment on attachment 152944 [details]
Patch

LGTM too.

[Kentaro Hara]

Comment on attachment 152944 [details]
Patch

rs=me, based on LGTMs from EFL guys.

[WebKit Review Bot]

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

[Hyerim Bae]

Created attachment 155996 [details]
Patch

[Raphael Kubo da Costa (:rakuco)]

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.

[Hyerim Bae]

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?

[Hyerim Bae]

Created attachment 156875 [details]
Patch

[Gyuyoung Kim]

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

[Hyerim Bae]

Created attachment 156886 [details]
Patch

[WebKit Review Bot]

Comment on attachment 156886 [details]
Patch

Clearing flags on attachment: 156886

Committed r124864: <http://trac.webkit.org/changeset/124864>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.