[Webkit-unassigned] [Bug 39456] [chromium] WebFrame::contentAsText should not return the text from hidden frames

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=39456


Darin Fisher (:fishd, Google) <fishd at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #56715|review?                     |review-
               Flag|                            |




--- Comment #8 from Darin Fisher (:fishd, Google) <fishd at chromium.org>  2010-06-10 11:33:20 PST ---
(From update of attachment 56715)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.