ash: Use mojo LocaleNotificationController interface.

ash: Use mojo LocaleNotificationController interface.

This removes a direct coupling between chrome/ and ash/common/. This
also does cleanup on the ash side where the controller was the only
member of an observer interface, which was superfluous.

BUG=656783
Committed: https://crrev.com/cf3c339b9bc6b61a00d354d3857c7de9a7dd65aa
Cr-Commit-Position: refs/heads/master@{#426040}

[Elliot Glaysher, 2016-10-13 20:57:20]

The CQ bit was checked by erg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-13 20:58:13]

Dry run: 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, 2016-10-13 21:03:23]

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

[commit-bot: I haz the power, 2016-10-13 21:03:24]

Dry run: Exceeded global retry quota

[Elliot Glaysher, 2016-10-13 21:06:40]

The CQ bit was checked by erg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-13 21:07:30]

Dry run: 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, 2016-10-13 21:19:32]

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

[commit-bot: I haz the power, 2016-10-13 21:19:33]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Elliot Glaysher, 2016-10-13 21:19:47]

The CQ bit was checked by erg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-13 21:20:03]

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

[Elliot Glaysher, 2016-10-13 22:15:28]

The CQ bit was checked by erg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-13 22:15:56]

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

[Elliot Glaysher, 2016-10-13 22:42:07]

erg@chromium.org changed reviewers:
+ msw@chromium.org

[Elliot Glaysher, 2016-10-13 22:42:07]

erg@chromium.org changed reviewers:
+ msw@chromium.org

[commit-bot: I haz the power, 2016-10-13 23:13:46]

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

[commit-bot: I haz the power, 2016-10-13 23:13:47]

Dry run: This issue passed the CQ dry run.

[msw, 2016-10-14 01:46:29]

Nice, just minor comments.

https://codereview.chromium.org/2420753002/diff/60001/ash/common/system/local...
File ash/common/system/locale/locale_notification_controller.h (right):

https://codereview.chromium.org/2420753002/diff/60001/ash/common/system/local...
ash/common/system/locale/locale_notification_controller.h:23: void
BindRequest(mojom::LocaleNotificationControllerRequest request);
nit: comment (copy from ShelfController or similar):
// Binds the mojom::LocaleNotificationController interface request to this
object.

https://codereview.chromium.org/2420753002/diff/60001/ash/common/system/local...
ash/common/system/locale/locale_notification_controller.h:27: void
NotifyLocaleChanged(
Keep the name as OnLocaleChanged (it's a handler that does something on a locale
change, not a function that notifies observers of locale changes).

https://codereview.chromium.org/2420753002/diff/60001/ash/common/wm_shell.h
File ash/common/wm_shell.h (right):

https://codereview.chromium.org/2420753002/diff/60001/ash/common/wm_shell.h#n...
ash/common/wm_shell.h:17: #include
"ash/common/system/locale/locale_notification_controller.h"
nit: use a fwd decl instead

https://codereview.chromium.org/2420753002/diff/60001/ash/common/wm_shell.h#n...
ash/common/wm_shell.h:477: std::unique_ptr<LocaleNotificationController>
locale_notification_controller_;
nit: abc order (after KeyboardUI)

https://codereview.chromium.org/2420753002/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/locale_change_guard.cc (left):

https://codereview.chromium.org/2420753002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/locale_change_guard.cc:222:
IDS_OPTIONS_SETTINGS_SECTION_TITLE_LANGUAGE);
Remove these three string resources now that they're not referenced.

https://codereview.chromium.org/2420753002/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/locale_change_guard.cc (right):

https://codereview.chromium.org/2420753002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/locale_change_guard.cc:56:
ConnectToLocaleNotificationController();
nit: call in Check() instead, or just inline the connect code there?

[Elliot Glaysher, 2016-10-14 17:42:00]

The CQ bit was checked by erg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-14 17:42:33]

Dry run: 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, 2016-10-14 19:07:27]

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

[commit-bot: I haz the power, 2016-10-14 19:07:28]

Dry run: This issue passed the CQ dry run.

[Elliot Glaysher, 2016-10-17 20:14:55]

The CQ bit was checked by erg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-17 20:15:21]

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

[Elliot Glaysher, 2016-10-17 20:30:45]

Description was changed from

==========
ash: Use mojo LocaleNotificationController interface.

This removes a direct coupling between chrome/ and ash/common/. This
also does cleanup on the ash side where the controller was the only
member of an observer interface, which was superfluous.

BUG=
==========

to

==========
ash: Use mojo LocaleNotificationController interface.

This removes a direct coupling between chrome/ and ash/common/. This
also does cleanup on the ash side where the controller was the only
member of an observer interface, which was superfluous.

BUG=656783
==========

[Elliot Glaysher, 2016-10-17 21:00:46]

ptal

I couldn't get this dialog to reproduce naturally on a device without the patch;
the feature appears to be broken and that's being tracked in issue 656200. After
applying the patch, I hard coded it so it didn't do any check that would reject
showing the dialog and confirmed that in non-mash builds it worked.

(In mash builds, it looks like showing notifications crashes mash deep inside
skia. Yay.)

[commit-bot: I haz the power, 2016-10-17 21:59:55]

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

[commit-bot: I haz the power, 2016-10-17 21:59:56]

Dry run: This issue passed the CQ dry run.

[msw, 2016-10-17 22:19:34]

lgtm with nits. I would appreciated it if you replied to my CL comments; using
the one-click 'Done' button makes this very easy for you, and seeing replies
makes reviewing easier for me.

https://codereview.chromium.org/2420753002/diff/100001/ash/public/interfaces/...
File ash/public/interfaces/locale.mojom (right):

https://codereview.chromium.org/2420753002/diff/100001/ash/public/interfaces/...
ash/public/interfaces/locale.mojom:7: enum LocaleNotificationResult {
nit: comment, mojoms should be thoroughly documented.
(you'll need OWNERS review, and they'll say the same)

https://codereview.chromium.org/2420753002/diff/100001/ash/public/interfaces/...
ash/public/interfaces/locale.mojom:14: OnLocaleChanged(string current, string
from, string to)
ditto nit: comment.

https://codereview.chromium.org/2420753002/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/locale_change_guard.cc (right):

https://codereview.chromium.org/2420753002/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/locale_change_guard.cc:22: #include
"chrome/grit/generated_resources.h"
nit: remove

https://codereview.chromium.org/2420753002/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/locale_change_guard.cc:68: auto connection =
content::ServiceManagerConnection::GetForProcess();
nit: James asked me to avoid auto for these types in another CL... I guess you
should probably do the same.

[Elliot Glaysher, 2016-10-17 22:35:03]

The CQ bit was checked by erg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-17 22:35:56]

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

[Elliot Glaysher, 2016-10-17 22:37:16]

erg@chromium.org changed reviewers:
+ tsepez@chromium.org

[Elliot Glaysher, 2016-10-17 22:37:17]

+tsepez for mojom changes

https://codereview.chromium.org/2420753002/diff/100001/ash/public/interfaces/...
File ash/public/interfaces/locale.mojom (right):

https://codereview.chromium.org/2420753002/diff/100001/ash/public/interfaces/...
ash/public/interfaces/locale.mojom:7: enum LocaleNotificationResult {
On 2016/10/17 22:19:34, msw wrote:
> nit: comment, mojoms should be thoroughly documented.
> (you'll need OWNERS review, and they'll say the same)

Done.

https://codereview.chromium.org/2420753002/diff/100001/ash/public/interfaces/...
ash/public/interfaces/locale.mojom:14: OnLocaleChanged(string current, string
from, string to)
On 2016/10/17 22:19:34, msw wrote:
> ditto nit: comment.

Done.

https://codereview.chromium.org/2420753002/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/locale_change_guard.cc (right):

https://codereview.chromium.org/2420753002/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/locale_change_guard.cc:22: #include
"chrome/grit/generated_resources.h"
On 2016/10/17 22:19:34, msw wrote:
> nit: remove

Done.

https://codereview.chromium.org/2420753002/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/locale_change_guard.cc:68: auto connection =
content::ServiceManagerConnection::GetForProcess();
On 2016/10/17 22:19:34, msw wrote:
> nit: James asked me to avoid auto for these types in another CL... I guess you
> should probably do the same.

Done.

[Tom Sepez, 2016-10-17 23:48:05]

mojom LGTM

https://codereview.chromium.org/2420753002/diff/120001/ash/public/interfaces/...
File ash/public/interfaces/locale.mojom (right):

https://codereview.chromium.org/2420753002/diff/120001/ash/public/interfaces/...
ash/public/interfaces/locale.mojom:15: // Displays a notification in ash
prompting the user on whether to accept a
nit: s/on//.

[Elliot Glaysher, 2016-10-17 23:56:54]

The CQ bit was checked by erg@chromium.org

[Elliot Glaysher, 2016-10-17 23:56:54]

The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org,
tsepez@chromium.org
Link to the patchset: https://codereview.chromium.org/2420753002/#ps140001
(title: "nit")

[Elliot Glaysher, 2016-10-17 23:57:02]

https://codereview.chromium.org/2420753002/diff/120001/ash/public/interfaces/...
File ash/public/interfaces/locale.mojom (right):

https://codereview.chromium.org/2420753002/diff/120001/ash/public/interfaces/...
ash/public/interfaces/locale.mojom:15: // Displays a notification in ash
prompting the user on whether to accept a
On 2016/10/17 23:48:04, Tom Sepez (ooo Oct 18-19) wrote:
> nit: s/on//.

Done.

[commit-bot: I haz the power, 2016-10-17 23:57:48]

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, 2016-10-18 00:06:59]

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

[commit-bot: I haz the power, 2016-10-18 00:07:00]

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

[Elliot Glaysher, 2016-10-18 00:08:18]

erg@chromium.org changed reviewers:
+ sky@chromium.org

[Elliot Glaysher, 2016-10-18 00:08:19]

+sky for OWNERS stamp

[sky, 2016-10-18 16:58:03]

Assuming the answer to my question is the delegate is only called once LGTM

https://codereview.chromium.org/2420753002/diff/140001/ash/common/system/loca...
File ash/common/system/locale/locale_notification_controller.cc (right):

https://codereview.chromium.org/2420753002/diff/140001/ash/common/system/loca...
ash/common/system/locale/locale_notification_controller.cc:58:
callback_.Run(ash::mojom::LocaleNotificationResult::ACCEPT);
I'm not familiar with the code in this class. Are you guaranteed that only one
of Close, Click or ButtonClick is called once?

[Elliot Glaysher, 2016-10-18 19:09:09]

The CQ bit was checked by erg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-18 19:09:26]

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

[Elliot Glaysher, 2016-10-18 19:41:57]

https://codereview.chromium.org/2420753002/diff/140001/ash/common/system/loca...
File ash/common/system/locale/locale_notification_controller.cc (right):

https://codereview.chromium.org/2420753002/diff/140001/ash/common/system/loca...
ash/common/system/locale/locale_notification_controller.cc:58:
callback_.Run(ash::mojom::LocaleNotificationResult::ACCEPT);
On 2016/10/18 16:58:03, sky wrote:
> I'm not familiar with the code in this class. Are you guaranteed that only one
> of Close, Click or ButtonClick is called once?

Done. (Added check in dtor, too.)

[commit-bot: I haz the power, 2016-10-18 20:35:28]

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

[commit-bot: I haz the power, 2016-10-18 20:35:30]

Dry run: This issue passed the CQ dry run.

[Elliot Glaysher, 2016-10-18 20:37:42]

The CQ bit was checked by erg@chromium.org

[Elliot Glaysher, 2016-10-18 20:37:43]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
msw@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2420753002/#ps160001
(title: "Uniquify callbacks")

[commit-bot: I haz the power, 2016-10-18 20:38:12]

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, 2016-10-18 20:49:25]

Description was changed from

==========
ash: Use mojo LocaleNotificationController interface.

This removes a direct coupling between chrome/ and ash/common/. This
also does cleanup on the ash side where the controller was the only
member of an observer interface, which was superfluous.

BUG=656783
==========

to

==========
ash: Use mojo LocaleNotificationController interface.

This removes a direct coupling between chrome/ and ash/common/. This
also does cleanup on the ash side where the controller was the only
member of an observer interface, which was superfluous.

BUG=656783
==========

[commit-bot: I haz the power, 2016-10-18 20:49:26]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-10-21 13:02:54]

Description was changed from

==========
ash: Use mojo LocaleNotificationController interface.

This removes a direct coupling between chrome/ and ash/common/. This
also does cleanup on the ash side where the controller was the only
member of an observer interface, which was superfluous.

BUG=656783
==========

to

==========
ash: Use mojo LocaleNotificationController interface.

This removes a direct coupling between chrome/ and ash/common/. This
also does cleanup on the ash side where the controller was the only
member of an observer interface, which was superfluous.

BUG=656783

Committed: https://crrev.com/cf3c339b9bc6b61a00d354d3857c7de9a7dd65aa
Cr-Commit-Position: refs/heads/master@{#426040}
==========

[commit-bot: I haz the power, 2016-10-21 13:02:55]

Patchset 9 (id:??) landed as
https://crrev.com/cf3c339b9bc6b61a00d354d3857c7de9a7dd65aa
Cr-Commit-Position: refs/heads/master@{#426040}