Implement the chrome.passwordsPrivate API.

Implement the chrome.passwordsPrivate API.

See https://docs.google.com/document/d/1vapDN-76XS4dLsNRFQP9NMnhDstPAC88vPuKblhgMok/edit?usp=sharing for details.

BUG=485227
TBR=dbeam@chromium.org
NOPRESUBMIT=true

Committed: https://crrev.com/03daaabaec0b367ca2a61ecebc33572ea79e4cf9
Cr-Commit-Position: refs/heads/master@{#334318}

[Kyle Horimoto, 2015-05-22 01:23:21]

khorimoto@chromium.org changed reviewers:
+ bauerb@chromium.org, dubroy@chromium.org, jhawkins@chromium.org,
orenb@chromium.org, rockot@chromium.org

[Kyle Horimoto, 2015-05-22 01:23:22]

orenb for:
Overall CL

bauerb for:
chrome/browser/android/password_ui_view_android.cc

rockot for:
chrome/browser/extensions/browser_context_keyed_service_factories.cc

dubroy for:
chrome/browser/ui/passwords/password_manager_presenter.cc
chrome/browser/ui/passwords/password_manager_presenter.h
chrome/browser/ui/passwords/password_ui_view.h

jhawkins for:
chrome/browser/ui/webui/options/password_manager_handler.cc
chrome/browser/ui/webui/options/password_manager_handler.h

[Bernhard Bauer, 2015-05-22 01:37:57]

Android trivially LGTM

[vabr (Chromium), 2015-05-22 09:00:24]

vabr@chromium.org changed reviewers:
+ vabr@chromium.org

[vabr (Chromium), 2015-05-22 09:00:25]

dubroy@ is no longer on the team.
chrome/browser/ui/passwords/* LGTM

Cheers,
Vaclav

https://codereview.chromium.org/1142693003/diff/180001/chrome/browser/ui/pass...
File chrome/browser/ui/passwords/password_ui_view.h (right):

https://codereview.chromium.org/1142693003/diff/180001/chrome/browser/ui/pass...
chrome/browser/ui/passwords/password_ui_view.h:29: // |index| the index of the
saved password entry.
nit: This comment line and some below seem to duplicate the above information.
Too verbose comments are hard to read, could you make it more succinct?

[James Hawkins, 2015-05-22 16:25:07]

lgtm

[Ken Rockot(use gerrit already), 2015-05-22 16:42:28]

BCKSF.cc lgtm

https://codereview.chromium.org/1142693003/diff/180001/chrome/browser/extensi...
File chrome/browser/extensions/api/passwords_private/passwords_private_api.h
(right):

https://codereview.chromium.org/1142693003/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/api/passwords_private/passwords_private_api.h:89:
class PlaintextPasswordObserver :
You can just declare the existence of the class here:

class PlaintextPasswordObserver;


and move the entire definition into the cc.

[Kyle Horimoto, 2015-05-22 17:52:54]

https://codereview.chromium.org/1142693003/diff/180001/chrome/browser/extensi...
File chrome/browser/extensions/api/passwords_private/passwords_private_api.h
(right):

https://codereview.chromium.org/1142693003/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/api/passwords_private/passwords_private_api.h:89:
class PlaintextPasswordObserver :
On 2015/05/22 16:42:28, Ken Rockot wrote:
> You can just declare the existence of the class here:
> 
> class PlaintextPasswordObserver;
> 
> 
> and move the entire definition into the cc.

Done.

https://codereview.chromium.org/1142693003/diff/180001/chrome/browser/ui/pass...
File chrome/browser/ui/passwords/password_ui_view.h (right):

https://codereview.chromium.org/1142693003/diff/180001/chrome/browser/ui/pass...
chrome/browser/ui/passwords/password_ui_view.h:29: // |index| the index of the
saved password entry.
On 2015/05/22 09:00:25, vabr (Chromium) wrote:
> nit: This comment line and some below seem to duplicate the above information.
> Too verbose comments are hard to read, could you make it more succinct?

Done.

[Kyle Horimoto, 2015-05-23 00:25:55]

khorimoto@chromium.org changed reviewers:
+ stevenjb@chromium.org
- orenb@chromium.org

[Kyle Horimoto, 2015-05-23 00:25:56]

Passing the overall review from orenb@ to stevenjb@ now that he'll be back
Monday.

[stevenjb, 2015-05-26 22:35:25]

I didn't do a complete review, because I fear we may need to change how
getPlaintextPassword is implemented, unless I missed something.

Unless we can guarantee that the callback passed to getPlaintextPassword will be
invoked, we may need to remove the callback from the API and add an event, e.g.
onShowPlaintextPassword, instead. Inspecting the code, I'm not convinced that we
can. LMKWYT.

(In the meanwhile, if you would like to separate out the other, simpler,
implementations I'll be happy to review them more thoroughly; they looked fine
at a glance. Otherwise I will wait until we resolve this concern before I do a
complete review).

https://codereview.chromium.org/1142693003/diff/220001/chrome/browser/extensi...
File chrome/browser/extensions/api/passwords_private/passwords_private_api.cc
(right):

https://codereview.chromium.org/1142693003/diff/220001/chrome/browser/extensi...
chrome/browser/extensions/api/passwords_private/passwords_private_api.cc:80:
PasswordsPrivateGetPlaintextPasswordFunction> fn_needing_reply,
This is in namespace extension, so all of the extensions:: can be removed which
will help with the formatting.

https://codereview.chromium.org/1142693003/diff/220001/chrome/browser/extensi...
chrome/browser/extensions/api/passwords_private/passwords_private_api.cc:150:
->GetTopLevelNativeWindow());
I don't think that we can guarantee that this will always call
OnPlaintextPasswordFetched(), which means this object will never get deleted and
will leak (and the JS callback will never be invoked).

https://codereview.chromium.org/1142693003/diff/220001/chrome/browser/extensi...
File
chrome/browser/extensions/api/passwords_private/passwords_private_delegate_impl.cc
(right):

https://codereview.chromium.org/1142693003/diff/220001/chrome/browser/extensi...
chrome/browser/extensions/api/passwords_private/passwords_private_delegate_impl.cc:118:
// If the URL/username pair does not exist in the map, do nothing.
I believe that this will definitely result in OnPlaintextPasswordFetched never
getting called.

https://codereview.chromium.org/1142693003/diff/220001/chrome/browser/extensi...
chrome/browser/extensions/api/passwords_private/passwords_private_delegate_impl.cc:128:
login_pair_to_index_map_[key]);
Inspecting this code briefly, it doesn't look like this guarantees that
ShowPassword() will get called.

[Kyle Horimoto, 2015-05-28 22:33:14]

khorimoto@chromium.org changed reviewers:
+ tbreisacher@chromium.org

[Kyle Horimoto, 2015-05-28 22:33:16]

Used an event instead of a callback. PTAL.

Also, +tbreisacher for the extern LG.

https://codereview.chromium.org/1142693003/diff/220001/chrome/browser/extensi...
File chrome/browser/extensions/api/passwords_private/passwords_private_api.cc
(right):

https://codereview.chromium.org/1142693003/diff/220001/chrome/browser/extensi...
chrome/browser/extensions/api/passwords_private/passwords_private_api.cc:80:
PasswordsPrivateGetPlaintextPasswordFunction> fn_needing_reply,
On 2015/05/26 22:35:25, stevenjb wrote:
> This is in namespace extension, so all of the extensions:: can be removed
which
> will help with the formatting.

N/A, since this has been removed.

https://codereview.chromium.org/1142693003/diff/220001/chrome/browser/extensi...
chrome/browser/extensions/api/passwords_private/passwords_private_api.cc:150:
->GetTopLevelNativeWindow());
On 2015/05/26 22:35:25, stevenjb wrote:
> I don't think that we can guarantee that this will always call
> OnPlaintextPasswordFetched(), which means this object will never get deleted
and
> will leak (and the JS callback will never be invoked).

Used an event instead.

https://codereview.chromium.org/1142693003/diff/220001/chrome/browser/extensi...
File
chrome/browser/extensions/api/passwords_private/passwords_private_delegate_impl.cc
(right):

https://codereview.chromium.org/1142693003/diff/220001/chrome/browser/extensi...
chrome/browser/extensions/api/passwords_private/passwords_private_delegate_impl.cc:118:
// If the URL/username pair does not exist in the map, do nothing.
On 2015/05/26 22:35:25, stevenjb wrote:
> I believe that this will definitely result in OnPlaintextPasswordFetched never
> getting called.

Used an event instead.

https://codereview.chromium.org/1142693003/diff/220001/chrome/browser/extensi...
chrome/browser/extensions/api/passwords_private/passwords_private_delegate_impl.cc:128:
login_pair_to_index_map_[key]);
On 2015/05/26 22:35:25, stevenjb wrote:
> Inspecting this code briefly, it doesn't look like this guarantees that
> ShowPassword() will get called.

Used an event instead.

[Kyle Horimoto, 2015-05-28 22:59:45]

khorimoto@chromium.org changed reviewers:
+ dbeam@chromium.org
- tbreisacher@chromium.org

[Kyle Horimoto, 2015-05-28 22:59:46]

+dbeam for generated externs file (tbreisacher is OOO).

[stevenjb, 2015-05-29 23:17:03]

Looking good, just a few questions.

https://codereview.chromium.org/1142693003/diff/240001/chrome/browser/extensi...
File
chrome/browser/extensions/api/passwords_private/passwords_private_delegate_impl.h
(right):

https://codereview.chromium.org/1142693003/diff/240001/chrome/browser/extensi...
chrome/browser/extensions/api/passwords_private/passwords_private_delegate_impl.h:58:
gfx::NativeWindow GetNativeWindow() const override;
Do we need this?

https://codereview.chromium.org/1142693003/diff/240001/chrome/browser/extensi...
File
chrome/browser/extensions/api/passwords_private/passwords_private_event_router.h
(right):

https://codereview.chromium.org/1142693003/diff/240001/chrome/browser/extensi...
chrome/browser/extensions/api/passwords_private/passwords_private_event_router.h:67:
// Whether this class is currently listening for changes to |TODO!!!!!|.
'TODO!!!!' ??

https://codereview.chromium.org/1142693003/diff/240001/chrome/browser/ui/pass...
File chrome/browser/ui/passwords/password_manager_presenter.cc (right):

https://codereview.chromium.org/1142693003/diff/240001/chrome/browser/ui/pass...
chrome/browser/ui/passwords/password_manager_presenter.cc:155:
base::UTF16ToUTF8(password_list_[index]->username_value),
Are these added parameters actually used anywhere?

https://codereview.chromium.org/1142693003/diff/240001/chrome/common/extensio...
File chrome/common/extensions/api/passwords_private.idl (right):

https://codereview.chromium.org/1142693003/diff/240001/chrome/common/extensio...
chrome/common/extensions/api/passwords_private.idl:69: static void
getPlaintextPassword(LoginPair loginPair);
nit: We should probably rename this 'requestPlaintextPassword'

[Kyle Horimoto, 2015-06-01 19:14:45]

+mpearson@chromium.org for:
extension_function_histogram_value.h
histograms.xml

https://codereview.chromium.org/1142693003/diff/240001/chrome/browser/extensi...
File
chrome/browser/extensions/api/passwords_private/passwords_private_delegate_impl.h
(right):

https://codereview.chromium.org/1142693003/diff/240001/chrome/browser/extensi...
chrome/browser/extensions/api/passwords_private/passwords_private_delegate_impl.h:58:
gfx::NativeWindow GetNativeWindow() const override;
On 2015/05/29 23:17:02, stevenjb wrote:
> Do we need this?

Yes - it's needed to show the reauthentication dialog on Windows when a
plaintext password is requested. The dialog needs to know which window to show
itself on top of. Plus, it's part of the pure abstract class from which this
class is deriving.

https://codereview.chromium.org/1142693003/diff/240001/chrome/browser/extensi...
File
chrome/browser/extensions/api/passwords_private/passwords_private_event_router.h
(right):

https://codereview.chromium.org/1142693003/diff/240001/chrome/browser/extensi...
chrome/browser/extensions/api/passwords_private/passwords_private_event_router.h:67:
// Whether this class is currently listening for changes to |TODO!!!!!|.
On 2015/05/29 23:17:02, stevenjb wrote:
> 'TODO!!!!' ??

Oops, good catch. Fixed.

https://codereview.chromium.org/1142693003/diff/240001/chrome/browser/ui/pass...
File chrome/browser/ui/passwords/password_manager_presenter.cc (right):

https://codereview.chromium.org/1142693003/diff/240001/chrome/browser/ui/pass...
chrome/browser/ui/passwords/password_manager_presenter.cc:155:
base::UTF16ToUTF8(password_list_[index]->username_value),
On 2015/05/29 23:17:02, stevenjb wrote:
> Are these added parameters actually used anywhere?

Yes. PasswordsPrivateDelegateImpl::ShowPassword() uses these values to notify
listeners of the origin URL and username. The onPlaintextPasswordRetrieved event
is one such observer, and it passes these values to the JS layer so that the JS
can determine which password to reveal in its UI.

https://codereview.chromium.org/1142693003/diff/240001/chrome/common/extensio...
File chrome/common/extensions/api/passwords_private.idl (right):

https://codereview.chromium.org/1142693003/diff/240001/chrome/common/extensio...
chrome/common/extensions/api/passwords_private.idl:69: static void
getPlaintextPassword(LoginPair loginPair);
On 2015/05/29 23:17:02, stevenjb wrote:
> nit: We should probably rename this 'requestPlaintextPassword'

Done.

[Kyle Horimoto, 2015-06-01 20:46:51]

khorimoto@chromium.org changed reviewers:
+ isherman@chromium.org

[Kyle Horimoto, 2015-06-01 20:46:51]

+isherman for:
extension_function_histogram_value.h
histograms.xml

[Ilya Sherman, 2015-06-01 20:47:50]

histograms lgtm

[stevenjb, 2015-06-01 21:31:58]

OK, apologies for the delays. I finally have my head wrapped around this CL.

There is some blurring of the view/model here that made this a bit challenging
to review. There is some cleanup that I think we should do, but we don't need to
do it here.

So, when all is said and done, I just have one more change request + a reminder
comment (I even filed an issue for it), then this will look good to me.

https://codereview.chromium.org/1142693003/diff/240001/chrome/browser/extensi...
File
chrome/browser/extensions/api/passwords_private/passwords_private_delegate_impl.h
(right):

https://codereview.chromium.org/1142693003/diff/240001/chrome/browser/extensi...
chrome/browser/extensions/api/passwords_private/passwords_private_delegate_impl.h:58:
gfx::NativeWindow GetNativeWindow() const override;
On 2015/06/01 19:14:44, Kyle Horimoto wrote:
> On 2015/05/29 23:17:02, stevenjb wrote:
> > Do we need this?
> 
> Yes - it's needed to show the reauthentication dialog on Windows when a
> plaintext password is requested. The dialog needs to know which window to show
> itself on top of. Plus, it's part of the pure abstract class from which this
> class is deriving.

See meta comment.

https://codereview.chromium.org/1142693003/diff/240001/chrome/browser/ui/pass...
File chrome/browser/ui/passwords/password_manager_presenter.cc (right):

https://codereview.chromium.org/1142693003/diff/240001/chrome/browser/ui/pass...
chrome/browser/ui/passwords/password_manager_presenter.cc:155:
base::UTF16ToUTF8(password_list_[index]->username_value),
On 2015/06/01 19:14:45, Kyle Horimoto wrote:
> On 2015/05/29 23:17:02, stevenjb wrote:
> > Are these added parameters actually used anywhere?
> 
> Yes. PasswordsPrivateDelegateImpl::ShowPassword() uses these values to notify
> listeners of the origin URL and username. The onPlaintextPasswordRetrieved
event
> is one such observer, and it passes these values to the JS layer so that the
JS
> can determine which password to reveal in its UI.

Acknowledged.

https://codereview.chromium.org/1142693003/diff/280001/chrome/browser/extensi...
File chrome/browser/extensions/api/passwords_private/passwords_private_api.cc
(right):

https://codereview.chromium.org/1142693003/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/api/passwords_private/passwords_private_api.cc:91:
->GetTopLevelNativeWindow());
It's kind of a minor thing, but I think I would prefer to pass the
RenderViewHost to the delegate rather than the NativeWindow.

https://codereview.chromium.org/1142693003/diff/280001/chrome/browser/extensi...
File
chrome/browser/extensions/api/passwords_private/passwords_private_delegate_impl.cc
(right):

https://codereview.chromium.org/1142693003/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/api/passwords_private/passwords_private_delegate_impl.cc:124:
native_window_ = native_window;
While this is fine, it's kind of confusing to follow (I thought there could be
an edge case if there are two settings pages open for the same browser context,
but not really since GetNativeWindow() is only going to be called from
PasswordManagerPresenter::RequestShowPassword).

This led me to wonder why GetNativeWindow() exists at all, why not just pass
native_window to RequestShowPassword? Possibly to avoid a hard dependency on
ui/gfx/native_widget_types.h in password_manager_presenter.h (which may not
exist on Android?), but given the dependencies that class already has, I think
it would be much cleaner.

I am not going to request that we clean this up first (although I would kind of
like to), but let's at least add a comment that assures us that native_window_
will only get called by GetNativeWindow() which will only be called by
RequestShowPassword.

Actually, I created an issue that we can refer to in the comment:
crbug.com/495290

[Kyle Horimoto, 2015-06-01 22:01:12]

https://codereview.chromium.org/1142693003/diff/280001/chrome/browser/extensi...
File chrome/browser/extensions/api/passwords_private/passwords_private_api.cc
(right):

https://codereview.chromium.org/1142693003/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/api/passwords_private/passwords_private_api.cc:91:
->GetTopLevelNativeWindow());
On 2015/06/01 21:31:57, stevenjb wrote:
> It's kind of a minor thing, but I think I would prefer to pass the
> RenderViewHost to the delegate rather than the NativeWindow.

Done.

[stevenjb, 2015-06-01 22:05:14]

lgtm

https://codereview.chromium.org/1142693003/diff/300001/chrome/browser/extensi...
File
chrome/browser/extensions/api/passwords_private/passwords_private_delegate_impl.cc
(right):

https://codereview.chromium.org/1142693003/diff/300001/chrome/browser/extensi...
chrome/browser/extensions/api/passwords_private/passwords_private_delegate_impl.cc:124:
// call use this value by calling GetNativeWindow().
Please at least reference crbug.com/495290 here, e.g.
// ... Note: This is safe because GetNativeWindow() will only be called
immediately from RequestShowPassword(). TODO(stevenjb): Pass this directly to
RequestShowPassword(), crbug.com/495290.

[Kyle Horimoto, 2015-06-01 22:34:52]

https://codereview.chromium.org/1142693003/diff/300001/chrome/browser/extensi...
File
chrome/browser/extensions/api/passwords_private/passwords_private_delegate_impl.cc
(right):

https://codereview.chromium.org/1142693003/diff/300001/chrome/browser/extensi...
chrome/browser/extensions/api/passwords_private/passwords_private_delegate_impl.cc:124:
// call use this value by calling GetNativeWindow().
On 2015/06/01 22:05:14, stevenjb wrote:
> Please at least reference crbug.com/495290 here, e.g.
> // ... Note: This is safe because GetNativeWindow() will only be called
> immediately from RequestShowPassword(). TODO(stevenjb): Pass this directly to
> RequestShowPassword(), crbug.com/495290.
> 

Done.

[Kyle Horimoto, 2015-06-01 22:36:50]

The CQ bit was checked by khorimoto@chromium.org

[Kyle Horimoto, 2015-06-01 22:36:51]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
rockot@chromium.org, vabr@chromium.org, jhawkins@chromium.org,
isherman@chromium.org, stevenjb@chromium.org
Link to the patchset: https://codereview.chromium.org/1142693003/#ps320001
(title: "stevenjb comment.")

[commit-bot: I haz the power, 2015-06-01 23:36:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1142693003/320001

[commit-bot: I haz the power, 2015-06-02 00:18:10]

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

[commit-bot: I haz the power, 2015-06-02 00:18:13]

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

[Kyle Horimoto, 2015-06-02 01:33:08]

The CQ bit was checked by khorimoto@chromium.org

[Kyle Horimoto, 2015-06-02 01:33:09]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
rockot@chromium.org, stevenjb@chromium.org, jhawkins@chromium.org,
vabr@chromium.org, isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/1142693003/#ps340001
(title: "Fixed MockPasswordUIView.")

[commit-bot: I haz the power, 2015-06-02 01:34:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1142693003/340001

[commit-bot: I haz the power, 2015-06-02 01:46:10]

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

[commit-bot: I haz the power, 2015-06-02 01:46:12]

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

[Kyle Horimoto, 2015-06-02 02:20:43]

The CQ bit was checked by khorimoto@chromium.org

[Kyle Horimoto, 2015-06-02 02:20:44]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
rockot@chromium.org, stevenjb@chromium.org, jhawkins@chromium.org,
vabr@chromium.org, isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/1142693003/#ps360001
(title: "Fix MockPasswordUIView again.")

[commit-bot: I haz the power, 2015-06-02 02:21:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1142693003/360001

[commit-bot: I haz the power, 2015-06-02 02:30:11]

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

[commit-bot: I haz the power, 2015-06-02 02:30:12]

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

[Kyle Horimoto, 2015-06-02 04:07:45]

The CQ bit was checked by khorimoto@chromium.org

[Kyle Horimoto, 2015-06-02 04:07:45]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
rockot@chromium.org, stevenjb@chromium.org, jhawkins@chromium.org,
vabr@chromium.org, isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/1142693003/#ps370001
(title: "Replace std::to_string with std:stringstream.")

[commit-bot: I haz the power, 2015-06-02 04:07:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1142693003/370001

[commit-bot: I haz the power, 2015-06-02 05:06:00]

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

[commit-bot: I haz the power, 2015-06-02 05:06:02]

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

[Kyle Horimoto, 2015-06-02 16:39:14]

The CQ bit was checked by khorimoto@chromium.org

[Kyle Horimoto, 2015-06-02 16:39:35]

The CQ bit was unchecked by khorimoto@chromium.org

[Kyle Horimoto, 2015-06-02 16:39:42]

The CQ bit was checked by khorimoto@chromium.org

[commit-bot: I haz the power, 2015-06-02 16:40:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1142693003/370001

[commit-bot: I haz the power, 2015-06-02 17:38:20]

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

[commit-bot: I haz the power, 2015-06-02 17:38:23]

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

[Kyle Horimoto, 2015-06-02 18:05:38]

The CQ bit was checked by khorimoto@chromium.org

[Kyle Horimoto, 2015-06-02 18:05:38]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
rockot@chromium.org, stevenjb@chromium.org, jhawkins@chromium.org,
vabr@chromium.org, isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/1142693003/#ps390001
(title: "Make PasswordsPrivateDelegateFactory::ServiceIsNULLWhileTesting()
return false.")

[commit-bot: I haz the power, 2015-06-02 18:07:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1142693003/390001

[Dan Beam, 2015-06-02 18:22:26]

externs lgtm

https://codereview.chromium.org/1142693003/diff/320001/third_party/closure_co...
File third_party/closure_compiler/externs/passwords_private.js (right):

https://codereview.chromium.org/1142693003/diff/320001/third_party/closure_co...
third_party/closure_compiler/externs/passwords_private.js:104: 
why so spacious?

[commit-bot: I haz the power, 2015-06-02 19:43:00]

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

[commit-bot: I haz the power, 2015-06-02 19:43:03]

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

[stevenjb, 2015-06-02 22:03:33]

https://codereview.chromium.org/1142693003/diff/390001/chrome/browser/extensi...
File
chrome/browser/extensions/api/passwords_private/passwords_private_event_router.cc
(right):

https://codereview.chromium.org/1142693003/diff/390001/chrome/browser/extensi...
chrome/browser/extensions/api/passwords_private/passwords_private_event_router.cc:46:
PasswordsPrivateDelegateFactory::GetForBrowserContext(context_);
This is the problem, the event router is causing PasswordsPrivateDelegateFactory
to be created from the destructor. (If you look at the call stack on a crash
when running locally you'll see the error occurs inside ~ProfileImpl; this is
trying to create a ProfileKeyedService while destroying Profile).

The solution is either to add a Shutdown override method and do this there (see
search_engines_private_event_router.cc for an example), or don't remove the
observer at all since this will get destroyed at the same time as
PasswordsPrivateDelegate (since they are both keyed services) and hopefully
isn't calling and observer methods in its destructor.

[Kyle Horimoto, 2015-06-04 21:43:24]

https://codereview.chromium.org/1142693003/diff/320001/third_party/closure_co...
File third_party/closure_compiler/externs/passwords_private.js (right):

https://codereview.chromium.org/1142693003/diff/320001/third_party/closure_co...
third_party/closure_compiler/externs/passwords_private.js:104: 
On 2015/06/02 18:22:25, Dan Beam wrote:
> why so spacious?

Not sure how this happened, but removed the extra spaces.

https://codereview.chromium.org/1142693003/diff/390001/chrome/browser/extensi...
File
chrome/browser/extensions/api/passwords_private/passwords_private_event_router.cc
(right):

https://codereview.chromium.org/1142693003/diff/390001/chrome/browser/extensi...
chrome/browser/extensions/api/passwords_private/passwords_private_event_router.cc:46:
PasswordsPrivateDelegateFactory::GetForBrowserContext(context_);
On 2015/06/02 22:03:32, stevenjb wrote:
> This is the problem, the event router is causing
PasswordsPrivateDelegateFactory
> to be created from the destructor. (If you look at the call stack on a crash
> when running locally you'll see the error occurs inside ~ProfileImpl; this is
> trying to create a ProfileKeyedService while destroying Profile).
> 
> The solution is either to add a Shutdown override method and do this there
(see
> search_engines_private_event_router.cc for an example), or don't remove the
> observer at all since this will get destroyed at the same time as
> PasswordsPrivateDelegate (since they are both keyed services) and hopefully
> isn't calling and observer methods in its destructor.

Done.

[Kyle Horimoto, 2015-06-04 21:43:32]

The CQ bit was checked by khorimoto@chromium.org

[Kyle Horimoto, 2015-06-04 21:43:32]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
rockot@chromium.org, stevenjb@chromium.org, dbeam@chromium.org,
jhawkins@chromium.org, vabr@chromium.org, isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/1142693003/#ps430001
(title: "Fixed spacing issue in externs.")

[commit-bot: I haz the power, 2015-06-04 21:45:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1142693003/430001

[commit-bot: I haz the power, 2015-06-04 22:00:58]

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

[commit-bot: I haz the power, 2015-06-04 22:01:00]

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

[Kyle Horimoto, 2015-06-04 23:56:13]

The CQ bit was checked by khorimoto@chromium.org

[Kyle Horimoto, 2015-06-04 23:56:14]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
rockot@chromium.org, stevenjb@chromium.org, dbeam@chromium.org,
jhawkins@chromium.org, vabr@chromium.org, isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/1142693003/#ps490001
(title: "Rebased.")

[commit-bot: I haz the power, 2015-06-04 23:56:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1142693003/490001

[commit-bot: I haz the power, 2015-06-05 00:18:32]

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

[commit-bot: I haz the power, 2015-06-05 00:18:34]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Kyle Horimoto, 2015-06-05 00:38:00]

The CQ bit was checked by khorimoto@chromium.org

[Kyle Horimoto, 2015-06-05 00:38:01]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
rockot@chromium.org, stevenjb@chromium.org, dbeam@chromium.org,
jhawkins@chromium.org, vabr@chromium.org, isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/1142693003/#ps510001
(title: "Fix test.")

[commit-bot: I haz the power, 2015-06-05 00:39:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1142693003/510001

[commit-bot: I haz the power, 2015-06-05 01:38:16]

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

[commit-bot: I haz the power, 2015-06-05 01:38:20]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Kyle Horimoto, 2015-06-09 02:46:43]

The CQ bit was checked by khorimoto@chromium.org

[Kyle Horimoto, 2015-06-09 02:46:45]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
rockot@chromium.org, stevenjb@chromium.org, dbeam@chromium.org,
jhawkins@chromium.org, vabr@chromium.org, isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/1142693003/#ps530001
(title: "Clean up in ShutDown().")

[commit-bot: I haz the power, 2015-06-09 02:47:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1142693003/530001

[commit-bot: I haz the power, 2015-06-09 03:42:33]

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

[commit-bot: I haz the power, 2015-06-09 03:42:36]

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

[stevenjb, 2015-06-09 19:25:54]

https://codereview.chromium.org/1142693003/diff/530001/chrome/browser/extensi...
File
chrome/browser/extensions/api/passwords_private/passwords_private_delegate_impl.cc
(right):

https://codereview.chromium.org/1142693003/diff/530001/chrome/browser/extensi...
chrome/browser/extensions/api/passwords_private/passwords_private_delegate_impl.cc:31:
password_manager_presenter_(this),
Since PasswordsPrivateDelegateImpl is a keyed service, it won't get destroyed
until the Profile is, which is after all UI needs to be shut down. This should
be a scoped_ptr<> and reset in Shutdown() (overridden from KeyedService).

[Kyle Horimoto, 2015-06-12 23:21:45]

https://codereview.chromium.org/1142693003/diff/530001/chrome/browser/extensi...
File
chrome/browser/extensions/api/passwords_private/passwords_private_delegate_impl.cc
(right):

https://codereview.chromium.org/1142693003/diff/530001/chrome/browser/extensi...
chrome/browser/extensions/api/passwords_private/passwords_private_delegate_impl.cc:31:
password_manager_presenter_(this),
On 2015/06/09 19:25:54, stevenjb wrote:
> Since PasswordsPrivateDelegateImpl is a keyed service, it won't get destroyed
> until the Profile is, which is after all UI needs to be shut down. This should
> be a scoped_ptr<> and reset in Shutdown() (overridden from KeyedService).

Done.

[Kyle Horimoto, 2015-06-12 23:21:59]

The CQ bit was checked by khorimoto@chromium.org

[Kyle Horimoto, 2015-06-12 23:22:00]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
rockot@chromium.org, stevenjb@chromium.org, dbeam@chromium.org,
jhawkins@chromium.org, vabr@chromium.org, isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/1142693003/#ps550001
(title: "stevenjb comment.")

[commit-bot: I haz the power, 2015-06-12 23:23:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1142693003/550001

[commit-bot: I haz the power, 2015-06-13 03:31:15]

Committed patchset #29 (id:550001)

[commit-bot: I haz the power, 2015-06-13 03:32:56]

Patchset 29 (id:??) landed as
https://crrev.com/03daaabaec0b367ca2a61ecebc33572ea79e4cf9
Cr-Commit-Position: refs/heads/master@{#334318}