Bug 92745 - [EFL] [GTK] Register Protocol Handler Client is never deleted
Summary: [EFL] [GTK] Register Protocol Handler Client is never deleted
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mikhail Pozdnyakov
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-07-31 06:01 PDT by Mikhail Pozdnyakov
Modified: 2012-08-06 01:02 PDT (History)
7 users (show)

See Also:


Attachments
patch (9.03 KB, patch)
2012-07-31 06:24 PDT, Mikhail Pozdnyakov
gns: review+
Details | Formatted Diff | Diff
to be landed (8.30 KB, patch)
2012-08-05 23:55 PDT, Mikhail Pozdnyakov
no flags Details | Formatted Diff | Diff
to be landed (8.31 KB, patch)
2012-08-06 00:08 PDT, Mikhail Pozdnyakov
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.