Bug 92745

Summary: [EFL] [GTK] Register Protocol Handler Client is never deleted
Product: WebKit Reporter: Mikhail Pozdnyakov <mikhail.pozdnyakov>
Component: WebKit Misc.Assignee: Mikhail Pozdnyakov <mikhail.pozdnyakov>
Status: RESOLVED FIXED    
Severity: Normal CC: dw.im, gns, gyuyoung.kim, kenneth, rakuco, tonikitoo, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch
gns: review+
to be landed
none
to be landed none

Description Mikhail Pozdnyakov 2012-07-31 06:01:31 PDT
Register Protocol Handler Client is never deleted on EFL and GTK ports.
Comment 1 Mikhail Pozdnyakov 2012-07-31 06:24:26 PDT
Created attachment 155515 [details]
patch
Comment 2 Kenneth Rohde Christiansen 2012-08-02 09:52:19 PDT
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
Comment 3 Mikhail Pozdnyakov 2012-08-03 00:17:33 PDT
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.
Comment 4 Mikhail Pozdnyakov 2012-08-03 00:21:26 PDT
(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.
Comment 5 Gustavo Noronha (kov) 2012-08-03 08:08:21 PDT
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.
Comment 6 Raphael Kubo da Costa (:rakuco) 2012-08-03 16:04:50 PDT
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.
Comment 7 Mikhail Pozdnyakov 2012-08-05 23:55:11 PDT
Created attachment 156608 [details]
to be landed

Considered feedback from Kenneth and Raphael.
Comment 8 Mikhail Pozdnyakov 2012-08-06 00:02:14 PDT
Comment on attachment 156608 [details]
to be landed

Did not include reviewer name into changelog.
Comment 9 Mikhail Pozdnyakov 2012-08-06 00:08:17 PDT
Created attachment 156611 [details]
to be landed

Added reviewer to the ChangeLog.
Comment 10 WebKit Review Bot 2012-08-06 01:02:33 PDT
Comment on attachment 156611 [details]
to be landed

Clearing flags on attachment: 156611

Committed r124742: <http://trac.webkit.org/changeset/124742>
Comment 11 WebKit Review Bot 2012-08-06 01:02:39 PDT
All reviewed patches have been landed.  Closing bug.