Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(297)

Issue 8404001: Add a browsertest for bug 75604 (Closed)

Created:
9 years, 1 month ago by gavinp
Modified:
9 years, 1 month ago
CC:
chromium-reviews, cbentzel+watch_chromium.org, jam, dpranke-watch+content_chromium.org, joi+watch-content_chromium.org, darin-cc_chromium.org, Paweł Hajdan Jr.
Visibility:
Public.

Description

Add a browsertest for bug 75604 This browser test tickles the webkit bug described in crbug.com/75604 . The bug is very hard to repliably reproduce in a layout test, so my plan is to land this browser_test, update webkit to fix the bug, and update this test as we garden past the fix. BUG=75604 Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=107949 Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=108864 Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=108868 Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=108878

Patch Set 1 : lintey fixey #

Total comments: 8

Patch Set 2 : remediate to Paweł's review #

Patch Set 3 : whoops, forgot test data #

Total comments: 5

Patch Set 4 : remediate to mmenke review #

Total comments: 2

Patch Set 5 : landing version #

Patch Set 6 : fixed test for upload #

Patch Set 7 : content:: #

Patch Set 8 : fixed buil.d #

Patch Set 9 : version to commit #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+204 lines, -0 lines) Patch
M chrome/chrome_tests.gypi View 1 2 3 4 5 7 1 chunk +1 line, -0 lines 0 comments Download
A chrome/test/data/webkit/async_script_abort_on_end.html View 1 2 7 1 chunk +8 lines, -0 lines 0 comments Download
A content/browser/net/url_request_abort_on_end_job.h View 1 2 3 4 7 1 chunk +52 lines, -0 lines 0 comments Download
A content/browser/net/url_request_abort_on_end_job.cc View 1 2 3 4 5 6 7 8 1 chunk +105 lines, -0 lines 1 comment Download
A content/browser/webkit_browsertest.cc View 1 2 3 4 5 7 1 chunk +36 lines, -0 lines 0 comments Download
M content/content_tests.gypi View 1 2 3 4 5 7 1 chunk +2 lines, -0 lines 0 comments Download

Messages

Total messages: 18 (0 generated)
gavinp
jam, please review this test, and help me figure out if I've put it in ...
9 years, 1 month ago (2011-10-27 03:23:55 UTC) #1
Paweł Hajdan Jr.
Drive-by. http://codereview.chromium.org/8404001/diff/7001/chrome/chrome_tests.gypi File chrome/chrome_tests.gypi (right): http://codereview.chromium.org/8404001/diff/7001/chrome/chrome_tests.gypi#newcode222 chrome/chrome_tests.gypi:222: '../content/browser/net/url_request_abort_on_end_job.h', Let's move those to test_support_content. Bonus points ...
9 years, 1 month ago (2011-10-27 08:52:57 UTC) #2
gavinp
As always, thanks for the review Paweł! http://codereview.chromium.org/8404001/diff/7001/chrome/chrome_tests.gypi File chrome/chrome_tests.gypi (right): http://codereview.chromium.org/8404001/diff/7001/chrome/chrome_tests.gypi#newcode222 chrome/chrome_tests.gypi:222: '../content/browser/net/url_request_abort_on_end_job.h', On ...
9 years, 1 month ago (2011-10-27 12:52:53 UTC) #3
gavinp
Just publishing my second upload.
9 years, 1 month ago (2011-10-27 13:02:52 UTC) #4
gavinp
And now publishing my third upload, wherein we make test data available for your perusal.
9 years, 1 month ago (2011-10-27 13:04:49 UTC) #5
Paweł Hajdan Jr.
Code I commented in the drive-by LGTM, thanks.
9 years, 1 month ago (2011-10-27 15:11:26 UTC) #6
gavinp
jam: *ping*
9 years, 1 month ago (2011-10-28 15:02:58 UTC) #7
jam
this lgtm although i'm not that familiar with this code. you'll need to get an ...
9 years, 1 month ago (2011-10-28 16:24:11 UTC) #8
gavinp
On 2011/10/28 16:24:11, John Abd-El-Malek wrote: > this lgtm although i'm not that familiar with ...
9 years, 1 month ago (2011-10-28 16:31:50 UTC) #9
mmenke
LGTM, modulo minor comments. http://codereview.chromium.org/8404001/diff/8002/content/browser/net/url_request_abort_on_end_job.cc File content/browser/net/url_request_abort_on_end_job.cc (right): http://codereview.chromium.org/8404001/diff/8002/content/browser/net/url_request_abort_on_end_job.cc#newcode86 content/browser/net/url_request_abort_on_end_job.cc:86: &URLRequestAbortOnEndJob::StartAsync)); You should use base::Bind ...
9 years, 1 month ago (2011-10-28 17:29:52 UTC) #10
gavinp
Thanks mmenke. I've uploaded my remediation to your review, and I will land this first ...
9 years, 1 month ago (2011-10-28 18:25:53 UTC) #11
mmenke
http://codereview.chromium.org/8404001/diff/8002/content/browser/net/url_request_abort_on_end_job.h File content/browser/net/url_request_abort_on_end_job.h (right): http://codereview.chromium.org/8404001/diff/8002/content/browser/net/url_request_abort_on_end_job.h#newcode17 content/browser/net/url_request_abort_on_end_job.h:17: class URLRequestAbortOnEndJob : public net::URLRequestJob { On 2011/10/28 18:25:53, ...
9 years, 1 month ago (2011-10-28 18:32:09 UTC) #12
gavinp
my comment didn't make it into the upload. Now it is in this upload. I'm ...
9 years, 1 month ago (2011-10-31 14:34:02 UTC) #13
gavinp
Mac fun, I reverted this. Crashed, but didn't register in the is_crashed() test. I'll fix ...
9 years, 1 month ago (2011-10-31 15:24:41 UTC) #14
gavinp
And here's what I'm going to land later today. The upstream webkit issue is now ...
9 years, 1 month ago (2011-11-07 14:34:26 UTC) #15
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gavinp@chromium.org/8404001/26001
9 years, 1 month ago (2011-11-07 15:55:48 UTC) #16
commit-bot: I haz the power
Change committed as 108878
9 years, 1 month ago (2011-11-07 17:04:00 UTC) #17
mmenke
9 years, 1 month ago (2011-11-09 02:09:26 UTC) #18
http://codereview.chromium.org/8404001/diff/26001/content/browser/net/url_req...
File content/browser/net/url_request_abort_on_end_job.cc (right):

http://codereview.chromium.org/8404001/diff/26001/content/browser/net/url_req...
content/browser/net/url_request_abort_on_end_job.cc:93: *bytes_read =
std::max(size_t(max_bytes), sizeof(kPageContent));
This should be min, methinks.

Powered by Google App Engine
This is Rietveld 408576698