Bug 86922 – [Chromium-Android] Run DumpRenderTree as an apk (C++ and gyp part)

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Xianzhu Wang]

C++ and gyp part of patch for bug 86862.

[Xianzhu Wang]

Created attachment 142821 [details]
patch

[Kent Tamura]

Comment on attachment 142821 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=142821&action=review

Looks ok.

> Tools/DumpRenderTree/DumpRenderTree.gyp/DumpRenderTree.gyp:83
> +                'ImageDiff',
> +                'TestNetscapePlugIn',
> +                'copy_TestNetscapePlugIn',

Why did you move them to here?  It looks a unnecessary change.

> Tools/DumpRenderTree/chromium/TestShellAndroid.cpp:45
> +const char optionInFifo[] = "--in-fifo=";
> +const char optionOutFifo[] = "--out-fifo=";

should be optionInFIFO and optionOutFIFO.
http://www.webkit.org/coding/coding-style.html#names-basic

> Tools/DumpRenderTree/chromium/TestShellAndroid.cpp:87
> +    const char* inFifo = 0;
> +    const char* outFifo = 0;

should be inFIFO and outFIFO.

[Tony Chang]

Comment on attachment 142821 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=142821&action=review

> Tools/DumpRenderTree/DumpRenderTree.gyp/DumpRenderTree.gyp:196
> +                ['os_posix!=1 or OS=="mac"', {
> +                    'sources/': [
> +                        ['exclude', 'Posix\\.cpp$'],

It's confusing to have os_posix include mac but the Posix.cpp file exclude mac, but I'm not sure of a better name for Linux or Android.

> Tools/DumpRenderTree/chromium/TestShellX11.cpp:36
>  #if USE(GTK)

Is this file used by aura and gtk? Should we merge TestShellGtk.cpp into this file (it's only like 12 lines of code) and put it behind an #if USE(GTK)?

[Xianzhu Wang]

Created attachment 143359 [details]
patch for landing

[Xianzhu Wang]

Comment on attachment 142821 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=142821&action=review

>> Tools/DumpRenderTree/DumpRenderTree.gyp/DumpRenderTree.gyp:83
>> +                'copy_TestNetscapePlugIn',
> 
> Why did you move them to here?  It looks a unnecessary change.

These had been there before http://trac.webkit.org/changeset/95171. We put the dependencies into the 'else' part of OS=="android" because at some time gyp didn't support excluding from dependencies. Now it is supported so I changed this to 'include all then exclude' style which is clearer and normally used in gyp files.

>> Tools/DumpRenderTree/DumpRenderTree.gyp/DumpRenderTree.gyp:196
>> +                        ['exclude', 'Posix\\.cpp$'],
> 
> It's confusing to have os_posix include mac but the Posix.cpp file exclude mac, but I'm not sure of a better name for Linux or Android.

In chromium there are also some places using such style (on mac some posix source files are excluded).
Actually the code in Posix.cpp file works on Mac. It is excluded just because Mac has a better implementation.

>> Tools/DumpRenderTree/chromium/TestShellAndroid.cpp:45
>> +const char optionOutFifo[] = "--out-fifo=";
> 
> should be optionInFIFO and optionOutFIFO.
> http://www.webkit.org/coding/coding-style.html#names-basic

Done.

>> Tools/DumpRenderTree/chromium/TestShellAndroid.cpp:87
>> +    const char* outFifo = 0;
> 
> should be inFIFO and outFIFO.

Done.

>> Tools/DumpRenderTree/chromium/TestShellX11.cpp:36
>>  #if USE(GTK)
> 
> Is this file used by aura and gtk? Should we merge TestShellGtk.cpp into this file (it's only like 12 lines of code) and put it behind an #if USE(GTK)?

I think so. Done.

[Xianzhu Wang]

Comment on attachment 143359 [details]
patch for landing

Will soon upload a new patch containing latest downstream changes.

[Xianzhu Wang]

Created attachment 143375 [details]
patch for landing

[Kent Tamura]

Comment on attachment 143375 [details]
patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=143375&action=review

> Tools/DumpRenderTree/DumpRenderTree.gypi:57
>              'chromium/WebPreferences.h',
> +            'chromium/WebThemeControlDRTWin.cpp',
> +            'chromium/WebThemeControlDRTWin.h',
> +            'chromium/WebThemeEngineDRTMac.mm',
> +            'chromium/WebThemeEngineDRTMac.h',
> +            'chromium/WebThemeEngineDRTWin.cpp',
> +            'chromium/WebThemeEngineDRTWin.h',
>              'chromium/WebUserMediaClientMock.cpp',

This patch includes a change which was not in the reviewed patch.

[Xianzhu Wang]

Comment on attachment 143375 [details]
patch for landing

Sorry. I should request another review. Thanks.

[Kent Tamura]

(In reply to comment #8)
> (From update of attachment 143375 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=143375&action=review
> 
> > Tools/DumpRenderTree/DumpRenderTree.gypi:57
> >              'chromium/WebPreferences.h',
> > +            'chromium/WebThemeControlDRTWin.cpp',
> > +            'chromium/WebThemeControlDRTWin.h',
> > +            'chromium/WebThemeEngineDRTMac.mm',
> > +            'chromium/WebThemeEngineDRTMac.h',
> > +            'chromium/WebThemeEngineDRTWin.cpp',
> > +            'chromium/WebThemeEngineDRTWin.h',
> >              'chromium/WebUserMediaClientMock.cpp',
> 
> This patch includes a change which was not in the reviewed patch.

Please change this in another patch.  It is unrelated to "Run DumpRenderTree as an apk", and we shouldn't do it in this bug.

[Xianzhu Wang]

Created attachment 143385 [details]
patch v4

Will file another bug for the conditions in DumpRenderTree.gypi.

[WebKit Review Bot]

Comment on attachment 143385 [details]
patch v4

Attachment 143385 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12767058

[Xianzhu Wang]

Created attachment 143394 [details]
last patch hopefully

Will commit-queue+ after passing ews.

[Xianzhu Wang]

Created attachment 143418 [details]
Yet another last patch (resolved conflicts)

[WebKit Review Bot]

Comment on attachment 143418 [details]
Yet another last patch (resolved conflicts)

Clearing flags on attachment: 143418

Committed r118115: <http://trac.webkit.org/changeset/118115>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.