The login dialog was being opened with the main browser window as a parent in...

The login dialog was being opened with the main browser window as a parent in every case, but in some cases that was inappropriate because the UI event that triggered it could be in the Options dialog or the Bookmarks Manager. So in those cases now the sync login dialog opens with respect to the appropriate window as a parent.

BUG=19776
TEST=See bug for repro steps.

[RandyPosynick, 2009-10-16 22:00:51]

First patch.

[tim (not reviewing), 2009-10-29 21:58:44]

It looks pretty good based on what I suggested, but I think I was wrong :( I
don't like the service having to hold the extra variable around.. it just feels
like something the wizard should handle (which is what Chris suggested
originally, I think). Please see my suggestion on line 43 of
sync_setup_wizard.h..

http://codereview.chromium.org/276072/diff/1/9
File chrome/browser/sync/profile_sync_service.h (right):

http://codereview.chromium.org/276072/diff/1/9#newcode104
Line 104: virtual void EnableForUser(gfx::NativeWindow parent_window);
can we call it setup_dialog_parent_window, and add a comment above like 
"Enables/disables sync for the user. This will cause a setup dialog to be shown,
and you may use |setup_dialog_parent_window| to parent this dialog from a window
of your choosing."

http://codereview.chromium.org/276072/diff/1/9#newcode151
Line 151: gfx::NativeWindow GetWizardParent() const {
please add virtual

http://codereview.chromium.org/276072/diff/1/9#newcode154
Line 154: void ShowLoginDialog(gfx::NativeWindow parent_window);
the name of the variable is fine here, but please add a comment explaining it.

http://codereview.chromium.org/276072/diff/1/8
File chrome/browser/sync/sync_setup_wizard.h (right):

http://codereview.chromium.org/276072/diff/1/8#newcode43
Line 43: class WizardParentDelegate {
I know I suggested going this route with the delegate, but..
upon seeing the extra variable in ProfileSyncService.. I just don't like my idea
anymore.  I think it would be preferable to have a SetParent method on this
wizard class.  It would do nothing if it was already open (already had a flow_).
Otherwise, it would just store it, and use it when it needed to open itself. 
Passing in NULL would be fine, and in this case the code that calls
browser->ShowHTMLDialog would stay as before and lookup the browser window. The
PSS would call SetParent from EnableForUser and ShowLoginDialog.

WDYT? (Really sorry for changing my mind.. it took your work here to make me
realize this)

http://codereview.chromium.org/276072/diff/1/8#newcode44
Line 44: gfx::NativeWindow GetWizardParent();
<stale comment after prev suggestion>
hmm.. you don't have a default implementation, yet you call this function, don't
you get an unresolved external error? or perhaps you didn't include the cc file
with the impl in the CL?  Also, this isn't declared virtual, so I don't see how
the ProfileSyncService::GetWizardParent would ever be invoked.
</stale comment>

http://codereview.chromium.org/276072/diff/1/4
File chrome/browser/views/bookmark_manager_view.cc (right):

http://codereview.chromium.org/276072/diff/1/4#newcode863
Line 863: sync_service_->EnableForUser(GetWindow()->GetNativeWindow());
what happens if you close the BookmarkManager? Does this close the wizard? 
Maybe we should make IsDialogModal return true?

[tim (not reviewing), 2009-10-30 01:51:24]

Darn. Neither of these approaches work perfectly -- EnableForUser is
asynchronous.  If the options dialog is closed before the wizard opens, the
parent pointer could be garbage. Either we need a bidirectional relationship
between the wizard and the requester/parent, or we figure out a way to mediate
via ProfileSyncService.  I need to think about it a bit more.