[webkit-reviews] review denied: [Bug 191303] DumpRenderTree should report unknown options : [Attachment 358263] Added fallback else for unknown option

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Jan 4 06:24:31 PST 2019


Frédéric Wang (:fredw) <fred.wang at free.fr> has denied darshan
<dkadu at igalia.com>'s request for review:
Bug 191303: DumpRenderTree should report unknown options
https://bugs.webkit.org/show_bug.cgi?id=191303

Attachment 358263: Added fallback else for unknown option

https://bugs.webkit.org/attachment.cgi?id=358263&action=review




--- Comment #4 from Frédéric Wang (:fredw) <fred.wang at free.fr> ---
Comment on attachment 358263
  --> https://bugs.webkit.org/attachment.cgi?id=358263
Added fallback else for unknown option

View in context: https://bugs.webkit.org/attachment.cgi?id=358263&action=review

Thanks for the patch. Can you please rebase it (it seems it does not apply on
tip of tree) and add the associated ChangeLog?

Also, have you actually tested your patch with a currently unknown option? If
you find-grep tests, I'm sure some of them already use such unhandled options
and you might need to add them into TestOptions.cpp and update the
expectations. Also, I would be curious to see if the LOG_ERROR actually shows
up in the test output.

> Tools/DumpRenderTree/TestOptions.cpp:113
> +	       LOG_ERROR("Unknown option %s",key.c_str());

Missing space after comma (please run check-webkit-style before uploading the
patch)

> Tools/DumpRenderTree/TestOptions.cpp:114
> +	       break;

I think this break is not needed. We can just ignore the option (as we do now)
and continue to the next pair.


More information about the webkit-reviews mailing list