show notification on locale change

For ChromeOS: when starting session:
if locale was changed (as a result of sync or because user preference differs from login locale): show notification.

BUG=chromium-os:9164
TEST=Manual

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=71320

[Denis Lagno, 2010-12-24 16:03:36]

please take a look:
glen@ for locale-related behaviour
brettw@ for TabContents/InfoBar interaction
nkostylev@ overall check

[Glen Murphy, 2010-12-24 16:07:07]

As per the other thread, we should not be using infobars for this sort
of notification. With only a few exceptions, infobars are for
content-related notifications.


On Fri, Dec 24, 2010 at 8:03 AM,  <dilmah@chromium.org> wrote:
> Reviewers: brettw, Glen Murphy, Nikita Kostylev,
>
> Message:
> please take a look:
> glen@ for locale-related behaviour
> brettw@ for TabContents/InfoBar interaction
> nkostylev@ overall check
>
> Description:
> For ChromeOS: when starting session:
> if locale was changed (as a result of sync or because user preference
> differs
> from login locale): show infobar.
> Keep track of accepted and rejected locales in order to not to annoy user.
>
> BUG=chromium-os:9164
> TEST=Manual
>
> Please review this at http://codereview.chromium.org/5976005/
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/src
>
> Affected files:
>  M chrome/app/generated_resources.grd
>  M chrome/browser/chromeos/dom_ui/language_options_handler.cc
>  M chrome/browser/chromeos/login/login_utils.cc
>  M chrome/browser/profiles/profile.h
>  M chrome/browser/profiles/profile.cc
>  M chrome/browser/profiles/profile_impl.h
>  M chrome/browser/profiles/profile_impl.cc
>  M chrome/browser/tab_contents/infobar_delegate.h
>  A chrome/browser/tab_contents/locale_change_guard.h
>  A chrome/browser/tab_contents/locale_change_guard.cc
>  M chrome/browser/tab_contents/tab_contents.cc
>  M chrome/chrome_browser.gypi
>  M chrome/common/pref_names.h
>  M chrome/common/pref_names.cc
>  M chrome/tools/chromeactions.txt
>
>
>

[Glen Murphy, 2010-12-24 16:16:37]

Oh, I didn't notice that you said you'd add system-wide notifications
later. OK, but if you want to do so, please make it a P1 for the same
release - we shouldn't ship an infobar-based UI for this to any stable
channel.


On Fri, Dec 24, 2010 at 8:06 AM, Glen Murphy <glen@chromium.org> wrote:
> As per the other thread, we should not be using infobars for this sort
> of notification. With only a few exceptions, infobars are for
> content-related notifications.
>
>
> On Fri, Dec 24, 2010 at 8:03 AM,  <dilmah@chromium.org> wrote:
>> Reviewers: brettw, Glen Murphy, Nikita Kostylev,
>>
>> Message:
>> please take a look:
>> glen@ for locale-related behaviour
>> brettw@ for TabContents/InfoBar interaction
>> nkostylev@ overall check
>>
>> Description:
>> For ChromeOS: when starting session:
>> if locale was changed (as a result of sync or because user preference
>> differs
>> from login locale): show infobar.
>> Keep track of accepted and rejected locales in order to not to annoy user.
>>
>> BUG=chromium-os:9164
>> TEST=Manual
>>
>> Please review this at http://codereview.chromium.org/5976005/
>>
>> SVN Base: svn://svn.chromium.org/chrome/trunk/src
>>
>> Affected files:
>>  M chrome/app/generated_resources.grd
>>  M chrome/browser/chromeos/dom_ui/language_options_handler.cc
>>  M chrome/browser/chromeos/login/login_utils.cc
>>  M chrome/browser/profiles/profile.h
>>  M chrome/browser/profiles/profile.cc
>>  M chrome/browser/profiles/profile_impl.h
>>  M chrome/browser/profiles/profile_impl.cc
>>  M chrome/browser/tab_contents/infobar_delegate.h
>>  A chrome/browser/tab_contents/locale_change_guard.h
>>  A chrome/browser/tab_contents/locale_change_guard.cc
>>  M chrome/browser/tab_contents/tab_contents.cc
>>  M chrome/chrome_browser.gypi
>>  M chrome/common/pref_names.h
>>  M chrome/common/pref_names.cc
>>  M chrome/tools/chromeactions.txt
>>
>>
>>
>

[Denis Lagno, 2010-12-24 16:20:19]

Are system-wide notifications specific for ChromeOS?
Could you point me some place in chromium source code where those notifications
are used?

[Denis Lagno, 2011-01-10 19:12:24]

Need to proceed.  Nikita is on vacation.  Replacing by Anton.

[Glen Murphy, 2011-01-10 19:21:39]

+johnnyg,oshima, who can talk about how the notifications API can be used.

[whywhat, 2011-01-11 10:24:14]

Do we need to do what we consider wrong already? Do we need infobar even in dev
release?

http://codereview.chromium.org/5976005/diff/48001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/login_utils.cc (right):

http://codereview.chromium.org/5976005/diff/48001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/login_utils.cc:333: std::string pref_locale =
Could you use several variables with more specific names instead of reusing this
one for storing three separate preferences?

http://codereview.chromium.org/5976005/diff/48001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/login_utils.cc:349: } else if
(pref->GetString(prefs::kApplicationLocaleBackup) != cur_locale) {
I guess pref->GetString(prefs::kApplicationLocaleBackup) could be stored in a
variable and nested if could be eliminated.

http://codereview.chromium.org/5976005/diff/48001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile.h (right):

http://codereview.chromium.org/5976005/diff/48001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile.h:489: virtual void ChangeApplicationLocale(
Comment?
Shouldn't be pure virtual functions like the others?

http://codereview.chromium.org/5976005/diff/48001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/5976005/diff/48001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl.cc:1344: const std::string& locale, bool
dont_sync) {
Why not use positive name, smth like sync?
Though I don't see how sync is related to what it affects in code. It looks like
preserve_current_locale or use_override or smth like that.

http://codereview.chromium.org/5976005/diff/48001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl.cc:1352: PrefService* prefs[] = {
I don't think using array is justified but the small loop in the end. Using
descriptive names for storages would be better than using magic indices.

http://codereview.chromium.org/5976005/diff/48001/chrome/browser/tab_contents...
File chrome/browser/tab_contents/locale_change_guard.cc (right):

http://codereview.chromium.org/5976005/diff/48001/chrome/browser/tab_contents...
chrome/browser/tab_contents/locale_change_guard.cc:42: if (button == BUTTON_OK)
So OK is revert and Cancel is Awesome? Not vice versa?
Why do you add Awesome, btw? Is this approved by UI folks? :)

http://codereview.chromium.org/5976005/diff/48001/chrome/browser/tab_contents...
File chrome/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/5976005/diff/48001/chrome/browser/tab_contents...
chrome/browser/tab_contents/tab_contents.cc:838: #if defined(OS_CHROMEOS)
If we have ifdef here why do all this stuff with LocaleChangeGuardImpl, etc?
Couldn't it be defined as one class somewhere under chrome/browser/chromeos/?

http://codereview.chromium.org/5976005/diff/48001/chrome/browser/tab_contents...
chrome/browser/tab_contents/tab_contents.cc:1992: // We use indices instead of
iterators in case CloseWindow does something
Don't see how this could happen unless a) there's a reference to container from
window to child_windows_ it's in and b) the container is changed, but b) would
make current index invalid too...

http://codereview.chromium.org/5976005/diff/48001/chrome/browser/tab_contents...
chrome/browser/tab_contents/tab_contents.cc:1994: for (size_t i =
child_windows_.size(); i--;) {
Why not do a loop from 0 to size() or, if it was a serious bottleneck and
optimized to compare with 0, leave as it is?
From the first glance it seemed like such loop would go out of bounds on the
first iteration and also never access element with index 0. I'd avoid such
tricks.

http://codereview.chromium.org/5976005/diff/48001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/5976005/diff/48001/chrome/chrome_browser.gypi#...
chrome/chrome_browser.gypi:2521: 'browser/tab_contents/locale_change_guard.cc',
Unittest?

[Denis Lagno, 2011-01-11 11:45:12]

On 2011/01/11 10:24:14, whywhat wrote:
> Do we need to do what we consider wrong already? Do we need infobar even in
dev
> release?

I still don't see how it is wrong.
For this moment the only person voiced his opposition to usage of infobar for
that purpose is glen@

[Denis Lagno, 2011-01-11 18:54:21]

Glen, let's make sure that we are on the same board.  Do you suggest using
chrome/browser/notifications/notification_ui_manager.h API?

[John Gregg, 2011-01-11 18:55:17]

On 2011/01/11 18:54:21, Denis Lagno wrote:
> Glen, let's make sure that we are on the same board.  Do you suggest using
> chrome/browser/notifications/notification_ui_manager.h API?

Yes, that is the right API to use for desktop notifications as Glen was
suggesting.

[oshima, 2011-01-11 19:15:24]

On Tue, Jan 11, 2011 at 10:55 AM, <johnnyg@chromium.org> wrote:

> On 2011/01/11 18:54:21, Denis Lagno wrote:
>
>> Glen, let's make sure that we are on the same board.  Do you suggest using
>> chrome/browser/notifications/notification_ui_manager.h API?
>>
>
> Yes, that is the right API to use for desktop notifications as Glen was
> suggesting.
>
>
> http://codereview.chromium.org/5976005/
>

Yes. Please use non-sticky system notification. Just search
SystemNotification under chromeos
and you should be able to find some examples.

- oshima

[Denis Lagno, 2011-01-12 15:33:27]

please take a look again

http://codereview.chromium.org/5976005/diff/48001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/login_utils.cc (right):

http://codereview.chromium.org/5976005/diff/48001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/login_utils.cc:333: std::string pref_locale =
On 2011/01/11 10:24:14, whywhat wrote:
> Could you use several variables with more specific names instead of reusing
this
> one for storing three separate preferences?

Done.

http://codereview.chromium.org/5976005/diff/48001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/login_utils.cc:349: } else if
(pref->GetString(prefs::kApplicationLocaleBackup) != cur_locale) {
On 2011/01/11 10:24:14, whywhat wrote:
> I guess pref->GetString(prefs::kApplicationLocaleBackup) could be stored in a
> variable and nested if could be eliminated.

Done.

http://codereview.chromium.org/5976005/diff/48001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/5976005/diff/48001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl.cc:1344: const std::string& locale, bool
dont_sync) {
On 2011/01/11 10:24:14, whywhat wrote:
> Why not use positive name, smth like sync?
> Though I don't see how sync is related to what it affects in code. It looks
like
> preserve_current_locale or use_override or smth like that.

OK.  I switched back to "keep_local".
It was original naming just thought that words "local" and "locale" are not very
nice seen together.

http://codereview.chromium.org/5976005/diff/48001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl.cc:1352: PrefService* prefs[] = {
On 2011/01/11 10:24:14, whywhat wrote:
> I don't think using array is justified but the small loop in the end. Using
> descriptive names for storages would be better than using magic indices.

Done.

http://codereview.chromium.org/5976005/diff/48001/chrome/browser/tab_contents...
File chrome/browser/tab_contents/locale_change_guard.cc (right):

http://codereview.chromium.org/5976005/diff/48001/chrome/browser/tab_contents...
chrome/browser/tab_contents/locale_change_guard.cc:42: if (button == BUTTON_OK)
On 2011/01/11 10:24:14, whywhat wrote:
> So OK is revert and Cancel is Awesome? Not vice versa?
> Why do you add Awesome, btw? Is this approved by UI folks? :)

it was deliberate.
Not applicable anymore.

http://codereview.chromium.org/5976005/diff/48001/chrome/browser/tab_contents...
File chrome/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/5976005/diff/48001/chrome/browser/tab_contents...
chrome/browser/tab_contents/tab_contents.cc:838: #if defined(OS_CHROMEOS)
On 2011/01/11 10:24:14, whywhat wrote:
> If we have ifdef here why do all this stuff with LocaleChangeGuardImpl, etc?
> Couldn't it be defined as one class somewhere under chrome/browser/chromeos/?

my first impression was that this stuff is not very chromeOS specific (it is
applicable to any platform as long as it could happen to change locale
automatically).
But OK, I've moved it into chrome/browser/chromeos

As for why LocaleChangeGuardImpl -- it exists just to remove all those gory
details from header into .cc file so to make header locale_change_guard.h as
simple as possible.

http://codereview.chromium.org/5976005/diff/48001/chrome/browser/tab_contents...
chrome/browser/tab_contents/tab_contents.cc:1992: // We use indices instead of
iterators in case CloseWindow does something
On 2011/01/11 10:24:14, whywhat wrote:
> Don't see how this could happen unless a) there's a reference to container
from
> window to child_windows_ it's in and b) the container is changed, but b) would
> make current index invalid too...

Disclaimer: this is not my code so below is just a speculation.
My understanding is that it can change container but not arbitrarily: just
remove current child_windows[i].
That means that all indices below i are still valid and point to the same
objects.
But if you want to use iterator to traverse this container then you cannot just
use single iterator because it would be invalidated.  So while it is still
possible to implement this loop using iterators (because std::vector::erase
invalidates only iterators after point of erase) but this would not be
straightforward).

That is the reason why loop is from end to begin -- in order to leave remaining
indices valid.

http://codereview.chromium.org/5976005/diff/48001/chrome/browser/tab_contents...
chrome/browser/tab_contents/tab_contents.cc:1994: for (size_t i =
child_windows_.size(); i--;) {
On 2011/01/11 10:24:14, whywhat wrote:
> Why not do a loop from 0 to size() or, if it was a serious bottleneck and
> optimized to compare with 0, leave as it is?
> From the first glance it seemed like such loop would go out of bounds on the
> first iteration and also never access element with index 0. I'd avoid such
> tricks. 

why loop in reverse order -- speculated above.

as for style of loop, I suppose that this style:
for (size_t i = N; i--;)
is very common idiom familiar to every seasoned C/C++ developer.

At least it is much less ugly than original code with static_cast.

http://codereview.chromium.org/5976005/diff/48001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/5976005/diff/48001/chrome/chrome_browser.gypi#...
chrome/chrome_browser.gypi:2521: 'browser/tab_contents/locale_change_guard.cc',
On 2011/01/11 10:24:14, whywhat wrote:
> Unittest?

I'll try to do it in a separate CL.

[whywhat, 2011-01-12 16:33:16]

http://codereview.chromium.org/5976005/diff/60001/chrome/app/generated_resour...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/5976005/diff/60001/chrome/app/generated_resour...
chrome/app/generated_resources.grd:4338: <!-- Locale Changed Info Bar-->
Not an Info Bar anymore, right?

http://codereview.chromium.org/5976005/diff/60001/chrome/app/generated_resour...
chrome/app/generated_resources.grd:4341: Interface language was changed "<ph
name="FROM_LOCALE">$1</ph>" => "<ph name="TO_LOCALE">$2</ph>" based on your
preference.
Why not " from $1 to $2"? Or ":" after changed

http://codereview.chromium.org/5976005/diff/60001/chrome/app/generated_resour...
chrome/app/generated_resources.grd:4343: <message
name="IDS_LOCALE_CHANGE_REVERT_MESSAGE" desc="Message shown when locale was
changed based on profile content.">
Description is the same as above.

http://codereview.chromium.org/5976005/diff/60001/chrome/browser/chromeos/loc...
File chrome/browser/chromeos/locale_change_guard.cc (right):

http://codereview.chromium.org/5976005/diff/60001/chrome/browser/chromeos/loc...
chrome/browser/chromeos/locale_change_guard.cc:27: LocaleChangeGuardImpl()
Could you put explicit private: modifier here to avoid any confusion?

http://codereview.chromium.org/5976005/diff/60001/chrome/browser/chromeos/loc...
chrome/browser/chromeos/locale_change_guard.cc:46:
browser->ExecuteCommand(IDC_EXIT);
Do we say in the message we show to user that we'll close the browser? Can user
loose his data due to that?

http://codereview.chromium.org/5976005/diff/60001/chrome/browser/chromeos/loc...
chrome/browser/chromeos/locale_change_guard.cc:50: // We want notification to be
shown no more than once per session.
nit: no more than -> only?

http://codereview.chromium.org/5976005/diff/60001/chrome/browser/chromeos/loc...
chrome/browser/chromeos/locale_change_guard.cc:54: // first time may be invoked
for a dummy profile.
nit: move "first time" to the end of the sentence?

http://codereview.chromium.org/5976005/diff/60001/chrome/browser/chromeos/loc...
chrome/browser/chromeos/locale_change_guard.cc:68: return;
We return in multiple places, leaving the object partially initialized (i.e.
profile id and to_locale_ are set, but from_locale_ is empty). Is it as
designed?

http://codereview.chromium.org/5976005/diff/60001/chrome/browser/chromeos/loc...
chrome/browser/chromeos/locale_change_guard.cc:78:
"8c386938-1e3f-11e0-ac7b-18a90520e2e5" /* arbitrary unique id */,
Could you make it a named constant?

http://codereview.chromium.org/5976005/diff/60001/chrome/browser/chromeos/loc...
chrome/browser/chromeos/locale_change_guard.cc:90: true /* urgent */, false /*
non-sticky */);
Could you write it like this:
true,    // urgent
false);  // non-sticky
to avoid violating style guide's rule about two spaces between code and comments
when on the same line?

http://codereview.chromium.org/5976005/diff/60001/chrome/browser/chromeos/loc...
chrome/browser/chromeos/locale_change_guard.cc:93: ~LocaleChangeGuardImpl() {
There was recent discussion about dtor and ctor being put together if possible
so it's easier to find them. Could you move this dtor after ctor definition?

http://codereview.chromium.org/5976005/diff/60001/chrome/browser/chromeos/loc...
chrome/browser/chromeos/locale_change_guard.cc:93: ~LocaleChangeGuardImpl() {
When the dtor will be executed? As I understand, only when the process will
exit. Is it desirable?

http://codereview.chromium.org/5976005/diff/60001/chrome/browser/chromeos/loc...
chrome/browser/chromeos/locale_change_guard.cc:114: std::string from_locale_,
to_locale_;
Declaration of two variables on a single line is not allowed by style guide.

http://codereview.chromium.org/5976005/diff/60001/chrome/browser/chromeos/loc...
chrome/browser/chromeos/locale_change_guard.cc:132: void
LocaleChangeGuard::Check(TabContents* tab_contents) {
I think using impl class here hurts readability: the header now has one method,
but would have only 4 if merge both classes into one. And cc file is harder to
read since there's one additional level of complexity to keep in mind.

http://codereview.chromium.org/5976005/diff/60001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/login_utils.cc (right):

http://codereview.chromium.org/5976005/diff/60001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/login_utils.cc:333: std::string
pref_locale_override =
Add DCHECK for pref?

http://codereview.chromium.org/5976005/diff/60001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/login_utils.cc:347: // If locale is accepted then
we do not want to show LocaleChangeInfoBar.
Info Bar -> notification?

http://codereview.chromium.org/5976005/diff/60001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_impl.h (right):

http://codereview.chromium.org/5976005/diff/60001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl.h:133: const std::string& locale, bool
dont_sync);
dont_sync -> keep_local

http://codereview.chromium.org/5976005/diff/60001/chrome/browser/tab_contents...
File chrome/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/5976005/diff/60001/chrome/browser/tab_contents...
chrome/browser/tab_contents/tab_contents.cc:1994: for (size_t i =
child_windows_.size(); i--;) {
I guess I'd run i from 0 to child_windows_.size() and use
child_windows_[child_windows_.size() - 1 - i].

I'd also add "Indices are enumerated in reverse order to keep the current index
always valid." to the comment.

http://codereview.chromium.org/5976005/diff/60001/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

http://codereview.chromium.org/5976005/diff/60001/chrome/common/pref_names.cc...
chrome/common/pref_names.cc:57: // Used to determine whether we need to show
Locale Change InfoBar.
InfoBar -> notification

http://codereview.chromium.org/5976005/diff/60001/chrome/tools/chromeactions.txt
File chrome/tools/chromeactions.txt (right):

http://codereview.chromium.org/5976005/diff/60001/chrome/tools/chromeactions....
chrome/tools/chromeactions.txt:16:
0x6cb0582a680ffec6	AboutFlags_instant-autocomplete-immediately
This looks like some bad merge

http://codereview.chromium.org/5976005/diff/60001/chrome/tools/chromeactions....
chrome/tools/chromeactions.txt:178: 0xa0ebbbdb0eca8d89	ConflictBadge
Ditto

[Denis Lagno, 2011-01-12 20:27:34]

http://codereview.chromium.org/5976005/diff/60001/chrome/app/generated_resour...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/5976005/diff/60001/chrome/app/generated_resour...
chrome/app/generated_resources.grd:4338: <!-- Locale Changed Info Bar-->
On 2011/01/12 16:33:16, whywhat wrote:
> Not an Info Bar anymore, right?

Done.

http://codereview.chromium.org/5976005/diff/60001/chrome/app/generated_resour...
chrome/app/generated_resources.grd:4341: Interface language was changed "<ph
name="FROM_LOCALE">$1</ph>" => "<ph name="TO_LOCALE">$2</ph>" based on your
preference.
On 2011/01/12 16:33:16, whywhat wrote:
> Why not " from $1 to $2"? Or ":" after changed

Done.

http://codereview.chromium.org/5976005/diff/60001/chrome/app/generated_resour...
chrome/app/generated_resources.grd:4343: <message
name="IDS_LOCALE_CHANGE_REVERT_MESSAGE" desc="Message shown when locale was
changed based on profile content.">
On 2011/01/12 16:33:16, whywhat wrote:
> Description is the same as above.

Done.

http://codereview.chromium.org/5976005/diff/60001/chrome/browser/chromeos/loc...
File chrome/browser/chromeos/locale_change_guard.cc (right):

http://codereview.chromium.org/5976005/diff/60001/chrome/browser/chromeos/loc...
chrome/browser/chromeos/locale_change_guard.cc:27: LocaleChangeGuardImpl()
On 2011/01/12 16:33:16, whywhat wrote:
> Could you put explicit private: modifier here to avoid any confusion?

not applicable anymore

http://codereview.chromium.org/5976005/diff/60001/chrome/browser/chromeos/loc...
chrome/browser/chromeos/locale_change_guard.cc:46:
browser->ExecuteCommand(IDC_EXIT);
On 2011/01/12 16:33:16, whywhat wrote:
> Do we say in the message we show to user that we'll close the browser? Can
user
> loose his data due to that?

It happens if user presses on the link.
Link is currently labelled with following text:

"If you are unhappy about it: click here to close browser, sign out and revert."

http://codereview.chromium.org/5976005/diff/60001/chrome/browser/chromeos/loc...
chrome/browser/chromeos/locale_change_guard.cc:50: // We want notification to be
shown no more than once per session.
On 2011/01/12 16:33:16, whywhat wrote:
> nit: no more than -> only?

Not sure..  "Only once" sounds to me as exactly once.
But in common case this notification is shown zero times, because it is not
needed in common case.

http://codereview.chromium.org/5976005/diff/60001/chrome/browser/chromeos/loc...
chrome/browser/chromeos/locale_change_guard.cc:54: // first time may be invoked
for a dummy profile.
On 2011/01/12 16:33:16, whywhat wrote:
> nit: move "first time" to the end of the sentence?

Done.

http://codereview.chromium.org/5976005/diff/60001/chrome/browser/chromeos/loc...
chrome/browser/chromeos/locale_change_guard.cc:68: return;
On 2011/01/12 16:33:16, whywhat wrote:
> We return in multiple places, leaving the object partially initialized (i.e.
> profile id and to_locale_ are set, but from_locale_ is empty). Is it as
> designed?

if it exited early in this function then it means that no notification was shown
so those values will not be used in any way so we do not care if they are empty.

http://codereview.chromium.org/5976005/diff/60001/chrome/browser/chromeos/loc...
chrome/browser/chromeos/locale_change_guard.cc:78:
"8c386938-1e3f-11e0-ac7b-18a90520e2e5" /* arbitrary unique id */,
On 2011/01/12 16:33:16, whywhat wrote:
> Could you make it a named constant?

I moved it into dedicated named method.

http://codereview.chromium.org/5976005/diff/60001/chrome/browser/chromeos/loc...
chrome/browser/chromeos/locale_change_guard.cc:90: true /* urgent */, false /*
non-sticky */);
On 2011/01/12 16:33:16, whywhat wrote:
> Could you write it like this:
> true,    // urgent
> false);  // non-sticky
> to avoid violating style guide's rule about two spaces between code and
comments
> when on the same line?

Done.

http://codereview.chromium.org/5976005/diff/60001/chrome/browser/chromeos/loc...
chrome/browser/chromeos/locale_change_guard.cc:93: ~LocaleChangeGuardImpl() {
On 2011/01/12 16:33:16, whywhat wrote:
> There was recent discussion about dtor and ctor being put together if possible
> so it's easier to find them. Could you move this dtor after ctor definition?

dtor removed.  not applicable anymore.

http://codereview.chromium.org/5976005/diff/60001/chrome/browser/chromeos/loc...
chrome/browser/chromeos/locale_change_guard.cc:93: ~LocaleChangeGuardImpl() {
On 2011/01/12 16:33:16, whywhat wrote:
> When the dtor will be executed? As I understand, only when the process will
> exit. Is it desirable?

You're rignt.  That is bad.  Reworked that part.

http://codereview.chromium.org/5976005/diff/60001/chrome/browser/chromeos/loc...
chrome/browser/chromeos/locale_change_guard.cc:114: std::string from_locale_,
to_locale_;
On 2011/01/12 16:33:16, whywhat wrote:
> Declaration of two variables on a single line is not allowed by style guide.

Done.

http://codereview.chromium.org/5976005/diff/60001/chrome/browser/chromeos/loc...
chrome/browser/chromeos/locale_change_guard.cc:132: void
LocaleChangeGuard::Check(TabContents* tab_contents) {
On 2011/01/12 16:33:16, whywhat wrote:
> I think using impl class here hurts readability: the header now has one
method,
> but would have only 4 if merge both classes into one. And cc file is harder to
> read since there's one additional level of complexity to keep in mind.

ok, I moved it into single class.
But I cannot say that I like this: it adds more includes into headers, so it
increases compile time and dependencies.

(there was zero includes in original version of locale_change_guard.h; now there
are 5 complex inlcudes).

http://codereview.chromium.org/5976005/diff/60001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/login_utils.cc (right):

http://codereview.chromium.org/5976005/diff/60001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/login_utils.cc:333: std::string
pref_locale_override =
On 2011/01/12 16:33:16, whywhat wrote:
> Add DCHECK for pref?

Done.

http://codereview.chromium.org/5976005/diff/60001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/login_utils.cc:347: // If locale is accepted then
we do not want to show LocaleChangeInfoBar.
On 2011/01/12 16:33:16, whywhat wrote:
> Info Bar -> notification?

Done.

http://codereview.chromium.org/5976005/diff/60001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_impl.h (right):

http://codereview.chromium.org/5976005/diff/60001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl.h:133: const std::string& locale, bool
dont_sync);
On 2011/01/12 16:33:16, whywhat wrote:
> dont_sync -> keep_local

Done.

http://codereview.chromium.org/5976005/diff/60001/chrome/browser/tab_contents...
File chrome/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/5976005/diff/60001/chrome/browser/tab_contents...
chrome/browser/tab_contents/tab_contents.cc:1994: for (size_t i =
child_windows_.size(); i--;) {
On 2011/01/12 16:33:16, whywhat wrote:
> I guess I'd run i from 0 to child_windows_.size() and use
> child_windows_[child_windows_.size() - 1 - i].
> 
> I'd also add "Indices are enumerated in reverse order to keep the current
index
> always valid." to the comment.

I've changed the loop.
However I fear to add comment about unknown code.

http://codereview.chromium.org/5976005/diff/60001/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

http://codereview.chromium.org/5976005/diff/60001/chrome/common/pref_names.cc...
chrome/common/pref_names.cc:57: // Used to determine whether we need to show
Locale Change InfoBar.
On 2011/01/12 16:33:16, whywhat wrote:
> InfoBar -> notification

Done.

http://codereview.chromium.org/5976005/diff/60001/chrome/tools/chromeactions.txt
File chrome/tools/chromeactions.txt (right):

http://codereview.chromium.org/5976005/diff/60001/chrome/tools/chromeactions....
chrome/tools/chromeactions.txt:16:
0x6cb0582a680ffec6	AboutFlags_instant-autocomplete-immediately
On 2011/01/12 16:33:16, whywhat wrote:
> This looks like some bad merge

no, that looks like somebody forget to run extract_actions.py script for his
changes

[oshima, 2011-01-12 21:11:53]

LGTM on system notification bits.

http://codereview.chromium.org/5976005/diff/75001/chrome/browser/chromeos/loc...
File chrome/browser/chromeos/locale_change_guard.h (right):

http://codereview.chromium.org/5976005/diff/75001/chrome/browser/chromeos/loc...
chrome/browser/chromeos/locale_change_guard.h:20: class LocaleChangeGuard {
description of the class would be nice.

[whywhat, 2011-01-13 10:09:43]

LGTM!

http://codereview.chromium.org/5976005/diff/60001/chrome/browser/chromeos/loc...
File chrome/browser/chromeos/locale_change_guard.cc (right):

http://codereview.chromium.org/5976005/diff/60001/chrome/browser/chromeos/loc...
chrome/browser/chromeos/locale_change_guard.cc:132: void
LocaleChangeGuard::Check(TabContents* tab_contents) {
On 2011/01/12 20:27:34, Denis Lagno wrote:
> On 2011/01/12 16:33:16, whywhat wrote:
> > I think using impl class here hurts readability: the header now has one
> method,
> > but would have only 4 if merge both classes into one. And cc file is harder
to
> > read since there's one additional level of complexity to keep in mind.
> 
> ok, I moved it into single class.
> But I cannot say that I like this: it adds more includes into headers, so it
> increases compile time and dependencies.
> 
> (there was zero includes in original version of locale_change_guard.h; now
there
> are 5 complex inlcudes).

Still I don't believe this will create a bottleneck in Chrome's compilation
time. And until this is true and proved I'd prefer readability and simplicity to
artificial reduction of include dependencies.
Thanks for bearing with me! :)

http://codereview.chromium.org/5976005/diff/60001/chrome/browser/tab_contents...
File chrome/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/5976005/diff/60001/chrome/browser/tab_contents...
chrome/browser/tab_contents/tab_contents.cc:1994: for (size_t i =
child_windows_.size(); i--;) {
On 2011/01/12 20:27:34, Denis Lagno wrote:
> On 2011/01/12 16:33:16, whywhat wrote:
> > I guess I'd run i from 0 to child_windows_.size() and use
> > child_windows_[child_windows_.size() - 1 - i].
> > 
> > I'd also add "Indices are enumerated in reverse order to keep the current
> index
> > always valid." to the comment.
> 
> I've changed the loop.
> However I fear to add comment about unknown code.

Yet, you don't fear to change the code itself like you know it, right? :)
Maybe revert the change to leave this beast untouched?

[Denis Lagno, 2011-01-13 10:46:44]

> Yet, you don't fear to change the code itself like you know it, right? :)
> Maybe revert the change to leave this beast untouched?

rewriting code is like job of any compiler (compiler performs zillions of such
rewrites leaving semantics intact).  Because properties of code can be
localized, seen and proven.

Adding comment is more subtle, I could easily confuse reader.

http://codereview.chromium.org/5976005/diff/75001/chrome/browser/chromeos/loc...
File chrome/browser/chromeos/locale_change_guard.h (right):

http://codereview.chromium.org/5976005/diff/75001/chrome/browser/chromeos/loc...
chrome/browser/chromeos/locale_change_guard.h:20: class LocaleChangeGuard {
On 2011/01/12 21:11:53, oshima wrote:
> description of the class would be nice.

Done.

[brettw, 2011-01-13 18:28:07]

http://codereview.chromium.org/5976005/diff/81001/chrome/browser/chromeos/loc...
File chrome/browser/chromeos/locale_change_guard.cc (right):

http://codereview.chromium.org/5976005/diff/81001/chrome/browser/chromeos/loc...
chrome/browser/chromeos/locale_change_guard.cc:62: std::string cur_locale =
g_browser_process->GetApplicationLocale();
I find the functions here quite hard to follow since they're just a mass of
code. Typically we would group code into a related block with a blank line
separating them and some kind of comment to make it easier to jump to what you
care about.

In this function, you're setting some class state interspersed with a whole lot
of early returns. It's not obvious that there's any ordering to this and I'm
left wondering what the state of the object should be after one of the early
returns. Can this also be better organized and documented?

In this particular case, you're checking the cur_locale against empty, but under
what cases might it be empty? It looks like it's never supposed to be empty, and
the BrowserProcess even has a DHCECK for this. If something weird will happen in
an "impossible case" then maybe there should be an assertion. In this case, I
suspect the function might even "not do something crazy" if the locale is empty.

[Denis Lagno, 2011-01-17 14:39:41]

http://codereview.chromium.org/5976005/diff/81001/chrome/browser/chromeos/loc...
File chrome/browser/chromeos/locale_change_guard.cc (right):

http://codereview.chromium.org/5976005/diff/81001/chrome/browser/chromeos/loc...
chrome/browser/chromeos/locale_change_guard.cc:62: std::string cur_locale =
g_browser_process->GetApplicationLocale();
On 2011/01/13 18:28:07, brettw wrote:
> I find the functions here quite hard to follow since they're just a mass of
> code. Typically we would group code into a related block with a blank line
> separating them and some kind of comment to make it easier to jump to what you
> care about.
> 
> In this function, you're setting some class state interspersed with a whole
lot
> of early returns. It's not obvious that there's any ordering to this and I'm
> left wondering what the state of the object should be after one of the early
> returns. Can this also be better organized and documented?
> 
> In this particular case, you're checking the cur_locale against empty, but
under
> what cases might it be empty? It looks like it's never supposed to be empty,
and
> the BrowserProcess even has a DHCECK for this. If something weird will happen
in
> an "impossible case" then maybe there should be an assertion. In this case, I
> suspect the function might even "not do something crazy" if the locale is
empty.

I've created separate CL to attempt to address this:
http://codereview.chromium.org/6249009/