Restore deleted tab context menu #3836

simonhong · 2019-10-31T09:20:34Z

Reopen closed tab, Close other tabs and Bookmark all tabs items
are restored to tab context menu.

Chromium removed some items from tab context menu by below CL.

CL: https://chromium-review.googlesource.com/c/chromium/src/+/1761666
Issue: https://bugs.chromium.org/p/chromium/issues/detail?id=515930

Update tab and frame context menus to match most recent UX specs.

This removes four entries from the tab context menu; changes the text on several
others to stop saying "tab(s)" explicitly; adds one entry to the frame context
menu; and changes the bookmark-related menu entry strings from "page" to "tab"
for consistency.

Most of the file changes here are due to renaming enums/APIs to match the string
changes.

Submitter Checklist:

Test Plan:

Enabled state of Bookmark all tabs and Reopen closed tab are same as frame's same context menu.
Close other tabs is enabled only when closing candidates exists.

yarn test brave_browser_tests --filter=*BraveTabMenuModelTest*

Reviewer Checklist:

New files have MPL-2.0 license header.
Request a security/privacy review as needed.
Adequate test coverage exists to prevent regressions
Verify test plan is specified in PR before merging to source

After-merge Checklist:

The associated issue milestone is set to the smallest version that the
changes has landed on.
All relevant documentation has been updated.

simonhong · 2019-10-31T22:39:56Z

Oh, there are some lint error and missing deps. updating now.

simonhong · 2019-11-01T00:22:47Z

Ready to review.

bsclifton · 2019-11-01T05:31:31Z

@simonhong can you link to the Chromium details (in the commit message)?
This was captured in Chromium tracker with https://bugs.chromium.org/p/chromium/issues/detail?id=515930

And then I believe it was removed with https://chromium.googlesource.com/chromium/src.git/+/32bb26026fb3fd768dda878f9193a5080593d5ec

browser/ui/tabs/tab_menu_model_delegate_proxy.cc

bsclifton · 2019-11-01T08:19:02Z

browser/ui/tabs/brave_tab_menu_model.cc

+      tab_strip_model_(tab_strip_model) {
+  // Take owner ship of TabMenuModelDelegateProxy instance.
+  // There is no way to instantiate it before calling ctor of base class.
+  delegate_proxy_.reset(TabMenuModel::delegate());


is this needed anymore? (how is delegate_proxy_ used?)

It needs because SimpleMenuModel doesn't own delegate.
BraveTabMenuModel should manage proxy's lifecycle.

This definitely does not look right to me. The TabMenuModel is not supposed to take ownership of the delegate and this seems likely to cause a crash at some point

The delegate is owned by the BrowserTabStripController and I'm fairly certain that this will crash if you close a window with a context menu open. At the very least it breaks the cancel behavior here

BrowserTabStripController::~BrowserTabStripController() { // When we get here the TabStrip is being deleted. We need to explicitly // cancel the menu, otherwise it may try to invoke something on the tabstrip // from its destructor. if (context_menu_contents_.get()) context_menu_contents_->Cancel(); model_->RemoveObserver(this); }

so I just noticed that you're creating a new proxy delegate there, but that's also a potential problem. Let's dm and discuss when you're available

I feel like this creates a bit of a convoluted chain of ownership and while it may or may not be ok, it seems like it would be easier to subclass BrowserTabStripController in BrowserViews and implement ShowContextMenuForTab. Then we can create our own delegate from the start with our own TabMenuModel and calling back into our own ExecuteCommandForTab. Then we can check the command ids in our BrowserTabStripController and either delegate to the TabStripModel or call the command in BraveBrowserTabStripController

then at least the commands have the same flow at least through ExecuteCommandForTab
and there are no confusing ownership chains

simonhong · 2019-11-02T00:44:47Z

Kindly ping to @bridiver :)

bsclifton

++

Reopen closed tab, Close other tabs and Bookmark all tabs items are restored to tab context menu. Chromium removed some items from tab context menu by below CL. CL: https://chromium-review.googlesource.com/c/chromium/src/+/1761666 Issue: https://bugs.chromium.org/p/chromium/issues/detail?id=515930 Update tab and frame context menus to match most recent UX specs. This removes four entries from the tab context menu; changes the text on several others to stop saying "tab(s)" explicitly; adds one entry to the frame context menu; and changes the bookmark-related menu entry strings from "page" to "tab" for consistency. Most of the file changes here are due to renaming enums/APIs to match the string changes.

This reverts commit 7723f74, reversing changes made to 4de8e57.

bridiver · 2019-11-04T20:05:27Z

patches/chrome-browser-ui-tabs-tab_strip_model.h.patch

+
+  private:
+   FRIEND_TEST_ALL_PREFIXES(TabStripModelTest, GetIndicesClosedByCommand);
+  friend class TabMenuModelDelegateProxy;


I don't understand why this friend is needed. Can you explain?

If this is needed, @bridiver added an example for how to do it cleaner under https://github.com/brave/brave-browser/wiki/Patching-Chromium#subclass-and-override

Revert "Merge pull request #3836 from brave/tab_menu_model"

bsclifton · 2019-11-04T20:09:50Z

Reverted with #3874 after conversation with @bridiver

bridiver · 2019-11-04T20:24:00Z

browser/ui/tabs/tab_menu_model_delegate_proxy.cc

+}
+
+bool TabMenuModelDelegateProxy::IsCommandIdChecked(int command_id) const {
+  return false;


shouldn't this be returning delegate_->IsCommandIdChecked

bridiver · 2019-11-04T20:25:11Z

browser/ui/tabs/tab_menu_model_delegate_proxy.cc

+  return indices;
+}
+
+bool TabMenuModelDelegateProxy::IsBraveCommandIds(int command_id) const {


nit: IsBraveCommandIds -> IsBraveCommandId
the use of Ids here is not grammatically correct

bridiver · 2019-11-04T20:26:49Z

browser/ui/tabs/tab_menu_model_delegate_proxy.cc

+  }
+}
+
+std::vector<int> TabMenuModelDelegateProxy::GetIndicesClosed(int index) const {


This method name is confused to me. Maybe it should be GetIndicesToClose?

bsclifton · 2019-11-04T20:26:51Z

Re-opened PR at #3875

cc: @simonhong on the above review comments

This reverts commit 7723f74, reversing changes made to 4de8e57.

simonhong added this to the 0.73.x - Nightly milestone Oct 31, 2019

simonhong self-assigned this Oct 31, 2019

simonhong force-pushed the tab_menu_model branch 4 times, most recently from 6f49623 to 8daa441 Compare October 31, 2019 21:57

simonhong requested review from bsclifton and bridiver October 31, 2019 22:25

simonhong marked this pull request as ready for review October 31, 2019 22:26

simonhong force-pushed the tab_menu_model branch from 8daa441 to 190fafd Compare October 31, 2019 22:26

simonhong force-pushed the tab_menu_model branch 2 times, most recently from 616fec5 to 30eb667 Compare October 31, 2019 23:39

simonhong force-pushed the tab_menu_model branch from 30eb667 to 983abe0 Compare November 1, 2019 01:21

simonhong force-pushed the tab_menu_model branch from 983abe0 to 63d7514 Compare November 1, 2019 05:43

bsclifton reviewed Nov 1, 2019

View reviewed changes

browser/ui/tabs/tab_menu_model_delegate_proxy.cc Outdated Show resolved Hide resolved

simonhong force-pushed the tab_menu_model branch 2 times, most recently from 23d4293 to e3db310 Compare November 1, 2019 07:32

bsclifton reviewed Nov 1, 2019

View reviewed changes

bsclifton approved these changes Nov 3, 2019

View reviewed changes

simonhong force-pushed the tab_menu_model branch from e3db310 to feff2ae Compare November 4, 2019 00:38

bsclifton merged commit 7723f74 into master Nov 4, 2019

bsclifton deleted the tab_menu_model branch November 4, 2019 19:37

bsclifton added a commit that referenced this pull request Nov 4, 2019

Revert "Merge pull request #3836 from brave/tab_menu_model"

3f90811

This reverts commit 7723f74, reversing changes made to 4de8e57.

bsclifton mentioned this pull request Nov 4, 2019

Revert "Merge pull request #3836 from brave/tab_menu_model" #3874

Merged

bridiver reviewed Nov 4, 2019

View reviewed changes

bsclifton added a commit that referenced this pull request Nov 4, 2019

Merge pull request #3874 from brave/revert-tab_menu_model

750c9a8

Revert "Merge pull request #3836 from brave/tab_menu_model"

bridiver reviewed Nov 4, 2019

View reviewed changes

bsclifton mentioned this pull request Nov 4, 2019

Restore deleted tab context menu #3875

Merged

32 tasks

bridiver reviewed Nov 4, 2019

View reviewed changes

fmarier pushed a commit to fmarier/brave-core that referenced this pull request Nov 6, 2019

Revert "Merge pull request brave#3836 from brave/tab_menu_model"

8fd603c

This reverts commit 7723f74, reversing changes made to 4de8e57.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restore deleted tab context menu #3836

Restore deleted tab context menu #3836

simonhong commented Oct 31, 2019 •

edited

Loading

simonhong commented Oct 31, 2019

simonhong commented Nov 1, 2019

bsclifton commented Nov 1, 2019

bsclifton Nov 1, 2019

simonhong Nov 1, 2019

bridiver Nov 4, 2019

bridiver Nov 4, 2019

bridiver Nov 4, 2019

bridiver Nov 4, 2019

bridiver Nov 4, 2019

simonhong commented Nov 2, 2019

bsclifton left a comment

bridiver Nov 4, 2019

bsclifton Nov 4, 2019

bsclifton commented Nov 4, 2019

bridiver Nov 4, 2019

bridiver Nov 4, 2019

bridiver Nov 4, 2019

bsclifton commented Nov 4, 2019 •

edited

Loading

Restore deleted tab context menu #3836

Restore deleted tab context menu #3836

Conversation

simonhong commented Oct 31, 2019 • edited Loading

Submitter Checklist:

Test Plan:

Reviewer Checklist:

After-merge Checklist:

simonhong commented Oct 31, 2019

simonhong commented Nov 1, 2019

bsclifton commented Nov 1, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonhong commented Nov 2, 2019

bsclifton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsclifton commented Nov 4, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsclifton commented Nov 4, 2019 • edited Loading

simonhong commented Oct 31, 2019 •

edited

Loading

bsclifton commented Nov 4, 2019 •

edited

Loading