Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

When closing last window, make sure buffer window is closed so that app exits #13242

Merged
merged 1 commit into from
Feb 22, 2018

Conversation

petemill
Copy link
Member

Only on win/linux, since we only handle 'window-all-closed' on those platforms.
Fix #13233

Submitter Checklist:

  • 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).
  • Tagged reviewers and labelled the pull request as needed.
  • Request a security/privacy review as needed. (Ask a Brave employee to help if you cannot access this document.)

Test Plan:

Specified on #13233

Reviewer Checklist:

  • Request a security/privacy review as needed if one was not already requested.

Tests

  • Adequate test coverage exists to prevent regressions
  • Tests should be independent and work correctly when run individually or as a suite ref
  • New files have MPL2 license header

@codecov-io
Copy link

codecov-io commented Feb 21, 2018

Codecov Report

Merging #13242 into master will decrease coverage by 0.02%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master   #13242      +/-   ##
==========================================
- Coverage   55.95%   55.92%   -0.03%     
==========================================
  Files         281      281              
  Lines       27846    27841       -5     
  Branches     4569     4565       -4     
==========================================
- Hits        15580    15570      -10     
- Misses      12266    12271       +5
Flag Coverage Δ
#unittest 55.92% <0%> (-0.03%) ⬇️
Impacted Files Coverage Δ
app/browser/windows.js 33.04% <0%> (-0.58%) ⬇️
app/browser/reducers/tabsReducer.js 40.63% <0%> (-0.56%) ⬇️
app/common/state/historyState.js 78.57% <0%> (+2.32%) ⬆️

…pp exits.

Only on win/linux, since we only handle 'window-all-closed' on those platforms.
Fix #13233
Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested and it works great 😄

@bsclifton bsclifton merged commit 12aa8bb into master Feb 22, 2018
@bsclifton bsclifton deleted the fix/quit-on-close-buffer-window branch February 22, 2018 05:32
bsclifton added a commit that referenced this pull request Feb 22, 2018
When closing last window, make sure buffer window is closed so that app exits
bsclifton added a commit that referenced this pull request Feb 22, 2018
When closing last window, make sure buffer window is closed so that app exits
@bsclifton
Copy link
Member

master 12aa8bb
0.22.x 42a3fa6
0.21.x c7a6d55

@armaanahluwalia
Copy link

@bsclifton Sorry to bombard you with messages but I believe there may be an error with 12aa8bb . Please do let me know if I'm not understanding something about the codebase. Not sure why the isDarwin() check is being performed. Could you shed some light for my benefit?

@bsclifton
Copy link
Member

bsclifton commented Mar 26, 2018

@armaanahluwalia the intention for this fix is to close a hidden window. @petemill introduced an optimization which has a window created but hidden- so that opening a new window is quick. When there are no more visible windows left, the code from this PR closes this hidden buffer window. This allows the program to exit normally when you choose File > Quit.

I don't believe this code is related to the bug you're investigating (#8164), since this has been a long standing problem. When choosing Brave > About Brave, you should see an error being output to the console

@armaanahluwalia
Copy link

@bsclifton I do believe the bug is a direct result since opening brave with buffer window disabled fixes 8164. I will double check and get back.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BraveBeta.exe process not always being terminated when closing Brave via title bar
4 participants