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

Issue 176883012: Set the original url correctly if the frame is loaded via loadData (Closed)

Created:
6 years, 9 months ago by hush (inactive)
Modified:
6 years, 8 months ago
CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, Randy Smith (Not in Mondays)
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Visibility:
Public.

Description

Set the original url correctly if the frame is loaded via loadData BUG=348234 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=264247

Patch Set 1 #

Total comments: 19

Patch Set 2 : Address comments about const and null checks #

Total comments: 10

Patch Set 3 : Replaced the unit test with a browser test. Added LoadDataWithBaseURL on shell #

Total comments: 1

Patch Set 4 : add // static comment #

Total comments: 14

Patch Set 5 : address comments and rebase #

Total comments: 2

Patch Set 6 : Rebase #

Patch Set 7 : Rebase correctly #

Unified diffs Side-by-side diffs Delta from patch set Stats (+83 lines, -5 lines) Patch
A content/browser/frame_host/navigation_controller_impl_browsertest.cc View 1 2 3 4 5 6 1 chunk +34 lines, -0 lines 0 comments Download
M content/content_tests.gypi View 1 2 3 4 5 1 chunk +1 line, -0 lines 0 comments Download
M content/public/test/content_browser_test_utils.h View 1 2 3 4 5 1 chunk +4 lines, -0 lines 0 comments Download
M content/public/test/content_browser_test_utils.cc View 1 2 3 4 5 1 chunk +9 lines, -0 lines 0 comments Download
M content/renderer/render_frame_impl.cc View 1 2 3 4 5 3 chunks +20 lines, -5 lines 0 comments Download
M content/shell/browser/shell.h View 1 2 3 4 5 1 chunk +3 lines, -0 lines 0 comments Download
M content/shell/browser/shell.cc View 1 2 3 4 5 1 chunk +12 lines, -0 lines 0 comments Download

Messages

Total messages: 34 (0 generated)
hush (inactive)
ptal
6 years, 9 months ago (2014-03-01 02:36:55 UTC) #1
jamesr
redirecting review to creis@
6 years, 9 months ago (2014-03-01 02:55:39 UTC) #2
Charlie Reis
[+darin,rdsmith for unreachable URL knowledge] I'm a little skeptical about the unreachable URL check, since ...
6 years, 9 months ago (2014-03-03 18:21:11 UTC) #3
hush (inactive)
Hi! Thanks for the comments! Please see my responses below https://codereview.chromium.org/176883012/diff/1/content/renderer/render_frame_impl.cc File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/176883012/diff/1/content/renderer/render_frame_impl.cc#newcode138 ...
6 years, 9 months ago (2014-03-03 23:12:51 UTC) #4
Charlie Reis
@rdsmith: Are you familiar enough with unreachable URL to take a look at this change? ...
6 years, 9 months ago (2014-03-04 20:32:02 UTC) #5
hush (inactive)
Thanks for the comments. I have uploaded a new patch set. https://codereview.chromium.org/176883012/diff/1/content/renderer/render_frame_impl.cc File content/renderer/render_frame_impl.cc (right): ...
6 years, 9 months ago (2014-03-04 23:22:32 UTC) #6
hush (inactive)
ping? Hi darin@ and rdsmith@ what are your opinions for the behavior change? if you ...
6 years, 9 months ago (2014-03-05 21:48:28 UTC) #7
Charlie Reis
On 2014/03/05 21:48:28, hush wrote: > ping? > Hi darin@ and rdsmith@ > what are ...
6 years, 9 months ago (2014-03-08 00:59:42 UTC) #8
darin (slow to review)
https://codereview.chromium.org/176883012/diff/20001/content/renderer/render_frame_impl.cc File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/176883012/diff/20001/content/renderer/render_frame_impl.cc#newcode2280 content/renderer/render_frame_impl.cc:2280: GURL RenderFrameImpl::GetOriginalRequestUrl(WebDataSource* ds) { nit: We aren't terribly consistent, ...
6 years, 9 months ago (2014-03-11 20:04:32 UTC) #9
darin (slow to review)
https://codereview.chromium.org/176883012/diff/20001/content/renderer/render_frame_impl_unittest.cc File content/renderer/render_frame_impl_unittest.cc (right): https://codereview.chromium.org/176883012/diff/20001/content/renderer/render_frame_impl_unittest.cc#newcode28 content/renderer/render_frame_impl_unittest.cc:28: class WebDataSourceMock : public WebDataSource { Note: This is ...
6 years, 9 months ago (2014-03-11 20:09:51 UTC) #10
hush (inactive)
Hi! Thanks for the comments! I will upload a new patch set with a browser ...
6 years, 9 months ago (2014-03-12 21:34:21 UTC) #11
sgurun-gerrit only
https://codereview.chromium.org/176883012/diff/40001/content/renderer/render_frame_impl.cc File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/176883012/diff/40001/content/renderer/render_frame_impl.cc#newcode2279 content/renderer/render_frame_impl.cc:2279: nit: add // static here (code convention, see how ...
6 years, 9 months ago (2014-03-13 00:47:47 UTC) #12
Randy Smith (Not in Mondays)
I don't think I have any special expertise in this code; I'm at the beginning ...
6 years, 9 months ago (2014-03-14 19:19:24 UTC) #13
hush (inactive)
ping?
6 years, 9 months ago (2014-03-24 21:44:53 UTC) #14
darin (slow to review)
Thanks, this is looking good. https://codereview.chromium.org/176883012/diff/60001/content/browser/frame_host/navigation_controller_impl_browsertest.cc File content/browser/frame_host/navigation_controller_impl_browsertest.cc (right): https://codereview.chromium.org/176883012/diff/60001/content/browser/frame_host/navigation_controller_impl_browsertest.cc#newcode1 content/browser/frame_host/navigation_controller_impl_browsertest.cc:1: // Copyright (c) 2014 ...
6 years, 9 months ago (2014-03-27 08:10:28 UTC) #15
hush (inactive)
Hi! I just uploaded a new patch and the tests are passing in the bots ...
6 years, 9 months ago (2014-03-27 20:15:00 UTC) #16
hush (inactive)
ping :)
6 years, 8 months ago (2014-04-07 16:48:18 UTC) #17
darin (slow to review)
LGTM https://codereview.chromium.org/176883012/diff/80001/content/renderer/render_frame_impl.cc File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/176883012/diff/80001/content/renderer/render_frame_impl.cc#newcode175 content/renderer/render_frame_impl.cc:175: static GURL GetOriginalRequestURL(WebDataSource* ds) { nit: It looks ...
6 years, 8 months ago (2014-04-15 22:18:49 UTC) #18
hush (inactive)
https://codereview.chromium.org/176883012/diff/80001/content/renderer/render_frame_impl.cc File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/176883012/diff/80001/content/renderer/render_frame_impl.cc#newcode175 content/renderer/render_frame_impl.cc:175: static GURL GetOriginalRequestURL(WebDataSource* ds) { I see "static" used ...
6 years, 8 months ago (2014-04-15 22:32:30 UTC) #19
hush (inactive)
The CQ bit was checked by hush@chromium.org
6 years, 8 months ago (2014-04-15 22:56:40 UTC) #20
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hush@chromium.org/176883012/80001
6 years, 8 months ago (2014-04-15 22:58:41 UTC) #21
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 8 months ago (2014-04-15 22:58:48 UTC) #22
commit-bot: I haz the power
Failed to apply patch for content/test/content_browser_test_utils.cc: While running patch -p1 --forward --force --no-backup-if-mismatch; can't find ...
6 years, 8 months ago (2014-04-15 22:58:49 UTC) #23
hush (inactive)
On 2014/04/15 22:58:49, I haz the power (commit-bot) wrote: > Failed to apply patch for ...
6 years, 8 months ago (2014-04-15 23:59:29 UTC) #24
hush (inactive)
The CQ bit was checked by hush@chromium.org
6 years, 8 months ago (2014-04-16 00:16:55 UTC) #25
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hush@chromium.org/176883012/100001
6 years, 8 months ago (2014-04-16 00:18:26 UTC) #26
hush (inactive)
The CQ bit was unchecked by hush@chromium.org
6 years, 8 months ago (2014-04-16 00:42:18 UTC) #27
hush (inactive)
The CQ bit was checked by hush@chromium.org
6 years, 8 months ago (2014-04-16 03:28:28 UTC) #28
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hush@chromium.org/176883012/120001
6 years, 8 months ago (2014-04-16 03:29:41 UTC) #29
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 8 months ago (2014-04-16 04:27:03 UTC) #30
commit-bot: I haz the power
Try jobs failed on following builders: tryserver.chromium on linux_chromium_chromeos_rel
6 years, 8 months ago (2014-04-16 04:27:03 UTC) #31
hush (inactive)
The CQ bit was checked by hush@chromium.org
6 years, 8 months ago (2014-04-16 16:42:09 UTC) #32
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hush@chromium.org/176883012/120001
6 years, 8 months ago (2014-04-16 16:42:26 UTC) #33
commit-bot: I haz the power
6 years, 8 months ago (2014-04-16 17:58:11 UTC) #34
Message was sent while issue was closed.
Change committed as 264247

Powered by Google App Engine
This is Rietveld 408576698