Bug 92010 – [Chromium] Move layoutTestMode to WebCore

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Mark Pilgrim (Google)]

[Chromium] Move layoutTestMode to WebCore

[Mark Pilgrim (Google)]

Created attachment 153818 [details]
Patch

[Mark Pilgrim (Google)]

Does not currently compile.

[Mark Pilgrim (Google)]

Correction: compiles, does not link.


out/Release/../../Source/WebKit/chromium/third_party/gold/gold64: out/Release/obj.target/Source/WebKit/chromium/libwebkit.a(out/Release/obj.target/Source/WebKit/chromium/../../../webkit/Source/WebKit/chromium/src/WebKit.o): in function WebKit::setLayoutTestMode(bool):WebKit.cpp(.text._ZN6WebKit17setLayoutTestModeEb+0x5): error: undefined reference to 'WebCore::LayoutTestMode::setLayoutTestMode(bool)' 
out/Release/../../Source/WebKit/chromium/third_party/gold/gold64: out/Release/obj.target/Source/WebCore/WebCore.gyp/libwebcore_rendering.a(out/Release/obj.target/Source/WebCore/WebCore.gyp/../../../webcore_rendering/Source/WebCore/rendering/RenderThemeChromiumSkia.o): in function WebCore::RenderThemeChromiumSkia::caretBlinkInterval() const:RenderThemeChromiumSkia.cpp(.text._ZNK7WebCore23RenderThemeChromiumSkia18caretBlinkIntervalEv+0x5): error: undefined reference to 'WebCore::LayoutTestMode::layoutTestMode()'
collect2: ld returned 1 exit status
make: *** [out/Release/TestWebKitAPI] Error 1
zsh: exit 2     Tools/Scripts/build-webkit --chromium


And other such niceness.

[Ryosuke Niwa]

I don't think we should be moving a class about layout test into WebCore. That to me is a layer violation.

[Adam Barth]

Comment on attachment 153818 [details]
Patch

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

> Source/WebCore/platform/LayoutTestMode.h:39
> +class LayoutTestMode {
> +public:
> +    static bool layoutTestMode();
> +    static void setLayoutTestMode(bool);

We don't really need this class wrapper.  We can just have layoutTestMode() and setLayoutTestMode() exist in the WebCore namespace.

> Source/WebCore/platform/LayoutTestMode.h:44
> +    static bool isLayoutTestModeEnabled;

In that approach, this can just be a static in the LayoutTestMode.cpp file.

We might take this opportunity to rename these functions to isRunningLayoutTest() or something else more WebKity.

[Adam Barth]

Comment on attachment 153818 [details]
Patch

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

> Source/WebCore/WebCore.gypi:290
> +            'platform/LayoutTestMode.h',

Looks like you didn't add platform/LayoutTestMode.cpp to WebCore.gypi, which is likely what's causing your linking trouble.

[WebKit Review Bot]

Comment on attachment 153818 [details]
Patch

Attachment 153818 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13329107

[Darin Fisher (:fishd, Google)]

(In reply to comment #4)
> I don't think we should be moving a class about layout test into WebCore. That to me is a layer violation.

This is not a new dependency.  You can see that in the Chromium port, WebCore already knows about this "layout test" boolean.  Mark is just moving it from one spot in WebCore to another.

[Mark Pilgrim (Google)]

Created attachment 153826 [details]
Patch

[Mark Pilgrim (Google)]

(In reply to comment #5)
> (From update of attachment 153818 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=153818&action=review
> 
> > Source/WebCore/platform/LayoutTestMode.h:39
> > +class LayoutTestMode {
> > +public:
> > +    static bool layoutTestMode();
> > +    static void setLayoutTestMode(bool);
> 
> We don't really need this class wrapper.

Removed in latest patch.

> 
> > Source/WebCore/platform/LayoutTestMode.h:44
> > +    static bool isLayoutTestModeEnabled;
> 
> In that approach, this can just be a static in the LayoutTestMode.cpp file.
> 
> We might take this opportunity to rename these functions to isRunningLayoutTest() or something else more WebKity.

Renamed in latest patch.

[Mark Pilgrim (Google)]

(In reply to comment #6)
> (From update of attachment 153818 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=153818&action=review
> 
> > Source/WebCore/WebCore.gypi:290
> > +            'platform/LayoutTestMode.h',
> 
> Looks like you didn't add platform/LayoutTestMode.cpp to WebCore.gypi, which is likely what's causing your linking trouble.

Thanks. Added in latest patch.

[Adam Barth]

Comment on attachment 153826 [details]
Patch

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

> Source/WebCore/platform/LayoutTestMode.h:32
> +#ifndef LayoutTestMode_h
> +#define LayoutTestMode_h

I wonder if LayoutTestMode.h is still the right name for this header...  Perhaps LayoutTestSupport.h (just LayoutTest.h doesn't seem right)?  Sorry to be so nit-picky about the names.  If you want to land it with this name, that's fine too.

[Mark Pilgrim (Google)]

Created attachment 153939 [details]
Patch

[Mark Pilgrim (Google)]

Comment on attachment 153939 [details]
Patch

Renamed to LayoutTestSupport.

[WebKit Review Bot]

Comment on attachment 153939 [details]
Patch

Clearing flags on attachment: 153939

Committed r123424: <http://trac.webkit.org/changeset/123424>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.