Revert 222124 "Makes BrowserStatusMonitor remove observers in de..."

Revert 222124 "Makes BrowserStatusMonitor remove observers in de..."

Failed unit tests on chromeos-asan:
  ChromeLauncherControllerTest.UnpinWithUninstall
  ChromeLauncherControllerTest.PendingInsertionOrder
  ChromeLauncherControllerTest.PersistLauncherItemPositions

http://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%20ASAN%20Tests%20%283%29/builds/9780

> Makes BrowserStatusMonitor remove observers in destructor
> 
> I'm seeing a crash where we have a dangling TabStripModelObserver,
> this is one possible place. At the same time I'm making the class only
> look for HOST_DESKTOP_TYPE_ASH browsers.
> 
> BUG=286162
> TEST=none
> R=skuhne@chromium.org
> 
> Review URL: https://chromiumcodereview.appspot.com/23455037

TBR=sky@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=222138

[please use gerrit instead, 2013-09-10 00:06:45]

Committed patchset #1 manually as r222138.

[sky, 2013-09-10 16:28:00]

GAH! Sorry.

Thanks for reverting.

On Mon, Sep 9, 2013 at 5:05 PM,  <rouslan@chromium.org> wrote:
> Reviewers: sky,
>
> Description:
> Revert 222124 "Makes BrowserStatusMonitor remove observers in de..."
>
> Failed unit tests on chromeos-asan:
>   ChromeLauncherControllerTest.UnpinWithUninstall
>   ChromeLauncherControllerTest.PendingInsertionOrder
>   ChromeLauncherControllerTest.PersistLauncherItemPositions
>
>
http://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%20...
>
>> Makes BrowserStatusMonitor remove observers in destructor
>
>
>> I'm seeing a crash where we have a dangling TabStripModelObserver,
>> this is one possible place. At the same time I'm making the class only
>> look for HOST_DESKTOP_TYPE_ASH browsers.
>
>
>> BUG=286162
>> TEST=none
>> R=skuhne@chromium.org
>
>
>> Review URL: https://chromiumcodereview.appspot.com/23455037
>
>
> TBR=sky@chromium.org
>
> Please review this at https://codereview.chromium.org/23538010/
>
> SVN Base: svn://svn.chromium.org/chrome/
>
> Affected files (+9, -23 lines):
>   M
>
trunk/src/chrome/browser/ui/ash/launcher/browser_shortcut_launcher_item_controller.cc
>   M     trunk/src/chrome/browser/ui/ash/launcher/browser_status_monitor.cc
>
>
> Index:
>
trunk/src/chrome/browser/ui/ash/launcher/browser_shortcut_launcher_item_controller.cc
> ===================================================================
> ---
>
trunk/src/chrome/browser/ui/ash/launcher/browser_shortcut_launcher_item_controller.cc
> (revision 222137)
> +++
>
trunk/src/chrome/browser/ui/ash/launcher/browser_shortcut_launcher_item_controller.cc
> (working copy)
> @@ -272,7 +272,7 @@
>        items.push_back(*it);
>    }
>    // If there are no suitable browsers we create a new one.
> -  if (items.empty()) {
> +  if (!items.size()) {
>      launcher_controller()->CreateNewWindow();
>      return;
>    }
> @@ -288,8 +288,8 @@
>      browser = items[0];
>    } else {
>      // If there is more then one suitable browser, we advance to the next
> if
> -    // |browser| is already active - or - check the last used browser if it
> can
> -    // be used.
> +    // |current_browser| is already active - or - check the last used
> browser
> +    // if it can be used.
>      std::vector<Browser*>::iterator i =
>          std::find(items.begin(), items.end(), browser);
>      if (i != items.end()) {
> @@ -311,7 +311,6 @@
>  bool
> BrowserShortcutLauncherItemController::IsBrowserRepresentedInBrowserList(
>      Browser* browser) {
>    return (browser &&
> -          browser->host_desktop_type() == chrome::HOST_DESKTOP_TYPE_ASH &&
>            (browser->is_type_tabbed() ||
>             !browser->is_app() ||
>             !browser->is_type_popup() ||
> Index: trunk/src/chrome/browser/ui/ash/launcher/browser_status_monitor.cc
> ===================================================================
> --- trunk/src/chrome/browser/ui/ash/launcher/browser_status_monitor.cc
> (revision 222137)
> +++ trunk/src/chrome/browser/ui/ash/launcher/browser_status_monitor.cc
> (working copy)
> @@ -27,8 +27,11 @@
>        observed_activation_clients_(this),
>        observed_root_windows_(this) {
>    DCHECK(launcher_controller_);
> -  BrowserList::AddObserver(this);
> +  BrowserList* browser_list =
> +      BrowserList::GetInstance(chrome::HOST_DESKTOP_TYPE_ASH);
>
> +  browser_list->AddObserver(this);
> +
>    // This check needs for win7_aura. Without this, all tests in
>    // ChromeLauncherController will fail in win7_aura.
>    if (ash::Shell::HasInstance()) {
> @@ -54,14 +57,10 @@
>    if (ash::Shell::HasInstance())
>      ash::Shell::GetInstance()->GetScreen()->RemoveObserver(this);
>
> -  BrowserList::RemoveObserver(this);
> -
>    BrowserList* browser_list =
>        BrowserList::GetInstance(chrome::HOST_DESKTOP_TYPE_ASH);
> -  for (BrowserList::const_iterator i = browser_list->begin();
> -       i != browser_list->end(); ++i) {
> -    OnBrowserRemoved(*i);
> -  }
> +
> +  browser_list->RemoveObserver(this);
>  }
>
>  void BrowserStatusMonitor::OnWindowActivated(aura::Window* gained_active,
> @@ -87,9 +86,6 @@
>  }
>
>  void BrowserStatusMonitor::OnBrowserAdded(Browser* browser) {
> -  if (browser->host_desktop_type() != chrome::HOST_DESKTOP_TYPE_ASH)
> -    return;
> -
>    browser->tab_strip_model()->AddObserver(this);
>
>    if (browser->is_type_popup() && browser->is_app()) {
> @@ -103,9 +99,6 @@
>  }
>
>  void BrowserStatusMonitor::OnBrowserRemoved(Browser* browser) {
> -  if (browser->host_desktop_type() != chrome::HOST_DESKTOP_TYPE_ASH)
> -    return;
> -
>    browser->tab_strip_model()->RemoveObserver(this);
>
>    if (browser_to_app_id_map_.find(browser) != browser_to_app_id_map_.end())
> {
> @@ -148,9 +141,6 @@
>    if (old_contents)
>      browser = chrome::FindBrowserWithWebContents(old_contents);
>
> -  if (browser && browser->host_desktop_type() !=
> chrome::HOST_DESKTOP_TYPE_ASH)
> -    return;
> -
>    // Update immediately on a tab change.
>    if (browser &&
>        (TabStripModel::kNoTab !=
> @@ -200,9 +190,6 @@
>          ChromeLauncherController::APP_STATE_INACTIVE;
>
>      Browser* browser = chrome::FindBrowserWithWebContents(contents);
> -    DCHECK(browser);
> -    if (browser->host_desktop_type() != chrome::HOST_DESKTOP_TYPE_ASH)
> -      return;
>      if (browser->tab_strip_model()->GetActiveWebContents() == contents) {
>        if (browser->window()->IsActive())
>          app_state = ChromeLauncherController::APP_STATE_WINDOW_ACTIVE;
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.