Closing last tab should open a new tab #14092
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
spylogsster
force-pushed
the
brave-1002
branch
3 times, most recently
from
105993b to
45b427e
Compare
|
Are you sure we need this by default? E.g. personally I'm kinda used to the existing behavior. Do we have an input from the product team on this? |
+1 IMO, It would be better to have option and disabled by default? |
|
I got this failure on macOS when I do |
|
Adding @rebron - should this be off by default? or maybe on by default only for macOS? I'm not sure I know of a browser that does this by default - and it's against what the OS behavior to open a new window after closing on Linux and Windows |
We should have this off by default probably even for macOS. |
|
@spylogsster can you create a profile preference for this (defaulted to false) and put a guard around the behavior? @rebron where should this setting live? I think a good place might be under System
|
spylogsster
force-pushed
the
brave-1002
branch
4 times, most recently
from
6bceab5 to
c851445
Compare
|
done |
Default behaviour wasn't changed, users should go and enable it in options to keep browser opened |
spylogsster
force-pushed
the
brave-1002
branch
4 times, most recently
from
5072fa6 to
a9eb316
Compare
I was thinking brave://settings/system but we had closing windows/quitting in Help Tips. |
bsclifton
reviewed
Jul 15, 2022
| @@ -9,6 +9,10 @@ | |||
| pref="{{prefs.brave.enable_window_closing_confirm}}" | |||
| label="$i18n{braveHelpTipsWarnBeforeClosingWindow}"> | |||
| </settings-toggle-button> | |||
| <settings-toggle-button class="cr-row" | |||
There was a problem hiding this comment.
Can we put this on the system page actually? Great naming on the preference though - as it matches the Firefox setting name 😄
spylogsster
force-pushed
the
brave-1002
branch
2 times, most recently
from
58e63e9 to
9dcad4e
Compare
|
I still can see above failure after |
bsclifton
approved these changes
Jul 18, 2022
|
Good catch @simonhong - I see that crash too, with Ctrl + W |
|
Here's the stacktrace from the crash I get after hitting Ctrl + W. I think it's the same as @simonhong's (except I'm on Windows) |
Fixed |
simonhong
approved these changes
Jul 19, 2022
browser/ui/brave_browser.cc
Outdated
| return; | ||
| } | ||
|
|
||
| base::ThreadTaskRunnerHandle::Get()->PostTask( |
There was a problem hiding this comment.
According to the document (https://chromium.googlesource.com/chromium/src/+/lkgr/docs/threading_and_tasks.md#posting-to-the-current-virtual_thread), base::SequencedTaskRunnerHandle::Get() is more preferred over base::ThreadTaskRunnerHandle::Get() when posting to current thread
spylogsster
force-pushed
the
brave-1002
branch
3 times, most recently
from
67f698b to
f7b1f27
Compare
rebron
approved these changes
Jul 19, 2022
|
Noticed we have this on Android, although with different text: Which has the preference key cc: @rebron @spylogsster |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolves brave/brave-browser#1002
tabs.mp4
Submitter Checklist:
QA/YesorQA/No;release-notes/includeorrelease-notes/exclude;OS/...) to the associated issuenpm run test -- brave_browser_tests,npm run test -- brave_unit_tests,npm run lint,npm run gn_check,npm run tslintgit rebase master(if needed)Reviewer Checklist:
gnAfter-merge Checklist:
changes has landed on
Test Plan: