Bug 72667 - [Chromium] Fix broken DRT build for Aura Linux
: [Chromium] Fix broken DRT build for Aura Linux
Status: RESOLVED FIXED
: WebKit
New Bugs
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
: 73244
:
  Show dependency treegraph
 
Reported: 2011-11-17 15:01 PST by
Modified: 2011-11-30 01:38 PST (History)


Attachments
Patch (4.57 KB, patch)
2011-11-17 15:05 PST, Fady Samuel
no flags Review Patch | Details | Formatted Diff | Diff
Patch (12.27 KB, patch)
2011-11-18 10:46 PST, Fady Samuel
no flags Review Patch | Details | Formatted Diff | Diff
Patch (11.65 KB, patch)
2011-11-18 10:48 PST, Fady Samuel
no flags Review Patch | Details | Formatted Diff | Diff
Patch (13.93 KB, patch)
2011-11-18 10:55 PST, Fady Samuel
no flags Review Patch | Details | Formatted Diff | Diff
Patch (14.26 KB, patch)
2011-11-18 14:05 PST, Fady Samuel
no flags Review Patch | Details | Formatted Diff | Diff
Patch (14.33 KB, patch)
2011-11-22 10:03 PST, Fady Samuel
no flags Review Patch | Details | Formatted Diff | Diff
Patch (14.71 KB, patch)
2011-11-23 20:48 PST, Fady Samuel
no flags Review Patch | Details | Formatted Diff | Diff
Patch (26.95 KB, patch)
2011-11-28 11:54 PST, Fady Samuel
no flags Review Patch | Details | Formatted Diff | Diff
Patch (26.90 KB, patch)
2011-11-28 12:11 PST, Fady Samuel
no flags Review Patch | Details | Formatted Diff | Diff
Patch (26.95 KB, patch)
2011-11-28 17:43 PST, Fady Samuel
no flags Review Patch | Details | Formatted Diff | Diff
Patch for landing (2.03 KB, patch)
2011-11-29 09:13 PST, Fady Samuel
no flags Review Patch | Details | Formatted Diff | Diff
Patch (26.96 KB, patch)
2011-11-29 09:21 PST, Fady Samuel
no flags Review Patch | Details | Formatted Diff | Diff
Patch for landing (26.96 KB, patch)
2011-11-29 09:22 PST, Fady Samuel
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-11-17 15:01:36 PST
[Chromium] Fix broken DRT build for Aura Linux
------- Comment #1 From 2011-11-17 15:05:47 PST -------
Created an attachment (id=115695) [details]
Patch
------- Comment #2 From 2011-11-17 16:23:04 PST -------
(From update of attachment 115695 [details])
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.
------- Comment #3 From 2011-11-18 07:30:21 PST -------
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.
------- Comment #4 From 2011-11-18 08:06:30 PST -------
(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!
------- Comment #5 From 2011-11-18 10:46:09 PST -------
Created an attachment (id=115833) [details]
Patch
------- Comment #6 From 2011-11-18 10:48:24 PST -------
Created an attachment (id=115834) [details]
Patch
------- Comment #7 From 2011-11-18 10:55:37 PST -------
Created an attachment (id=115837) [details]
Patch
------- Comment #8 From 2011-11-18 11:13:23 PST -------
(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$']] } ].
------- Comment #9 From 2011-11-18 14:05:56 PST -------
Created an attachment (id=115871) [details]
Patch
------- Comment #10 From 2011-11-18 14:11:35 PST -------
(In reply to comment #8)
> (From update of attachment 115837 [details] [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?
------- Comment #11 From 2011-11-18 14:22:01 PST -------
(From update of attachment 115837 [details])
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.
------- Comment #12 From 2011-11-18 14:45:48 PST -------
(From update of attachment 115871 [details])
Attachment 115871 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10518212
------- Comment #13 From 2011-11-18 15:49:06 PST -------
(From update of attachment 115871 [details])
r- since we're missing the the exclude gyp rules.
------- Comment #14 From 2011-11-22 10:03:44 PST -------
Created an attachment (id=116238) [details]
Patch
------- Comment #15 From 2011-11-22 23:37:40 PST -------
(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.

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

What happened to the WTF_USE_GTK define in WebKit.gyp?
------- Comment #16 From 2011-11-23 20:28:17 PST -------
(In reply to comment #15)
> (From update of attachment 116238 [details] [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!
------- Comment #17 From 2011-11-23 20:48:13 PST -------
Created an attachment (id=116478) [details]
Patch
------- Comment #18 From 2011-11-23 22:31:35 PST -------
(From update of attachment 116478 [details])
Attachment 116478 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10640084

New failing tests:
plugins/keyboard-events.html
------- Comment #19 From 2011-11-24 08:58:08 PST -------
Why drop all the FontConfig stuff when not using GTK? Does SKIA not use FontConfig still?
------- Comment #20 From 2011-11-24 09:28:56 PST -------
(From update of attachment 116478 [details])
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.
------- Comment #21 From 2011-11-28 08:41:25 PST -------
(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.
------- Comment #22 From 2011-11-28 08:50:00 PST -------
(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.
------- Comment #23 From 2011-11-28 08:54:08 PST -------
(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.
------- Comment #24 From 2011-11-28 10:57:01 PST -------
(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.
------- Comment #25 From 2011-11-28 11:54:02 PST -------
Created an attachment (id=116795) [details]
Patch
------- Comment #26 From 2011-11-28 12:00:03 PST -------
(From update of attachment 116795 [details])
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?
------- Comment #27 From 2011-11-28 12:10:35 PST -------
(From update of attachment 116795 [details])
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.
------- Comment #28 From 2011-11-28 12:11:40 PST -------
Created an attachment (id=116798) [details]
Patch
------- Comment #29 From 2011-11-28 12:21:32 PST -------
(From update of attachment 116798 [details])
Clearing flags on attachment: 116798

Committed r101273: <http://trac.webkit.org/changeset/101273>
------- Comment #30 From 2011-11-28 12:21:40 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #31 From 2011-11-28 13:40:03 PST -------
Reverted in http://trac.webkit.org/changeset/101276 due to test failures.
------- Comment #32 From 2011-11-28 17:43:41 PST -------
Created an attachment (id=116858) [details]
Patch
------- Comment #33 From 2011-11-29 09:13:33 PST -------
Created an attachment (id=116981) [details]
Patch for landing
------- Comment #34 From 2011-11-29 09:14:48 PST -------
Oops, uploaded the wrong patch. Reuploading.
------- Comment #35 From 2011-11-29 09:21:36 PST -------
Created an attachment (id=116984) [details]
Patch
------- Comment #36 From 2011-11-29 09:22:52 PST -------
Created an attachment (id=116985) [details]
Patch for landing
------- Comment #37 From 2011-11-30 01:38:07 PST -------
(From update of attachment 116985 [details])
Clearing flags on attachment: 116985

Committed r101464: <http://trac.webkit.org/changeset/101464>
------- Comment #38 From 2011-11-30 01:38:14 PST -------
All reviewed patches have been landed.  Closing bug.