[DRT] TestOptions should not be ObjC.
Created attachment 335368 [details] Patch
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.
Created attachment 335382 [details] Patch
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.
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?
(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?
(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?
Created attachment 335610 [details] Patch
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?
Correct. Looks like this file was modified, so I'll need to rebase.
Created attachment 335615 [details] Patch
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.
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
Created attachment 335625 [details] Patch (manual)
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.
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.
Created attachment 335635 [details] Patch (manual)
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.
Created attachment 335637 [details] Patch (manual)
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.
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...
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. :)
Comment on attachment 335637 [details] Patch (manual) R=me. Please make sure the bots are green before landing.
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.
Comment on attachment 335637 [details] Patch (manual) Clearing flags on attachment: 335637 Committed r229565: <https://trac.webkit.org/changeset/229565>
All reviewed patches have been landed. Closing bug.
<rdar://problem/38400275>