Skip to content

Commit

Permalink
Revert "Delete tab pulsing feature."
Browse files Browse the repository at this point in the history
This reverts commit 1dd9000.

Reason for revert: breaks linux-jumbo-rel (failure log: https://logs.chromium.org/logs/chromium/buildbucket/cr-buildbucket.appspot.com/8924308573690053872/+/steps/compile/0/stdout)

Original change's description:
> Delete tab pulsing feature.
> 
> Previously, when hovering tab context menu items "Close tabs to the
> right" and "Close other tabs", the affected tabs would pulse. This patch
> deletes that feature. This caused SimpleMenuModel::CommandIdHighlighted
> and transitively MenuModel::HighlightChangedTo to become unused. This
> also caused TabStrip::animation_container_ to become effectively unused,
> since pulsing was the only animation that needed to be coordinated
> between tabs.
> 
> Bug: 921243
> Change-Id: Iae4ef39d979fa5f5a96ffc465275f25385f5949f
> Reviewed-on: https://chromium-review.googlesource.com/c/1407958
> Commit-Queue: Bret Sepulveda <bsep@chromium.org>
> Reviewed-by: Drew Wilson <atwilson@chromium.org>
> Reviewed-by: Scott Violet <sky@chromium.org>
> Reviewed-by: Avi Drissman <avi@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#622657}

TBR=avi@chromium.org,sky@chromium.org,atwilson@chromium.org,bsep@chromium.org

Change-Id: I96285c3f0b2601cc59c98b5d8abde97fd04afdf4
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 921243
Reviewed-on: https://chromium-review.googlesource.com/c/1409886
Reviewed-by: Kevin Marshall <kmarshall@chromium.org>
Commit-Queue: Kevin Marshall <kmarshall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#622668}
  • Loading branch information
Kevin Marshall authored and Commit Bot committed Jan 15, 2019
1 parent 62ba9b1 commit c75dbc5
Show file tree
Hide file tree
Showing 25 changed files with 264 additions and 77 deletions.
11 changes: 11 additions & 0 deletions chrome/browser/status_icons/status_icon_menu_model.cc
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,12 @@ struct StatusIconMenuModel::ItemState {
gfx::Image icon;
};

////////////////////////////////////////////////////////////////////////////////
// StatusIconMenuModel::Delegate, public:

void StatusIconMenuModel::Delegate::CommandIdHighlighted(int command_id) {
}

////////////////////////////////////////////////////////////////////////////////
// StatusIconMenuModel, public:

Expand Down Expand Up @@ -163,6 +169,11 @@ void StatusIconMenuModel::NotifyMenuStateChanged() {
////////////////////////////////////////////////////////////////////////////////
// StatusIconMenuModel, private:

void StatusIconMenuModel::CommandIdHighlighted(int command_id) {
if (delegate_)
delegate_->CommandIdHighlighted(command_id);
}

void StatusIconMenuModel::ExecuteCommand(int command_id, int event_flags) {
if (delegate_)
delegate_->ExecuteCommand(command_id, event_flags);
Expand Down
5 changes: 5 additions & 0 deletions chrome/browser/status_icons/status_icon_menu_model.h
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,10 @@ class StatusIconMenuModel
public:
class Delegate {
public:
// Notifies the delegate that the item with the specified command id was
// visually highlighted within the menu.
virtual void CommandIdHighlighted(int command_id);

// Performs the action associates with the specified command id.
// The passed |event_flags| are the flags from the event which issued this
// command and they can be examined to find modifier keys.
Expand Down Expand Up @@ -93,6 +97,7 @@ class StatusIconMenuModel

private:
// Overridden from ui::SimpleMenuModel::Delegate:
void CommandIdHighlighted(int command_id) override;
void ExecuteCommand(int command_id, int event_flags) override;

struct ItemState;
Expand Down
42 changes: 21 additions & 21 deletions chrome/browser/ui/tabs/tab_strip_model.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1200,6 +1200,27 @@ void TabStripModel::ExecuteContextMenuCommand(int context_index,
}
}

std::vector<int> TabStripModel::GetIndicesClosedByCommand(
int index,
ContextMenuCommand id) const {
DCHECK(ContainsIndex(index));
DCHECK(id == CommandCloseTabsToRight || id == CommandCloseOtherTabs);
bool is_selected = IsTabSelected(index);
int last_unclosed_tab = -1;
if (id == CommandCloseTabsToRight) {
last_unclosed_tab =
is_selected ? selection_model_.selected_indices().back() : index;
}

// NOTE: callers expect the vector to be sorted in descending order.
std::vector<int> indices;
for (int i = count() - 1; i > last_unclosed_tab; --i) {
if (i != index && !IsTabPinned(i) && (!is_selected || !IsTabSelected(i)))
indices.push_back(i);
}
return indices;
}

bool TabStripModel::WillContextMenuMute(int index) {
std::vector<int> indices = GetIndicesForCommand(index);
return !chrome::AreAllTabsMuted(*this, indices);
Expand Down Expand Up @@ -1316,27 +1337,6 @@ std::vector<int> TabStripModel::GetIndicesForCommand(int index) const {
return selection_model_.selected_indices();
}

std::vector<int> TabStripModel::GetIndicesClosedByCommand(
int index,
ContextMenuCommand id) const {
DCHECK(ContainsIndex(index));
DCHECK(id == CommandCloseTabsToRight || id == CommandCloseOtherTabs);
bool is_selected = IsTabSelected(index);
int last_unclosed_tab = -1;
if (id == CommandCloseTabsToRight) {
last_unclosed_tab =
is_selected ? selection_model_.selected_indices().back() : index;
}

// NOTE: callers expect the vector to be sorted in descending order.
std::vector<int> indices;
for (int i = count() - 1; i > last_unclosed_tab; --i) {
if (i != index && !IsTabPinned(i) && (!is_selected || !IsTabSelected(i)))
indices.push_back(i);
}
return indices;
}

bool TabStripModel::IsNewTabAtEndOfTabStrip(WebContents* contents) const {
const GURL& url = contents->GetLastCommittedURL();
return url.SchemeIs(content::kChromeUIScheme) &&
Expand Down
15 changes: 6 additions & 9 deletions chrome/browser/ui/tabs/tab_strip_model.h
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@
#include <vector>

#include "base/containers/span.h"
#include "base/gtest_prod_util.h"
#include "base/macros.h"
#include "base/memory/weak_ptr.h"
#include "base/observer_list.h"
Expand Down Expand Up @@ -379,6 +378,12 @@ class TabStripModel {
void ExecuteContextMenuCommand(int context_index,
ContextMenuCommand command_id);

// Returns a vector of indices of the tabs that will close when executing the
// command |id| for the tab at |index|. The returned indices are sorted in
// descending order.
std::vector<int> GetIndicesClosedByCommand(int index,
ContextMenuCommand id) const;

// Returns true if 'CommandToggleTabAudioMuted' will mute. |index| is the
// index supplied to |ExecuteContextMenuCommand|.
bool WillContextMenuMute(int index);
Expand Down Expand Up @@ -419,8 +424,6 @@ class TabStripModel {
bool ShouldResetOpenerOnActiveTabChange(content::WebContents* contents) const;

private:
FRIEND_TEST_ALL_PREFIXES(TabStripModelTest, GetIndicesClosedByCommand);

class WebContentsData;
struct DetachedWebContents;
struct DetachNotifications;
Expand Down Expand Up @@ -457,12 +460,6 @@ class TabStripModel {
// increasing order.
std::vector<int> GetIndicesForCommand(int index) const;

// Returns a vector of indices of the tabs that will close when executing the
// command |id| for the tab at |index|. The returned indices are sorted in
// descending order.
std::vector<int> GetIndicesClosedByCommand(int index,
ContextMenuCommand id) const;

// Returns true if the specified WebContents is a New Tab at the end of
// the tabstrip. We check for this because opener relationships are _not_
// forgotten for the New Tab page opened as a result of a New Tab gesture
Expand Down
58 changes: 30 additions & 28 deletions chrome/browser/ui/tabs/tab_strip_model_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -427,6 +427,20 @@ class TabStripModelTest : public ChromeRenderViewHostTestHarness {
return actual;
}

std::string GetIndicesClosedByCommandAsString(
const TabStripModel& model,
int index,
TabStripModel::ContextMenuCommand id) const {
std::vector<int> indices = model.GetIndicesClosedByCommand(index, id);
std::string result;
for (size_t i = 0; i < indices.size(); ++i) {
if (i != 0)
result += " ";
result += base::IntToString(indices[i]);
}
return result;
}

void PrepareTabstripForSelectionTest(TabStripModel* model,
int tab_count,
int pinned_count,
Expand Down Expand Up @@ -1328,46 +1342,34 @@ TEST_F(TabStripModelTest, GetIndicesClosedByCommand) {
TabStripModel tabstrip(&delegate, profile());
EXPECT_TRUE(tabstrip.empty());

const auto indicesClosedAsString =
[&tabstrip](int index, TabStripModel::ContextMenuCommand id) {
std::vector<int> indices =
tabstrip.GetIndicesClosedByCommand(index, id);
std::string result;
for (size_t i = 0; i < indices.size(); ++i) {
if (i != 0)
result += " ";
result += base::IntToString(indices[i]);
}
return result;
};

for (int i = 0; i < 5; ++i)
tabstrip.AppendWebContents(CreateWebContents(), true);

EXPECT_EQ("4 3 2 1",
indicesClosedAsString(0, TabStripModel::CommandCloseTabsToRight));
EXPECT_EQ("4 3 2",
indicesClosedAsString(1, TabStripModel::CommandCloseTabsToRight));
GetIndicesClosedByCommandAsString(
tabstrip, 0, TabStripModel::CommandCloseTabsToRight));
EXPECT_EQ("4 3 2", GetIndicesClosedByCommandAsString(
tabstrip, 1, TabStripModel::CommandCloseTabsToRight));

EXPECT_EQ("4 3 2 1",
indicesClosedAsString(0, TabStripModel::CommandCloseOtherTabs));
EXPECT_EQ("4 3 2 0",
indicesClosedAsString(1, TabStripModel::CommandCloseOtherTabs));
EXPECT_EQ("4 3 2 1", GetIndicesClosedByCommandAsString(
tabstrip, 0, TabStripModel::CommandCloseOtherTabs));
EXPECT_EQ("4 3 2 0", GetIndicesClosedByCommandAsString(
tabstrip, 1, TabStripModel::CommandCloseOtherTabs));

// Pin the first two tabs. Pinned tabs shouldn't be closed by the close other
// commands.
tabstrip.SetTabPinned(0, true);
tabstrip.SetTabPinned(1, true);

EXPECT_EQ("4 3 2",
indicesClosedAsString(0, TabStripModel::CommandCloseTabsToRight));
EXPECT_EQ("4 3",
indicesClosedAsString(2, TabStripModel::CommandCloseTabsToRight));
EXPECT_EQ("4 3 2", GetIndicesClosedByCommandAsString(
tabstrip, 0, TabStripModel::CommandCloseTabsToRight));
EXPECT_EQ("4 3", GetIndicesClosedByCommandAsString(
tabstrip, 2, TabStripModel::CommandCloseTabsToRight));

EXPECT_EQ("4 3 2",
indicesClosedAsString(0, TabStripModel::CommandCloseOtherTabs));
EXPECT_EQ("4 3",
indicesClosedAsString(2, TabStripModel::CommandCloseOtherTabs));
EXPECT_EQ("4 3 2", GetIndicesClosedByCommandAsString(
tabstrip, 0, TabStripModel::CommandCloseOtherTabs));
EXPECT_EQ("4 3", GetIndicesClosedByCommandAsString(
tabstrip, 2, TabStripModel::CommandCloseOtherTabs));

tabstrip.CloseAllTabs();
EXPECT_TRUE(tabstrip.empty());
Expand Down
3 changes: 3 additions & 0 deletions chrome/browser/ui/toolbar/back_forward_menu_model.cc
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,9 @@ ui::MenuModel* BackForwardMenuModel::GetSubmenuModelAt(int index) const {
return nullptr;
}

void BackForwardMenuModel::HighlightChangedTo(int index) {
}

void BackForwardMenuModel::ActivatedAt(int index) {
ActivatedAt(index, 0);
}
Expand Down
1 change: 1 addition & 0 deletions chrome/browser/ui/toolbar/back_forward_menu_model.h
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ class BackForwardMenuModel : public ui::MenuModel {
ui::ButtonMenuItemModel* GetButtonMenuItemAt(int index) const override;
bool IsEnabledAt(int index) const override;
MenuModel* GetSubmenuModelAt(int index) const override;
void HighlightChangedTo(int index) override;
void ActivatedAt(int index) override;
void ActivatedAt(int index, int event_flags) override;
void MenuWillShow() override;
Expand Down
2 changes: 2 additions & 0 deletions chrome/browser/ui/views/menu_model_adapter_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,8 @@ class CommonMenuModel : public ui::MenuModel {

ui::MenuModel* GetSubmenuModelAt(int index) const override { return nullptr; }

void HighlightChangedTo(int index) override {}

void ActivatedAt(int index) override {}

void SetMenuModelDelegate(ui::MenuModelDelegate* delegate) override {}
Expand Down
54 changes: 52 additions & 2 deletions chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc
Original file line number Diff line number Diff line change
Expand Up @@ -92,8 +92,11 @@ BrowserView* GetSourceBrowserViewInTabDragging() {
class BrowserTabStripController::TabContextMenuContents
: public ui::SimpleMenuModel::Delegate {
public:
TabContextMenuContents(Tab* tab, BrowserTabStripController* controller)
: tab_(tab), controller_(controller) {
TabContextMenuContents(Tab* tab,
BrowserTabStripController* controller)
: tab_(tab),
controller_(controller),
last_command_(TabStripModel::CommandFirst) {
model_.reset(new TabMenuModel(
this, controller->model_,
controller->tabstrip_->GetModelIndexOfTab(tab)));
Expand All @@ -102,6 +105,11 @@ class BrowserTabStripController::TabContextMenuContents
views::MenuRunner::HAS_MNEMONICS | views::MenuRunner::CONTEXT_MENU));
}

~TabContextMenuContents() override {
if (controller_)
controller_->tabstrip_->StopAllHighlighting();
}

void Cancel() {
controller_ = NULL;
}
Expand All @@ -128,14 +136,25 @@ class BrowserTabStripController::TabContextMenuContents
accelerator) :
false;
}
void CommandIdHighlighted(int command_id) override {
controller_->StopHighlightTabsForCommand(last_command_, tab_);
last_command_ = static_cast<TabStripModel::ContextMenuCommand>(command_id);
controller_->StartHighlightTabsForCommand(last_command_, tab_);
}
void ExecuteCommand(int command_id, int event_flags) override {
// Executing the command destroys |this|, and can also end up destroying
// |controller_|. So stop the highlights before executing the command.
controller_->tabstrip_->StopAllHighlighting();
controller_->ExecuteCommandForTab(
static_cast<TabStripModel::ContextMenuCommand>(command_id),
tab_);
}

void MenuClosed(ui::SimpleMenuModel* /*source*/) override {
if (controller_)
controller_->tabstrip_->StopAllHighlighting();
}

private:
std::unique_ptr<TabMenuModel> model_;
std::unique_ptr<views::MenuRunner> menu_runner_;
Expand All @@ -146,6 +165,10 @@ class BrowserTabStripController::TabContextMenuContents
// A pointer back to our hosting controller, for command state information.
BrowserTabStripController* controller_;

// The last command that was selected, so that we can start/stop highlighting
// appropriately as the user moves through the menu.
TabStripModel::ContextMenuCommand last_command_;

DISALLOW_COPY_AND_ASSIGN(TabContextMenuContents);
};

Expand Down Expand Up @@ -584,6 +607,33 @@ void BrowserTabStripController::SetTabDataAt(content::WebContents* web_contents,
TabRendererDataFromModel(web_contents, model_index, EXISTING_TAB));
}

void BrowserTabStripController::StartHighlightTabsForCommand(
TabStripModel::ContextMenuCommand command_id,
Tab* tab) {
if (command_id == TabStripModel::CommandCloseOtherTabs ||
command_id == TabStripModel::CommandCloseTabsToRight) {
int model_index = tabstrip_->GetModelIndexOfTab(tab);
if (IsValidIndex(model_index)) {
std::vector<int> indices =
model_->GetIndicesClosedByCommand(model_index, command_id);
for (std::vector<int>::const_iterator i(indices.begin());
i != indices.end(); ++i) {
tabstrip_->StartHighlight(*i);
}
}
}
}

void BrowserTabStripController::StopHighlightTabsForCommand(
TabStripModel::ContextMenuCommand command_id,
Tab* tab) {
if (command_id == TabStripModel::CommandCloseTabsToRight ||
command_id == TabStripModel::CommandCloseOtherTabs) {
// Just tell all Tabs to stop pulsing - it's safe.
tabstrip_->StopAllHighlighting();
}
}

void BrowserTabStripController::AddTab(WebContents* contents,
int index,
bool is_active) {
Expand Down
7 changes: 7 additions & 0 deletions chrome/browser/ui/views/tabs/browser_tab_strip_controller.h
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,13 @@ class BrowserTabStripController : public TabStripController,
// Invokes tabstrip_->SetTabData.
void SetTabDataAt(content::WebContents* web_contents, int model_index);

void StartHighlightTabsForCommand(
TabStripModel::ContextMenuCommand command_id,
Tab* tab);
void StopHighlightTabsForCommand(
TabStripModel::ContextMenuCommand command_id,
Tab* tab);

// Adds a tab.
void AddTab(content::WebContents* contents, int index, bool is_active);

Expand Down
Loading

0 comments on commit c75dbc5

Please sign in to comment.