[webkit-reviews] review denied: [Bug 39456] [chromium] WebFrame::contentAsText should not return the text from hidden frames : [Attachment 56715] Style fixing

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied Jay Civelli
<jcivelli at chromium.org>'s request for review:
Bug 39456: [chromium] WebFrame::contentAsText should not return the text from
hidden frames
https://bugs.webkit.org/show_bug.cgi?id=39456

Attachment 56715: Style fixing
https://bugs.webkit.org/attachment.cgi?id=56715&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
WebKitTools/DumpRenderTree/chromium/DumpRenderTree.cpp:61
 +	webkit_support::SetUpTestEnvironment(false);
It might have been nicer to define a method name like
SetUpTestingEnvironmentForUnitTests instead of using
a boolean parameter.  That way, it is clear what is
being requested at the callsite.

WebKit/chromium/public/WebCString.h:87
 +	bool operator<(const WebCString& other) const;
This would require a WEBKIT_API prefix in order to link WebKit
as a DLL.  Please add that prefix so we don't have to add it
later once the DLL build is functional.
Also, I think it would be better to write a compare function
and then add global operator<, that way type coercion can work
better.  Take a look at how operator== is supported in the
WebKit API.

  WEBKIT_API int compare(const WebCString&) const;


WebKit/chromium/public/WebURL.h:80
 +	}
ditto.	also, please avoid adding extra new lines.

WebKit/chromium/tests/RunAllTests.cpp:42
 +	// has initialized AtExitManager and ICU.
nit: please avoid mentioning AtExitManager since that is a
chromium detail.  if AtExitManager is ever renamed, then no
one will know to update this comment.

WebKit/chromium/tests/WebFrameTest.cpp:31
 +  // Basic tests that verify our KURL's interface behaves the same as the
this comment is wrong.

WebKit/chromium/tests/WebFrameTest.cpp:34
 +  #include "config.h"
no need to include config.h unless you are using WebCore headers.

WebKit/chromium/tests/WebFrameTest.cpp:38
 +  #include "webkit/support/webkit_support.h"
please use angle brackets for webkit_support.h since it is not part of the
WebKit project.  #include <webkit/support/webkit_support.h>

WebKit/chromium/tests/WebFrameTest.cpp:48
 +  using namespace WebCore;
are you really using WebCore?

WebKit/chromium/tests/WebFrameTest.cpp:59
 +	    TestWebKitClient* client =
static_cast<TestWebKitClient*>(webkit_support::GetWebKitClient());
It seems like this should be a webkit_support method instead of assuming
the implementation of webkit_support::GetWebKitClient is a TestWebKitClient.
How about a webkit_support::SetURLLoaderFactory(client, factory) method?
WebURLLoaderFactory should probably be defined in its own header file
or as part of webkit_support.h.  the idea being to avoid having to
dip into the implementation code for the WebKitClient used by the
webkit support layer.

WebKit/chromium/tests/WebFrameTest.cpp:61
 +	}
nit: insert new line here

WebKit/chromium/tests/WebFrameTest.cpp:97
 +	for (int i = 0; i < arraysize(files); ++i) {
arraysize comes from base/basictypes.h.  it'd be nice to avoid
a direct dependency on basictypes.h.

WebKit/chromium/tests/WebURLLoaderMock.cpp:29
 +  #include "config.h"
no need for config.h

WebKit/chromium/tests/WebURLLoaderMockFactory.cpp:46
 +	    responseInfo.m_filePath =
can ResponseInfo::m_filePath be a WebString instead?

WebKit/chromium/tests/WebURLLoaderMockFactory.cpp:49
 +	    ASSERT(WebCore::fileExists(responseInfo.m_filePath));
instead of using WebCore::fileExists, can you use WebFileSystem::fileExists
instead?
you can get WebFileSystem from WebKitClient.  i think it would be good
to avoid using WebCore here if we can.
WebKit/chromium/tests/WebURLLoaderMockFactory.cpp:64
 +	    WebKit::WebURLResponse response;
i recommend a using namespace WebKit at the top of this file.