Use the same browser instance in the sync confirmation dialog.

Use the same browser instance in the sync confirmation dialog.

Before this CL, the sync confirmation dialog used the active browser
when dismissing the dialog. In some cases, the active browser is different
than the browser that actually presented the sync confirmation dialog
and this leads to unexpected behavior.

This CL fixes the browser instance used by the sync confirmation dialog
and ensures that the same instace is used throughout the lifetime of the
sync confirmation dialog.

TEST=See bug
BUG=694476
Review-Url: https://codereview.chromium.org/2771113003
Cr-Commit-Position: refs/heads/master@{#460345}
Committed: https://chromium.googlesource.com/chromium/src/+/fc05776e6cc541dc9f4b6652876be72549f41682

[msarda, 2017-03-24 17:12:12]

Description was changed from

==========
Use the right browser instance in the sync confirmation dialog

Before this CL, the sync confirmation dialog used the active browser
when dismissing the dialog. In some cases, the active browser is different
than the browser that actually presented the sync confirmation dialog
and this leads to unexpected bahavior.

This CL fixes the browser instance used by the sync confirmation dialog
and ensures that the same instace is used throughout the lifetime of the
sync confirmation dialog.

TEST=See bug
BUG=694476
==========

to

==========
Use the same browser instance in the sync confirmation dialog.

Before this CL, the sync confirmation dialog used the active browser
when dismissing the dialog. In some cases, the active browser is different
than the browser that actually presented the sync confirmation dialog
and this leads to unexpected bahavior.

This CL fixes the browser instance used by the sync confirmation dialog
and ensures that the same instace is used throughout the lifetime of the
sync confirmation dialog.

TEST=See bug
BUG=694476
==========

[msarda, 2017-03-24 17:12:20]

Description was changed from

==========
Use the same browser instance in the sync confirmation dialog.

Before this CL, the sync confirmation dialog used the active browser
when dismissing the dialog. In some cases, the active browser is different
than the browser that actually presented the sync confirmation dialog
and this leads to unexpected bahavior.

This CL fixes the browser instance used by the sync confirmation dialog
and ensures that the same instace is used throughout the lifetime of the
sync confirmation dialog.

TEST=See bug
BUG=694476
==========

to

==========
Use the same browser instance in the sync confirmation dialog.

Before this CL, the sync confirmation dialog used the active browser
when dismissing the dialog. In some cases, the active browser is different
than the browser that actually presented the sync confirmation dialog
and this leads to unexpected behavior.

This CL fixes the browser instance used by the sync confirmation dialog
and ensures that the same instace is used throughout the lifetime of the
sync confirmation dialog.

TEST=See bug
BUG=694476
==========

[msarda, 2017-03-24 17:23:29]

msarda@chromium.org changed reviewers:
+ anthonyvd@chromium.org, tommycli@chromium.org

[msarda, 2017-03-24 17:23:30]

Tommy,
I know you are not an owner of these files, however this is a WebUI related to
CL and I'd like your opinion if this is the right way to implement it. Thank
you.

Anthony,
Please take a look at this CL as you are the original author of this code. If it
looks good, then I plan to do a similar patch for the sign-in error dialog that
has the same bug.

Thank you.

[msarda, 2017-03-24 17:24:09]

Patchset #2 (id:20001) has been deleted

[msarda, 2017-03-24 17:24:16]

Patchset #1 (id:1) has been deleted

[msarda, 2017-03-28 08:15:00]

Friendly ping.

This is a really bad user experience and it would be great to get it fixed soon.
Ideally I'd like to merge it to M58.

I'm OOO next week so I'd like to get land this CL this week.

[anthonyvd, 2017-03-28 15:17:07]

lgtm. Thanks for the good change!

[msarda, 2017-03-28 15:21:45]

msarda@chromium.org changed reviewers:
+ pkasting@chromium.org

[msarda, 2017-03-28 15:21:46]

pkasting@chromium.org: Please review changes in
chrome/browser/ui/cocoa/profiles/
chrome/browser/ui/views/profiles/

[tommycli, 2017-03-28 16:24:47]

lgtm this is a good change... and i didn't see anything that looked funny to me.

https://codereview.chromium.org/2771113003/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/signin/sync_confirmation_handler.h (right):

https://codereview.chromium.org/2771113003/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/sync_confirmation_handler.h:70: // Weak reference
to the browser that shown the sync confirmation dialog.
*that showed

[Peter Kasting, 2017-03-28 22:39:37]

LGTM

[msarda, 2017-03-29 07:53:42]

The CQ bit was checked by msarda@chromium.org

[msarda, 2017-03-29 07:53:43]

The patchset sent to the CQ was uploaded after l-g-t-m from
tommycli@chromium.org, pkasting@chromium.org, anthonyvd@chromium.org
Link to the patchset: https://codereview.chromium.org/2771113003/#ps60001
(title: "Nit")

[commit-bot: I haz the power, 2017-03-29 07:53:53]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-29 07:56:18]

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

[commit-bot: I haz the power, 2017-03-29 07:56:19]

Try jobs failed on following builders:
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[msarda, 2017-03-29 08:24:01]

Thank you for the review.

https://codereview.chromium.org/2771113003/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/signin/sync_confirmation_handler.h (right):

https://codereview.chromium.org/2771113003/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/sync_confirmation_handler.h:70: // Weak reference
to the browser that shown the sync confirmation dialog.
On 2017/03/28 16:24:47, tommycli wrote:
> *that showed

Done.

[msarda, 2017-03-29 08:24:26]

The CQ bit was checked by msarda@chromium.org

[msarda, 2017-03-29 08:24:27]

The patchset sent to the CQ was uploaded after l-g-t-m from
tommycli@chromium.org, pkasting@chromium.org, anthonyvd@chromium.org
Link to the patchset: https://codereview.chromium.org/2771113003/#ps80001
(title: "Rebase")

[commit-bot: I haz the power, 2017-03-29 08:24:43]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-29 08:36:18]

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

[commit-bot: I haz the power, 2017-03-29 08:36:19]

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

[msarda, 2017-03-29 09:12:31]

The CQ bit was checked by msarda@chromium.org

[msarda, 2017-03-29 09:12:31]

The patchset sent to the CQ was uploaded after l-g-t-m from
tommycli@chromium.org, pkasting@chromium.org, anthonyvd@chromium.org
Link to the patchset: https://codereview.chromium.org/2771113003/#ps100001
(title: "Rebase & compile fix")

[commit-bot: I haz the power, 2017-03-29 09:12:48]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[msarda, 2017-03-29 09:19:40]

Patchset #3 (id:80001) has been deleted

[commit-bot: I haz the power, 2017-03-29 09:28:16]

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

[commit-bot: I haz the power, 2017-03-29 09:28:17]

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

[msarda, 2017-03-29 09:52:50]

The CQ bit was checked by msarda@chromium.org

[msarda, 2017-03-29 09:52:50]

The patchset sent to the CQ was uploaded after l-g-t-m from
tommycli@chromium.org, pkasting@chromium.org, anthonyvd@chromium.org
Link to the patchset: https://codereview.chromium.org/2771113003/#ps120001
(title: "Fix compile")

[commit-bot: I haz the power, 2017-03-29 09:53:24]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-29 10:49:28]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1490781170070940,
"parent_rev": "57c4423dceffc997459ae34640abdfc1f067cc2e", "commit_rev":
"fc05776e6cc541dc9f4b6652876be72549f41682"}

[commit-bot: I haz the power, 2017-03-29 10:50:10]

Description was changed from

==========
Use the same browser instance in the sync confirmation dialog.

Before this CL, the sync confirmation dialog used the active browser
when dismissing the dialog. In some cases, the active browser is different
than the browser that actually presented the sync confirmation dialog
and this leads to unexpected behavior.

This CL fixes the browser instance used by the sync confirmation dialog
and ensures that the same instace is used throughout the lifetime of the
sync confirmation dialog.

TEST=See bug
BUG=694476
==========

to

==========
Use the same browser instance in the sync confirmation dialog.

Before this CL, the sync confirmation dialog used the active browser
when dismissing the dialog. In some cases, the active browser is different
than the browser that actually presented the sync confirmation dialog
and this leads to unexpected behavior.

This CL fixes the browser instance used by the sync confirmation dialog
and ensures that the same instace is used throughout the lifetime of the
sync confirmation dialog.

TEST=See bug
BUG=694476

Review-Url: https://codereview.chromium.org/2771113003
Cr-Commit-Position: refs/heads/master@{#460345}
Committed:
https://chromium.googlesource.com/chromium/src/+/fc05776e6cc541dc9f4b6652876b...
==========

[commit-bot: I haz the power, 2017-03-29 10:50:11]

Committed patchset #4 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/fc05776e6cc541dc9f4b6652876b...

[Finnur, 2017-03-29 11:31:13]

A revert of this CL (patchset #4 id:120001) has been created in
https://codereview.chromium.org/2787453002/ by finnur@chromium.org.

The reason for reverting is: Reverting due to linker error on Linux ChromiumOS
Builder (dbg):

FAILED: chrome 
../../chrome/browser/ui/views/profiles/signin_view_controller_delegate_views.cc:190:
error: undefined reference to
'SyncConfirmationUI::InitializeMessageHandlerWithBrowser(Browser*)'
clang: error: linker command failed with exit code 1 (use -v to see invocation)

.