[password autofill] Remove references to PasswordForm from RenderViewImpl

[password autofill] Remove references to PasswordForm from RenderViewImpl

autofill::PasswordAutofillAgent now manages submitted passwords instead. Browser side classes other than PasswordManager which want to monitor these submits do so via registering a callback with PasswordManager::AddSubmissionsCallback().

This is the first step in moving content::PasswordForm to autofill::PasswordForm. 

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

[Garrett Casto, 2013-07-25 16:42:54]

jam: Changes to content.
jln: Changes to messages.
isherman: Changes to autofill/password manager.
rogerta: Changes to OneClickSigninHelper
tburkard: Changes to PrerenderTabHelper
avi: Changes to browser_tab_contents.cc
yfriedman: Changes to tab_android.cc

[tburkard, 2013-07-25 16:59:16]

https://codereview.chromium.org/19705013/diff/39001/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_tab_helper.cc (right):

https://codereview.chromium.org/19705013/diff/39001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_tab_helper.cc:154: void
PrerenderTabHelper::CreateForWebContentsWithPasswordManager(
Can't this be done as a template in web_contents_user_data, analogous to the
formerly used implementation, so that other classes who might need this can use
it as well?

[jam, 2013-07-25 17:31:33]

content lgtm

https://codereview.chromium.org/19705013/diff/39001/chrome/browser/password_m...
File chrome/browser/password_manager/password_manager.h (right):

https://codereview.chromium.org/19705013/diff/39001/chrome/browser/password_m...
chrome/browser/password_manager/password_manager.h:48: virtual void
PasswordSubmitted(const content::PasswordForm& form) = 0;
it's best to avoid single-method interfaces, you can use a callback instead. see
HostZoomMap::ZoomLevelChangedCallback as an example

[Garrett Casto, 2013-07-25 17:34:05]

https://codereview.chromium.org/19705013/diff/39001/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_tab_helper.cc (right):

https://codereview.chromium.org/19705013/diff/39001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_tab_helper.cc:154: void
PrerenderTabHelper::CreateForWebContentsWithPasswordManager(
On 2013/07/25 16:59:17, tburkard wrote:
> Can't this be done as a template in web_contents_user_data, analogous to the
> formerly used implementation, so that other classes who might need this can
use
> it as well?

Do you mean something generic like
WebContentsUserData::CreateForWebContentsWithArgs(), or actually
WebContentsUserData::CreateForWebContentsWithPasswordManager(). I could possibly
see some use for the former, but the latter seems too specific, especially since
it would be in content referencing something in chrome. In either case, there
aren't that many classes that make use of this right now, so I doubt it's worth
the effort.

[tburkard, 2013-07-25 17:42:52]

prerender lgtm

[Yaron, 2013-07-25 19:49:51]

tab_android lgtm

[Garrett Casto, 2013-07-25 20:00:00]

https://codereview.chromium.org/19705013/diff/39001/chrome/browser/password_m...
File chrome/browser/password_manager/password_manager.h (right):

https://codereview.chromium.org/19705013/diff/39001/chrome/browser/password_m...
chrome/browser/password_manager/password_manager.h:48: virtual void
PasswordSubmitted(const content::PasswordForm& form) = 0;
On 2013/07/25 17:31:33, jam wrote:
> it's best to avoid single-method interfaces, you can use a callback instead.
see
> HostZoomMap::ZoomLevelChangedCallback as an example

Done.

[Ilya Sherman, 2013-07-27 01:09:47]

Thanks, Garrett.  Looks pretty nice at a high level :)

https://codereview.chromium.org/19705013/diff/54001/chrome/browser/password_m...
File chrome/browser/password_manager/password_manager.cc (right):

https://codereview.chromium.org/19705013/diff/54001/chrome/browser/password_m...
chrome/browser/password_manager/password_manager.cc:251:
ProvisionallySavePassword(password_form);
Should you be calling pending_login_managers_.clear() here, to match the
previous behavior of DidNavigateAnyFrame()?

https://codereview.chromium.org/19705013/diff/54001/chrome/browser/password_m...
chrome/browser/password_manager/password_manager.cc:252: for (size_t i = 0; i <
submission_callbacks_.size(); ++i)
Optional nit: I prefer to always use curly braces to enclose loop bodies, even
in the case of one-line loop bodies.  I *think* that's the common style in most
of the Chromium code that I've seen, as well.

https://codereview.chromium.org/19705013/diff/54001/chrome/browser/password_m...
File chrome/browser/password_manager/password_manager.h (right):

https://codereview.chromium.org/19705013/diff/54001/chrome/browser/password_m...
chrome/browser/password_manager/password_manager.h:47:
PasswordSubmittedCallback;
nit: Please indent four more spaces.

https://codereview.chromium.org/19705013/diff/54001/chrome/browser/password_m...
chrome/browser/password_manager/password_manager.h:50: // owners of these
callbacks have the same lifetime as the PasswordManager.
That's not entirely accurate -- there is some ordering to the destruction of
objects dangling off a WebContents.  But that's really more to the point of
whether base::Unretained() is safe to use or not.

https://codereview.chromium.org/19705013/diff/54001/chrome/browser/password_m...
chrome/browser/password_manager/password_manager.h:51: void
AddSubmissionCallback(const PasswordSubmittedCallback& callback);
Could you add some test coverage for these callbacks?

https://codereview.chromium.org/19705013/diff/54001/chrome/browser/password_m...
chrome/browser/password_manager/password_manager.h:99: friend class
PasswordManagerTest;
Rather than friending the test class -- which allows the class to muck with
*all* of the private state for this object, please just make
OnPasswordFormSubmitted have protected visibility, and create a subclass for
testing that ups the visibility to public.

https://codereview.chromium.org/19705013/diff/54001/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_tab_helper.cc (right):

https://codereview.chromium.org/19705013/diff/54001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_tab_helper.cc:169: base::Unretained(this))),
Why is base::Unretained() safe here?

https://codereview.chromium.org/19705013/diff/54001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_tab_helper.cc:171: DCHECK(password_manager);
nit: This seems redundant with the immediate use of password_manager on the line
below.

https://codereview.chromium.org/19705013/diff/54001/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_tab_helper.h (right):

https://codereview.chromium.org/19705013/diff/54001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_tab_helper.h:110:
PasswordManager::PasswordSubmittedCallback password_callback_;
Why does this need to be stored as a member variable?

https://codereview.chromium.org/19705013/diff/54001/chrome/browser/ui/browser...
File chrome/browser/ui/browser_tab_contents.cc (right):

https://codereview.chromium.org/19705013/diff/54001/chrome/browser/ui/browser...
chrome/browser/ui/browser_tab_contents.cc:146: web_contents,
PasswordManager::FromWebContents(web_contents));
Hmm, why not just call PasswordManager::FromWebContents(web_contents) within
prerender::PrerenderTabHelper::CreateForWebContents()?

https://codereview.chromium.org/19705013/diff/54001/chrome/browser/ui/browser...
chrome/browser/ui/browser_tab_contents.cc:183: web_contents,
PasswordManager::FromWebContents(web_contents));
Ditto.

https://codereview.chromium.org/19705013/diff/54001/chrome/browser/ui/sync/on...
File chrome/browser/ui/sync/one_click_signin_helper.cc (right):

https://codereview.chromium.org/19705013/diff/54001/chrome/browser/ui/sync/on...
chrome/browser/ui/sync/one_click_signin_helper.cc:565: base::Unretained(this)))
{
Why is base::Unretained() safe here?

https://codereview.chromium.org/19705013/diff/54001/chrome/browser/ui/sync/on...
chrome/browser/ui/sync/one_click_signin_helper.cc:1006: // get lost if one of
the redirects requires context swap.
nit: Is this comment still relevant?  If so, should it be moved into the
password manager code?

https://codereview.chromium.org/19705013/diff/54001/chrome/browser/ui/sync/on...
File chrome/browser/ui/sync/one_click_signin_helper.h (right):

https://codereview.chromium.org/19705013/diff/54001/chrome/browser/ui/sync/on...
chrome/browser/ui/sync/one_click_signin_helper.h:213: void
PasswordSubmitted(const content::PasswordForm& form);
nit: Docs?

https://codereview.chromium.org/19705013/diff/54001/chrome/browser/ui/sync/on...
chrome/browser/ui/sync/one_click_signin_helper.h:265:
PasswordManager::PasswordSubmittedCallback password_callback_;
Why does this need to be stored as a class member here?

https://codereview.chromium.org/19705013/diff/54001/chrome/test/data/password...
File chrome/test/data/password/password_form.html (right):

https://codereview.chromium.org/19705013/diff/54001/chrome/test/data/password...
chrome/test/data/password/password_form.html:6: <input type="submit"
id="input_submit_button" name="input_submit_button">
nit: Do you really need ids for the <input>s?  Doesn't seem like they ought to
matter.

https://codereview.chromium.org/19705013/diff/54001/components/autofill/conte...
File components/autofill/content/renderer/password_autofill_agent.cc (right):

https://codereview.chromium.org/19705013/diff/54001/components/autofill/conte...
components/autofill/content/renderer/password_autofill_agent.cc:435: // into a
hidden field and then clear the password. (Issue 28910.)
nit: Could you make the bug reference into an http://crbug.com hyperlink?

https://codereview.chromium.org/19705013/diff/54001/components/autofill/conte...
components/autofill/content/renderer/password_autofill_agent.cc:438:
provisional_forms_[frame].reset(content::CreatePasswordForm(form).release());
Is there a guarantee that we will free elements from this map?  I'm a little
worried that this can create a slow memory leak (though since form submissions
are relatively rare, it's not a particularly bad memory leak).

https://codereview.chromium.org/19705013/diff/54001/components/autofill/conte...
components/autofill/content/renderer/password_autofill_agent.cc:447: // password
value so we get the users typed password, not the value that
nit: "users" -> "user's"

https://codereview.chromium.org/19705013/diff/54001/components/autofill/conte...
components/autofill/content/renderer/password_autofill_agent.cc:451: if
(submitted_form.get()) {
nit: No need for .get()

https://codereview.chromium.org/19705013/diff/54001/components/autofill/conte...
components/autofill/content/renderer/password_autofill_agent.cc:452: if
(provisional_forms_[frame].get() &&
nit: Ditto

https://codereview.chromium.org/19705013/diff/54001/components/autofill/conte...
components/autofill/content/renderer/password_autofill_agent.cc:456: }
nit: Please leave a blank line after this one.

https://codereview.chromium.org/19705013/diff/54001/components/autofill/conte...
components/autofill/content/renderer/password_autofill_agent.cc:462:
*submitted_form.get()));
nit: No need for .get()

https://codereview.chromium.org/19705013/diff/54001/components/autofill/conte...
components/autofill/content/renderer/password_autofill_agent.cc:477:
provisional_forms_[frame].get()) {
nit: No need for .get()

https://codereview.chromium.org/19705013/diff/54001/components/autofill/conte...
components/autofill/content/renderer/password_autofill_agent.cc:480:
*provisional_forms_[frame].get()));
nit: No need for .get()

https://codereview.chromium.org/19705013/diff/54001/components/autofill/conte...
components/autofill/content/renderer/password_autofill_agent.cc:482:
provisional_forms_.erase(frame);
Can we clear the whole map if the |frame| is a top-level frame, either here or
when a load commits?

https://codereview.chromium.org/19705013/diff/54001/components/autofill/conte...
File components/autofill/content/renderer/password_autofill_agent.h (right):

https://codereview.chromium.org/19705013/diff/54001/components/autofill/conte...
components/autofill/content/renderer/password_autofill_agent.h:72:
linked_ptr<content::PasswordForm> > FrameToPasswordForm;
nit: Please include "Map" in this type name.

https://codereview.chromium.org/19705013/diff/54001/components/autofill/conte...
components/autofill/content/renderer/password_autofill_agent.h:136:
FrameToPasswordForm provisional_forms_;
Optional nit: Perhaps |provisionally_saved_forms_|?

[Garrett Casto, 2013-08-03 00:38:41]

Sorry for the delay. PTAL

https://chromiumcodereview.appspot.com/19705013/diff/54001/chrome/browser/pas...
File chrome/browser/password_manager/password_manager.cc (right):

https://chromiumcodereview.appspot.com/19705013/diff/54001/chrome/browser/pas...
chrome/browser/password_manager/password_manager.cc:251:
ProvisionallySavePassword(password_form);
On 2013/07/27 01:09:48, Ilya Sherman wrote:
> Should you be calling pending_login_managers_.clear() here, to match the
> previous behavior of DidNavigateAnyFrame()?

I actually did this on purpose, though now that I think about it again I should
probably put it back. I was thinking that there was no reason we couldn't handle
different subframe logins on the same main page, but there may be some issues
with multiple infobars, etc. that I should just avoid.

https://chromiumcodereview.appspot.com/19705013/diff/54001/chrome/browser/pas...
chrome/browser/password_manager/password_manager.cc:252: for (size_t i = 0; i <
submission_callbacks_.size(); ++i)
On 2013/07/27 01:09:48, Ilya Sherman wrote:
> Optional nit: I prefer to always use curly braces to enclose loop bodies, even
> in the case of one-line loop bodies.  I *think* that's the common style in
most
> of the Chromium code that I've seen, as well.

Done.

https://chromiumcodereview.appspot.com/19705013/diff/54001/chrome/browser/pas...
File chrome/browser/password_manager/password_manager.h (right):

https://chromiumcodereview.appspot.com/19705013/diff/54001/chrome/browser/pas...
chrome/browser/password_manager/password_manager.h:47:
PasswordSubmittedCallback;
On 2013/07/27 01:09:48, Ilya Sherman wrote:
> nit: Please indent four more spaces.

Done.

https://chromiumcodereview.appspot.com/19705013/diff/54001/chrome/browser/pas...
chrome/browser/password_manager/password_manager.h:50: // owners of these
callbacks have the same lifetime as the PasswordManager.
On 2013/07/27 01:09:48, Ilya Sherman wrote:
> That's not entirely accurate -- there is some ordering to the destruction of
> objects dangling off a WebContents.  But that's really more to the point of
> whether base::Unretained() is safe to use or not.

I tried to clear this comment up a bit. Not sure if this is clearer or not.

https://chromiumcodereview.appspot.com/19705013/diff/54001/chrome/browser/pas...
chrome/browser/password_manager/password_manager.h:51: void
AddSubmissionCallback(const PasswordSubmittedCallback& callback);
On 2013/07/27 01:09:48, Ilya Sherman wrote:
> Could you add some test coverage for these callbacks?

Done.

https://chromiumcodereview.appspot.com/19705013/diff/54001/chrome/browser/pas...
chrome/browser/password_manager/password_manager.h:99: friend class
PasswordManagerTest;
On 2013/07/27 01:09:48, Ilya Sherman wrote:
> Rather than friending the test class -- which allows the class to muck with
> *all* of the private state for this object, please just make
> OnPasswordFormSubmitted have protected visibility, and create a subclass for
> testing that ups the visibility to public.

As you wish. I personally don't have a problem with this for testing code.

https://chromiumcodereview.appspot.com/19705013/diff/54001/chrome/browser/pre...
File chrome/browser/prerender/prerender_tab_helper.cc (right):

https://chromiumcodereview.appspot.com/19705013/diff/54001/chrome/browser/pre...
chrome/browser/prerender/prerender_tab_helper.cc:169: base::Unretained(this))),
On 2013/07/27 01:09:48, Ilya Sherman wrote:
> Why is base::Unretained() safe here?

Deletion of the PasswordManager and this object happen in ~WebContentsImpl() and
this deletion and the callback invocation happen on the UI thread.

https://chromiumcodereview.appspot.com/19705013/diff/54001/chrome/browser/pre...
chrome/browser/prerender/prerender_tab_helper.cc:171: DCHECK(password_manager);
On 2013/07/27 01:09:48, Ilya Sherman wrote:
> nit: This seems redundant with the immediate use of password_manager on the
line
> below.

I just wanted to give a slightly better error message, though I suppose it
should be pretty obvious in either case. Removed.

https://chromiumcodereview.appspot.com/19705013/diff/54001/chrome/browser/pre...
File chrome/browser/prerender/prerender_tab_helper.h (right):

https://chromiumcodereview.appspot.com/19705013/diff/54001/chrome/browser/pre...
chrome/browser/prerender/prerender_tab_helper.h:110:
PasswordManager::PasswordSubmittedCallback password_callback_;
On 2013/07/27 01:09:48, Ilya Sherman wrote:
> Why does this need to be stored as a member variable?

Leftover from an earlier iteration. Removed.

https://chromiumcodereview.appspot.com/19705013/diff/54001/chrome/browser/ui/...
File chrome/browser/ui/browser_tab_contents.cc (right):

https://chromiumcodereview.appspot.com/19705013/diff/54001/chrome/browser/ui/...
chrome/browser/ui/browser_tab_contents.cc:146: web_contents,
PasswordManager::FromWebContents(web_contents));
On 2013/07/27 01:09:48, Ilya Sherman wrote:
> Hmm, why not just call PasswordManager::FromWebContents(web_contents) within
> prerender::PrerenderTabHelper::CreateForWebContents()?

We could, but then there would be a hidden dependency on the fact that we setup
the password manager before we setup the PrerenderTabHelper. I was trying to
make that dependency explicit this way.

https://chromiumcodereview.appspot.com/19705013/diff/54001/chrome/browser/ui/...
File chrome/browser/ui/sync/one_click_signin_helper.cc (right):

https://chromiumcodereview.appspot.com/19705013/diff/54001/chrome/browser/ui/...
chrome/browser/ui/sync/one_click_signin_helper.cc:565: base::Unretained(this)))
{
On 2013/07/27 01:09:48, Ilya Sherman wrote:
> Why is base::Unretained() safe here?

Yes, for the same reason as the PrerenderTabHelper.

https://chromiumcodereview.appspot.com/19705013/diff/54001/chrome/browser/ui/...
chrome/browser/ui/sync/one_click_signin_helper.cc:1006: // get lost if one of
the redirects requires context swap.
On 2013/07/27 01:09:48, Ilya Sherman wrote:
> nit: Is this comment still relevant?  If so, should it be moved into the
> password manager code?

I kept the gist of this in password manager code. I'll remove this now.

https://chromiumcodereview.appspot.com/19705013/diff/54001/chrome/browser/ui/...
File chrome/browser/ui/sync/one_click_signin_helper.h (right):

https://chromiumcodereview.appspot.com/19705013/diff/54001/chrome/browser/ui/...
chrome/browser/ui/sync/one_click_signin_helper.h:213: void
PasswordSubmitted(const content::PasswordForm& form);
On 2013/07/27 01:09:48, Ilya Sherman wrote:
> nit: Docs?

Done.

https://chromiumcodereview.appspot.com/19705013/diff/54001/chrome/browser/ui/...
chrome/browser/ui/sync/one_click_signin_helper.h:265:
PasswordManager::PasswordSubmittedCallback password_callback_;
On 2013/07/27 01:09:48, Ilya Sherman wrote:
> Why does this need to be stored as a class member here?

Same as PrerenderTabHelper. Removed.

https://chromiumcodereview.appspot.com/19705013/diff/54001/chrome/test/data/p...
File chrome/test/data/password/password_form.html (right):

https://chromiumcodereview.appspot.com/19705013/diff/54001/chrome/test/data/p...
chrome/test/data/password/password_form.html:6: <input type="submit"
id="input_submit_button" name="input_submit_button">
On 2013/07/27 01:09:48, Ilya Sherman wrote:
> nit: Do you really need ids for the <input>s?  Doesn't seem like they ought to
> matter.

I use GetElementById() in the test.

https://chromiumcodereview.appspot.com/19705013/diff/54001/components/autofil...
File components/autofill/content/renderer/password_autofill_agent.cc (right):

https://chromiumcodereview.appspot.com/19705013/diff/54001/components/autofil...
components/autofill/content/renderer/password_autofill_agent.cc:435: // into a
hidden field and then clear the password. (Issue 28910.)
On 2013/07/27 01:09:48, Ilya Sherman wrote:
> nit: Could you make the bug reference into an http://crbug.com hyperlink?

Done.

https://chromiumcodereview.appspot.com/19705013/diff/54001/components/autofil...
components/autofill/content/renderer/password_autofill_agent.cc:438:
provisional_forms_[frame].reset(content::CreatePasswordForm(form).release());
On 2013/07/27 01:09:48, Ilya Sherman wrote:
> Is there a guarantee that we will free elements from this map?  I'm a little
> worried that this can create a slow memory leak (though since form submissions
> are relatively rare, it's not a particularly bad memory leak).

I was actually thinking about this earlier, and I don't know for certain. I
think that there might be a leak if the user tries to log in to a subframe but
fails and then navigates the main page away. I can add code to delete this in
FrameClosing() to be extra safe.

https://chromiumcodereview.appspot.com/19705013/diff/54001/components/autofil...
components/autofill/content/renderer/password_autofill_agent.cc:447: // password
value so we get the users typed password, not the value that
On 2013/07/27 01:09:48, Ilya Sherman wrote:
> nit: "users" -> "user's"

Done.

https://chromiumcodereview.appspot.com/19705013/diff/54001/components/autofil...
components/autofill/content/renderer/password_autofill_agent.cc:451: if
(submitted_form.get()) {
On 2013/07/27 01:09:48, Ilya Sherman wrote:
> nit: No need for .get()

Done.

https://chromiumcodereview.appspot.com/19705013/diff/54001/components/autofil...
components/autofill/content/renderer/password_autofill_agent.cc:452: if
(provisional_forms_[frame].get() &&
On 2013/07/27 01:09:48, Ilya Sherman wrote:
> nit: Ditto

linked_ptr<> aren't convertible to bool, so this doesn't work.

https://chromiumcodereview.appspot.com/19705013/diff/54001/components/autofil...
components/autofill/content/renderer/password_autofill_agent.cc:456: }
On 2013/07/27 01:09:48, Ilya Sherman wrote:
> nit: Please leave a blank line after this one.

Done.

https://chromiumcodereview.appspot.com/19705013/diff/54001/components/autofil...
components/autofill/content/renderer/password_autofill_agent.cc:462:
*submitted_form.get()));
On 2013/07/27 01:09:48, Ilya Sherman wrote:
> nit: No need for .get()

Done.

https://chromiumcodereview.appspot.com/19705013/diff/54001/components/autofil...
components/autofill/content/renderer/password_autofill_agent.cc:477:
provisional_forms_[frame].get()) {
On 2013/07/27 01:09:48, Ilya Sherman wrote:
> nit: No need for .get()

Doesn't work.

https://chromiumcodereview.appspot.com/19705013/diff/54001/components/autofil...
components/autofill/content/renderer/password_autofill_agent.cc:480:
*provisional_forms_[frame].get()));
On 2013/07/27 01:09:48, Ilya Sherman wrote:
> nit: No need for .get()

Done.

https://chromiumcodereview.appspot.com/19705013/diff/54001/components/autofil...
components/autofill/content/renderer/password_autofill_agent.cc:480:
*provisional_forms_[frame].get()));
On 2013/07/27 01:09:48, Ilya Sherman wrote:
> nit: No need for .get()

Done.

https://chromiumcodereview.appspot.com/19705013/diff/54001/components/autofil...
components/autofill/content/renderer/password_autofill_agent.cc:482:
provisional_forms_.erase(frame);
On 2013/07/27 01:09:48, Ilya Sherman wrote:
> Can we clear the whole map if the |frame| is a top-level frame, either here or
> when a load commits?

Sure.

https://chromiumcodereview.appspot.com/19705013/diff/54001/components/autofil...
File components/autofill/content/renderer/password_autofill_agent.h (right):

https://chromiumcodereview.appspot.com/19705013/diff/54001/components/autofil...
components/autofill/content/renderer/password_autofill_agent.h:72:
linked_ptr<content::PasswordForm> > FrameToPasswordForm;
On 2013/07/27 01:09:48, Ilya Sherman wrote:
> nit: Please include "Map" in this type name.

Done.

https://chromiumcodereview.appspot.com/19705013/diff/54001/components/autofil...
components/autofill/content/renderer/password_autofill_agent.h:136:
FrameToPasswordForm provisional_forms_;
On 2013/07/27 01:09:48, Ilya Sherman wrote:
> Optional nit: Perhaps |provisionally_saved_forms_|?

Done.

[jln (very slow on Chromium), 2013-08-05 22:48:12]

*_messages.h lgtm

I didn't look at the submission_callbacks_ to check for mistakes there. Let me
know if you need me to take a look.

[Ilya Sherman, 2013-08-06 09:34:10]

LGTM if you're ok with converting base::Unretained()'s to WeakPtrs.  If not,
let's chat about that over IM or something :)

Thanks!

https://chromiumcodereview.appspot.com/19705013/diff/54001/chrome/browser/pre...
File chrome/browser/prerender/prerender_tab_helper.cc (right):

https://chromiumcodereview.appspot.com/19705013/diff/54001/chrome/browser/pre...
chrome/browser/prerender/prerender_tab_helper.cc:169: base::Unretained(this))),
On 2013/08/03 00:38:42, Garrett Casto wrote:
> On 2013/07/27 01:09:48, Ilya Sherman wrote:
> > Why is base::Unretained() safe here?
> 
> Deletion of the PasswordManager and this object happen in ~WebContentsImpl()
and
> this deletion and the callback invocation happen on the UI thread.

So, while you might be right, this strikes me as relying too much on
implementation details that might easily change underneath you.  I'd very much
prefer to just use a WeakPtr here and sleep that much better at night :)

https://chromiumcodereview.appspot.com/19705013/diff/54001/chrome/browser/ui/...
File chrome/browser/ui/browser_tab_contents.cc (right):

https://chromiumcodereview.appspot.com/19705013/diff/54001/chrome/browser/ui/...
chrome/browser/ui/browser_tab_contents.cc:146: web_contents,
PasswordManager::FromWebContents(web_contents));
On 2013/08/03 00:38:42, Garrett Casto wrote:
> On 2013/07/27 01:09:48, Ilya Sherman wrote:
> > Hmm, why not just call PasswordManager::FromWebContents(web_contents) within
> > prerender::PrerenderTabHelper::CreateForWebContents()?
> 
> We could, but then there would be a hidden dependency on the fact that we
setup
> the password manager before we setup the PrerenderTabHelper. I was trying to
> make that dependency explicit this way.

Okay, fair enough :)

https://chromiumcodereview.appspot.com/19705013/diff/75001/chrome/browser/pas...
File chrome/browser/password_manager/password_manager_browsertest.cc (right):

https://chromiumcodereview.appspot.com/19705013/diff/75001/chrome/browser/pas...
chrome/browser/password_manager/password_manager_browsertest.cc:210: #if
defined(OS_WIN) && defined(USE_ASH)
nit: IMO it's slightly better to mark the test as DISABLED_ on these platforms,
so that it doesn't always take time to run and succeed (which is actually a
somewhat nontrivial amount of time for a browser test).

[Roger Tawa OOO till Jul 10th, 2013-08-06 20:30:41]

one_click_signin_helper lgtm

[Garrett Casto, 2013-08-12 23:59:37]

+ben for browser_tab_contents as Avi is OOO.

[Avi (use Gerrit), 2013-08-13 04:44:54]

browser_tab_contents LGTM

Ben is even more OOO than I am. :)

[commit-bot: I haz the power, 2013-08-15 22:29:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gcasto@chromium.org/19705013/120001

[commit-bot: I haz the power, 2013-08-16 01:09:16]

Change committed as 217900