Bug 67590

Created attachment 106315 [details] Proposed patch.

Comment on attachment 106315 [details] Proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=106315&action=review > Tools/DumpRenderTree/chromium/DumpRenderTree.cpp:68 > +static const char optionPrintReady[] = "--print-ready"; 'print-ready' seems like an odd name, as there's no visible printing as far as the user of the tool is concerned. How about 'send-ready-command' or similar? > Tools/DumpRenderTree/chromium/LayoutTestController.cpp:63 > +#include <clocale> Why are these needed on Android? If this is a deficiency in bionic, we should have a bug to track this, like in http://code.google.com/p/android/issues/detail?id=19764 > Tools/DumpRenderTree/chromium/TestShellAndroid.cpp:58 > + signal(SIGALRM, AlarmHandler); The signal() man page recommends using sigaction() instead. > Tools/DumpRenderTree/chromium/TestShellAndroid.cpp:77 > +} Will these methods ever be implemented? I think you should add a comment/FIXME/DCHECK etc.

(In reply to comment #2) > (From update of attachment 106315 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=106315&action=review > > > Tools/DumpRenderTree/chromium/DumpRenderTree.cpp:68 > > +static const char optionPrintReady[] = "--print-ready"; > > 'print-ready' seems like an odd name, as there's no visible printing as far as the user of the tool is concerned. How about 'send-ready-command' or similar? > How about 'send-ready'? > > Tools/DumpRenderTree/chromium/LayoutTestController.cpp:63 > > +#include <clocale> > > Why are these needed on Android? If this is a deficiency in bionic, we should have a bug to track this, like in http://code.google.com/p/android/issues/detail?id=19764 > I don't think it's a bug in bionic, because LC_ALL should be defined in clocale and tolower should be in cctype. I guess we can build DRT on other platforms because other headers happen to include the defines, so explicitly including the files here should be fine. > > Tools/DumpRenderTree/chromium/TestShellAndroid.cpp:58 > > + signal(SIGALRM, AlarmHandler); > > The signal() man page recommends using sigaction() instead. > Ok. > > Tools/DumpRenderTree/chromium/TestShellAndroid.cpp:77 > > +} > > Will these methods ever be implemented? I think you should add a comment/FIXME/DCHECK etc. I do not expect they will be implemented. Anyway, I will add some comments.

Comment on attachment 106315 [details] Proposed patch. Attachment 106315 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9587794

Created attachment 106383 [details] Proposed patch 2.

Attachment 106383 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/chromium/ChangeLog', u'Sourc..." exit_code: 1 Tools/DumpRenderTree/chromium/TestShellAndroid.cpp:57: alarm_action is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Tools/DumpRenderTree/chromium/TestShellAndroid.cpp:62: Use 0 instead of NULL. [readability/null] [5] Tools/DumpRenderTree/chromium/TestShellGtk.cpp:177: alarm_action is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Tools/DumpRenderTree/chromium/TestShellGtk.cpp:182: Use 0 instead of NULL. [readability/null] [5] Total errors found: 4 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 106387 [details] Proposed patch 3.

Comment on attachment 106387 [details] Proposed patch 3. View in context: https://bugs.webkit.org/attachment.cgi?id=106387&action=review > Source/WebKit/chromium/WebKit.gyp:1130 > + # FIXME(zhenghao): Reserve for upstream in the near future. This wasn't in the previous patch and I don't understand the comment. Can you explain? > Tools/DumpRenderTree/chromium/TestShellAndroid.cpp:74 > +// TODO: Implement the following metheds if needed. WebKit uses FIXME, not TODO. And this is rather vague. If you don't know of a reason why they'd be used, I don't think you should use a FIXME - use 'not required on Android' or an ASSERT_NOT_REACHED(). This can always be changed if we later find a reason why they're needed. > Tools/DumpRenderTree/chromium/TestShellGtk.cpp:180 > + alarmAction.sa_flags = SA_RESETHAND; Mention this fix in the ChangeLog > Tools/DumpRenderTree/chromium/TestShellGtk.cpp:-188 > - signal(SIGALRM, SIG_DFL); Why don't we need to reset the previous handler?

(In reply to comment #8) > (From update of attachment 106387 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=106387&action=review > > > Source/WebKit/chromium/WebKit.gyp:1130 > > + # FIXME(zhenghao): Reserve for upstream in the near future. > > This wasn't in the previous patch and I don't understand the comment. Can you explain? See comment #4. It breaks other builds, so exclude the newly added file here in !os(android) part. And the os(android) part is reserved for patches in the near future. > > > Tools/DumpRenderTree/chromium/TestShellAndroid.cpp:74 > > +// TODO: Implement the following metheds if needed. > > WebKit uses FIXME, not TODO. And this is rather vague. If you don't know of a reason why they'd be used, I don't think you should use a FIXME - use 'not required on Android' or an ASSERT_NOT_REACHED(). This can always be changed if we later find a reason why they're needed. We cannot use ASSERT_NOT_REACHED() there, because we will reach them. All we need to do is to provide an null implementation for now. 'not required' is also not suitable, IMHO. I think no comment there is reasonable, like patch 1. > > > Tools/DumpRenderTree/chromium/TestShellGtk.cpp:180 > > + alarmAction.sa_flags = SA_RESETHAND; > > Mention this fix in the ChangeLog Ok. > > > Tools/DumpRenderTree/chromium/TestShellGtk.cpp:-188 > > - signal(SIGALRM, SIG_DFL); > > Why don't we need to reset the previous handler? alarmAction.sa_flags = SA_RESETHAND; This will reset the handler.

Created attachment 106403 [details] Proposed patch 4.

Comment on attachment 106403 [details] Proposed patch 4. View in context: https://bugs.webkit.org/attachment.cgi?id=106403&action=review > Source/WebKit/chromium/WebKit.gyp:1131 > + ['OS=="android"', { > + # FIXME(zhenghao): Reserve for upstream in the near future. > + },{ # OS!="android" This is weird. Just flip your condition (OS!="android") and remove the else clause. > Tools/ChangeLog:11 > + - Wait until receiving '#READY' from DRT, so that DRT won't miss any > + input commands before it starts. Can you explain your architecture to me? It sounds like DRT runs on Android, but it's not clear to me if new-run-webkit-tests runs on Android or the desktop. It seems unfortunate that DRT needs to acknowledge that it has started so I was trying to brainstorm ways to avoid this. > Tools/DumpRenderTree/chromium/DumpRenderTree.cpp:151 > + bool sendReady = false; // Effective only in testShellMode. Maybe print a warning to stderr if --send-ready is used without --test-shell? > Tools/DumpRenderTree/chromium/TestShellAndroid.cpp:41 > +static void AlarmHandler(int signatl) Nit: Remove signatl (WebKit style normally just omits unused params)? > Tools/DumpRenderTree/chromium/TestShellAndroid.cpp:81 > +{ > +} notImplemented()

Comment on attachment 106403 [details] Proposed patch 4. View in context: https://bugs.webkit.org/attachment.cgi?id=106403&action=review >> Tools/DumpRenderTree/chromium/TestShellAndroid.cpp:81 >> +} > > notImplemented() I thought that notImplemented() was for methods that aren't (yet) implemented because we don't expect them to be called? I think that here, that's not the case. These methods are expected to be called, but the implementation is intentionally empty. Is there a function or macro to denote that, or is a comment best?

(In reply to comment #12) > (From update of attachment 106403 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=106403&action=review > > >> Tools/DumpRenderTree/chromium/TestShellAndroid.cpp:81 > >> +} > > > > notImplemented() > > I thought that notImplemented() was for methods that aren't (yet) implemented because we don't expect them to be called? I think that here, that's not the case. These methods are expected to be called, but the implementation is intentionally empty. Is there a function or macro to denote that, or is a comment best? In that case, I agree that a comment would be better. Something along the lines of, "// Nothing to do here."

(In reply to comment #11) > (From update of attachment 106403 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=106403&action=review > > > Source/WebKit/chromium/WebKit.gyp:1131 > > + ['OS=="android"', { > > + # FIXME(zhenghao): Reserve for upstream in the near future. > > + },{ # OS!="android" > > This is weird. Just flip your condition (OS!="android") and remove the else clause. I will upstream some stuff in the first brace very soon. Is it acceptable to leave it there? If not, I can remove it. My intention here is to facilitate our regular merge effort. It will generate minimum diff when we merge upstream code now. > > > Tools/ChangeLog:11 > > + - Wait until receiving '#READY' from DRT, so that DRT won't miss any > > + input commands before it starts. > > Can you explain your architecture to me? It sounds like DRT runs on Android, but it's not clear to me if new-run-webkit-tests runs on Android or the desktop. Sure! I intend to elaborate on that when I upstream NRWT. Anyway, let me explain it here. NRWT on host machine (linux/mac) | Android shell connecting host and device (adb shell) | DRT on Android As Android shell is rather limited, we cannot run NRWT on it. 'adb shell' can forward stdin/stdout between host and device, but unfortunately it doesn't ensure integrity, like an unreliable 'pipe'. > > It seems unfortunate that DRT needs to acknowledge that it has started so I was trying to brainstorm ways to avoid this. If we send the first command to 'adb shell' and unfortunately DRT is not ready to receive stdin at that time, 'adb shell' will throw the command away. Then NRWT will wait for the DRT output, and DRT will wait for the NRWT input, which becomes a dead lock. That's why we need a way to make sure DRT is ready when NRWT sends the first command. And 'adb shell' is also not reliable to send the EOF to DRT, so we add the 'QUIT' command. > > > Tools/DumpRenderTree/chromium/DumpRenderTree.cpp:151 > > + bool sendReady = false; // Effective only in testShellMode. > > Maybe print a warning to stderr if --send-ready is used without --test-shell? Ok. > > > Tools/DumpRenderTree/chromium/TestShellAndroid.cpp:41 > > +static void AlarmHandler(int signatl) > > Nit: Remove signatl (WebKit style normally just omits unused params)? Ok. > > > Tools/DumpRenderTree/chromium/TestShellAndroid.cpp:81 > > +{ > > +} > > notImplemented() As Steve said, I intentionally leave them empty. If I put notImplemented() there, DRT will output annoying warning every time the functions are reached.

Created attachment 106550 [details] Proposed patch 5. > > > > > Tools/DumpRenderTree/chromium/TestShellGtk.cpp:-188 > > > - signal(SIGALRM, SIG_DFL); > > > > Why don't we need to reset the previous handler? > > alarmAction.sa_flags = SA_RESETHAND; > This will reset the handler. Sorry, I'm wrong at this point. SA_RESETHAND will only reset handler when the signal is received. Seems we only need to install the handler once for DRT, and no need to install&reset handler for every test. But I still keep it the old way here.

Comment on attachment 106550 [details] Proposed patch 5. View in context: https://bugs.webkit.org/attachment.cgi?id=106550&action=review This patch is OK, but I'm worried it's going to hurt us in the long run. Specifically, we want to move away from the --test-shell API and switch to the same API as the other DRTs. This patch is basically adding a third API that we have to support. That said, handling QUIT seems pretty harmless. I would perhaps remove the --send-ready command line option and just put that in platformInit() since I doubt the other chromium ports will ever use that flag. I think most of the difficulty in maintaining this is going to end up in new-run-webkit-tests. Maybe abarth or dpranke have an opinion about this. > Source/WebKit/chromium/WebKit.gyp:1131 > + ['OS=="android"', { > + # FIXME(zhenghao): Reserve for upstream in the near future. > + },{ # OS!="android" Nit: I would still remove this and empty block and just add it in the next patch. > Tools/DumpRenderTree/chromium/TestShellAndroid.cpp:84 > +void openStartupDialog() > +{ > + // Nothing to do here. > +} On second thought, I think a notImplemented() is appropriate here. You only get here if --testshell-startup-dialog is passed on the command line (normally for debugging). It seems like in that case you would want to print something to stdout.

(In reply to comment #16) > (From update of attachment 106550 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=106550&action=review > > This patch is OK, but I'm worried it's going to hurt us in the long run. Specifically, we want to move away from the --test-shell API and switch to the same API as the other DRTs. This patch is basically adding a third API that we have to support. > > That said, handling QUIT seems pretty harmless. I would perhaps remove the --send-ready command line option and just put that in platformInit() since I doubt the other chromium ports will ever use that flag. I have tried to put it there, but platformInit is too early in execution and flakiness still happens on Android. I admit it is only useful to Android, but I cannot come to a better way for now. When changing the --test-shell API, we can change it accordingly. > > I think most of the difficulty in maintaining this is going to end up in new-run-webkit-tests. Maybe abarth or dpranke have an opinion about this. > > > Source/WebKit/chromium/WebKit.gyp:1131 > > + ['OS=="android"', { > > + # FIXME(zhenghao): Reserve for upstream in the near future. > > + },{ # OS!="android" > > Nit: I would still remove this and empty block and just add it in the next patch. Ok. > > > Tools/DumpRenderTree/chromium/TestShellAndroid.cpp:84 > > +void openStartupDialog() > > +{ > > + // Nothing to do here. > > +} > > On second thought, I think a notImplemented() is appropriate here. You only get here if --testshell-startup-dialog is passed on the command line (normally for debugging). It seems like in that case you would want to print something to stdout. Ok.

Created attachment 106678 [details] Proposed patch 6.

(In reply to comment #17) > (In reply to comment #16) > > (From update of attachment 106550 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=106550&action=review > > > > This patch is OK, but I'm worried it's going to hurt us in the long run. Specifically, we want to move away from the --test-shell API and switch to the same API as the other DRTs. This patch is basically adding a third API that we have to support. > > > > That said, handling QUIT seems pretty harmless. I would perhaps remove the --send-ready command line option and just put that in platformInit() since I doubt the other chromium ports will ever use that flag. > > I have tried to put it there, but platformInit is too early in execution and flakiness still happens on Android. I admit it is only useful to Android, but I cannot come to a better way for now. When changing the --test-shell API, we can change it accordingly. Can we put this behind an #if OS(ANDROID) block? That seems like less complexity than a command line flag. With that change, this LGTM, but I think we want to hide as much of the differences in NRWT as possible. For example, rather than sending QUIT to drt, can we use adb to get the pid and send a kill command? Or rather than having drt print #READY, can we send test URLs until we get text back from DRT?

Created attachment 106822 [details] Proposed patch 7.

(In reply to comment #19) > (In reply to comment #17) > > (In reply to comment #16) > > > (From update of attachment 106550 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=106550&action=review > > > > > > This patch is OK, but I'm worried it's going to hurt us in the long run. Specifically, we want to move away from the --test-shell API and switch to the same API as the other DRTs. This patch is basically adding a third API that we have to support. > > > > > > That said, handling QUIT seems pretty harmless. I would perhaps remove the --send-ready command line option and just put that in platformInit() since I doubt the other chromium ports will ever use that flag. > > > > I have tried to put it there, but platformInit is too early in execution and flakiness still happens on Android. I admit it is only useful to Android, but I cannot come to a better way for now. When changing the --test-shell API, we can change it accordingly. > > Can we put this behind an #if OS(ANDROID) block? That seems like less complexity than a command line flag. Ok. Good idea. Maybe this is better. > > With that change, this LGTM, but I think we want to hide as much of the differences in NRWT as possible. For example, rather than sending QUIT to drt, can we use adb to get the pid and send a kill command? Or rather than having drt print #READY, can we send test URLs until we get text back from DRT? I will investigate the possibilities.

Comment on attachment 106822 [details] Proposed patch 7. Clearing flags on attachment: 106822 Committed r94862: <http://trac.webkit.org/changeset/94862>

All reviewed patches have been landed. Closing bug.