[Webkit-unassigned] [Bug 39456] [chromium] WebFrame::contentAsText should not return the text from hidden frames
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Jun 14 15:00:51 PDT 2010
https://bugs.webkit.org/show_bug.cgi?id=39456
--- Comment #10 from Jay Civelli <jcivelli at chromium.org> 2010-06-14 15:00:50 PST ---
(In reply to comment #8)
> (From update of attachment 56715 [details])
> 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.
Done.
> 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;
Done.
> WebKit/chromium/public/WebURL.h:80
> + }
> ditto. also, please avoid adding extra new lines.
Done.
> 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.
Changed.
> WebKit/chromium/tests/WebFrameTest.cpp:31
> + // Basic tests that verify our KURL's interface behaves the same as the
> this comment is wrong.
Comment removed.
> WebKit/chromium/tests/WebFrameTest.cpp:34
> + #include "config.h"
> no need to include config.h unless you are using WebCore headers.
Done.
> 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>
Done.
> WebKit/chromium/tests/WebFrameTest.cpp:48
> + using namespace WebCore;
> are you really using WebCore?
Removed.
> 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.
I moved the mocking part to webkit_support in Chromium (http://codereview.chromium.org/2749020).
> WebKit/chromium/tests/WebFrameTest.cpp:61
> + }
> nit: insert new line here
Done.
> 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.
Now using sizeof(array) / sizeof(type).
All comments below now apply to the Chromium CL mentioned above.
> 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.
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list