Refactor NavigationHandleImpl::RegisterNavigationThrottles.

Refactor NavigationHandleImpl::RegisterNavigationThrottles.

This CL removes |throttles_to_register| and introduces |AddThrottle()|.
This will be easier to know understand in which order the throttle are
inserted into |throttle_|.

Some throttle were not inserted in the right order. AncestorThrottle
and MixedContentThrottle for instance.

BUG=733099
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2967363002
Cr-Commit-Position: refs/heads/master@{#485911}
Committed: https://chromium.googlesource.com/chromium/src/+/b7c5f00adf724e496dfa6f866e770276239ede47

[arthursonzogni, 2017-07-06 11:38:30]

Description was changed from

==========
Refactor NavigationHandleImpl::RegisterNavigationThrottles.

This CL removes |throttles_to_register| and introduces |AddThrottle()|.
This will be easier to know understand in which order the throttle are
inserted into |throttle_|.

BUG=733099
==========

to

==========
Refactor NavigationHandleImpl::RegisterNavigationThrottles.

This CL removes |throttles_to_register| and introduces |AddThrottle()|.
This will be easier to know understand in which order the throttle are
inserted into |throttle_|.

BUG=733099
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[arthursonzogni, 2017-07-06 11:42:33]

Patchset #1 (id:1) has been deleted

[arthursonzogni, 2017-07-06 11:42:39]

Patchset #1 (id:20001) has been deleted

[arthursonzogni, 2017-07-06 11:53:38]

Patchset #1 (id:40001) has been deleted

[arthursonzogni, 2017-07-06 11:56:53]

Patchset #1 (id:60001) has been deleted

[arthursonzogni, 2017-07-06 11:57:36]

The CQ bit was checked by arthursonzogni@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-06 11:57:46]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[arthursonzogni, 2017-07-06 12:01:57]

arthursonzogni@chromium.org changed reviewers:
+ clamy@chromium.org

[arthursonzogni, 2017-07-06 12:01:57]

Hi Camille,

Could you please take a look?

Prior to this CL, the order of execution of the NavigationThrottle was not easy
to check. Now it is, but the order looks slightly weird to me. Is it the same
for you?

+CC nasko FYI.

[commit-bot: I haz the power, 2017-07-06 12:38:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-07-06 12:38:48]

Dry run: Try jobs failed on following builders:
  linux_site_isolation on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isol...)

[arthursonzogni, 2017-07-06 15:46:52]

Patchset #2 (id:100001) has been deleted

[arthursonzogni, 2017-07-06 15:46:59]

The CQ bit was checked by arthursonzogni@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-06 15:47:15]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-06 15:49:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-07-06 15:49:49]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[arthursonzogni, 2017-07-07 08:53:40]

Hi Camille,

I think the current order of NavigationThrottle is wrong.
If you look at the current code, AncestorThrottle and FormSubmissionThrottle are
AFTER MixedContentNavigationThrottle in |throttles_|. The comment above
MixedContentNavigationThrottle suggests they should be BEFORE instead.

+One additional question: Why the throttles registered with
RegisterThrottleForTesting need to be at the end of the vector?

[arthursonzogni, 2017-07-07 08:58:08]

The CQ bit was checked by arthursonzogni@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-07 08:58:19]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-07 10:47:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-07-07 10:47:15]

Dry run: This issue passed the CQ dry run.

[arthursonzogni, 2017-07-10 09:19:53]

Description was changed from

==========
Refactor NavigationHandleImpl::RegisterNavigationThrottles.

This CL removes |throttles_to_register| and introduces |AddThrottle()|.
This will be easier to know understand in which order the throttle are
inserted into |throttle_|.

BUG=733099
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Refactor NavigationHandleImpl::RegisterNavigationThrottles.

This CL removes |throttles_to_register| and introduces |AddThrottle()|.
This will be easier to know understand in which order the throttle are
inserted into |throttle_|.

Some throttle were not inserted in the right order. AncestorThrottle
and MixedContentThrottle for instance.

BUG=733099
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[arthursonzogni, 2017-07-10 09:19:59]

The CQ bit was checked by arthursonzogni@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-10 09:20:12]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[SaschaKuhn24, 2017-07-10 09:37:17]

saschakuhn24@gmail.com changed reviewers:
+ SaschaKuhn24@gmail.com

[commit-bot: I haz the power, 2017-07-10 10:02:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-07-10 10:02:12]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[arthursonzogni, 2017-07-10 12:15:34]

Patchset #2 (id:120001) has been deleted

[arthursonzogni, 2017-07-10 12:15:41]

Patchset #2 (id:140001) has been deleted

[arthursonzogni, 2017-07-10 12:15:48]

Patchset #2 (id:160001) has been deleted

[arthursonzogni, 2017-07-10 12:17:05]

The CQ bit was checked by arthursonzogni@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-10 12:17:08]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-10 13:21:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-07-10 13:21:22]

Dry run: This issue passed the CQ dry run.

[arthursonzogni, 2017-07-10 13:34:18]

arthursonzogni@chromium.org changed reviewers:
+ saschakuhn24@gmail.com
- SaschaKuhn24@gmail.com

[arthursonzogni, 2017-07-10 13:34:19]

I understood why testing throttles needs to be at the end of the vector and I
reordered some throttles.

[clamy, 2017-07-11 12:32:37]

Thanks! The functionality lgtm, some comments about comments.

https://codereview.chromium.org/2967363002/diff/180001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://codereview.chromium.org/2967363002/diff/180001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.cc:1094: // Note: |throttle_|
might not be empty. Some NavigationThrottle might have
s/NavigationThrottle/NavigationThrottles

https://codereview.chromium.org/2967363002/diff/180001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.cc:1095: // been registered
with RegisterThrottleForTesting. These must reside at the
s/These must reside ... elies on this./
TestNavigationManagerThrottle expects that the NavigationThrottles added for
test are the last NavigationThrottles to execute. Take them out while appending
the rest of the NavigationThrottles.

https://codereview.chromium.org/2967363002/diff/180001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.cc:1118:
throttles_.insert(throttles_.end(),
nit: maybe add a comment
// Insert all testing NavigationThrottles last.

https://codereview.chromium.org/2967363002/diff/180001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl.h (right):

https://codereview.chromium.org/2967363002/diff/180001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.h:432: // Append a new
NavigationThrottle (if any) to |throttle_|.
I would rephrase this as
// Takes ownership of |throttle| and appends it to |throttle_|.

[arthursonzogni, 2017-07-11 14:36:35]

Thanks!

https://codereview.chromium.org/2967363002/diff/180001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl.h (right):

https://codereview.chromium.org/2967363002/diff/180001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.h:432: // Append a new
NavigationThrottle (if any) to |throttle_|.
On 2017/07/11 12:32:37, clamy wrote:
> I would rephrase this as
> // Takes ownership of |throttle| and appends it to |throttle_|.

Done.

[arthursonzogni, 2017-07-11 14:37:24]

https://codereview.chromium.org/2967363002/diff/180001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://codereview.chromium.org/2967363002/diff/180001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.cc:1094: // Note: |throttle_|
might not be empty. Some NavigationThrottle might have
On 2017/07/11 12:32:37, clamy wrote:
> s/NavigationThrottle/NavigationThrottles

Done.

https://codereview.chromium.org/2967363002/diff/180001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.cc:1095: // been registered
with RegisterThrottleForTesting. These must reside at the
On 2017/07/11 12:32:37, clamy wrote:
> s/These must reside ... elies on this./
> TestNavigationManagerThrottle expects that the NavigationThrottles added for
> test are the last NavigationThrottles to execute. Take them out while
appending
> the rest of the NavigationThrottles.

Done.

https://codereview.chromium.org/2967363002/diff/180001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.cc:1118:
throttles_.insert(throttles_.end(),
On 2017/07/11 12:32:37, clamy wrote:
> nit: maybe add a comment
> // Insert all testing NavigationThrottles last.

Done.

[arthursonzogni, 2017-07-11 14:37:29]

The CQ bit was checked by arthursonzogni@chromium.org

[arthursonzogni, 2017-07-11 14:37:29]

The patchset sent to the CQ was uploaded after l-g-t-m from clamy@chromium.org
Link to the patchset: https://codereview.chromium.org/2967363002/#ps200001
(title: "Addressed comments (@clamy) (and rebase...)")

[commit-bot: I haz the power, 2017-07-11 14:37:42]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-11 16:02:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-07-11 16:02:16]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[arthursonzogni, 2017-07-12 09:22:41]

The CQ bit was checked by arthursonzogni@chromium.org

[commit-bot: I haz the power, 2017-07-12 09:22:53]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-12 10:07:55]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1499851361466840,
"parent_rev": "a1b6731c183e2273c3f3b013bf6db0bfe66fdb2f", "commit_rev":
"b7c5f00adf724e496dfa6f866e770276239ede47"}

[commit-bot: I haz the power, 2017-07-12 10:08:11]

Description was changed from

==========
Refactor NavigationHandleImpl::RegisterNavigationThrottles.

This CL removes |throttles_to_register| and introduces |AddThrottle()|.
This will be easier to know understand in which order the throttle are
inserted into |throttle_|.

Some throttle were not inserted in the right order. AncestorThrottle
and MixedContentThrottle for instance.

BUG=733099
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Refactor NavigationHandleImpl::RegisterNavigationThrottles.

This CL removes |throttles_to_register| and introduces |AddThrottle()|.
This will be easier to know understand in which order the throttle are
inserted into |throttle_|.

Some throttle were not inserted in the right order. AncestorThrottle
and MixedContentThrottle for instance.

BUG=733099
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2967363002
Cr-Commit-Position: refs/heads/master@{#485911}
Committed:
https://chromium.googlesource.com/chromium/src/+/b7c5f00adf724e496dfa6f866e77...
==========

[commit-bot: I haz the power, 2017-07-12 10:08:17]

Committed patchset #3 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/b7c5f00adf724e496dfa6f866e77...