Summary: | [Chromium] Fix broken DRT build for Aura Linux | ||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Fady Samuel <fsamuel> | ||||||||||||||||||||||||||||
Component: | New Bugs | Assignee: | 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
Fady Samuel
2011-11-17 15:01:36 PST
Created attachment 115695 [details]
Patch
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. 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. (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! Created attachment 115833 [details]
Patch
Created attachment 115834 [details]
Patch
Created attachment 115837 [details]
Patch
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$']] } ]. Created attachment 115871 [details]
Patch
(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 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 on attachment 115871 [details] Patch Attachment 115871 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10518212 Comment on attachment 115871 [details]
Patch
r- since we're missing the the exclude gyp rules.
Created attachment 116238 [details]
Patch
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? (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! Created attachment 116478 [details]
Patch
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 Why drop all the FontConfig stuff when not using GTK? Does SKIA not use FontConfig still? 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. (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. (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. (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. (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. Created attachment 116795 [details]
Patch
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 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. Created attachment 116798 [details]
Patch
Comment on attachment 116798 [details] Patch Clearing flags on attachment: 116798 Committed r101273: <http://trac.webkit.org/changeset/101273> All reviewed patches have been landed. Closing bug. Reverted in http://trac.webkit.org/changeset/101276 due to test failures. Created attachment 116858 [details]
Patch
Created attachment 116981 [details]
Patch for landing
Oops, uploaded the wrong patch. Reuploading. Created attachment 116984 [details]
Patch
Created attachment 116985 [details]
Patch for landing
Comment on attachment 116985 [details] Patch for landing Clearing flags on attachment: 116985 Committed r101464: <http://trac.webkit.org/changeset/101464> All reviewed patches have been landed. Closing bug. |