feat: multi browser views support for BrowserWindow #16064

summeroff · 2018-12-14T00:45:24Z

Description of Change

new feature: multi browser views support for BrowserWindow

Add functions addBrowserView, removeBroserView, getBrowserViews to BrowserWindow class.

Existing API setBrowserView and getBrowserView changed to use new API inside.

This functionality was mentioned in the initial BrowserView feature PR #9166 . And asked few times since then. Recently in a discussion #10622 .

Checklist

PR description included and stakeholders cc'd
npm test passes for npm run test -match=BrowserView, all-test run have few fails even on unchanged 2-0-x branch.
tests are added
relevant documentation is changed or added
PR title follows semantic commit guidelines

Release Notes

Notes: multi browser views support for BrowserWindow

Add functions addBrowserView, removeBroserView, getBrowserViews to BrowserWindow class. Existing API as setBrowserView and getBrowserView not changed and keep working in single BrowserView mode.

…tream-labs/electron into sl_multi_browser_views_support

welcome · 2018-12-14T00:45:27Z

💖 Thanks for opening this pull request! 💖

We use semantic commit messages to streamline the release process. Before your pull request can be merged, you should update your pull request title to start with a semantic prefix.

Examples of commit messages with semantic prefixes:

fix: don't overwrite prevent_default if default wasn't prevented
feat: add app.isPackaged() method
docs: app.isDefaultProtocolClient is now available on Linux

Things that will help get your PR across the finish line:

Follow the JavaScript, C++, and Python coding style.
Run npm run lint locally to catch formatting errors earlier.
Document any user-facing changes you've made following the documentation styleguide.
Include tests when adding/changing behavior.
Include screenshots and animated GIFs whenever possible.

We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.

zcbenz · 2018-12-14T01:20:21Z

atom/browser/api/atom_api_window.cc

@@ -914,6 +915,65 @@ void Window::ResetBrowserView() {
  browser_view_.Reset();
 }

+void Window::AddBrowserView(v8::Local<v8::Value> value) {
+  mate::Handle<BrowserView> browser_view;
+  if (value->IsNull() || value->IsUndefined()) {


There is no need to check null, type check can be done with:

if (mate::ConvertFromV8(isolate(), value, &browser_view)) { } else { // Throw type error. }

Window::RemoveBrowserView can also be modified in the same way.

Work of ConvertFromV8 is done by template implementation for corresponding type. For most types this indeed have check for null. But mate::Handle<> FromV8 converter does something opposite.

if (val->IsNull() || val->IsUndefined()) { *out = mate::Handle<T>(); return true; }

And that lead to crash if null is passed. So I have to add check back.
But change it to value->IsObject()
Is it OK?

Do I have to add "Throw type error"? Prev API does not do it and as AddBrowserView works now as fall back for SetBrowserView. Then SetBrowserView start to throwing error too. But sending null to setBrowserView is required way to remove BrowserView what was set before.

zcbenz · 2018-12-14T01:24:26Z

atom/browser/api/atom_api_window.cc

+}
+
+void Window::ResetBrowserViews() {
+  for (auto views_iter = browser_views_.begin();


Can use range-based for loop here:

for (auto item : browser_views_) { item.second.Reset() }

Other for loops can also be modified as such.

zcbenz · 2018-12-14T01:28:32Z

atom/browser/native_window.h

@@ -156,6 +156,8 @@ class NativeWindow : public base::SupportsUserData,
  virtual void SetMenu(AtomMenuModel* menu);
  virtual void SetParentWindow(NativeWindow* parent);
  virtual void SetBrowserView(NativeBrowserView* browser_view) = 0;


We can remove the NativeWindow::SetBrowserView implementation, and add a simple fallback for it by reusing the new API:

BrowserWindow.prototype.setBrowserView = (view) => { this.resetBrowserViews() this.addBrowserView(view) }

I have tried to keep APIs separate to simplify usage in old code and new code written with new API. Even thinking about more explicit way what code can request new functionality.
Your suggestion is good as it is let add new API without keeping legacy code and will not affect
existing user code.
I have changed code in accordance with your suggestion.

zcbenz · 2018-12-14T01:32:37Z

docs/api/browser-window.md

+
+* `browserView` [BrowserView](browser-view.md)
+
+Replacement API for setBrowserView supporting work with multi browser views.


I think we should have a clear definition on the behavior when using setBrowserView and addBrowserView together, and have it documented. I suggest:

Make setBrowserView an alias for removeAllBrowserViews() && addBrowserView(view).

Make getBrowserView throw if there are multiple browser views, and return getBrowserViews()[0] as fallback.

documentation updated

for loops changed to range based setBrowserView replaced with removeall and addone

zcbenz · 2018-12-18T02:40:43Z

Sorry I just noticed that this PR is targeted for 2.0.x. According to our versioning policy, we will not add new features to 2.0.x: https://www.electronjs.org/docs/tutorial/electron-versioning.

Do you mind rebasing your changes on master branch?

This code itself looks good to me now.

summeroff · 2018-12-18T03:00:12Z

Will it goes only to electron 4 from master or it can be backported to 3-0-x too?

zcbenz · 2018-12-18T12:57:28Z

New features will be included in 5.0, currently we don't have plans to publish minor updates for 3.x/4.x for new features.

summeroff · 2018-12-19T22:23:37Z

feature code has been moved to a branch originated from master and new PR #16148 submitted.

summeroff added 4 commits December 7, 2018 04:55

feat: add multi BrowserView support to BrowserWindow

b4edb93

Add functions addBrowserView, removeBroserView, getBrowserViews to BrowserWindow class. Existing API as setBrowserView and getBrowserView not changed and keep working in single BrowserView mode.

fix: missed browserview autoresize support

157057e

fix: missed browserview autoresize support

4baf49e

Merge branch 'sl_multi_browser_views_support' of https://github.com/s…

144539c

…tream-labs/electron into sl_multi_browser_views_support

summeroff requested review from a team December 14, 2018 00:45

summeroff added 2 commits December 14, 2018 04:10

fix: wrong copypast type for var in mac native code

e7b4326

fix: lint error in mac code

8070f95

zcbenz requested changes Dec 14, 2018

View reviewed changes

summeroff added 4 commits December 15, 2018 01:03

fix: for loops and impl setBrowserView

aee4853

for loops changed to range based setBrowserView replaced with removeall and addone

style: codestyle fix for long lines

c385ae6

fix: requered return from non void func

5fffd9c

docs: update for multi browserview support

b36fd3f

summeroff closed this Dec 19, 2018

summeroff deleted the sl_multi_browser_views_support branch December 19, 2018 15:39

summeroff changed the base branch from 2-0-x to master December 19, 2018 16:11

summeroff mentioned this pull request Dec 19, 2018

feat: add multi BrowserView support to BrowserWindow #16148

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: multi browser views support for BrowserWindow #16064

feat: multi browser views support for BrowserWindow #16064

summeroff commented Dec 14, 2018 •

edited

welcome bot commented Dec 14, 2018

zcbenz Dec 14, 2018

summeroff Dec 17, 2018

zcbenz Dec 14, 2018

summeroff Dec 15, 2018

zcbenz Dec 14, 2018

summeroff Dec 15, 2018

zcbenz Dec 14, 2018

summeroff Dec 17, 2018

zcbenz commented Dec 18, 2018 •

edited

summeroff commented Dec 18, 2018

zcbenz commented Dec 18, 2018

summeroff commented Dec 19, 2018


		* `browserView` [BrowserView](browser-view.md)

		Replacement API for setBrowserView supporting work with multi browser views.

feat: multi browser views support for BrowserWindow #16064

feat: multi browser views support for BrowserWindow #16064

Conversation

summeroff commented Dec 14, 2018 • edited

Description of Change

Checklist

Release Notes

welcome bot commented Dec 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zcbenz commented Dec 18, 2018 • edited

summeroff commented Dec 18, 2018

zcbenz commented Dec 18, 2018

summeroff commented Dec 19, 2018

summeroff commented Dec 14, 2018 •

edited

zcbenz commented Dec 18, 2018 •

edited