Bug 129263

Summary: WebKitFindOptions shouldn't expose WEBKIT_FIND_OPTIONS_SHOW_{OVERLAY,FIND_INDICATOR,HIGHLIGHT}
Product: WebKit Reporter: Enrique Ocaña <eocanha>
Component: WebKit2Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: berto, cgarcia, commit-queue, gustavo, mrobinson, svillar
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Enrique Ocaña 2014-02-24 11:48:40 PST
The values mentioned in the summary are used only for internal operations and shouldn't be available to API users in Source/WebKit2/UIProcess/API/gtk/WebKitFindController.h
Comment 1 Enrique Ocaña 2014-02-24 12:05:23 PST
Created attachment 225079 [details]
Patch
Comment 2 WebKit Commit Bot 2014-02-24 12:07:30 PST
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Comment 3 Enrique Ocaña 2014-02-25 06:26:56 PST
Sergio and I talked yesterday about trying to convert early from WebKitFindOptions to WebKit::FindOptions in webKitFindControllerPerform(), store the result in the uint32_t temporary variable and then operate on it using WebKit::FindOptions values.

However, I've realized that some functions such as webkit_find_controller_search_next() store values from the WebKit::FindOptions domain in WebKitFindController->priv->findOptions. This made me think that the only reasonable way to address this issue is to change the semantics of WebKitFindController->priv->findOptions:

Before, it was considered to be in the WebKitFindOptions domain. Now, it's considered to be in the WebKit::FindOptions domain.

This means that now we need an additional toWebKitFindOptions() method to perform the conversion in the WebKit::FindOptions --> WebKitFindOptions way, so that we car return the value in webkit_find_controller_get_options(). I've renamed the old method that converted WebKitFindOptions --> WebKit::FindOptions as toFindOptions().

I hope this justification to be useful for the review process.
Comment 4 Sergio Villar Senin 2014-02-25 09:11:18 PST
Comment on attachment 225079 [details]
Patch

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

Almost there. I'd suggest some changes tough.

> Source/WebKit2/ChangeLog:7
> +

You still need a short description of the problem/fix here even if you add specific comments per file.

> Source/WebKit2/UIProcess/API/gtk/WebKitFindController.cpp:99
> +

I don't see the point of having two functions. Let's just have the one receiving a uint32_t and let it do all the conversion, we don't need an extra cast and a function call (even if it's inline)

> Source/WebKit2/UIProcess/API/gtk/WebKitFindController.cpp:112
>  }

Same here, I think we only use the second one, so let's keep that and move the implementation of the conversion there.

> Source/WebKit2/UIProcess/API/gtk/WebKitFindController.cpp:433
> +    webKitFindControllerSetSearchData(findController, searchText, toFindOptions(static_cast<WebKitFindOptions>(findOptions)), maxMatchCount);

Just call the uint32_t version, you don't need the cast.

> Source/WebKit2/UIProcess/API/gtk/WebKitFindController.cpp:490
> +    webKitFindControllerSetSearchData(findController, searchText, toFindOptions(static_cast<WebKitFindOptions>(findOptions)), maxMatchCount);

Ditto.
Comment 5 Enrique Ocaña 2014-02-25 11:10:20 PST
Created attachment 225163 [details]
Patch
Comment 6 Sergio Villar Senin 2014-02-26 03:38:52 PST
Comment on attachment 225163 [details]
Patch

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

r=me provided you fix the issues I mention.

> Source/WebKit2/UIProcess/API/gtk/WebKitFindController.cpp:86
> +namespace FindOptions {

Let's use a different function name instead of adding another namespace, see bellow.

> Source/WebKit2/UIProcess/API/gtk/WebKitFindController.cpp:88
> +static inline WebKit::FindOptions toFindOptions(uint32_t type)

I'd suggest toWebFindOptions for example.

BTW, I spot this in the previous review but forgot to add a comment. Let's change the name of the argument, "type" does not mean anything, we need a more meaningful name like findOptions for example.

> Source/WebKit2/UIProcess/API/gtk/WebKitFindController.cpp:97
> +static inline WebKitFindOptions toWebKitFindOptions(uint32_t type)

Same thing about the name of the argument.

> Source/WebKit2/UIProcess/API/gtk/WebKitFindController.cpp:106
> +} // namespace WebKitFindController

We can remove this then.
Comment 7 Enrique Ocaña 2014-02-26 05:03:13 PST
Created attachment 225248 [details]
Patch
Comment 8 WebKit Commit Bot 2014-02-26 05:46:03 PST
Comment on attachment 225248 [details]
Patch

Clearing flags on attachment: 225248

Committed r164715: <http://trac.webkit.org/changeset/164715>
Comment 9 WebKit Commit Bot 2014-02-26 05:46:06 PST
All reviewed patches have been landed.  Closing bug.