Show interstitial from ManagedModeNavigationObserver instead of ManagedModeResourceThrottle.

Show interstitial from ManagedModeNavigationObserver instead of ManagedModeResourceThrottle.


BUG=168772
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=191182

[Bernhard Bauer, 2013-03-28 13:53:48]

Please review.

[Adrian Kuegel, 2013-03-28 14:28:34]

LGTM with nits.

https://codereview.chromium.org/12887005/diff/19001/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_mode_navigation_observer.cc (right):

https://codereview.chromium.org/12887005/diff/19001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_mode_navigation_observer.cc:304: if
(!navigation_observer) {
When can this happen? In my code I don't handle it so far, so if this is
something we should catch, I should do that, too.

https://codereview.chromium.org/12887005/diff/19001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_mode_navigation_observer.cc:319:
web_contents(), url,
Shouldn't |url| go one line below?

https://codereview.chromium.org/12887005/diff/19001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_mode_navigation_observer.cc:586: if (state_
== RECORDING_URLS_AFTER_PREVIEW) {
Looks like the braces can be ommitted.

https://codereview.chromium.org/12887005/diff/19001/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_mode_navigation_observer.h (right):

https://codereview.chromium.org/12887005/diff/19001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_mode_navigation_observer.h:34: const
base::Callback<void(bool)>& callback);
nit: You could use a typedef for this kind of callback function.

https://codereview.chromium.org/12887005/diff/19001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_mode_navigation_observer.h:160:
std::vector<base::Callback<void(bool)> > callbacks_;
Nit: you could document this variable.

https://codereview.chromium.org/12887005/diff/19001/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_mode_resource_throttle.cc (right):

https://codereview.chromium.org/12887005/diff/19001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_mode_resource_throttle.cc:138:
render_process_host_id_, render_view_id_, url,
Could |render_view_id_| and |url| be in an extra line each?

[Bernhard Bauer, 2013-03-28 15:03:30]

https://codereview.chromium.org/12887005/diff/19001/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_mode_navigation_observer.cc (right):

https://codereview.chromium.org/12887005/diff/19001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_mode_navigation_observer.cc:304: if
(!navigation_observer) {
On 2013/03/28 14:28:34, Adrian Kuegel wrote:
> When can this happen? In my code I don't handle it so far, so if this is
> something we should catch, I should do that, too.

If we have a WebContents that doesn't have the helper objects associated with
it, which is (roughly speaking) any WebContents that doesn't live in a tab.
Examples are extension popups or constrained web dialogs.

https://codereview.chromium.org/12887005/diff/19001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_mode_navigation_observer.cc:319:
web_contents(), url,
On 2013/03/28 14:28:34, Adrian Kuegel wrote:
> Shouldn't |url| go one line below?

For method *calls* the rule is to put the arguments on separate lines if it
makes the code more readable. In this case, I think |web_contents()| and |url|
are short enough that it doesn't. WDYT?

https://codereview.chromium.org/12887005/diff/19001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_mode_navigation_observer.cc:586: if (state_
== RECORDING_URLS_AFTER_PREVIEW) {
On 2013/03/28 14:28:34, Adrian Kuegel wrote:
> Looks like the braces can be ommitted.

Done.

https://codereview.chromium.org/12887005/diff/19001/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_mode_navigation_observer.h (right):

https://codereview.chromium.org/12887005/diff/19001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_mode_navigation_observer.h:34: const
base::Callback<void(bool)>& callback);
On 2013/03/28 14:28:34, Adrian Kuegel wrote:
> nit: You could use a typedef for this kind of callback function.

Done.

https://codereview.chromium.org/12887005/diff/19001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_mode_navigation_observer.h:160:
std::vector<base::Callback<void(bool)> > callbacks_;
On 2013/03/28 14:28:34, Adrian Kuegel wrote:
> Nit: you could document this variable.

Done.

https://codereview.chromium.org/12887005/diff/19001/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_mode_resource_throttle.cc (right):

https://codereview.chromium.org/12887005/diff/19001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_mode_resource_throttle.cc:138:
render_process_host_id_, render_view_id_, url,
On 2013/03/28 14:28:34, Adrian Kuegel wrote:
> Could |render_view_id_| and |url| be in an extra line each?

See my comments in
https://codereview.chromium.org/12887005/diff/19001/chrome/browser/managed_mo....
If you feel that it improves readability, I'll do it, but it's not required.

[Adrian Kuegel, 2013-03-28 15:11:11]

https://codereview.chromium.org/12887005/diff/19001/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_mode_navigation_observer.cc (right):

https://codereview.chromium.org/12887005/diff/19001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_mode_navigation_observer.cc:304: if
(!navigation_observer) {
Ok, I will then also check this in my current patch.

https://codereview.chromium.org/12887005/diff/19001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_mode_navigation_observer.cc:319:
web_contents(), url,
Thanks for the explanation. Ok, this is fine for me.

https://codereview.chromium.org/12887005/diff/19001/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_mode_resource_throttle.cc (right):

https://codereview.chromium.org/12887005/diff/19001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_mode_resource_throttle.cc:138:
render_process_host_id_, render_view_id_, url,
I think it is readable enough as it is.

[commit-bot: I haz the power, 2013-03-28 15:22:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bauerb@chromium.org/12887005/33001

[commit-bot: I haz the power, 2013-03-28 18:51:14]

Change committed as 191182