[webkit-reviews] review granted: [Bug 102676] [chromium] move methods that only use the WebKit API from DRTTestRunner to TestRunner : [Attachment 174954] Patch

[bugzilla-daemon at webkit.org]

Tony Chang <tony at chromium.org> has granted jochen at chromium.org's request for
review:
Bug 102676: [chromium] move methods that only use the WebKit API from
DRTTestRunner to TestRunner
https://bugs.webkit.org/show_bug.cgi?id=102676

Attachment 174954: Patch
https://bugs.webkit.org/attachment.cgi?id=174954&action=review

------- Additional Comments from Tony Chang <tony at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=174954&action=review


> Tools/ChangeLog:12
> +	   In addition, I've replaced parsePageNumber with the more commonly
used
> +	   cppVariantToInt32, changed logErrorToConsole to just print the error

> +	   message instead of logging a real console error, moved abortModal to

> +	   the list of stubbed out methods, and removed
> +	   setAutomaticLinkDetectionEnabled which wasn't used anywhere.

The ChangeLog section below is normally where you put comments like this.  This
section is normally about why you are making this change.

> Tools/DumpRenderTree/chromium/TestRunner/src/TestRunner.cpp:732
> +void TestRunner::logErrorToConsole(const string& text)
> +{
> +    m_delegate->printMessage(string("CONSOLE MESSAGE: JavaScript ERROR: ") +
text + "\n");
> +}

Does this always produce messages in the same order if the test has a mix of
console.log and calls that might go through here?  I guess in practice, no test
should go through this code path (i.e., will any tests fail if you change the
output format and remove the prefix)?  

It's a bit confusing to name this logErrorToConsole when it's not actually
logging an error to the javascript console.  Maybe rename it to something like
"printErrorMessage"?