[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