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, gustavo, gyuyoung.kim, kenneth, rakuco, tonikitoo, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Mikhail Pozdnyakov
2012-07-31 06:01:31 PDT
Created attachment 155515 [details]
patch
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 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. (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 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 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.
Created attachment 156608 [details]
to be landed
Considered feedback from Kenneth and Raphael.
Comment on attachment 156608 [details]
to be landed
Did not include reviewer name into changelog.
Created attachment 156611 [details]
to be landed
Added reviewer to the ChangeLog.
Comment on attachment 156611 [details] to be landed Clearing flags on attachment: 156611 Committed r124742: <http://trac.webkit.org/changeset/124742> All reviewed patches have been landed. Closing bug. |