Bug 92010

Created attachment 153818 [details] Patch

Does not currently compile.

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.

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

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.

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.

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

(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.

Created attachment 153826 [details] Patch

(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.

(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.

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.

Created attachment 153939 [details] Patch

Comment on attachment 153939 [details] Patch Renamed to LayoutTestSupport.

Comment on attachment 153939 [details] Patch Clearing flags on attachment: 153939 Committed r123424: <http://trac.webkit.org/changeset/123424>

All reviewed patches have been landed. Closing bug.