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

Issue 2698623006: PlzNavigate: add support for BLOCK_REQUEST during redirects (Closed)

Created:
3 years, 10 months ago by arthursonzogni
Modified:
3 years, 9 months ago
Reviewers:
clamy, Charlie Reis, nasko
CC:
chromium-reviews, jam, nasko+codewatch_chromium.org, darin-cc_chromium.org, creis+watch_chromium.org, nasko, alexmos
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

PlzNavigate: add support for BLOCK_REQUEST during redirects This CL adds support for blocking requests in NavigationRequest during redirects. It also fixes an issue in NavigationHandle, without PlzNavigate activated, where the NavigationHandle would not be properly recognized at error page commit if the navigation was blocked by a NavigationThrottle during a redirect. BUG=685074, 695421 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2698623006 Cr-Commit-Position: refs/heads/master@{#457387} Committed: https://chromium.googlesource.com/chromium/src/+/5b5c309859412def817a518a7dddbf413cd892e9

Patch Set 1 #

Patch Set 2 : Rebase + fix problem without PlzNavigate. #

Patch Set 3 : Rebase. #

Patch Set 4 : Take CL from: https://codereview.chromium.org/2729433003/ #

Total comments: 10

Patch Set 5 : Nit. #

Patch Set 6 : Abandon support of BLOCK_REQUEST on redirect without PlzNavigate. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+96 lines, -3 lines) Patch
M content/browser/frame_host/navigation_handle_impl.cc View 1 2 3 4 5 3 chunks +4 lines, -2 lines 0 comments Download
M content/browser/frame_host/navigation_handle_impl_browsertest.cc View 1 2 3 4 5 3 chunks +82 lines, -0 lines 0 comments Download
M content/browser/frame_host/navigation_request.cc View 1 2 3 4 5 1 chunk +8 lines, -0 lines 0 comments Download
M content/public/browser/navigation_throttle.h View 1 2 3 4 5 1 chunk +2 lines, -1 line 0 comments Download

Messages

Total messages: 59 (44 generated)
arthursonzogni
Hi Camille, Please, could you take a look? Thanks!
3 years, 10 months ago (2017-02-16 16:40:46 UTC) #3
arthursonzogni
Hi Camille, I fix the problem that occurs WITHOUT PlzNavigate. It is perfectly summarized by ...
3 years, 10 months ago (2017-02-17 14:33:42 UTC) #15
arthursonzogni
So, I was telling you silliness. The second navigation was not due to a reload ...
3 years, 10 months ago (2017-02-24 11:59:31 UTC) #33
arthursonzogni
Hi Charlie, I come back. When I was OOO, Camille tries to solve the same ...
3 years, 9 months ago (2017-03-06 15:56:51 UTC) #38
Charlie Reis
One concern below about the difference between the modes. I wonder if we can find ...
3 years, 9 months ago (2017-03-09 17:39:03 UTC) #41
arthursonzogni
Thanks Charlie! Some answers below: https://codereview.chromium.org/2698623006/diff/140001/content/browser/frame_host/navigation_handle_impl.cc File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/2698623006/diff/140001/content/browser/frame_host/navigation_handle_impl.cc#newcode330 content/browser/frame_host/navigation_handle_impl.cc:330: url_ = redirect_chain_.back(); On ...
3 years, 9 months ago (2017-03-10 11:55:28 UTC) #42
Charlie Reis
https://codereview.chromium.org/2698623006/diff/140001/content/browser/frame_host/navigation_handle_impl.cc File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/2698623006/diff/140001/content/browser/frame_host/navigation_handle_impl.cc#newcode755 content/browser/frame_host/navigation_handle_impl.cc:755: NOTREACHED(); On 2017/03/10 11:55:28, arthursonzogni wrote: > On 2017/03/09 ...
3 years, 9 months ago (2017-03-13 20:48:02 UTC) #43
clamy
On 2017/03/13 20:48:02, Charlie Reis (slow) wrote: > https://codereview.chromium.org/2698623006/diff/140001/content/browser/frame_host/navigation_handle_impl.cc > File content/browser/frame_host/navigation_handle_impl.cc (right): > > ...
3 years, 9 months ago (2017-03-14 16:20:38 UTC) #44
Charlie Reis
On 2017/03/14 16:20:38, clamy wrote: > https://codereview.chromium.org/2698623006/diff/140001/content/browser/frame_host/navigation_handle_impl_browsertest.cc#newcode1112 > > content/browser/frame_host/navigation_handle_impl_browsertest.cc:1112: > > finished_navigation = {kRedirectingUrl}; ...
3 years, 9 months ago (2017-03-14 16:28:09 UTC) #45
clamy
On 2017/03/14 16:28:09, Charlie Reis (slow) wrote: > On 2017/03/14 16:20:38, clamy wrote: > > ...
3 years, 9 months ago (2017-03-14 16:37:00 UTC) #46
arthursonzogni
Thanks for the reviews! We tried overriding blink url by using ResourceMsg_RequestComplete. It was a ...
3 years, 9 months ago (2017-03-15 16:45:13 UTC) #50
Charlie Reis
On 2017/03/15 16:45:13, arthursonzogni wrote: > Thanks for the reviews! > > We tried overriding ...
3 years, 9 months ago (2017-03-15 17:44:49 UTC) #51
arthursonzogni
On 2017/03/15 17:44:49, Charlie Reis (slow) wrote: > Ok. I'm disappointed we won't get to ...
3 years, 9 months ago (2017-03-16 09:46:21 UTC) #54
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2698623006/200001
3 years, 9 months ago (2017-03-16 09:50:57 UTC) #56
commit-bot: I haz the power
3 years, 9 months ago (2017-03-16 09:56:14 UTC) #59
Message was sent while issue was closed.
Committed patchset #6 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/5b5c309859412def817a518a7ddd...

Powered by Google App Engine
This is Rietveld 408576698