Bug 72667 – [Chromium] Fix broken DRT build for Aura Linux

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Fady Samuel]

[Chromium] Fix broken DRT build for Aura Linux

[Fady Samuel]

Created attachment 115695 [details]
Patch

[Tony Chang]

Comment on attachment 115695 [details]
Patch

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

> Tools/DumpRenderTree/chromium/TestShell.cpp:753
> +#if OS(LINUX) && !USE(GTK)

This looks like TestShellAndroid.cpp.  Here is my suggestion:

Make a TestShellLinux.cpp that has TestShell::waitTestFinished(), AlarmHandler(int), and checkLayoutTestSystemDependencies().  These methods appear to be the same everywhere.

Add a TestShellStub.cpp that has the other methods and use that on Android and Aura.

Alternately, you could just make a TestShellAura.cpp that is basically the same as TestShellAndroid.cpp with the belief that these will diverge over time.

[Fady Samuel]

I'm a bit confused. How does it decide to use TestShellGtk v.s. TestShellWin for example? They are both defined under variables in drt_files in Tools/DumpRenderTree/DumpRenderTree.gypi. I'm doing a bit more digging to better understand how this build works.

[Fady Samuel]

(In reply to comment #3)
> I'm a bit confused. How does it decide to use TestShellGtk v.s. TestShellWin for example? They are both defined under variables in drt_files in Tools/DumpRenderTree/DumpRenderTree.gypi. I'm doing a bit more digging to better understand how this build works.

Ohh, I just found out that a lot of magic happens in Source/WebKit/chromium/WebKit.gyp where files are excluded by regexp based on the platform! Cool! Very unintuitive, but very cool!

[Fady Samuel]

Created attachment 115833 [details]
Patch

[Fady Samuel]

Created attachment 115834 [details]
Patch

[Fady Samuel]

Created attachment 115837 [details]
Patch

[Tony Chang]

Comment on attachment 115837 [details]
Patch

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

One minor change, but otherwise, this looks great.

> Tools/DumpRenderTree/chromium/TestShellStub.cpp:34
> +#if OS(LINUX) && !USE(GTK)

I would remove the #if and instead, in WebKit.gyp, exclude the file except on Aura.  That is, in the DumpRenderTree target, add the condition: ['use_aura==0', { 'sources/': [['exclude', 'Stub\\.cpp$']] } ].

[Fady Samuel]

Created attachment 115871 [details]
Patch

[Fady Samuel]

(In reply to comment #8)
> (From update of attachment 115837 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=115837&action=review
> 
> One minor change, but otherwise, this looks great.
> 
> > Tools/DumpRenderTree/chromium/TestShellStub.cpp:34
> > +#if OS(LINUX) && !USE(GTK)
> 
> I would remove the #if and instead, in WebKit.gyp, exclude the file except on Aura.  That is, in the DumpRenderTree target, add the condition: ['use_aura==0', { 'sources/': [['exclude', 'Stub\\.cpp$']] } ].

use_aura doesn't seem to be exposed in the webkit repo. Also, should I be excluding all stub files or just TestShellStub?

[Tony Chang]

Comment on attachment 115837 [details]
Patch

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

>>> Tools/DumpRenderTree/chromium/TestShellStub.cpp:34
>>> +#if OS(LINUX) && !USE(GTK)
>> 
>> I would remove the #if and instead, in WebKit.gyp, exclude the file except on Aura.  That is, in the DumpRenderTree target, add the condition: ['use_aura==0', { 'sources/': [['exclude', 'Stub\\.cpp$']] } ].
> 
> use_aura doesn't seem to be exposed in the webkit repo. Also, should I be excluding all stub files or just TestShellStub?

use_aura should be pulled in via build/common.gypi.  We pull a bunch of chromium code when you run update-webkit --chromium.  Does your checkout not have Source/WebKit/chromium/build/common.gypi?

You're right, you should just exclude TestShellStub.cpp, not all Stub.cpp files.

[WebKit Review Bot]

Comment on attachment 115871 [details]
Patch

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

[Tony Chang]

Comment on attachment 115871 [details]
Patch

r- since we're missing the the exclude gyp rules.

[Fady Samuel]

Created attachment 116238 [details]
Patch

[Tony Chang]

Comment on attachment 116238 [details]
Patch

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

> Source/WebKit/chromium/WebKit.gyp:1097
> +                ['use_aura==0', {
> +                    'sources/': [
> +                        ['exclude', 'TestShellStub\\.cpp$'],

Is this needed?  It looks like we only add the file if it's aura or android.

> Tools/DumpRenderTree/chromium/EventSender.cpp:579
> +#if OS(LINUX) && USE(GTK)

What happened to the WTF_USE_GTK define in WebKit.gyp?

[Fady Samuel]

(In reply to comment #15)
> (From update of attachment 116238 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=116238&action=review
> 
> > Source/WebKit/chromium/WebKit.gyp:1097
> > +                ['use_aura==0', {
> > +                    'sources/': [
> > +                        ['exclude', 'TestShellStub\\.cpp$'],
> 
> Is this needed?  It looks like we only add the file if it's aura or android.

I'm not sure how else to exclude it? I believe you suggested this?
> 
> > Tools/DumpRenderTree/chromium/EventSender.cpp:579
> > +#if OS(LINUX) && USE(GTK)
> 
> What happened to the WTF_USE_GTK define in WebKit.gyp?

Oops, accidentally reverted, thanks for catching this!

[Fady Samuel]

Created attachment 116478 [details]
Patch

[WebKit Review Bot]

Comment on attachment 116478 [details]
Patch

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

New failing tests:
plugins/keyboard-events.html

[Dana Jansens]

Why drop all the FontConfig stuff when not using GTK? Does SKIA not use FontConfig still?

[Tony Chang]

Comment on attachment 116478 [details]
Patch

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

> Source/WebKit/chromium/WebKit.gyp:756
> +                    'defines': [
> +                        'WTF_USE_GTK'

I think this has to be WTF_USE_GTK=1.  My guess is that's why the test is failing.

> Tools/DumpRenderTree/DumpRenderTree.gypi:74
> +            ['(OS=="linux" and toolkit_uses_gtk!="1") or OS=="android"', {

Nit: I think you don't want the "s around the 1.  I think if you fix that, you don't need the exclude line.  TestShellStub.cpp is only included if it passes this condition.

[Fady Samuel]

(In reply to comment #19)
> Why drop all the FontConfig stuff when not using GTK? Does SKIA not use FontConfig still?

(In reply to comment #19)
> Why drop all the FontConfig stuff when not using GTK? Does SKIA not use FontConfig still?

I'm not sure what you're referring to Dana.

[Dana Jansens]

(In reply to comment #21)
> (In reply to comment #19)
> > Why drop all the FontConfig stuff when not using GTK? Does SKIA not use FontConfig still?
> 
> (In reply to comment #19)
> > Why drop all the FontConfig stuff when not using GTK? Does SKIA not use FontConfig still?
> 
> I'm not sure what you're referring to Dana.

You left setupFontconfig() in TestShellGTK rather than moving it to some place that it will still be used for Aura.

[Fady Samuel]

(In reply to comment #22)
> (In reply to comment #21)
> > (In reply to comment #19)
> > > Why drop all the FontConfig stuff when not using GTK? Does SKIA not use FontConfig still?
> > 
> > (In reply to comment #19)
> > > Why drop all the FontConfig stuff when not using GTK? Does SKIA not use FontConfig still?
> > 
> > I'm not sure what you're referring to Dana.
> 
> You left setupFontconfig() in TestShellGTK rather than moving it to some place that it will still be used for Aura.

Ohh sorry, good catch. It doesn't seem like Android build needs this but I bet Aura build does. I'll add it back behind a define for Aura.

[Fady Samuel]

(In reply to comment #23)
> (In reply to comment #22)
> > (In reply to comment #21)
> > > (In reply to comment #19)
> > > > Why drop all the FontConfig stuff when not using GTK? Does SKIA not use FontConfig still?
> > > 
> > > (In reply to comment #19)
> > > > Why drop all the FontConfig stuff when not using GTK? Does SKIA not use FontConfig still?
> > > 
> > > I'm not sure what you're referring to Dana.
> > 
> > You left setupFontconfig() in TestShellGTK rather than moving it to some place that it will still be used for Aura.
> 
> Ohh sorry, good catch. It doesn't seem like Android build needs this but I bet Aura build does. I'll add it back behind a define for Aura.

So we want to call setupFontconfig on GTK + AURA but not on other builds (like Android). I don't want to put too much code in TestShellStub, and I don't want to replicate too much code across files but it looks like setupFontconfig has to be in either in TestShell.cpp or replicated before both GTK and non-GTK (behind a define). 

I can't think of a pretty way to do this. Code replication is bad, but defines in code is not too nice either :( I'll have a patch up shortly and we can discuss.

[Fady Samuel]

Created attachment 116795 [details]
Patch

[Tony Chang]

Comment on attachment 116795 [details]
Patch

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

I would maybe just put all of TestShellStub and TestShellGtk into TestShellLinux (it's just 2 small functions now), but this is fine too.  I can do a follow up change with that.

> Source/WebKit/chromium/ChangeLog:8
> +        Don't use TestShellStub if not building for Aura or Android.

Nit: remove this sentence

> Tools/DumpRenderTree/chromium/TestShellLinux.cpp:201
> +#endif

Nit: Can you add a // !OS(ANDROID) here?

[Fady Samuel]

Comment on attachment 116795 [details]
Patch

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

>> Source/WebKit/chromium/ChangeLog:8
>> +        Don't use TestShellStub if not building for Aura or Android.
> 
> Nit: remove this sentence

Done.

>> Tools/DumpRenderTree/chromium/TestShellLinux.cpp:201

> 
> Nit: Can you add a // !OS(ANDROID) here?

Done.

[Fady Samuel]

Created attachment 116798 [details]
Patch

[Fady Samuel]

Comment on attachment 116798 [details]
Patch

Clearing flags on attachment: 116798

Committed r101273: <http://trac.webkit.org/changeset/101273>

[Fady Samuel]

All reviewed patches have been landed.  Closing bug.

[Tony Chang]

Reverted in http://trac.webkit.org/changeset/101276 due to test failures.

[Fady Samuel]

Created attachment 116858 [details]
Patch

[Fady Samuel]

Created attachment 116981 [details]
Patch for landing

[Fady Samuel]

Oops, uploaded the wrong patch. Reuploading.

[Fady Samuel]

Created attachment 116984 [details]
Patch

[Fady Samuel]

Created attachment 116985 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 116985 [details]
Patch for landing

Clearing flags on attachment: 116985

Committed r101464: <http://trac.webkit.org/changeset/101464>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.