Clean up WebNavigationObserver by taking out password specific callbacks, and...

Clean up WebNavigationObserver by taking out password specific callbacks, and make PasswordManager just dispatch the IPC message directly to achieve this.  I've also combined WebNavigationObserver with IPC message filtering (I hadn't seen the former when I added it).  This allows a little more cleanup in AutoFillManager.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=72155

[Ben Goodger (Google), 2011-01-20 22:34:44]

I haven't even opened this yet and I can tell it's going to be awesome.

-Ben

On Thu, Jan 20, 2011 at 2:27 PM, <jam@chromium.org> wrote:

> Reviewers: Ben Goodger,
>
> Description:
> Clean up WebNavigationObserver by taking out password specific callbacks,
> and
> make PasswordManager just dispatch the IPC message directly to achieve
> this.
> I've also combined WebNavigationObserver with IPC message filtering (I
> hadn't
> seen the former when I added it).  This allows a little more cleanup in
> AutoFillManager.
>
> Please review this at http://codereview.chromium.org/6368011/
>
> SVN Base: svn://chrome-svn/chrome/trunk/src/
>
> Affected files:
>  M     chrome/browser/autocomplete_history_manager.h
>  M     chrome/browser/autofill/autofill_manager.h
>  M     chrome/browser/autofill/autofill_manager.cc
>  M     chrome/browser/autofill/form_structure_browsertest.cc
>  M     chrome/browser/password_manager/password_manager.h
>  M     chrome/browser/password_manager/password_manager.cc
>  M     chrome/browser/renderer_host/render_view_host.h
>  M     chrome/browser/renderer_host/render_view_host.cc
>  M     chrome/browser/renderer_host/render_view_host_delegate.h
>  M     chrome/browser/tab_contents/tab_contents.h
>  M     chrome/browser/tab_contents/tab_contents.cc
>  M     chrome/browser/tab_contents/web_navigation_observer.h
>  M     chrome/browser/translate/translate_manager.cc
>  M     chrome/browser/ui/login/login_prompt.cc
>
>
>

[Ben Goodger (Google), 2011-01-20 22:38:24]

http://codereview.chromium.org/6368011/diff/15001/chrome/browser/translate/tr...
File chrome/browser/translate/translate_manager.cc (right):

http://codereview.chromium.org/6368011/diff/15001/chrome/browser/translate/tr...
chrome/browser/translate/translate_manager.cc:518: tab->TranslateStarted();
Does this need to go through the tab? It'd be nice to have WebNavigationObserver
not know about the Translate feature. The alternative here is to use a
Notification.

[jam, 2011-01-20 23:26:59]

http://codereview.chromium.org/6368011/diff/15001/chrome/browser/translate/tr...
File chrome/browser/translate/translate_manager.cc (right):

http://codereview.chromium.org/6368011/diff/15001/chrome/browser/translate/tr...
chrome/browser/translate/translate_manager.cc:518: tab->TranslateStarted();
On 2011/01/20 22:38:24, Ben Goodger wrote:
> Does this need to go through the tab? It'd be nice to have
WebNavigationObserver
> not know about the Translate feature. The alternative here is to use a
> Notification.

good point.  I didn't like doing this either (actually TabContents is already
too involved in the details of translation, it would be good to separate this
out some more in future changes).  I looked into it and we can reuse the
page_translated notification, for this purpose it works as well as the
about_to_translate one.

[Ben Goodger (Google), 2011-01-20 23:44:09]

http://codereview.chromium.org/6368011/diff/15001/chrome/browser/translate/tr...
File chrome/browser/translate/translate_manager.cc (right):

http://codereview.chromium.org/6368011/diff/15001/chrome/browser/translate/tr...
chrome/browser/translate/translate_manager.cc:518: tab->TranslateStarted();
OK - can you add that as a TODO then if you don't want to do it here?

[jam, 2011-01-20 23:48:18]

http://codereview.chromium.org/6368011/diff/15001/chrome/browser/translate/tr...
File chrome/browser/translate/translate_manager.cc (right):

http://codereview.chromium.org/6368011/diff/15001/chrome/browser/translate/tr...
chrome/browser/translate/translate_manager.cc:518: tab->TranslateStarted();
On 2011/01/20 23:44:09, Ben Goodger wrote:
> OK - can you add that as a TODO then if you don't want to do it here?

I wasn't clear: I actually ended up using it in this change, patch was uploaded.

[jam, 2011-01-20 23:54:25]

On Thu, Jan 20, 2011 at 3:48 PM, <jam@chromium.org> wrote:

>
>
>
http://codereview.chromium.org/6368011/diff/15001/chrome/browser/translate/tr...
> File chrome/browser/translate/translate_manager.cc (right):
>
>
>
http://codereview.chromium.org/6368011/diff/15001/chrome/browser/translate/tr...
> chrome/browser/translate/translate_manager.cc:518:
> tab->TranslateStarted();
> On 2011/01/20 23:44:09, Ben Goodger wrote:
>
>> OK - can you add that as a TODO then if you don't want to do it here?
>>
>
> I wasn't clear: I actually ended up using it in this change, patch was
> uploaded.


What I think should be done later is not make TabContents so tied up with
translation, which it is at the moment.

>
>
> http://codereview.chromium.org/6368011/
>

[jam, 2011-01-21 01:53:15]

ok looks like there was a test failure with making the form fill data get reset
later.  so i've revert that part of the change (i.e. having TranslateManager
know about AutoFill), and will leave that to another time.

[Ben Goodger (Google), 2011-01-21 02:42:32]

OK. Avi is getting started working on reducing the dependencies of TC
(moving more stuff to TCW), so can help here too if you like.

-Ben

On Thu, Jan 20, 2011 at 5:53 PM, <jam@chromium.org> wrote:

> ok looks like there was a test failure with making the form fill data get
> reset
> later.  so i've revert that part of the change (i.e. having
> TranslateManager
> know about AutoFill), and will leave that to another time.
>
>
> http://codereview.chromium.org/6368011/
>

[jam, 2011-01-21 16:57:45]

to be clear, so lgtm?

On Thu, Jan 20, 2011 at 6:42 PM, Ben Goodger (Google) <ben@chromium.org>wrote:

> OK. Avi is getting started working on reducing the dependencies of TC
> (moving more stuff to TCW), so can help here too if you like.
>
> -Ben
>
>
> On Thu, Jan 20, 2011 at 5:53 PM, <jam@chromium.org> wrote:
>
>> ok looks like there was a test failure with making the form fill data get
>> reset
>> later.  so i've revert that part of the change (i.e. having
>> TranslateManager
>> know about AutoFill), and will leave that to another time.
>>
>>
>> http://codereview.chromium.org/6368011/
>>
>
>

[Ben Goodger (Google), 2011-01-21 18:21:17]

LGTM btw