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

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Jay Civelli]

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.

[Jay Civelli]

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.

[Jay Civelli]

Created attachment 56649 [details]
Minor cosmetic fixes.

[WebKit Review Bot]

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.

[WebKit Review Bot]

Attachment 56649 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/2286381

[Jay Civelli]

Created attachment 56715 [details]
Style fixing

[WebKit Review Bot]

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.

[WebKit Review Bot]

Attachment 56715 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/2264408

[Darin Fisher (:fishd, Google)]

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.

[Jay Civelli]

Created attachment 58705 [details]
Applied fishd suggested changes

[Jay Civelli]

(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.

[Jay Civelli]

Created attachment 58706 [details]
Fixed WebURL operator<

[Jay Civelli]

Created attachment 58707 [details]
Fixed ChangeLog

[Darin Fisher (:fishd, Google)]

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.

[Jay Civelli]

Created attachment 58734 [details]
Applied fishd suggested changes

[Jay Civelli]

(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.

[Jay Civelli]

Created attachment 58794 [details]
Fixed conflict when synching

[WebKit Review Bot]

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.

[WebKit Review Bot]

Attachment 58794 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3336196

[Eric Seidel (no email)]

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.

[Darin Fisher (:fishd, Google)]

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.

[Jay Civelli]

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.

[WebKit Review Bot]

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.

[WebKit Review Bot]

Attachment 59296 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3290562

[Jay Civelli]

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.

[WebKit Review Bot]

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.

[WebKit Review Bot]

Attachment 59395 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3338623

[WebKit Commit Bot]

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

[Jay Civelli]

Created attachment 59774 [details]
Fixed DEPS which was conflicting with another CL

[WebKit Review Bot]

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.

[WebKit Review Bot]

Attachment 59774 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3324764

[WebKit Commit Bot]

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>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Jay Civelli]

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.

[Jay Civelli]

Created attachment 63650 [details]
Patch for landing

[Jay Civelli]

Reopening the bug so the patch can be landed.

[Jay Civelli]

Created attachment 63956 [details]
Patch for landing

[WebKit Commit Bot]

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).

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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.

[WebKit Commit Bot]

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).

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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+.

[Jay Civelli]

Created attachment 64604 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 64604 [details]
Patch for landing

Clearing flags on attachment: 64604

Committed r65516: <http://trac.webkit.org/changeset/65516>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[John Gregg]

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

[Jay Civelli]

Created attachment 65154 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 65154 [details]
Patch for landing

Clearing flags on attachment: 65154

Committed r65860: <http://trac.webkit.org/changeset/65860>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.