Bug 96732

Summary: Add *explicit* keyword to constructors to WebKit2/UIProcess
Product: WebKit Reporter: Gyuyoung Kim <gyuyoung.kim>
Component: WebKit EFLAssignee: Gyuyoung Kim <gyuyoung.kim>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, eric.carlson, feature-media-reviews, haraken, lucas.de.marchi, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

Gyuyoung Kim
Reported 2012-09-13 23:40:58 PDT
As a step of *explicit* cleanup for constructors which have only one parameter, *explicit* is added to WebKit2/UIProcess in order to avoid implicit type conversion.
Attachments
Patch (13.57 KB, patch)
2012-09-13 23:45 PDT, Gyuyoung Kim 		no flags
DetailsFormatted DiffDiff
Patch (12.77 KB, patch)
2012-09-14 17:49 PDT, Gyuyoung Kim 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Gyuyoung Kim
Comment 1 2012-09-13 23:45:50 PDT
Created attachment 164058 [details] Patch
Gyuyoung Kim
Comment 2 2012-09-13 23:46:32 PDT
This patch should pass all ews except for chromium.
Chris Dumez
Comment 3 2012-09-13 23:49:02 PDT
Comment on attachment 164058 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=164058&action=review > Source/WebKit2/UIProcess/WebPreferences.h:83 > + explicit WebPreferences(const WebPreferences&); This is a copy constructor. Please don't add explicit here.
Kentaro Hara
Comment 4 2012-09-14 01:32:39 PDT
Comment on attachment 164058 [details] Patch Marking r+ with nits: - Please remove 'explicit' from the copy constructor that Christophe pointed out. - Please rebase your patch. - Please keep watching bots when landing.
Kentaro Hara
Comment 5 2012-09-14 01:33:09 PDT
Comment on attachment 164058 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=164058&action=review > Source/WebKit2/UIProcess/WebPreferences.h:82 > + explicit WebPreferences(const String& identifier); Nit: 'identifier' is not needed.
Mikhail Pozdnyakov
Comment 6 2012-09-14 01:41:00 PDT
Comment on attachment 164058 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=164058&action=review > Source/WebKit2/UIProcess/Authentication/AuthenticationDecisionListener.h:53 > + explicit AuthenticationDecisionListener(AuthenticationChallengeProxy*); Just wondering, does it make a lot of sense to make private constructors explicit?
Gyuyoung Kim
Comment 7 2012-09-14 17:49:42 PDT
Created attachment 164257 [details] Patch
Gyuyoung Kim
Comment 8 2012-09-14 18:03:55 PDT
(In reply to comment #6) > Just wondering, does it make a lot of sense to make private constructors explicit? Compared to public constructor, private one seems unlikely to be used wrongly. However, in my humble opinion, to use explicit keyword is a little bit more safe than not to use it in open source case. For example, anyone can touch source code in factory functions of class inside with yoda style.
WebKit Review Bot
Comment 9 2012-09-14 23:14:17 PDT
Comment on attachment 164257 [details] Patch Clearing flags on attachment: 164257 Committed r128683: <http://trac.webkit.org/changeset/128683>
WebKit Review Bot
Comment 10 2012-09-14 23:14:22 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.