Fix managed mode allow/block flow

Fix managed mode allow/block flow

This patch fixes most of the problems with the allow/block flow:
 - removes got_user_gesture
 - uses the redirect list for URLs instead of recording them every time
 - better identifies when the user has clicked on a link and when the page
   was redirected.

R=bauerb@chromium.org
BUG=168772
TEST=browser_tests --gtest_filter=ManagedModeBlockModeTest.*


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

[Sergiu, 2013-04-08 08:41:10]

Please take a look. Thanks.

[Bernhard Bauer, 2013-04-08 09:50:50]

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

https://codereview.chromium.org/13533017/diff/14001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_mode_navigation_observer.h:127: bool
after_click_on_link_;
What does this flag mean?

[Sergiu, 2013-04-08 12:19:48]

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

https://codereview.chromium.org/13533017/diff/14001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_mode_navigation_observer.h:127: bool
after_click_on_link_;
On 2013/04/08 09:50:50, Bernhard Bauer wrote:
> What does this flag mean? 

I've added a (pretty large) comment.

[Bernhard Bauer, 2013-04-08 12:33:07]

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

https://codereview.chromium.org/13533017/diff/18001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_mode_navigation_observer.h:127: // There are
two starting points for a new navigation:
Nit: newline before the comment please.

https://codereview.chromium.org/13533017/diff/18001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_mode_navigation_observer.h:135: bool
after_click_on_link_;
It's still not clear to me from the comment how the flag tracks that. From the
implementation, it seems you're tracking the fact that we got a
DidNavigateMainFrame() call, which means we're done with redirects. Maybe it
would be better to call the flag |finished_redirects_| or something then?

Also (and perhaps more importantly), what about page-initiated redirects after
DidNavigateMainFrame()? Are we regarding these as user clicks?

[Sergiu, 2013-04-08 14:11:09]

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

https://codereview.chromium.org/13533017/diff/18001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_mode_navigation_observer.h:127: // There are
two starting points for a new navigation:
On 2013/04/08 12:33:07, Bernhard Bauer wrote:
> Nit: newline before the comment please.

Done.

https://codereview.chromium.org/13533017/diff/18001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_mode_navigation_observer.h:135: bool
after_click_on_link_;
On 2013/04/08 12:33:07, Bernhard Bauer wrote:
> It's still not clear to me from the comment how the flag tracks that. From the
> implementation, it seems you're tracking the fact that we got a
> DidNavigateMainFrame() call, which means we're done with redirects. Maybe it
> would be better to call the flag |finished_redirects_| or something then?
> 
> Also (and perhaps more importantly), what about page-initiated redirects after
> DidNavigateMainFrame()? Are we regarding these as user clicks?

Changed the variable name, it makes more sense indeed. Like I said in one of the
TODO's in the .cc file, yes, we are regarding those as they would come from the
user. As a result there is one scenario where I think this approach fails and
that is when we have a client-side redirect that leads to a different domain.
I'm still thinking of a way cover that scenario as well.

[Bernhard Bauer, 2013-04-08 14:14:25]

lgtm

[commit-bot: I haz the power, 2013-04-08 21:25:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergiu@chromium.org/13533017/24001

[commit-bot: I haz the power, 2013-04-08 23:34:38]

Change committed as 192948