Bug 102676 – [chromium] move methods that only use the WebKit API from DRTTestRunner to TestRunner

• Component: New Bugs
• Status: RESOLVED FIXED

[chromium] move methods that only use the WebKit API from DRTTestRunner to TestRunner

Created attachment 174954 [details]
Patch

[Tony Chang]

Comment on attachment 174954 [details]
Patch

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"?

Created attachment 175011 [details]
Patch

(In reply to comment #2)
> (From update of attachment 174954 [details])
> 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.

I've moved the comment about logErrorToConsole down to the function list. The other comments refer to deleted code which is not listed below, so I left them here

> 
> > 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)?  

Yes and yes.

> 
> 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"?

done

Created attachment 175012 [details]
Patch for landing

Comment on attachment 175012 [details]
Patch for landing

Turns out that the css3/filters tests set a web preference we don't support, and so we actually depend on the correct format in logErrorToConsole aka printErrorMessage

[WebKit Review Bot]

Comment on attachment 175012 [details]
Patch for landing

Clearing flags on attachment: 175012

Committed r135184: <http://trac.webkit.org/changeset/135184>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.