Bug 88542

Created attachment 152577 [details] Patch

Comment on attachment 152577 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152577&action=review > Tools/DumpRenderTree/chromium/TestEventPrinter.cpp:40 > +#if OS(ANDROID) > +#include "third_party/WebKit/Source/WebCore/platform/text/Base64.h" > +#endif I thought you moved Base64.h to WTF so that we can include it via #include <wtf/text/Base64.h> ? > Tools/DumpRenderTree/chromium/TestEventPrinter.cpp:131 > + WebCore::base64Encode(reinterpret_cast<const char*>(imageData), imageSize, base64, true); We're not allowed to call WebCore symbols directly.

(In reply to comment #2) > (From update of attachment 152577 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=152577&action=review > > > Tools/DumpRenderTree/chromium/TestEventPrinter.cpp:40 > > +#if OS(ANDROID) > > +#include "third_party/WebKit/Source/WebCore/platform/text/Base64.h" > > +#endif > > I thought you moved Base64.h to WTF so that we can include it via #include <wtf/text/Base64.h> ? > > > Tools/DumpRenderTree/chromium/TestEventPrinter.cpp:131 > > + WebCore::base64Encode(reinterpret_cast<const char*>(imageData), imageSize, base64, true); > > We're not allowed to call WebCore symbols directly. Sorry, I mixed up upstream/downstream versions again :(

Created attachment 152585 [details] Patch

Comment on attachment 152585 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152585&action=review > Tools/DumpRenderTree/chromium/TestEventPrinter.cpp:40 > +#if OS(ANDROID) > +#include <wtf/text/Base64.h> > +#endif I would just include this header unconditionally. > Tools/DumpRenderTree/chromium/TestEventPrinter.cpp:132 > + imageData = reinterpret_cast<const unsigned char*>(base64.data()); reinterpret_cast -> static_cast > Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:497 > + webkit.WebKitDriver.start(self, pixel_tests, per_test_args) You can use the "super" Python idiom here instead. > Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:510 > + webkit.WebKitDriver._start(self, pixel_tests, per_test_args) here too > Tools/Scripts/webkitpy/layout_tests/port/server_process.py:-56 > - This change looks spurious. > Tools/Scripts/webkitpy/layout_tests/port/server_process.py:68 > + self._universal_newlines = universal_newlines I couldn't figure out what this variable does. > Tools/Scripts/webkitpy/layout_tests/port/server_process.py:324 > + elif not self._host.platform.is_win(): > # Closing stdin/stdout/stderr hangs sometimes on OS X, I know you didn't add this, but yuck! > Tools/Scripts/webkitpy/layout_tests/port/webkit.py:-424 > - This change looks spurious.

LGTM with the comments above. You might want to have dpranke take a look before landing.

Comment on attachment 152585 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152585&action=review >> Tools/DumpRenderTree/chromium/TestEventPrinter.cpp:40 >> +#endif > > I would just include this header unconditionally. Done. >> Tools/DumpRenderTree/chromium/TestEventPrinter.cpp:132 >> + imageData = reinterpret_cast<const unsigned char*>(base64.data()); > > reinterpret_cast -> static_cast base64.data() returns 'const char*' so reinterpret_cast is required. >> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:497 >> + webkit.WebKitDriver.start(self, pixel_tests, per_test_args) > > You can use the "super" Python idiom here instead. Done. >> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:510 >> + webkit.WebKitDriver._start(self, pixel_tests, per_test_args) > > here too Done. >> Tools/Scripts/webkitpy/layout_tests/port/server_process.py:-56 >> - > > This change looks spurious. Done. >> Tools/Scripts/webkitpy/layout_tests/port/server_process.py:68 >> + self._universal_newlines = universal_newlines > > I couldn't figure out what this variable does. It will be passed to subprocess.Popen(). Added comments. >> Tools/Scripts/webkitpy/layout_tests/port/server_process.py:324 >> # Closing stdin/stdout/stderr hangs sometimes on OS X, > > I know you didn't add this, but yuck! Dirk, do you know how these code came? (Anyway, I removed '(see restarts(), above)') >> Tools/Scripts/webkitpy/layout_tests/port/webkit.py:-424 >> - > > This change looks spurious. Done.

Created attachment 152606 [details] Patch

Comment on attachment 152606 [details] Patch The change basically looks pretty good; I have a couple of comments, below. I'm not that up on the things you had to do specifically for android (and how talking to DRT is different on Android from the old ChromiumDriver). So I took a cursory look at all of the redirecting you're doing, but I didn't sweat the details. I wasn't sure if Adam has already done that or knows that code better than me, but if you want me to do a detailed review, let me know. Once this lands and the patch gets cleaner it would be good to see how much more of this we can move into driver.py and server_process.py to reduce the diff, but I don't want to take on too much at once. View in context: https://bugs.webkit.org/attachment.cgi?id=152606&action=review > Tools/ChangeLog:8 > + Test shell mode is about to be deperecated. nit: it's already deprecated; it's about to be removed :). > Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:156 > + super(ChromiumAndroidPort, self).__init__(host, port_name, **kwargs) since all you're doing is calling super(), you can just delete this method. > Tools/Scripts/webkitpy/layout_tests/port/server_process.py:-56 > - did you mean to delete this line? > Tools/Scripts/webkitpy/layout_tests/port/server_process.py:326 > # Closing stdin/stdout/stderr hangs sometimes on OS X, I'm not sure if the OS X comment is still valid (I think this was true in python 2.4 on tiger or something), but we can test that separately. > Tools/Scripts/webkitpy/layout_tests/port/webkit.py:555 > + test_begin_time = time.time() maybe you should just move start_time after self.start()? We don't need/want two different variables here (and your change means start_time is unused, I think). > Tools/Scripts/webkitpy/layout_tests/port/webkit_unittest.py:255 > + return first_line + "\n" + self.read_stdout(deadline, remaining_size) Are you sure this doesn't break things? > Tools/Scripts/webkitpy/layout_tests/port/webkit_unittest.py:297 > + 'Content-Length: 9', See comment in webkit_unittest.py:255.

Comment on attachment 152606 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152606&action=review >> Tools/ChangeLog:8 >> + Test shell mode is about to be deperecated. > > nit: it's already deprecated; it's about to be removed :). Done. >> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:156 >> + super(ChromiumAndroidPort, self).__init__(host, port_name, **kwargs) > > since all you're doing is calling super(), you can just delete this method. No. There is still other code below. >> Tools/Scripts/webkitpy/layout_tests/port/server_process.py:-56 >> - > > did you mean to delete this line? No. Restored. >> Tools/Scripts/webkitpy/layout_tests/port/webkit.py:555 >> + test_begin_time = time.time() > > maybe you should just move start_time after self.start()? We don't need/want two different variables here (and your change means start_time is unused, I think). start_time is still used when calling self._port._get_crash_log(..., newer_than=start_time) below. If use test_begin_time there, we may lose the crash during startup. >> Tools/Scripts/webkitpy/layout_tests/port/webkit_unittest.py:255 >> + return first_line + "\n" + self.read_stdout(deadline, remaining_size) > > Are you sure this doesn't break things? See comment below. >> Tools/Scripts/webkitpy/layout_tests/port/webkit_unittest.py:297 >> + 'Content-Length: 9', > > See comment in webkit_unittest.py:255. The original 'Content-Length: 8' worked because the implementation of MockServerProcess.read_stdout() was incomplete (which just returned the whole line despite of size). Now read_stdout() has been modified to simulate what a normal python input stream does, so Content-Length should include the '\n' before #EOF if any. This is what webkit.py and DumpRenderTree have been doing.

Created attachment 152620 [details] Patch

(In reply to comment #10) > >> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:156 > >> + super(ChromiumAndroidPort, self).__init__(host, port_name, **kwargs) > > > > since all you're doing is calling super(), you can just delete this method. > > No. There is still other code below. > Oh, you're right. Sorry! I'm not sure how I missed those lines. > >> Tools/Scripts/webkitpy/layout_tests/port/webkit.py:555 > >> + test_begin_time = time.time() > > > > maybe you should just move start_time after self.start()? We don't need/want two different variables here (and your change means start_time is unused, I think). > > start_time is still used when calling self._port._get_crash_log(..., newer_than=start_time) below. If use test_begin_time there, we may lose the crash during startup. > Ah, okay. > >> Tools/Scripts/webkitpy/layout_tests/port/webkit_unittest.py:297 > >> + 'Content-Length: 9', > > > > See comment in webkit_unittest.py:255. > > The original 'Content-Length: 8' worked because the implementation of MockServerProcess.read_stdout() was incomplete (which just returned the whole line despite of size). Now read_stdout() has been modified to simulate what a normal python input stream does, so Content-Length should include the '\n' before #EOF if any. This is what webkit.py and DumpRenderTree have been doing. Great.

Comment on attachment 152620 [details] Patch Clearing flags on attachment: 152620 Committed r122780: <http://trac.webkit.org/changeset/122780>

All reviewed patches have been landed. Closing bug.