Bug 183487 – [DRT] TestOptions should not be ObjC.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Ross Kirsling]

[DRT] TestOptions should not be ObjC.

[Ross Kirsling]

Created attachment 335368 [details]
Patch

[EWS Watchlist]

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]

Created attachment 335382 [details]
Patch

[EWS Watchlist]

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 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]

(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]

(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]

Created attachment 335610 [details]
Patch

[Per Arne Vollan]

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]

Correct. Looks like this file was modified, so I'll need to rebase.

[Ross Kirsling]

Created attachment 335615 [details]
Patch

[EWS Watchlist]

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]

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]

Created attachment 335625 [details]
Patch (manual)

[EWS Watchlist]

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 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]

Created attachment 335635 [details]
Patch (manual)

[EWS Watchlist]

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]

Created attachment 335637 [details]
Patch (manual)

[EWS Watchlist]

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]

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]

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 on attachment 335637 [details]
Patch (manual)

R=me. Please make sure the bots are green before landing.

[Ross Kirsling]

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 on attachment 335637 [details]
Patch (manual)

Clearing flags on attachment: 335637

Committed r229565: <https://trac.webkit.org/changeset/229565>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/38400275>