The WebFrame::contentAsText method should not return the text from hidden frames. Some web pages include some hidden frames with junk content in them. This content is not supposed to be visible on the page, and for that reason should not be indexed or used to determine the page's language.
Created attachment 56648 [details] Initial patch Made WebFrame not report the text from hidden frames (some pages contain hidden frames with garbage text that should not be indexed or used to detect the page's language). Also added a way to mock URL loading in unit-tests.
Created attachment 56649 [details] Minor cosmetic fixes.
Attachment 56649 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1 Last 3072 characters of output: der this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] WebKit/chromium/src/WebFrameImpl.cpp:239: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] WebKit/chromium/tests/WebURLLoaderMock.h:48: This { should be at the end of the previous line [whitespace/braces] [4] WebKit/chromium/tests/WebURLLoaderMock.h:67: Could not find a newline character at the end of the file. [whitespace/ending_newline] [5] WebKit/chromium/tests/WebURLLoaderMock.cpp:30: Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] WebKit/chromium/tests/WebURLLoaderMock.cpp:34: Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] WebKit/chromium/tests/WebURLLoaderMock.cpp:40: Use 0 instead of NULL. [readability/null] [5] WebKit/chromium/tests/data/iframes_test.html:1: One or more unexpected \r (^M) found; better to use only a \n [whitespace/carriage_return] [1] Suppressing further [whitespace/carriage_return] reports for this file. WebKit/chromium/tests/data/invisible_iframe.html:1: One or more unexpected \r (^M) found; better to use only a \n [whitespace/carriage_return] [1] Suppressing further [whitespace/carriage_return] reports for this file. WebKit/chromium/tests/WebURLLoaderMockFactory.h:33: Alphabetical sorting problem. [build/include_order] [4] WebKit/chromium/tests/WebURLLoaderMockFactory.h:35: Alphabetical sorting problem. [build/include_order] [4] WebKit/chromium/tests/WebURLLoaderMockFactory.h:57: This { should be at the end of the previous line [whitespace/braces] [4] WebKit/chromium/tests/WebFrameTest.cpp:36: Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] WebKit/chromium/tests/WebFrameTest.cpp:39: Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] WebKit/chromium/tests/WebFrameTest.cpp:42: Alphabetical sorting problem. [build/include_order] [4] WebKit/chromium/tests/WebFrameTest.cpp:55: This { should be at the end of the previous line [whitespace/braces] [4] WebKit/chromium/tests/WebFrameTest.cpp:67: Use 0 instead of NULL. [readability/null] [5] WebKit/chromium/tests/WebFrameTest.cpp:88: This { should be at the end of the previous line [whitespace/braces] [4] WebKit/chromium/tests/WebFrameTest.cpp:107: Use 0 instead of NULL. [readability/null] [5] WebKit/chromium/tests/data/zero_sized_iframe.html:1: One or more unexpected \r (^M) found; better to use only a \n [whitespace/carriage_return] [1] Suppressing further [whitespace/carriage_return] reports for this file. Total errors found: 43 in 18 files If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 56649 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/2286381
Created attachment 56715 [details] Style fixing
Attachment 56715 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1 WebKit/chromium/tests/WebURLLoaderMockFactory.cpp:30: Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] WebKit/chromium/tests/WebURLLoaderMockFactory.cpp:32: Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] WebKit/chromium/tests/WebURLLoaderMockFactory.cpp:33: Found header this file implements after a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] WebKit/chromium/tests/WebURLLoaderMockFactory.h:34: Alphabetical sorting problem. [build/include_order] [4] WebKit/chromium/tests/WebFrameTest.cpp:36: Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] WebKit/chromium/tests/WebFrameTest.cpp:38: Alphabetical sorting problem. [build/include_order] [4] WebKit/chromium/tests/WebFrameTest.cpp:39: Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Total errors found: 7 in 18 files If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 56715 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/2264408
Comment on attachment 56715 [details] Style fixing 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.
Created attachment 58705 [details] Applied fishd suggested changes
(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.
Created attachment 58706 [details] Fixed WebURL operator<
Created attachment 58707 [details] Fixed ChangeLog
Comment on attachment 58707 [details] Fixed ChangeLog WebKit/chromium/public/WebCString.h:72 + bool lessThan(const WebCString& other) const; needs the WEBKIT_API prefix WebKit/chromium/src/WebCString.cpp:44 + bool WebCString::lessThan(const WebCString& other) const why not implement 'int compare(other)' instead? given that you are using strncmp, you basically get compare for free. WebKit/chromium/tests/WebFrameTest.cpp:60 + webkit_support::RegisterMockedURL(url, response, WebString::fromUTF8(filePath.c_str())); nit: no need for the .c_str() WebKit/chromium/tests/WebFrameTest.cpp:77 + response.setMIMEType(WebString::fromUTF8("text/html")); nit: you can just type response.setMIMEType("text/html"), and the right thing will happen. WebKit/chromium/tests/WebFrameTest.cpp:82 + WebURL webURL = GURL(rootURL + files[i]); we should really have a constructor for WebURL that parses its input. that doesn't have to be part of this patch.
Created attachment 58734 [details] Applied fishd suggested changes
(In reply to comment #13) > (From update of attachment 58707 [details]) > WebKit/chromium/public/WebCString.h:72 > + bool lessThan(const WebCString& other) const; > needs the WEBKIT_API prefix > > WebKit/chromium/src/WebCString.cpp:44 > + bool WebCString::lessThan(const WebCString& other) const > why not implement 'int compare(other)' instead? given that you > are using strncmp, you basically get compare for free. Done, change to lessThan to compare. > WebKit/chromium/tests/WebFrameTest.cpp:60 > + webkit_support::RegisterMockedURL(url, response, WebString::fromUTF8(filePath.c_str())); > nit: no need for the .c_str() Removed. > WebKit/chromium/tests/WebFrameTest.cpp:77 > + response.setMIMEType(WebString::fromUTF8("text/html")); > nit: you can just type response.setMIMEType("text/html"), and the right thing will happen. Done. > WebKit/chromium/tests/WebFrameTest.cpp:82 > + WebURL webURL = GURL(rootURL + files[i]); > we should really have a constructor for WebURL that parses its input. > that doesn't have to be part of this patch. More complicated than I thought initially, will have to be done in a subsequent CL.
Created attachment 58794 [details] Fixed conflict when synching
Attachment 58794 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1 WebKit/chromium/tests/WebFrameTest.cpp:31: Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] WebKit/chromium/tests/WebFrameTest.cpp:34: Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Total errors found: 2 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 58794 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/3336196
Comment on attachment 58734 [details] Applied fishd suggested changes Cleared Darin Fisher's review+ from obsolete attachment 58734 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Comment on attachment 58794 [details] Fixed conflict when synching I think you need to include a change to chromium/DEPS to pick up the latest webkit_support interface.
Created attachment 59296 [details] Modified Chromium DEPS The related webkit support patch on Chromium has landed. I updated the DEPS to point to the new version.
Attachment 59296 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1 WebKit/chromium/tests/WebFrameTest.cpp:31: Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] WebKit/chromium/tests/WebFrameTest.cpp:34: Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Total errors found: 2 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 59296 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/3290562
Created attachment 59395 [details] Added missing HTML test files DEPS rolled. I built and run the tests manually to ensure Webkit stand-alone builds fine.
Attachment 59395 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1 WebKit/chromium/tests/WebFrameTest.cpp:31: Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] WebKit/chromium/tests/WebFrameTest.cpp:34: Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Total errors found: 2 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 59395 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/3338623
Comment on attachment 59395 [details] Added missing HTML test files Rejecting patch 59395 from commit-queue. Failed to run "[u'/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Darin Fisher', u'--force']" exit_code: 1 Last 500 characters of output: ILED -- saving rejects to file WebKit/chromium/DEPS.rej patching file WebKit/chromium/WebKit.gyp patching file WebKit/chromium/src/WebFrameImpl.cpp patching file WebKit/chromium/tests/RunAllTests.cpp patching file WebKit/chromium/tests/WebFrameTest.cpp patching file WebKit/chromium/tests/data/iframes_test.html patching file WebKit/chromium/tests/data/invisible_iframe.html patching file WebKit/chromium/tests/data/visible_iframe.html patching file WebKit/chromium/tests/data/zero_sized_iframe.html Full output: http://webkit-commit-queue.appspot.com/results/3346027
Created attachment 59774 [details] Fixed DEPS which was conflicting with another CL
Attachment 59774 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1 WebKit/chromium/tests/WebFrameTest.cpp:31: Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] WebKit/chromium/tests/WebFrameTest.cpp:34: Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Total errors found: 2 in 9 files If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 59774 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/3324764
Comment on attachment 59774 [details] Fixed DEPS which was conflicting with another CL Clearing flags on attachment: 59774 Committed r61943: <http://trac.webkit.org/changeset/61943>
All reviewed patches have been landed. Closing bug.
Created attachment 60180 [details] Making WebFrameTest Windows only I made WebFrameTest Windows only. On Linux and Mac we get ASSERTS triggered when the WebCore renderer classes attempt to retrieve Fonts. Running this test on Windows only is probably sufficient for now for this CL.
Created attachment 63650 [details] Patch for landing
Reopening the bug so the patch can be landed.
Created attachment 63956 [details] Patch for landing
Comment on attachment 63956 [details] Patch for landing Rejecting patch 63956 from commit-queue. Unexpected failure when processing patch! Please file a bug against webkit-patch. Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', 'land-attachment', '--force-clean', '--build', '--non-interactive', '--ignore-builders', '--build-style=both', '--quiet', 63956, '--test', '--parent-command=commit-queue', '--no-update']" exit_code: 1 Last 500 characters of output: ion=edit Fetching: https://bugs.webkit.org/show_bug.cgi?id=39456&ctype=xml Processing 1 patch from 1 bug. Cleaning working directory Processing patch 63956 from bug 39456. NOBODY (OOPS!) found in /Users/eseidel/Projects/CommitQueue/WebKit/chromium/ChangeLog does not appear to be a valid reviewer according to committers.py. ERROR: /Users/eseidel/Projects/CommitQueue/WebKit/chromium/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).
Comment on attachment 59395 [details] Added missing HTML test files Cleared Darin Fisher's review+ from obsolete attachment 59395 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Comment on attachment 63956 [details] Patch for landing I'm confused why this got rejected. Maybe a bug in svn-apply? Going to try again just in case.
Oh. Silly. When you use land-safely, it's supposed to fill in the reviewer for you. The commit-queue only fills in reviewers when r+ is set on the patch.
by "it" I mean, webkit-patch land-safely fills in the reviewer locally, from the information on the bug, then it posts a new patch including the filled in reviewer and cq+.
Created attachment 64604 [details] Patch for landing
Comment on attachment 64604 [details] Patch for landing Clearing flags on attachment: 64604 Committed r65516: <http://trac.webkit.org/changeset/65516>
Had to revert r65516, it caused chromium webkit_unit_tests to crash on Mac and Linux. See http://build.chromium.org/buildbot/waterfall.fyi/builders/Webkit%20Mac%20%28webkit.org%29/builds/23578/steps/webkit_unit_tests/logs/stdio http://build.chromium.org/buildbot/waterfall.fyi/builders/Webkit%20Linux%20%28webkit.org%29/builds/35468/steps/webkit_unit_tests/logs/stdio
Created attachment 65154 [details] Patch for landing
Comment on attachment 65154 [details] Patch for landing Clearing flags on attachment: 65154 Committed r65860: <http://trac.webkit.org/changeset/65860>