Bug 112751 - [chromium] move WebThemeEngine implementations to TestRunner library
Summary: [chromium] move WebThemeEngine implementations to TestRunner library
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: jochen
URL:
Keywords:
Depends on: 112788
Blocks:
  Show dependency treegraph
 
Reported: 2013-03-19 15:49 PDT by jochen
Modified: 2013-03-20 23:19 PDT (History)
8 users (show)

See Also:


Attachments
Patch (150.61 KB, patch)
2013-03-19 15:50 PDT, jochen
no flags Details | Formatted Diff | Diff
Patch (150.91 KB, patch)
2013-03-19 16:05 PDT, jochen
no flags Details | Formatted Diff | Diff
Patch for landing (150.94 KB, patch)
2013-03-19 17:41 PDT, jochen
no flags Details | Formatted Diff | Diff
Patch for landing (150.92 KB, patch)
2013-03-19 22:33 PDT, jochen
no flags Details | Formatted Diff | Diff
Patch for landing (150.96 KB, patch)
2013-03-20 22:13 PDT, jochen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description jochen 2013-03-19 15:49:58 PDT
[chromium] move WebThemeEngine implementations to TestRunner library
Comment 1 jochen 2013-03-19 15:50:34 PDT
Created attachment 193940 [details]
Patch
Comment 2 jochen 2013-03-19 15:52:31 PDT
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 :-/
Comment 3 WebKit Review Bot 2013-03-19 15:52:32 PDT
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 4 Adam Barth 2013-03-19 15:54:00 PDT
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....
Comment 5 Adam Barth 2013-03-19 15:54:55 PDT
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?
Comment 6 jochen 2013-03-19 15:56:13 PDT
(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
Comment 7 jochen 2013-03-19 16:05:36 PDT
Created attachment 193946 [details]
Patch
Comment 8 jochen 2013-03-19 16:06:36 PDT
(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
Comment 9 Adam Barth 2013-03-19 17:22:11 PDT
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.
Comment 10 jochen 2013-03-19 17:41:38 PDT
Created attachment 193955 [details]
Patch for landing
Comment 11 jochen 2013-03-19 17:42:06 PDT
(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...
Comment 12 WebKit Review Bot 2013-03-19 19:01:14 PDT
Comment on attachment 193955 [details]
Patch for landing

Clearing flags on attachment: 193955

Committed r146290: <http://trac.webkit.org/changeset/146290>
Comment 13 WebKit Review Bot 2013-03-19 19:01:18 PDT
All reviewed patches have been landed.  Closing bug.
Comment 14 Kenneth Russell 2013-03-19 19:13:02 PDT
This broke the Chromium Mac build:
http://build.webkit.org/builders/Chromium%20Mac%20Release/builds/58149

Rolling it out.
Comment 15 Kenneth Russell 2013-03-19 19:14:49 PDT
Reverted r146290 for reason:

Broke Chromium Mac build.

Committed r146293: <http://trac.webkit.org/changeset/146293>
Comment 16 Kenneth Russell 2013-03-19 20:02:20 PDT
FYI, this broke the Windows build too:
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Win%20Builder/builds/37214
Comment 17 jochen 2013-03-19 22:33:18 PDT
Created attachment 193987 [details]
Patch for landing
Comment 18 WebKit Review Bot 2013-03-19 23:01:50 PDT
Comment on attachment 193987 [details]
Patch for landing

Clearing flags on attachment: 193987

Committed r146304: <http://trac.webkit.org/changeset/146304>
Comment 19 WebKit Review Bot 2013-03-19 23:01:56 PDT
All reviewed patches have been landed.  Closing bug.
Comment 20 WebKit Review Bot 2013-03-20 04:37:12 PDT
Re-opened since this is blocked by bug 112788
Comment 21 jochen 2013-03-20 22:13:05 PDT
Created attachment 194183 [details]
Patch for landing
Comment 22 WebKit Review Bot 2013-03-20 23:19:15 PDT
Comment on attachment 194183 [details]
Patch for landing

Clearing flags on attachment: 194183

Committed r146439: <http://trac.webkit.org/changeset/146439>
Comment 23 WebKit Review Bot 2013-03-20 23:19:20 PDT
All reviewed patches have been landed.  Closing bug.