Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(1144)

Issue 6291011: Removes debugging code as cause of crash was found. (Closed)

Created:
9 years, 11 months ago by sky
Modified:
9 years, 7 months ago
CC:
chromium-reviews
Visibility:
Public.

Description

Removes debugging code as cause of crash was found. I'm TBRing as this is just a removing of unneeded CHECKs. BUG=34135 TEST=none TBR=ben@chromium.org Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=72396

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+8 lines, -27 lines) Patch
M chrome/browser/tabs/tab_strip_model.cc View 3 chunks +1 line, -10 lines 0 comments Download
M chrome/browser/tabs/tab_strip_model_order_controller.h View 1 chunk +1 line, -2 lines 0 comments Download
M chrome/browser/tabs/tab_strip_model_order_controller.cc View 2 chunks +6 lines, -15 lines 0 comments Download

Messages

Total messages: 2 (0 generated)
sky
9 years, 11 months ago (2011-01-24 17:27:44 UTC) #1
Ben Goodger (Google)
9 years, 11 months ago (2011-01-25 17:01:44 UTC) #2
OK

On Mon, Jan 24, 2011 at 9:27 AM, <sky@chromium.org> wrote:

> Reviewers: Ben Goodger,
>
> Description:
> Removes debugging code as cause of crash was found.
>
> I'm TBRing as this is just a removing of unneeded CHECKs.
>
> BUG=34135
> TEST=none
> TBR=ben@chromium.org
>
> Please review this at http://codereview.chromium.org/6291011/
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/src
>
> Affected files:
>  M chrome/browser/tabs/tab_strip_model.cc
>  M chrome/browser/tabs/tab_strip_model_order_controller.h
>  M chrome/browser/tabs/tab_strip_model_order_controller.cc
>
>
> Index: chrome/browser/tabs/tab_strip_model.cc
> diff --git a/chrome/browser/tabs/tab_strip_model.cc
> b/chrome/browser/tabs/tab_strip_model.cc
> index
>
d6e5bc1d7810f1acbe183eaa6b84033d46477b9f..b58eb9036af8167c7f3117ac0500a11b96077c60
> 100644
> --- a/chrome/browser/tabs/tab_strip_model.cc
> +++ b/chrome/browser/tabs/tab_strip_model.cc
> @@ -141,8 +141,6 @@ void TabStripModel::InsertTabContentsAt(int index,
>     }
>     // Anything opened by a link we deem to have an opener.
>     data->SetGroup(&selected_contents->controller());
> -    // TODO(sky): nuke when we figure out what is causing 34135.
> -    CHECK(data->opener != &(contents->controller()));
>   } else if ((add_types & ADD_INHERIT_OPENER) && selected_contents) {
>     if (foreground) {
>       // Forget any existing relationships, we don't want to make things
> too
> @@ -150,8 +148,6 @@ void TabStripModel::InsertTabContentsAt(int index,
>       ForgetAllOpeners();
>     }
>     data->opener = &selected_contents->controller();
> -    // TODO(sky): nuke when we figure out what is causing 34135.
> -    CHECK(data->opener != &(contents->controller()));
>   }
>
>   contents_data_.insert(contents_data_.begin() + index, data);
> @@ -213,12 +209,7 @@ TabContentsWrapper*
> TabStripModel::DetachTabContentsAt(int index) {
>   DCHECK(ContainsIndex(index));
>
>   TabContentsWrapper* removed_contents = GetContentsAt(index);
> -  // TODO(sky): nuke reason and old_data when we figure out what is
> causing
> -  // 34135.
> -  volatile int reason = 0;
> -  int next_selected_index =
> -      order_controller_->DetermineNewSelectedIndex(index, &reason);
> -  volatile TabContentsData old_data = *contents_data_.at(index);
> +  int next_selected_index =
> order_controller_->DetermineNewSelectedIndex(index);
>   delete contents_data_.at(index);
>   contents_data_.erase(contents_data_.begin() + index);
>   ForgetOpenersAndGroupsReferencing(&(removed_contents->controller()));
> Index: chrome/browser/tabs/tab_strip_model_order_controller.cc
> diff --git a/chrome/browser/tabs/tab_strip_model_order_controller.cc
> b/chrome/browser/tabs/tab_strip_model_order_controller.cc
> index
>
52730ef3886b8f48e56c2d938b3e1a6fc292e7c5..bd309fa4e176e871450bf924d6cb66d4a7879f9f
> 100644
> --- a/chrome/browser/tabs/tab_strip_model_order_controller.cc
> +++ b/chrome/browser/tabs/tab_strip_model_order_controller.cc
> @@ -64,7 +64,7 @@ int
> TabStripModelOrderController::DetermineInsertionIndexForAppending() {
>  }
>
>  int TabStripModelOrderController::DetermineNewSelectedIndex(
> -    int removing_index, volatile int* reason) const {
> +    int removing_index) const {
>   int tab_count = tabstrip_->count();
>   DCHECK(removing_index >= 0 && removing_index < tab_count);
>   NavigationController* parent_opener =
> @@ -75,43 +75,34 @@ int
> TabStripModelOrderController::DetermineNewSelectedIndex(
>   NavigationController* removed_controller =
>       &tabstrip_->GetTabContentsAt(removing_index)->controller();
>   // The parent opener should never be the same as the controller being
> removed.
> -  CHECK(parent_opener != removed_controller);
> +  DCHECK(parent_opener != removed_controller);
>   int index =
> tabstrip_->GetIndexOfNextTabContentsOpenedBy(removed_controller,
>                                                            removing_index,
>                                                            false);
> -  if (index != TabStripModel::kNoTab) {
> -    *reason = 1;
> +  if (index != TabStripModel::kNoTab)
>     return GetValidIndex(index, removing_index);
> -  }
>
>   if (parent_opener) {
>     // If the tab was in a group, shift selection to the next tab in the
> group.
>     int index = tabstrip_->GetIndexOfNextTabContentsOpenedBy(parent_opener,
>
>  removing_index,
>                                                              false);
> -    if (index != TabStripModel::kNoTab) {
> -      *reason = 2;
> +    if (index != TabStripModel::kNoTab)
>       return GetValidIndex(index, removing_index);
> -    }
>
>     // If we can't find a subsequent group member, just fall back to the
>     // parent_opener itself. Note that we use "group" here since opener is
>     // reset by select operations..
>     index = tabstrip_->GetIndexOfController(parent_opener);
> -    if (index != TabStripModel::kNoTab) {
> -      *reason = 3;
> +    if (index != TabStripModel::kNoTab)
>       return GetValidIndex(index, removing_index);
> -    }
>   }
>
>   // No opener set, fall through to the default handler...
>   int selected_index = tabstrip_->selected_index();
> -  if (selected_index >= (tab_count - 1)) {
> -    *reason = 4;
> +  if (selected_index >= (tab_count - 1))
>     return selected_index - 1;
> -  }
>
> -  *reason = 5;
>   return selected_index;
>  }
>
> Index: chrome/browser/tabs/tab_strip_model_order_controller.h
> diff --git a/chrome/browser/tabs/tab_strip_model_order_controller.h
> b/chrome/browser/tabs/tab_strip_model_order_controller.h
> index
>
2f3629128fd62c8b988a2297030bfe5541a5b6ef..5ecaa2c948b65896a4c3ca068f27bc78cb7771d3
> 100644
> --- a/chrome/browser/tabs/tab_strip_model_order_controller.h
> +++ b/chrome/browser/tabs/tab_strip_model_order_controller.h
> @@ -40,8 +40,7 @@ class TabStripModelOrderController : public
> TabStripModelObserver {
>   int DetermineInsertionIndexForAppending();
>
>   // Determine where to shift selection after a tab is closed.
> -  // TODO(sky): nuke reason when we figure out what is causing 34135.
> -  int DetermineNewSelectedIndex(int removed_index, volatile int* reason)
> const;
> +  int DetermineNewSelectedIndex(int removed_index) const;
>
>   // Overridden from TabStripModelObserver:
>   virtual void TabSelectedAt(TabContentsWrapper* old_contents,
>
>
>

Powered by Google App Engine
This is Rietveld 408576698