Bug 183487

Summary: [DRT] TestOptions should not be ObjC.
Product: WebKit Reporter: Ross Kirsling <ross.kirsling>
Component: New BugsAssignee: Ross Kirsling <ross.kirsling>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, dbates, don.olmstead, ews-watchlist, pvollan, stephan.szabo, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 179299    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch (manual)
none
Patch (manual)
none
Patch (manual) none

Ross Kirsling
Reported 2018-03-08 17:01:57 PST
[DRT] TestOptions should not be ObjC.
Attachments
Patch (24.65 KB, patch)
2018-03-08 17:07 PST, Ross Kirsling
no flags
Patch (24.73 KB, patch)
2018-03-08 19:10 PST, Ross Kirsling
no flags
Patch (24.89 KB, patch)
2018-03-12 11:26 PDT, Ross Kirsling
no flags
Patch (25.22 KB, patch)
2018-03-12 11:55 PDT, Ross Kirsling
no flags
Patch (manual) (19.30 KB, patch)
2018-03-12 12:35 PDT, Ross Kirsling
no flags
Patch (manual) (19.17 KB, patch)
2018-03-12 13:54 PDT, Ross Kirsling
no flags
Patch (manual) (19.10 KB, patch)
2018-03-12 14:01 PDT, Ross Kirsling
no flags
Ross Kirsling
Comment 1 2018-03-08 17:07:06 PST
EWS Watchlist
Comment 2 2018-03-08 19:09:59 PST
Attachment 335368 [details] did not pass style-queue: ERROR: Tools/DumpRenderTree/TestOptions.cpp:29: Streams are highly discouraged. [readability/streams] [3] Total errors found: 1 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.
Ross Kirsling
Comment 3 2018-03-08 19:10:34 PST
EWS Watchlist
Comment 4 2018-03-08 19:13:06 PST
Attachment 335382 [details] did not pass style-queue: ERROR: Tools/DumpRenderTree/TestOptions.cpp:29: Streams are highly discouraged. [readability/streams] [3] Total errors found: 1 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.
Per Arne Vollan
Comment 5 2018-03-10 07:49:54 PST
Comment on attachment 335382 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=335382&action=review > Tools/DumpRenderTree/TestOptions.h:45 > + TestOptions(const std::string&, const std::string&); Would it make sense to only have one path parameter, instead of two?
Ross Kirsling
Comment 6 2018-03-10 13:55:18 PST
(In reply to Per Arne Vollan from comment #5) > Comment on attachment 335382 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=335382&action=review > > > Tools/DumpRenderTree/TestOptions.h:45 > > + TestOptions(const std::string&, const std::string&); > > Would it make sense to only have one path parameter, instead of two? I thought about doing so, but ended up aligning with the signature of the method that this logic was originally copied from: https://github.com/WebKit/webkit/blob/master/Tools/WebKitTestRunner/TestController.cpp#L1010 Would you prefer I go the other way?
Per Arne Vollan
Comment 7 2018-03-12 11:18:08 PDT
(In reply to Ross Kirsling from comment #6) > (In reply to Per Arne Vollan from comment #5) > > Comment on attachment 335382 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=335382&action=review > > > > > Tools/DumpRenderTree/TestOptions.h:45 > > > + TestOptions(const std::string&, const std::string&); > > > > Would it make sense to only have one path parameter, instead of two? > > I thought about doing so, but ended up aligning with the signature of the > method that this logic was originally copied from: > https://github.com/WebKit/webkit/blob/master/Tools/WebKitTestRunner/ > TestController.cpp#L1010 > > Would you prefer I go the other way? I see. I think keeping it as is, is fine then. Perhaps you could add the parameter names in the header file for readability?
Ross Kirsling
Comment 8 2018-03-12 11:26:39 PDT
Per Arne Vollan
Comment 9 2018-03-12 11:34:12 PDT
Comment on attachment 335610 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=335610&action=review > Tools/DumpRenderTree/TestOptions.cpp:1 > +/* Was this file moved from Tools/DumpRenderTree/TestOptions.mm?
Ross Kirsling
Comment 10 2018-03-12 11:50:45 PDT
Correct. Looks like this file was modified, so I'll need to rebase.
Ross Kirsling
Comment 11 2018-03-12 11:55:05 PDT
EWS Watchlist
Comment 12 2018-03-12 11:57:49 PDT
Attachment 335615 [details] did not pass style-queue: ERROR: Tools/DumpRenderTree/TestOptions.cpp:29: Streams are highly discouraged. [readability/streams] [3] Total errors found: 1 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.
Ross Kirsling
Comment 13 2018-03-12 12:31:00 PDT
Interestingly, in my local git diff, I have: > --- a/Tools/DumpRenderTree/TestOptions.mm > +++ b/Tools/DumpRenderTree/TestOptions.cpp But `webkit-patch upload` turns this into: > --- /dev/null > +++ b/Tools/DumpRenderTree/TestOptions.cpp > --- a/Tools/DumpRenderTree/TestOptions.mm > +++ /dev/null
Ross Kirsling
Comment 14 2018-03-12 12:35:41 PDT
Created attachment 335625 [details] Patch (manual)
EWS Watchlist
Comment 15 2018-03-12 12:38:15 PDT
Attachment 335625 [details] did not pass style-queue: ERROR: Tools/DumpRenderTree/TestOptions.cpp:29: Streams are highly discouraged. [readability/streams] [3] Total errors found: 1 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.
Per Arne Vollan
Comment 16 2018-03-12 13:16:58 PDT
Comment on attachment 335625 [details] Patch (manual) View in context: https://bugs.webkit.org/attachment.cgi?id=335625&action=review > Tools/DumpRenderTree/TestOptions.h:47 > + bool webViewIsCompatibleWithOptions(const TestOptions& other) const; I think we can leave this method as is, since the added parameter name is generic.
Ross Kirsling
Comment 17 2018-03-12 13:54:01 PDT
Created attachment 335635 [details] Patch (manual)
EWS Watchlist
Comment 18 2018-03-12 13:57:25 PDT
Attachment 335635 [details] did not pass style-queue: ERROR: Tools/DumpRenderTree/TestOptions.cpp:29: Streams are highly discouraged. [readability/streams] [3] Total errors found: 1 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.
Ross Kirsling
Comment 19 2018-03-12 14:01:54 PDT
Created attachment 335637 [details] Patch (manual)
EWS Watchlist
Comment 20 2018-03-12 14:03:44 PDT
Attachment 335637 [details] did not pass style-queue: ERROR: Tools/DumpRenderTree/TestOptions.cpp:29: Streams are highly discouraged. [readability/streams] [3] Total errors found: 1 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.
Ross Kirsling
Comment 21 2018-03-12 14:18:12 PDT
I don't know why this style error won't go away. You can see from the diff that I'm no longer touching any lines related to the ifstream...
Ross Kirsling
Comment 22 2018-03-12 16:15:40 PDT
Anyway, if anyone is concerned about the style violation, it goes back to the first iteration of the style checker (https://bugs.webkit.org/show_bug.cgi?id=25884). The rule comes from Google's style guide (https://google.github.io/styleguide/cppguide.html#Streams), which contains the quote: > Use streams only when they are the best tool for the job. This is typically the case when the I/O is ad-hoc, local, human-readable, and targeted at other developers rather than end-users. So I think we're probably okay here. :)
Per Arne Vollan
Comment 23 2018-03-12 16:19:47 PDT
Comment on attachment 335637 [details] Patch (manual) R=me. Please make sure the bots are green before landing.
Ross Kirsling
Comment 24 2018-03-12 18:03:16 PDT
Comment on attachment 335637 [details] Patch (manual) Note: The WinCairo failure here is a fluke; there's no behavioral change here from the previous patch attempt.
WebKit Commit Bot
Comment 25 2018-03-12 18:29:50 PDT
Comment on attachment 335637 [details] Patch (manual) Clearing flags on attachment: 335637 Committed r229565: <https://trac.webkit.org/changeset/229565>
WebKit Commit Bot
Comment 26 2018-03-12 18:29:52 PDT
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 27 2018-03-12 18:30:26 PDT
Note You need to log in before you can comment on or make changes to this bug.