Bug 88542 – [Chromium-android] Don't use test_shell mode of DRT

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Xianzhu Wang]

Chromium-android is using test_shell mode of DRT which is about to be deprecated.

[Xianzhu Wang]

Created attachment 152577 [details]
Patch

[Adam Barth]

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.

[Xianzhu Wang]

(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 :(

[Xianzhu Wang]

Created attachment 152585 [details]
Patch

[Adam Barth]

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.

[Adam Barth]

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

[Xianzhu Wang]

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.

[Xianzhu Wang]

Created attachment 152606 [details]
Patch

[Dirk Pranke]

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.

[Xianzhu Wang]

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.

[Xianzhu Wang]

Created attachment 152620 [details]
Patch

[Dirk Pranke]

(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.

[WebKit Review Bot]

Comment on attachment 152620 [details]
Patch

Clearing flags on attachment: 152620

Committed r122780: <http://trac.webkit.org/changeset/122780>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.