Bug 112751 – [chromium] move WebThemeEngine implementations to TestRunner library

• Component: New Bugs
• Status: RESOLVED FIXED

[chromium] move WebThemeEngine implementations to TestRunner library

Created attachment 193940 [details]
Patch

For some reason this doesn't compile on Windows:


57>E:\b\build\slave\win_layout\build\src\third_party\WebKit\Tools\DumpRenderTree\chromium\TestRunner\src\WebThemeControlDRTWin.cpp(280): error C3861: 'failedAssertion': identifier not found
57>E:\b\build\slave\win_layout\build\src\third_party\WebKit\Tools\DumpRenderTree\chromium\TestRunner\src\WebThemeControlDRTWin.cpp(313): error C3861: 'failedAssertion': identifier not found
57>E:\b\build\slave\win_layout\build\src\third_party\WebKit\Tools\DumpRenderTree\chromium\TestRunner\src\WebThemeControlDRTWin.cpp(318): error C3861: 'failedAssertion': identifier not found
57>E:\b\build\slave\win_layout\build\src\third_party\WebKit\Tools\DumpRenderTree\chromium\TestRunner\src\WebThemeControlDRTWin.cpp(484): error C3861: 'failedAssertion': identifier not found

These are WEBKIT_ASSERT_NOT_REACHED() calls which is defined in WebCommon.h included by TestCommon.h included by WebThemeControlDRTWin.cpp

So given that the macro expands, and it's also working in the other files that use it, I have no idea what's going on :-/

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Adam Barth]

Comment on attachment 193940 [details]
Patch

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

> Tools/DumpRenderTree/DumpRenderTree.gypi:93
> +            'chromium/TestRunner/src/WebThemeControlDRTWin.cpp',
> +            'chromium/TestRunner/src/WebThemeControlDRTWin.h',

The suffix DRT doesn't really make sense inside TestRunner....

[Adam Barth]

Comment on attachment 193940 [details]
Patch

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

> Tools/DumpRenderTree/chromium/TestRunner/src/WebThemeControlDRTWin.cpp:41
> +#include "skia/ext/skia_utils_win.h"

Do we need to add a dependency on skia to the TestRunner GYP declarations?

(In reply to comment #5)
> (From update of attachment 193940 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=193940&action=review
> 
> > Tools/DumpRenderTree/chromium/TestRunner/src/WebThemeControlDRTWin.cpp:41
> > +#include "skia/ext/skia_utils_win.h"
> 
> Do we need to add a dependency on skia to the TestRunner GYP declarations?

It's already there for the platform canvas in WebTestProxy

Created attachment 193946 [details]
Patch

(In reply to comment #4)
> (From update of attachment 193940 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=193940&action=review
> 
> > Tools/DumpRenderTree/DumpRenderTree.gypi:93
> > +            'chromium/TestRunner/src/WebThemeControlDRTWin.cpp',
> > +            'chromium/TestRunner/src/WebThemeControlDRTWin.h',
> 
> The suffix DRT doesn't really make sense inside TestRunner....

Renamed WebTheme(.*)DRT(.*) to WebTestTheme$1$2

[Adam Barth]

Comment on attachment 193946 [details]
Patch

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

rs=me

> Tools/DumpRenderTree/chromium/TestRunner/src/TestInterfaces.h:90
> +    std::auto_ptr<WebKit::WebThemeEngine> m_themeEngine;

Isn't this the bad case for auto_ptr?  Rather than forward declaring WebThemeEngine, we should probably #include it.

Created attachment 193955 [details]
Patch for landing

(In reply to comment #9)
> (From update of attachment 193946 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=193946&action=review
> 
> rs=me
> 
> > Tools/DumpRenderTree/chromium/TestRunner/src/TestInterfaces.h:90
> > +    std::auto_ptr<WebKit::WebThemeEngine> m_themeEngine;
> 
> Isn't this the bad case for auto_ptr?  Rather than forward declaring WebThemeEngine, we should probably #include it.

done

I also figured out the reason for the compile failure, it was missing a using namespace WebKit...

[WebKit Review Bot]

Comment on attachment 193955 [details]
Patch for landing

Clearing flags on attachment: 193955

Committed r146290: <http://trac.webkit.org/changeset/146290>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Kenneth Russell]

This broke the Chromium Mac build:
http://build.webkit.org/builders/Chromium%20Mac%20Release/builds/58149

Rolling it out.

[Kenneth Russell]

Reverted r146290 for reason:

Broke Chromium Mac build.

Committed r146293: <http://trac.webkit.org/changeset/146293>

[Kenneth Russell]

FYI, this broke the Windows build too:
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Win%20Builder/builds/37214

Created attachment 193987 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 193987 [details]
Patch for landing

Clearing flags on attachment: 193987

Committed r146304: <http://trac.webkit.org/changeset/146304>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by bug 112788

Created attachment 194183 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 194183 [details]
Patch for landing

Clearing flags on attachment: 194183

Committed r146439: <http://trac.webkit.org/changeset/146439>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.