Issue 10391177: Add TrayLocale for locale change notifications.

Issue 10391177: Add TrayLocale for locale change notifications. (Closed)

Created:
8 years, 7 months ago by stevenjb

Modified:
8 years, 7 months ago

Reviewers:
sadrul

CC:
chromium-reviews, stevenjb+watch_chromium.org, sadrul, nkostylev+watch_chromium.org, davemoore+watch_chromium.org, ben+watch_chromium.org

Base URL:
svn://svn.chromium.org/chrome/trunk/src

Visibility:
Public.

More Reviews

Description

Add TrayLocale for locale change notifications. TBR=ben for ash/ string changes BUG=124726 TEST=On a device wuth ash-notify enabled in about:flags, change the locale and re-login. Notification should appear. Revert should revert the change after re-login. If ignored, the notification should appear again after re-login. Closing the notification should clear it. Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=137700

Patch Set 1 #

Total comments: 6

Patch Set 2 : Move LocaleObserver to its own file, create locale subdir #

Patch Set 3 : Rebase w/ icons in tot #

Created: 8 years, 7 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+242 lines, -4 lines)			Patch
M	ash/ash.gyp	View	1	1 chunk	+2 lines, -0 lines	0 comments	Download
M	ash/ash_strings.grd	View	1	1 chunk	+6 lines, -0 lines	0 comments	Download
A	ash/system/locale/locale_observer.h	View	1	1 chunk	+33 lines, -0 lines	0 comments	Download
A	ash/system/locale/tray_locale.h	View	1	1 chunk	+52 lines, -0 lines	0 comments	Download
A	ash/system/locale/tray_locale.cc	View	1	1 chunk	+115 lines, -0 lines	0 comments	Download
M	ash/system/tray/system_tray.h	View	1	3 chunks	+5 lines, -0 lines	0 comments	Download
M	ash/system/tray/system_tray.cc	View	1	5 chunks	+5 lines, -0 lines	0 comments	Download
M	chrome/browser/chromeos/locale_change_guard.h	View	1	3 chunks	+7 lines, -2 lines	0 comments	Download
M	chrome/browser/chromeos/locale_change_guard.cc	View		5 chunks	+17 lines, -2 lines	0 comments	Download

Messages

Total messages: 3 (0 generated)

Expand Messages | Collapse Messages

sadrul

LGTM with some nits. http://codereview.chromium.org/10391177/diff/1/ash/system/tray_locale.cc File ash/system/tray_locale.cc (right): http://codereview.chromium.org/10391177/diff/1/ash/system/tray_locale.cc#newcode32 ash/system/tray_locale.cc:32: -blank line http://codereview.chromium.org/10391177/diff/1/ash/system/tray_locale.cc#newcode59 ash/system/tray_locale.cc:59: // ...

8 years, 7 months ago (2012-05-17 03:15:43 UTC) #2

stevenjb

8 years, 7 months ago (2012-05-17 16:58:57 UTC) #3

Apologies, forgot to upload the rebase separately.

http://codereview.chromium.org/10391177/diff/1/ash/system/tray_locale.cc
File ash/system/tray_locale.cc (right):

http://codereview.chromium.org/10391177/diff/1/ash/system/tray_locale.cc#newc...
ash/system/tray_locale.cc:32: 
On 2012/05/17 03:15:44, sadrul wrote:
> -blank line

Done.

http://codereview.chromium.org/10391177/diff/1/ash/system/tray_locale.cc#newc...
ash/system/tray_locale.cc:59: // Overridden from TrayNotificationView:
On 2012/05/17 03:15:44, sadrul wrote:
> '.' at the end (or ':' at the end in line 53)

Done.

http://codereview.chromium.org/10391177/diff/1/ash/system/tray_locale.h
File ash/system/tray_locale.h (right):

http://codereview.chromium.org/10391177/diff/1/ash/system/tray_locale.h#newco...
ash/system/tray_locale.h:15: class ASH_EXPORT LocaleChangeDelegate {
On 2012/05/17 03:15:44, sadrul wrote:
> LocaleChangeDelegate and LocaleObserver could move into different files.
Fair point. 
Created a locale/ subdirectory
Moved this and locale_observer.h to system/locale
Made Delegate a member of LocaleObserver.

Expand Messages | Collapse Messages