Add new context menu to entire tab bar (including Reopen last closed tab)

[philkloose]

• Submitted a ticket for my issue if one did not already exist.
• Used Github auto-closing keywords in the commit message.
• Added/updated tests for this change (for new code or code which already has tests).
• Ran git rebase -i to squash commits (if needed).

This is my attempt to resolve #8303.
Note -- this is really my first pull request beyond a README grammar change, so it's quite possible I overlooked something.

Test Plan:
Open a new tab, visit a site (i.e. https://google.com), close the tab and then right-click in the space to the right of any open tabs. You should be presented with a new context menu with an option to reopen the last closed tab.

[bsclifton]

@philkloose awesome! thanks for taking a stab at it 😄 I'm going to test this out here- I think it'll work great, with a few tweaks. Hang tight...

[bsclifton]

@bradleyrichter I wanted to propose a change to context menus shown when you right click the tabs bar. Here's what things look like currently:

macOS:

Windows (because of issues with draggability)

---

@philkloose has a PR which updates the behavior for both platforms to show a new "Reopen Last Closed Tab" option.

---

We could update this menu to show some more useful options, in addition to the one that was added with this PR. At a bare minimum, we can offer what's already there plus this new option:

What do you think? 😄

[bsclifton]

@philkloose can you update the menu that shows up?

Instead of the single item (Reopen Last Closed Tab), can we show all of these:

If you can get the menu items in there, I can help you implement each of the features 😄

[philkloose]

I actually have this all working on my machine, but git and I are in a deathmatch at the moment and it's winning. I couldn't be more of a newbie when it comes to git and it's a sad sight here to see me try to wrangle it. Eventually (hopefully tomorrow) I will figure out how to get all my changes into one commit.

[bsclifton]

@philkloose no problem 😄 I'll take a look at the PR here and help you figure it all out 👍

[bsclifton]

Awesome job with this, @philkloose! ❤️ This has a great look and feel- very useful

Looks like the call for mute isn't working, I'll take a look into that.

With regards to git, I'm not sure what your experience level is, but here's what I did to square things away 😄 :

# add your remote to my local copy of the repo
# you don't need to do this, but it's handy for testing other people's forks
git remote add philkloose git@github.com:philkloose/browser-laptop.git
git fetch philkloose

# add "upstream" as the master repo for brave
git remote add upstream git@github.com:brave/browser-laptop.git
git fetch upstream

# checkout your branch and rebase
git checkout issue-8303
git rebase -i upstream/master

At this point, it brought up an interactive mode which has some instructions. I saw you had a revert commit in there which undos all the great work you did. I dropped that commit and then squashed the rest (top-most one left as pick, then I put d for the revert commit and s for the others)

[NejcZdovc]

@bsclifton should we rename this PR, because we didn't just add reopen item, but completely new menu item.

[bsclifton]

@NejcZdovc good idea! updated 😄

[philkloose]

@bsclifton Thanks for fixing up my git mess. I really need to spend some time getting comfortable with it. You can't even imagine how many dead branches I'd created locally trying to resolve it yesterday.

I made a commit to get "Mute All" working but I had to undo some of my previous work to do so. Originally I preferred to try to add the menu item to commonMenu.js, but after failing to get it to actually function properly I just moved it back and built it in contextMenus.js.

More importantly, before you accept these commits I'd like you to take a look at something because I always have a fear that I'm headed in some direction that will later be regretted. Before I started doing anything at all on this issue, there was a tabsToolbarTemplateInit() that built a context menu that seems far more tuned for the bookmarksToolbar. You can see it in the screenshot you provided earlier.

Should this be renamed to something like bookmarksToolbarTemplateInit() in order to reduce confusion, or am I missing something bigger here? Sometimes I'm not confident about what I'm seeing because the hitboxes on these toolbars and buttons seem particularly bad on my machine.

[bsclifton]

@philkloose you're very welcome about the git help 😄

I see your solution but I think there's an even easier way to do it... stay tuned

[bsclifton]

@philkloose update checked in! Let me know what you think 😄

Regarding renaming, I think that's a great idea 😄 Please do feel free to give more accurate names to the methods used to generate the menus

[philkloose]

Ah, looks good. I see why mine wasn't working now. I am now concerned about the fact that the menu item doesn't strictly describe what it's going to do. If it's truly a "Mute Tabs" button (and I think it should be to help in those panicked instances where really need it), it should not function as a "Toggle Mute State Across All Tabs" button. As it stands now, if you have two tabs open and playing (one muted and one not muted), it will just swap the states. That seems weird. I'm hesitant to change the generateMuteFrameList() function right now, but I'm hoping to look at this a bit more tomorrow.

[bsclifton]

@philkloose that is a good concern... if you'd like, we can leave the mute menu item out of this PR and merge... and we can create a follow up issue for that (if you're interested)?

As-is, your patch adds a lot of value (even without mute) 😄

[philkloose]

That works for me. Can I leave you to amend(?) the commit? It'll save me about 3 hours and a bit of hair. I spent a while reading up on git basics and doing tutorial today, so hopefully my phobia will lessen in time.

[bsclifton]

@philkloose absolutely 😄 I'll create a follow up issue and tag you in it 👍

[bradleyrichter]

Yes. Mute-all needs to live up to its name. : ) Maybe a master mute layer that retains previous mute state when master-mute is turned off again.

…

On Apr 27, 2017, at 2:58 PM, philkloose ***@***.***> wrote: Ah, looks good. I see why mine wasn't working now. I am now concerned about the fact that the menu item doesn't strictly describe what it's going to do. If it's truly a "Mute Tabs" button (and I think it should be to help in those panicked instances where really need it), it should not function as a "Toggle Mute State Across All Tabs" button. As it stands now, if you have two tabs open and playing (one muted and one not muted), it will just swap the states. That seems weird. I'm hesitant to change the generateMuteFrameList() function right now, but I'm hoping to look at this a bit more tomorrow. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[bsclifton]

Merged! 🎉

Congrats on your first commit to Brave, @philkloose 😄 This will be part of our next release: 0.15.2