show notification on locale change

chrome/browser/tab_contents/tab_contents.cc

Index: chrome/browser/tab_contents/tab_contents.cc
diff --git a/chrome/browser/tab_contents/tab_contents.cc b/chrome/browser/tab_contents/tab_contents.cc
index 563279e72dd89e51a4664748d8cdefdec71d1c97..514e36351d20df4a3d4d3103a1d17345929504ed 100644
--- a/chrome/browser/tab_contents/tab_contents.cc
+++ b/chrome/browser/tab_contents/tab_contents.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
@@ -70,6 +70,7 @@
 #include "chrome/browser/sessions/session_types.h"
 #include "chrome/browser/tab_contents/infobar_delegate.h"
 #include "chrome/browser/tab_contents/interstitial_page.h"
+#include "chrome/browser/tab_contents/locale_change_guard.h"
 #include "chrome/browser/tab_contents/navigation_entry.h"
 #include "chrome/browser/tab_contents/provisional_load_details.h"
 #include "chrome/browser/tab_contents/tab_contents_delegate.h"
@@ -834,6 +835,9 @@ void TabContents::DidBecomeSelected() {
 
   WebCacheManager::GetInstance()->ObserveActivity(GetRenderProcessHost()->id());
   last_selected_time_ = base::TimeTicks::Now();
+#if defined(OS_CHROMEOS)

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

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/?

[Denis Lagno, 2011/01/12 15:33:28]

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.

+  LocaleChangeGuard::Check(this);
+#endif
 }
 
 void TabContents::FadeForInstant(bool animate) {
@@ -1987,8 +1991,7 @@ void TabContents::CloseConstrainedWindows() {
   // Clear out any constrained windows since we are leaving this page entirely.
   // We use indices instead of iterators in case CloseWindow does something

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

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...

[Denis Lagno, 2011/01/12 15:33:28]

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.

   // that may invalidate an iterator.
-  int size = static_cast<int>(child_windows_.size());
-  for (int i = size - 1; i >= 0; --i) {
+  for (size_t i = child_windows_.size(); i--;) {

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

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. 

[Denis Lagno, 2011/01/12 15:33:28]

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.

     ConstrainedWindow* window = child_windows_[i];
     if (window) {
       window->CloseConstrainedWindow();