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

[bugzilla-daemon at webkit.org]

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





--- Comment #22 from Dongwoo Joshua Im <dw.im at samsung.com>  2012-05-07 22:47:21 PST ---
(In reply to comment #21)
> (From update of attachment 135498 [details])
> 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.
> 

Only registerProtocolHandler function is implemented in the WebKit currently,
so customHandlersDataCreate & customHandlersDataDelete functions would seem unnecessary.

But, as you know, I'm planning to implement 5 more functions which related to registerProtocolHandler,
and they will use customHandlersDataCreate & customHandlersDataDelete fuctions.
That's why I implement these two functions.

I'll implement the 5 functions just after this patch is merged into the WebKit trunk.


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

As we discussed in this bug, (and also in this comment)
I'll implement the 5 more functions which related to registerProtocolHandler.

That's why you suggested me to make new file rather than implement in the ewk_private.cpp.

If you think it is reasonable that 6 more functions are added into the ewk_private.cpp,
I'll move it to the file.



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

OK. I'll fix this.


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

OK. I'll fix this.


> > 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')"
> 

OK, I try to make it.


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

OK. I'll fix this.

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