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

Issue 626913004: Set the ui::PAGE_TRANSITION_CLIENT_REDIRECT flag. (Closed)

Created:
6 years, 2 months ago by Fredrik Öhrn
Modified:
6 years, 1 month ago
CC:
chromium-reviews, mkwst+moarreviews-renderer_chromium.org, darin-cc_chromium.org, nasko+codewatch_chromium.org, jam, creis+watch_chromium.org, mkosiba (inactive)
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Project:
chromium
Visibility:
Public.

Description

Set the ui::PAGE_TRANSITION_CLIENT_REDIRECT flag. This patch sets ui::PAGE_TRANSITION_CLIENT_REDIRECT at the beginning of a request which allows it to be used for decisions when hooking into navigation via the components/navigation_interception API. Committed: https://crrev.com/19893e6bce9005ef4a406fc61f0555c2e0f21d50 Cr-Commit-Position: refs/heads/master@{#304425}

Patch Set 1 #

Total comments: 1

Patch Set 2 : Fix namespace #

Total comments: 5

Patch Set 3 : Use correct data source #

Patch Set 4 : Add check. #

Total comments: 1

Patch Set 5 : Add testcase #

Total comments: 2

Patch Set 6 : Fix nits and use PageTransitionFromInt for now #

Patch Set 7 : Rebase #

Patch Set 8 : Fix typo #

Total comments: 1

Patch Set 9 : Fix test case flakiness #

Unified diffs Side-by-side diffs Delta from patch set Stats (+60 lines, -0 lines) Patch
M content/browser/loader/resource_dispatcher_host_browsertest.cc View 1 2 3 4 5 6 7 8 2 chunks +48 lines, -0 lines 0 comments Download
M content/renderer/render_frame_impl.cc View 1 2 3 4 5 6 1 chunk +5 lines, -0 lines 0 comments Download
A content/test/data/client_redirect.html View 1 2 3 4 5 6 7 8 1 chunk +7 lines, -0 lines 0 comments Download

Messages

Total messages: 43 (11 generated)
Fredrik Öhrn
The content API documents the PAGE_TRANSITION_CLIENT_REDIRECT flag but forgot to set it. It's useful to ...
6 years, 2 months ago (2014-10-07 09:39:48 UTC) #2
mkosiba (inactive)
https://codereview.chromium.org/626913004/diff/1/content/renderer/render_frame_impl.cc File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/626913004/diff/1/content/renderer/render_frame_impl.cc#newcode2639 content/renderer/render_frame_impl.cc:2639: if (data_source->isClientRedirect()) { This pretty much matches stuff we ...
6 years, 2 months ago (2014-10-07 11:38:49 UTC) #4
Fredrik Öhrn
Ping?
6 years, 1 month ago (2014-11-07 13:25:56 UTC) #5
Mike West
On 2014/11/07 13:25:56, Fredrik Öhrn wrote: > Ping? This LGTM. I'd suggest fleshing out the ...
6 years, 1 month ago (2014-11-07 13:38:07 UTC) #7
Fredrik Öhrn
On 2014/11/07 13:38:07, Mike West wrote: > On 2014/11/07 13:25:56, Fredrik Öhrn wrote: > > ...
6 years, 1 month ago (2014-11-07 13:45:27 UTC) #8
nasko
https://codereview.chromium.org/626913004/diff/20001/content/renderer/render_frame_impl.cc File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/626913004/diff/20001/content/renderer/render_frame_impl.cc#newcode2639 content/renderer/render_frame_impl.cc:2639: if (data_source->isClientRedirect()) { This doesn't seem right, as it ...
6 years, 1 month ago (2014-11-07 16:50:05 UTC) #9
Fredrik Öhrn
https://codereview.chromium.org/626913004/diff/20001/content/renderer/render_frame_impl.cc File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/626913004/diff/20001/content/renderer/render_frame_impl.cc#newcode2639 content/renderer/render_frame_impl.cc:2639: if (data_source->isClientRedirect()) { On 2014/11/07 16:50:05, nasko wrote: > ...
6 years, 1 month ago (2014-11-10 12:51:58 UTC) #10
nasko
Please include a test case for what you are changing to ensure we don't regress ...
6 years, 1 month ago (2014-11-10 15:01:44 UTC) #11
Fredrik Öhrn
On 2014/11/10 15:01:44, nasko wrote: > Please include a test case for what you are ...
6 years, 1 month ago (2014-11-10 16:32:39 UTC) #12
nasko
>Of course, however, I'm unsure how to add a test for this as it wouldn't ...
6 years, 1 month ago (2014-11-10 17:05:57 UTC) #13
Fredrik Öhrn
On 2014/11/10 17:05:57, nasko wrote: > >Of course, however, I'm unsure how to add a ...
6 years, 1 month ago (2014-11-11 17:12:27 UTC) #14
nasko
On 2014/11/11 17:12:27, Fredrik Öhrn wrote: > On 2014/11/10 17:05:57, nasko wrote: > > >Of ...
6 years, 1 month ago (2014-11-13 00:09:32 UTC) #15
nasko
Really adding creis@ now.
6 years, 1 month ago (2014-11-13 00:09:58 UTC) #17
Charlie Reis
Test LGTM with nits. Can you update the CL description now that the flag is ...
6 years, 1 month ago (2014-11-13 00:49:35 UTC) #18
Fredrik Öhrn
On 2014/11/13 00:09:32, nasko wrote: > On 2014/11/11 17:12:27, Fredrik Öhrn wrote: > > > ...
6 years, 1 month ago (2014-11-13 12:57:08 UTC) #19
Fredrik Öhrn
On 2014/11/13 00:49:35, Charlie Reis wrote: > Test LGTM with nits. > > Can you ...
6 years, 1 month ago (2014-11-13 12:59:26 UTC) #20
nasko
LGTM
6 years, 1 month ago (2014-11-13 15:20:33 UTC) #21
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/626913004/100001
6 years, 1 month ago (2014-11-13 15:33:14 UTC) #23
commit-bot: I haz the power
Try jobs failed on following builders: android_chromium_gn_compile_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromium_gn_compile_dbg/builds/16831) ios_rel_device on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/builds/32825)
6 years, 1 month ago (2014-11-13 15:36:11 UTC) #25
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/626913004/120001
6 years, 1 month ago (2014-11-13 15:56:00 UTC) #27
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/5424)
6 years, 1 month ago (2014-11-13 16:20:47 UTC) #29
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/626913004/140001
6 years, 1 month ago (2014-11-13 17:11:31 UTC) #31
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/5448)
6 years, 1 month ago (2014-11-13 18:03:23 UTC) #33
Fredrik Öhrn
On 2014/11/13 18:03:23, I haz the power (commit-bot) wrote: > Try jobs failed on following ...
6 years, 1 month ago (2014-11-14 12:59:52 UTC) #34
Charlie Reis
https://codereview.chromium.org/626913004/diff/140001/content/browser/loader/resource_dispatcher_host_browsertest.cc File content/browser/loader/resource_dispatcher_host_browsertest.cc (right): https://codereview.chromium.org/626913004/diff/140001/content/browser/loader/resource_dispatcher_host_browsertest.cc#newcode514 content/browser/loader/resource_dispatcher_host_browsertest.cc:514: NavigateToURL(shell(), A client redirect will commit twice, right? Maybe ...
6 years, 1 month ago (2014-11-14 18:05:54 UTC) #35
Fredrik Öhrn
On 2014/11/14 18:05:54, Charlie Reis wrote: > https://codereview.chromium.org/626913004/diff/140001/content/browser/loader/resource_dispatcher_host_browsertest.cc > File content/browser/loader/resource_dispatcher_host_browsertest.cc (right): > > https://codereview.chromium.org/626913004/diff/140001/content/browser/loader/resource_dispatcher_host_browsertest.cc#newcode514 ...
6 years, 1 month ago (2014-11-17 11:42:44 UTC) #36
nasko
That looks better. LGTM
6 years, 1 month ago (2014-11-17 14:44:16 UTC) #37
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/626913004/160001
6 years, 1 month ago (2014-11-17 14:49:48 UTC) #39
commit-bot: I haz the power
Committed patchset #9 (id:160001)
6 years, 1 month ago (2014-11-17 16:00:09 UTC) #40
commit-bot: I haz the power
Patchset 9 (id:??) landed as https://crrev.com/19893e6bce9005ef4a406fc61f0555c2e0f21d50 Cr-Commit-Position: refs/heads/master@{#304425}
6 years, 1 month ago (2014-11-17 16:00:50 UTC) #41
Fredrik Öhrn
Thanks everyone for the help and your patience, this review turned out noisier than I ...
6 years, 1 month ago (2014-11-17 16:06:12 UTC) #42
nasko
6 years, 1 month ago (2014-11-17 16:46:57 UTC) #43
Message was sent while issue was closed.
On 2014/11/17 16:06:12, Fredrik Öhrn wrote:
> Thanks everyone for the help and your patience, this review turned out noisier
> than I expected.

No problem. CLs aren't always as easy as they seem initially :) Keep the patches
coming though!

Powered by Google App Engine
This is Rietveld 408576698