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

Create a Buffer Window in order to pre-render and open new windows faster #12438

Merged
merged 1 commit into from
Feb 16, 2018

Conversation

petemill
Copy link
Member

@petemill petemill commented Dec 29, 2017

A buffer window is created at startup, and any time a previous buffer window is detached in order to be utilized as a real window.
This type of window is not shown and has no tabs (pinned or non-pinned) until it is shown.
State is not persisted for Buffer Windows until they are utilized as real windows.

A new command-line flag is introduced: '--debug-window-events' which shows logging around Buffer Window creation and utilization, as well as all other window events. No additional logging should be present without this flag.

I had built some of this functionality for #11720 (Tabs transitions) in order to decrease the time to detach a tab to a new window. At the same time, a request came through Asana for the performance initiative to do this same thing for every window. This PR is the result of adapting the code from #11720 for all (compatible) window creation.

Fix #12437

Test Plan:

Window creation

  • Brave starts with 1 window
  • A new window can be opened
    • The window opens instantly without any wait

Window + frame creation

  • Open a tab that has links on it
  • Right-click any link and choose 'Open in New Window'
    • The window opens instantly, and the tab with the link's url is opened

Window persistance

  • Open at least 2 windows, with at least 2 tabs in each
  • Quit Brave
  • Open Brave again
    • All windows and tabs are re-opened
    • No extra windows are re-opened

Pinned tabs

  • Open 2 windows
  • Pin a tab in window 1
    • Tab is pinned in window 1 and window 2
  • Open a new window
    • Window opens instantly
    • new window (3) has the pinned tab

Window focus (especially for Windows OS due to brave/muon#424)

  • Create a new window
    • The window opens instantly
    • The window gets focus instantly
    • The window does not lose focus, and the urlbar can be typed in without any further action
      Especially on Windows: ensure that always when a new window is created, that the new window gets and keeps focus (in the url bar for example)

Detach tab to new window

  • Create 1 window with mulitple tabs
  • Detach a tab (right-click --> Detach, or drag the tab away from Window)
    • Ensure the tab's new window was created from a buffer window
    • Ensure the window is positioned correctly by the mouse cursor (if dragged out), as per previous version of Brave.

Popup window

  • Open two windows with two different sites in each window
  • Pin 1 site in 1 window
    • Site will be pinned in Window 1 and Window 2
  • Visit http://www.popuptest.com/goodpopups.html
  • Click "Good Popup Basic tab structure #1"
    • Popup should open in a new window
    • Popup window should be buffer window
    • Popup window should be sized small (according to the popup request)
    • Popup window should not have any pinned tabs
  • Pin 1 site in window 2
    • Window 1 and Window 2 should get the new pinned site
    • Popup window should still not have any pinned tabs

Logging

  • start Brave with the flag --debug-window-events

    • observe console log entries for all window events
  • start brave without the above flag

    • observe no console log entries

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.)

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

@petemill petemill added this to the 0.21.x (Developer Channel) milestone Dec 29, 2017
@petemill petemill self-assigned this Dec 29, 2017
@codecov-io
Copy link

codecov-io commented Dec 29, 2017

Codecov Report

Merging #12438 into master will decrease coverage by 0.35%.
The diff coverage is 28.06%.

@@            Coverage Diff             @@
##           master   #12438      +/-   ##
==========================================
- Coverage   56.29%   55.94%   -0.36%     
==========================================
  Files         278      279       +1     
  Lines       27585    27797     +212     
  Branches     4497     4550      +53     
==========================================
+ Hits        15530    15550      +20     
- Misses      12055    12247     +192
Flag Coverage Δ
#unittest 55.94% <28.06%> (-0.36%) ⬇️
Impacted Files Coverage Δ
js/actions/appActions.js 19.67% <ø> (ø) ⬆️
app/browser/reducers/windowsReducer.js 80.85% <100%> (-0.3%) ⬇️
app/common/state/pinnedSitesState.js 77.27% <100%> (-0.71%) ⬇️
app/cmdLine.js 39.42% <100%> (-0.39%) ⬇️
app/browser/windows.js 33.99% <30.51%> (-3.09%) ⬇️
js/state/frameStateUtil.js 61.15% <57.14%> (-1.87%) ⬇️
app/sessionStore.js 88.06% <72.72%> (-0.51%) ⬇️
app/common/lib/browserWindowUtil.js 8% <8%> (ø)
... and 3 more

@petemill
Copy link
Member Author

petemill commented Jan 5, 2018

On the Windows platform, new windows that are created from Buffer Windows have their focus stolen by the next created Buffer Window (which happens immediately) due to brave/muon#424

@petemill
Copy link
Member Author

petemill commented Feb 8, 2018

This is no longer blocked since brave/muon#424 is fixed, and is ready for review @bsclifton. I added a few test cases to this PR.

@NejcZdovc
Copy link
Contributor

@petemill please rebase

@petemill
Copy link
Member Author

petemill commented Feb 8, 2018

@NejcZdovc good catch - rebased!

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.

Great job on this one! The change is extremely noticeable; this'll be huge

I reviewed the rest of the way, didn't find any additional problems. Just wanted to call out the two issues we saw when debugging together:

  1. When detaching, extra properties were not handled resulting in a new window being created (instead of using buffered). These properties had the following values:
disposition: undefined,
positionByMouseCursor: true,
checkMaximized: true
  1. When exiting, it seems like the buffered window is being saved. ex: closing with 1 window results in 2 windows when relaunching. Closing with 2 windows results in 3 windows when relaunching

…s more instantly

A buffer window is created at startup, and any time a previous buffer window is detached in order to be utilized as a real window.
This type of window is not shown and has no tabs (pinned or non-pinned) until it is shown.
State is not persisted for Buffer Windows until they are utilized as real windows.

A new command-line flag is introduced: '--debug-window-events' which shows logging around Buffer Window creation and utilization, as well as all other window events. No additional logging should be present without this flag.

Fix #12437
@petemill
Copy link
Member Author

Thanks @bsclifton for finding those issues. I have fixed those issues and updated the description with three additional test cases:

  • Popup windows (use buffer window and behave as previously)
  • Tab detaching to new window uses buffer window
  • Restarting Brave does not result in additional restored Window each time.

I also found that the focused window wasn't restored as the focused window on app restart. I fixed it because I thought it was a regression introduced here, but looks like it wasn't, so I created another PR for it at #13161

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.

Ran through steps and it's looking great! I ensured logging correctly showed buffer being used for popups and when detaching. Pinned tabs worked as expected. Nice work! 😄 👍

@bsclifton bsclifton merged commit 4c54e81 into master Feb 16, 2018
@bsclifton bsclifton deleted the feature/buffer-windows branch February 16, 2018 16:26
bsclifton added a commit that referenced this pull request Feb 16, 2018
Create a Buffer Window in order to pre-render and open new windows faster
@bsclifton
Copy link
Member

bsclifton commented Feb 16, 2018

master 4c54e81
0.22.x ad9306a
0.21.x 0a5334c

petemill pushed a commit that referenced this pull request Feb 16, 2018
Create a Buffer Window in order to pre-render and open new windows faster
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.

4 participants