Maximize state / position for all windows is now saved to session store

[bsclifton]

BrowserWindow's maximize state and position is now recorded and used (stored via WindowState).
ex: if you maximize Brave and quit, it will be maximized the next time you launch it. If window was not maximized, it'll be restored to the same x/y position it was at last session.

• Submitted a ticket for my issue if one did not already exist.
• Used Github auto-closing keywords in the commit message.
• Ran git rebase -i to squash commits if needed.

Fixes #928
Fixes #146

[bbondy]

@bridiver pls review.

[bsclifton]

@bridiver have you had a chance to check this out?

[bridiver]

won't this set all windows to be maximized on restore if any window is maximized? Maybe we can push this into window state and make it a more generalized size restore? Represent maximized as -1 or something?

[bsclifton]

Per the comments in #2135

Let me try fixing this up tonite- remembering each individual state shouldn't be bad 😄

[bsclifton]

After reviewing again, this change as-is seems to be pretty nice to me. I agree that state can be better saved as an enum (to accomodate fullscreen as a possible state also). I'll look at addressing that as a potential fix for #2135

@bridiver, this change does have Brave use the maximized setting for any new windows you create using the either the app menu "File -> New Window" or the context menu "Open Link in New Window". However, Brave doesn't seem to save the state for anything other than the last Window that was opened (which is consistent with changes in this PR).

Here's an example for reference:

• I open the browser
• Brave will call createWindow() to create the initial window. With this PR, it'll respect the new maximize state.
• Brave will reload whatever content I had open last time, based on saved frame state. Let's call this Window 1.
• I can right click on any given link, open in new window, and I'll have a new window; Window 2. It's important to note that Window 2 also gets created by via createWindow()... so if Window 1 was maximized, it will be too.
• With Window 2 now open, if I close Window 1, those tabs (frames) are lost forever. Brave's app state will only persist the last open window.
• I close Brave
• I reopen Brave
• It shows the tabs that were saved with Window 2

I cannot close Brave when multiple windows are open. I can forcibly close Brave with two windows open and when the app starts again, it only creates one window (with the content from window 1)... presumably because the app state wasn't persisted on exit.

[bsclifton]

Leaving this PR as-is until you get a chance to respond @bridiver

I did get full screen and position mostly working, but I did run into issues and I think they would be better as a separate PR.

[bridiver]

We are definitely saving the state of all windows. I checked on win7 to make sure there isn't a windows-only bug. I created two windows with different tabs and they were both restored when I started Brave again. I'm not sure why you're unable to close Brave with more than one window open, what happens when you try file->quit or ctrl-q with more than one window?

Right now there isn't a good way to match an enum or similar to the actual window tabs contents because all of the ids are session-only, but you could store the window size (position would be great too) in the window state and pull it from action.restoredState when creating the window. There are a couple of different ways to do this:

1. You could move the event listener to main.js
2. You could route a window action from the browser process using queryInfo.windowId
   Both would allow you to keep the window size attached to the correct windowState on restore

[bsclifton]

@bridiver wow, good call- that was definitely my bad. My mistake was not remembering / trying the File => Quit (I had ctrl+c'ed it in the console where I started it via npm start). I just created multiple windows and used the quit menu and verified the window states are persisted 😄

So the goal is to persist each of the BrowserWindow instances:

• size (was already done)
• position
• fullscreen status (would be better as a separate PR)
• maximized status

It looks like when the browser comes online, it reloads that state here:
https://github.com/brave/browser-laptop/blob/master/app/index.js#L301

[bsclifton]

@bridiver like you're suggesting, I can put handler on either main.js or trigger a window action to actually store the updated state.

It should be saved already (after the first run), thanks to the additional logic here:
https://github.com/brave/browser-laptop/blob/master/app/sessionStore.js#L50

Then I could finally revisit the createWindow call and consider the new windowState variable when creating the window

[bsclifton]

Ready for review! @bridiver @bbondy

This now stores maximize state + window position (which should work good across multiple monitors- unfortunately, I don't have a good way to test). If that does work, I'm sure @bradleyrichter will be happy 😄

[bridiver]

I think you can use remote.getCurrentWindow().on here instead of relaying with ipc messages
see
https://github.com/electron/electron/blob/master/docs/api/remote.md

[bsclifton]

This is great, thanks! I'll update this tonite to help keep things nice and clean 😄

[bsclifton]

Should be good to merge 😄

[bridiver]

I squashed the commits so this didn't close automatically, but still seems to be an issue with maximize state that I didn't notice until afterwards. If neither window is maximized then position works correctly, but if one window is maximized, both windows restore as maximized

[bridiver]

I see the problem now. OSX never actually sets the maximized state so it's just restoring the windows using the default size. Never mind.
++

[luixxiul]

788a0e7

[bridiver]

if you have some time it would be great if we could get a test for this in sessionStoreTest

[bsclifton]

That sounds great- I've never ran the tests, but would love to check it out. Will start looking at this tonite