[Webkit-unassigned] [Bug 73638] [EFL] Implement 'registerProtocolHandler' function

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=73638





--- Comment #21 from Raphael Kubo da Costa (rakuco) <rakuco at webkit.org>  2012-05-03 20:38:53 PST ---
(From update of attachment 135498)
View in context: https://bugs.webkit.org/attachment.cgi?id=135498&action=review

> Source/WebKit/efl/WebCoreSupport/ChromeClientEfl.cpp:621
> +    Ewk_Custom_Handlers_Data* data = customHandlersDataCreate(m_view, scheme.utf8().data(), baseURL.utf8().data(), url.utf8().data(), title.utf8().data());
> +    ewk_custom_handlers_register_protocol_handler(data);
> +    customHandlersDataDelete(data);

Since the functions creating and destroying the data are only used in this method, I think they could be all squashed here instead of being implemented separately above. If you do that, I think it even makes sense not to use eina_stringshare at all and reduce the code even more.

> Source/WebKit/efl/ewk/ewk_custom_handlers.cpp:31
> +bool ewk_custom_handlers_register_protocol_handler(Ewk_Custom_Handlers_Data* data)
> +{
> +    EINA_SAFETY_ON_NULL_RETURN_VAL(data->o, false);
> +    evas_object_smart_callback_call(data->o, "protocolhandler,register", data);
> +    return true;
> +}

As we have been discussing in other bugs (such as bug 74921), at least for now it is recommended to put functions which act on a given object together with others. This function simply emits a signal on an ewk_view, so it can clearly be implemented as a private ewk_view function, like the others already defined that also emit ewk_view signals.

This way, not only can you get rid of this whole file, but you can also remove the "o" parameter from the Ewk_Custom_Handlers_Data struct.

> Source/WebKit/efl/ewk/ewk_view.h:63
> + *  - "protocolhandler,register", Ewk_Custom_Handlers_Data: add a handler url for the given protocol.

Nit: we usually describe the signals in a sort of passive voice. You'd normally say something like "a URL handler for the given protocol has been added [or registered]" here.

> Source/WebKit/efl/ewk/ewk_view.h:290
> +struct _Ewk_Custom_Handlers_Data {

We usually use singular forms for struct names.

> Source/WebKit/efl/ewk/ewk_view.h:291
> +    Evas_Object *o; /**< Reference to the view object. */

"o" is not a very descriptive name for this object.

> Source/WebKit/efl/ewk/ewk_view.h:295
> +    const char *target; /**< Reference to the protocols that will be handled. */
> +    const char *base_url; /**< Reference to the resolved url if the url is relative url. */
> +    const char *url; /**< Reference to the url which will handle the given protocol. */
> +    const char *title; /**< Reference to the descriptive title of the handler. */

It'd be useful to give an example for each field. For example: "Reference to the descriptive title of the handler (eg. 'Foo Bar')"

> Source/cmake/OptionsEfl.cmake:95
> +WEBKIT_FEATURE(ENABLE_REGISTER_PROTOCOL_HANDLER "Enable the custom protocol handler" DEFAULT ON)

You'll need to rebase this part; the features are defined in WebKitFeatures.cmake and have their default values overridden in Options${PORT}.cmake.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.