Bug 92745 – [EFL] [GTK] Register Protocol Handler Client is never deleted

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Mikhail Pozdnyakov]

Register Protocol Handler Client is never deleted on EFL and GTK ports.

[Mikhail Pozdnyakov]

Created attachment 155515 [details]
patch

[Kenneth Rohde Christiansen]

Comment on attachment 155515 [details]
patch

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

Why are you doing EFL and GTK at the same time? Better get a GTK guy to have a quick look as well

> Source/WebKit/efl/WebCoreSupport/RegisterProtocolHandlerClientEfl.h:57
> +
> +    explicit RegisterProtocolHandlerClientEfl(Evas_Object* view);
>  };

As it is private maybe the explicit doesnt really matter

> Source/WebKit/efl/ewk/ewk_view.cpp:259
>  #if ENABLE(INPUT_TYPE_COLOR)
>      WebCore::ColorChooserClient* colorChooserClient;
>  #endif
> +#if ENABLE(REGISTER_PROTOCOL_HANDLER) || ENABLE(CUSTOM_SCHEME_HANDLER)
> +    OwnPtr<WebCore::RegisterProtocolHandlerClientEfl> registerProtocolHandlerClient;
> +#endif

I really wonder how much it makes sense to make the EFL port support not having these things always enabled. Basically it should be guarded if you don't want to officially turn it on yet, but if not it doesnt make much sense

[Mikhail Pozdnyakov]

Comment on attachment 155515 [details]
patch

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

>> Source/WebKit/efl/WebCoreSupport/RegisterProtocolHandlerClientEfl.h:57
>>  };
> 
> As it is private maybe the explicit doesnt really matter

Agree, do not need explicit here.

>> Source/WebKit/efl/ewk/ewk_view.cpp:259
>> +#endif
> 
> I really wonder how much it makes sense to make the EFL port support not having these things always enabled. Basically it should be guarded if you don't want to officially turn it on yet, but if not it doesnt make much sense

I am quite OK with removing guards, but at the moment there are everywhere including WebCore RegisterProtocolHanler - related stuff. And removing those is for sure is out of this bug scope.

[Mikhail Pozdnyakov]

(In reply to comment #2)
> (From update of attachment 155515 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=155515&action=review
> 
> Why are you doing EFL and GTK at the same time? 
The fixes for both ports are completely identical, and the code being fixed was also introduced with one commit cd6b2961205c1278ffd06ef1617dc09d2c599240.

[Gustavo Noronha (kov)]

Comment on attachment 155515 [details]
patch

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

>>> Source/WebKit/efl/WebCoreSupport/RegisterProtocolHandlerClientEfl.h:57
>>>  };
>> 
>> As it is private maybe the explicit doesnt really matter
> 
> Agree, do not need explicit here.

I won't set cq+ because of these comments.

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 155515 [details]
patch

Nitpicking a little, I wonder if it really makes sense to add Intel's copyright to all those files; not that many lines have been touched after all.

[Mikhail Pozdnyakov]

Created attachment 156608 [details]
to be landed

Considered feedback from Kenneth and Raphael.

[Mikhail Pozdnyakov]

Comment on attachment 156608 [details]
to be landed

Did not include reviewer name into changelog.

[Mikhail Pozdnyakov]

Created attachment 156611 [details]
to be landed

Added reviewer to the ChangeLog.

[WebKit Review Bot]

Comment on attachment 156611 [details]
to be landed

Clearing flags on attachment: 156611

Committed r124742: <http://trac.webkit.org/changeset/124742>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.