Summary: | [Qt] Add a command line option to capture stdout and stderr for DumpRenderTree | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Laszlo Gombos <laszlo.gombos> | ||||||||||||||||||
Component: | Tools / Tests | Assignee: | qi <qi.2.zhang> | ||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||
Severity: | Normal | CC: | commit-queue, koshuin, qi.2.zhang, s.mathur, webkit.review.bot | ||||||||||||||||||
Priority: | P5 | Keywords: | Qt, QtTriaged | ||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||
Hardware: | PC | ||||||||||||||||||||
OS: | All | ||||||||||||||||||||
Attachments: |
|
Description
Laszlo Gombos
2011-03-14 12:30:50 PDT
Created attachment 85807 [details]
patch
Here is the proposal, welcome the comments.
Looks reasonable to me. Comment on attachment 85807 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=85807&action=review Overall the patch looks great. r- to fix the build break. > Tools/ChangeLog:8 > + Provide an option for DumpRenderTree to redirect stdout and stderr. This line does not seem to add information as it just repeats the bug title. Perhaps you should describe how the patch work in plain English. > Tools/DumpRenderTree/qt/DumpRenderTreeQt.cpp:684 > + freopen(qPrintable(m_redirectErrorFileName), "w", stderr); This does not build in my environment (Linux) - not sure why it passed on EWS. Fails with the following error: DumpRenderTreeQt.cpp: In member function ‘void WebCore::DumpRenderTree::processArgsLine(const QStringList&)’: DumpRenderTreeQt.cpp:682: error: ignoring return value of ‘FILE* freopen(const char*, const char*, FILE*)’, declared with attribute warn_unused_result DumpRenderTreeQt.cpp:684: error: ignoring return value of ‘FILE* freopen(const char*, const char*, FILE*)’, declared with attribute warn_unused_result You can perhaps check the return value of the freopen call - if (freopen(qPrintable(m_redirectOutputFileName), "w", stdout) == 0) {..}. > Tools/DumpRenderTree/qt/main.cpp:155 > qDebug() << "Or folder containing test files: DumpRenderTree [-v|--pixel-tests] dirpath"; I would prefer to have more error-handling for the command-line. It appears that "DumpRenderTree -o FileName" is crashing (as there is no test specified) without any warning. Created attachment 85927 [details]
patch2
Changed the code based on the comments.
Attachment 85927 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/DumpRenderTree/q..." exit_code: 1
Tools/DumpRenderTree/qt/main.cpp:84: Extra space for operator ++; [whitespace/operators] [4]
Total errors found: 1 in 4 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 85928 [details]
patch3
fixed the style issue.
Comment on attachment 85928 [details]
patch3
find DumpRenderTree has 2 different way to call, current change may broke the run-webkit-tests.
Created attachment 85960 [details]
patch4
minor change.
Comment on attachment 85960 [details] patch4 View in context: https://bugs.webkit.org/attachment.cgi?id=85960&action=review > Tools/DumpRenderTree/qt/DumpRenderTreeQt.cpp:693 > + if (!m_redirectOutputFileName.isEmpty()) { > + if (!freopen(qPrintable(m_redirectOutputFileName), "w", stdout)) { > + qDebug() << "Redirect STDOUT failed."; > + return; > + } > + } > + if (!m_redirectErrorFileName.isEmpty()) { > + if (!freopen(qPrintable(m_redirectErrorFileName), "w", stderr)) { > + qDebug() << "Redirect STDERR failed."; > + return; > + } > + } > + This looks like a strange place to do this. > Tools/DumpRenderTree/qt/DumpRenderTreeQt.h:109 > + void setRedirectOutputFileName(const QString s) { m_redirectOutputFileName = s; } > + void setRedirectErrorFileName(const QString s) { m_redirectErrorFileName = s; } This should be ref to const. > Tools/DumpRenderTree/qt/main.cpp:73 > + if (++index < arguments.count() && !arguments.at(index).startsWith("-")) !arguments.at(index).startsWith("-")) -> a filename can start with "-"... :) The side effect with ++index is a bit ugly, you can do better. > Tools/DumpRenderTree/qt/main.cpp:87 > +bool validOptions(QStringList& args) > +{ > + int count = 0; > + > + for (int i = 1; i < args.size(); ++i) > + if (!args.at(i).startsWith('-')) > + count++; > + > + return count >= 1; > +} I don't follow here, at least the function name could be improved. > Tools/DumpRenderTree/qt/main.cpp:93 > + fflush(stderr); Unecesary. > Tools/DumpRenderTree/qt/main.cpp:192 > + int outputIndex = args.indexOf("-o"); > + if (outputIndex != -1) > + dumper.setRedirectOutputFileName(takeOptionValue(args, outputIndex)); > + > + int errorIndex = args.indexOf("-e"); > + if (errorIndex != -1) > + dumper.setRedirectErrorFileName(takeOptionValue(args, errorIndex)); > + Maybe use long option name? -o and -e are not very intuitive. You could use QLatin1String for the literals. (In reply to comment #9) > (From update of attachment 85960 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=85960&action=review > > > Tools/DumpRenderTree/qt/DumpRenderTreeQt.cpp:693 > > + if (!m_redirectOutputFileName.isEmpty()) { > > + if (!freopen(qPrintable(m_redirectOutputFileName), "w", stdout)) { > > + qDebug() << "Redirect STDOUT failed."; > > + return; > > + } > > + } > > + if (!m_redirectErrorFileName.isEmpty()) { > > + if (!freopen(qPrintable(m_redirectErrorFileName), "w", stderr)) { > > + qDebug() << "Redirect STDERR failed."; > > + return; > > + } > > + } > > + > > This looks like a strange place to do this. Maybe I can do this at main.cpp, the reason I put the code here is in main.cpp I have no place to put the "stop freopen" code. I don't think main.cpp can connect to DumpRenderTreeQt's quit()??!! My purpose is to keep the start and stop in the same class. > > > Tools/DumpRenderTree/qt/DumpRenderTreeQt.h:109 > > + void setRedirectOutputFileName(const QString s) { m_redirectOutputFileName = s; } > > + void setRedirectErrorFileName(const QString s) { m_redirectErrorFileName = s; } > > This should be ref to const. > > > Tools/DumpRenderTree/qt/main.cpp:73 > > + if (++index < arguments.count() && !arguments.at(index).startsWith("-")) > > !arguments.at(index).startsWith("-")) -> a filename can start with "-"... :) really? you mean "-abc.html" is a valid name? I copy this code from other place, actually in the main.cpp, this is a assumption that start with "-" should be a option like "-v". > > The side effect with ++index is a bit ugly, you can do better. > > > Tools/DumpRenderTree/qt/main.cpp:87 > > +bool validOptions(QStringList& args) > > +{ > > + int count = 0; > > + > > + for (int i = 1; i < args.size(); ++i) > > + if (!args.at(i).startsWith('-')) > > + count++; > > + > > + return count >= 1; > > +} > > I don't follow here, at least the function name could be improved. DumpRenderTreeQt need at least one test case name (not start with "-"), the code here just check if we do have. > > > Tools/DumpRenderTree/qt/main.cpp:93 > > + fflush(stderr); > > Unecesary. I found if I don't put fflush here, some time another stderr message (LEAK) just jump into my two print out, make it looks ugly. > > > Tools/DumpRenderTree/qt/main.cpp:192 > > + int outputIndex = args.indexOf("-o"); > > + if (outputIndex != -1) > > + dumper.setRedirectOutputFileName(takeOptionValue(args, outputIndex)); > > + > > + int errorIndex = args.indexOf("-e"); > > + if (errorIndex != -1) > > + dumper.setRedirectErrorFileName(takeOptionValue(args, errorIndex)); > > + > > Maybe use long option name? -o and -e are not very intuitive. > > You could use QLatin1String for the literals. Maybe you like : -stdout and -stderr Created attachment 86079 [details]
patch5
Change based on discussion with Laszlo and comments from Benjamin.
1. Make the redirect work for standalone mode and "-" mode.
2. Because 1, I move the code for starting redirect into main.cpp.
3. Changed DumpRenderTreeQt.h based on Benjanmin's comments.
4. Based Benjanmin's comments, "-bac.html" is valid test case name. The assumption to get test case name is wrong, I make a function to identify the option name, then all the others will be consider as valid option value.
5. Changed the redirection option name as "--stdout" and "--stderr"
Created attachment 86189 [details]
patch6
Do some improvement.
Created attachment 86191 [details]
patch6
sorry, upload wrong patch.
Comment on attachment 86191 [details] patch6 View in context: https://bugs.webkit.org/attachment.cgi?id=86191&action=review > Tools/DumpRenderTree/qt/DumpRenderTreeQt.h:108 > + void setRedirectOutputFileName(const QString& s) { m_redirectOutputFileName = s; } name would be more descriptive than s Also, please check whether we use Filename or FileName in Qt APIs. I believe we use the former. > Tools/DumpRenderTree/qt/main.cpp:79 > +// Get the option value and remove the option and value from arguments list I dont think this comment is needed. The code is very condense and easy to understand and the method name is also quite descriptive > Tools/DumpRenderTree/qt/main.cpp:201 > + fprintf(stderr, "Redirect STDOUT failed."); stdout redirection failed. - Redirection of stdout failed. > Tools/DumpRenderTree/qt/main.cpp:202 > + exit(0); Isn't that an error? > Tools/DumpRenderTree/qt/main.cpp:228 > + // Go to read STDIN Comments like this one are not that useful and might end up bitrotting. Created attachment 86314 [details] patch7 Change based on comment 14. I found in qwebpage.h used "FileName", so I didn't change my code (see the first comment is comment 14) Comment on attachment 86314 [details]
patch7
r=me.
Comment on attachment 86314 [details] patch7 Clearing flags on attachment: 86314 Committed r81588: <http://trac.webkit.org/changeset/81588> All reviewed patches have been landed. Closing bug. |