Bug 72667

Summary: [Chromium] Fix broken DRT build for Aura Linux
Product: WebKit Reporter: Fady Samuel <fsamuel>
Component: New BugsAssignee: Fady Samuel <fsamuel>
Status: RESOLVED FIXED    
Severity: Normal CC: danakj, dglazkov, rjkroege, tkent, tony, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 73244    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch for landing
none
Patch
none
Patch for landing none

Description Fady Samuel 2011-11-17 15:01:36 PST
[Chromium] Fix broken DRT build for Aura Linux
Comment 1 Fady Samuel 2011-11-17 15:05:47 PST
Created attachment 115695 [details]
Patch
Comment 2 Tony Chang 2011-11-17 16:23:04 PST
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.
Comment 3 Fady Samuel 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 Fady Samuel 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 Fady Samuel 2011-11-18 10:46:09 PST
Created attachment 115833 [details]
Patch
Comment 6 Fady Samuel 2011-11-18 10:48:24 PST
Created attachment 115834 [details]
Patch
Comment 7 Fady Samuel 2011-11-18 10:55:37 PST
Created attachment 115837 [details]
Patch
Comment 8 Tony Chang 2011-11-18 11:13:23 PST
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$']] } ].
Comment 9 Fady Samuel 2011-11-18 14:05:56 PST
Created attachment 115871 [details]
Patch
Comment 10 Fady Samuel 2011-11-18 14:11:35 PST
(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?
Comment 11 Tony Chang 2011-11-18 14:22:01 PST
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.
Comment 12 WebKit Review Bot 2011-11-18 14:45:48 PST
Comment on attachment 115871 [details]
Patch

Attachment 115871 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10518212
Comment 13 Tony Chang 2011-11-18 15:49:06 PST
Comment on attachment 115871 [details]
Patch

r- since we're missing the the exclude gyp rules.
Comment 14 Fady Samuel 2011-11-22 10:03:44 PST
Created attachment 116238 [details]
Patch
Comment 15 Tony Chang 2011-11-22 23:37:40 PST
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?
Comment 16 Fady Samuel 2011-11-23 20:28:17 PST
(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!
Comment 17 Fady Samuel 2011-11-23 20:48:13 PST
Created attachment 116478 [details]
Patch
Comment 18 WebKit Review Bot 2011-11-23 22:31:35 PST
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
Comment 19 Dana Jansens 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 Tony Chang 2011-11-24 09:28:56 PST
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.
Comment 21 Fady Samuel 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 Dana Jansens 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 Fady Samuel 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 Fady Samuel 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 Fady Samuel 2011-11-28 11:54:02 PST
Created attachment 116795 [details]
Patch
Comment 26 Tony Chang 2011-11-28 12:00:03 PST
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?
Comment 27 Fady Samuel 2011-11-28 12:10:35 PST
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.
Comment 28 Fady Samuel 2011-11-28 12:11:40 PST
Created attachment 116798 [details]
Patch
Comment 29 Fady Samuel 2011-11-28 12:21:32 PST
Comment on attachment 116798 [details]
Patch

Clearing flags on attachment: 116798

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

Clearing flags on attachment: 116985

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