BrowserView: Remove webview and initialize BrowserView.

[vsvipul]

What's this PR do?
Implements BrowserView with similar functionality as webview.
View is a class which we will use, it extends electron's BrowserView
class. ViewManager will be used to create, manage and delete views
in the main process. Removes all usage of webview and adds
an initial implementation of BrowserView.

You have tested this PR on:

• Windows
• Linux/Ubuntu
• macOS

[vsvipul]

I think this is ready. @akashnimare @abhigyank . Should i wait for this to be merged or add subsequent commits on top of this PR ?

[akashnimare]

add subsequent commits on top of this PR

Do this.

[abhigyank]

I think you need to rebase with zulip:browserview branch otherwise the two extra commits from master would also come in the changes.

[vsvipul]

I think you need to rebase with zulip:browserview branch otherwise the two extra commits from master would also come in the changes.

I think it will be better to keep this branch updated with master as well, so that we don't face merge conflicts while merging into master later.

[priyank-p]

Do export default const ViewManager = new ViewManager(); or something else but not this export = syntax. We need to stop using this.

[vsvipul]

export default const ViewManager = new ViewManager(); This doesn't seem to be working. Can you try this?

[priyank-p]

So I made a mistake here, this could be either export default new ViewManager() or

const viewManager = new ViewManager();
export default viewManager();

// then in index.ts
import ViewManager from './viewmanager';

I prefer the second one just because export default new ViewManager(); looks awkward to me (but that just me).

[akashnimare]

@vsvipul is this being taken care of?

[abhigyank]

This is some really good work. One important observation that I have - Moving to BrowserView shifts almost everything to the main process. Most of the functions surrounding webview previously was in renderer/main.js since webview was in renderer. In this change, we have kept the structure same and in places where webview functions are called we are using ipc to move those calls to the main process - even for functions like create, reload, load, switch etc. Now that browserView resides in the main process, would it make sense for us move all these important browserView actions entirely to the main process from renderer/main instead of doing ipc calls?
I am not sure if ipc calls use much CPU or memory, but I think it would make sense to have the the basic initialisation actions moved to main process.

[vsvipul]

I am not sure if ipc calls use much CPU or memory, but I think it would make sense to have the the basic initialisation actions moved to main process.

@abhigyank Yes, the initialization can be moved to main process, but switching tabs, and everything else, will still require ipc Calls.

[priyank-p]

@vsvipul I reviewed the first two commits and left minor comments. I don't like the passing index programmatically and having to manage it; I do rather the code handle that. Let's come up with a better model here, I guess we can come up with a better model if you describe what exactly it's needed for.

[priyank-p]

I think I mentioned this before but, the allowing a programmer to pass and track IDs to pass into this index field here is very brittle; it seems to call for trouble. Can we make this auto generated and make readonly property?

[vsvipul]

@vsvipul I reviewed the first two commits and left minor comments. I don't like the passing index programmatically and having to manage it; I do rather the code handle that. Let's come up with a better model here, I guess we can come up with a better model if you describe what exactly it's needed for.

Index is needed to create a view with that particular index so that we can easily remove and change to that view using only the index later, nothing else is required. What better model do you propose?

[priyank-p]

@vsvipul OK, here is what we can do, we can have a internal variable in view.t say viewIndex that is set to 0 at first. Then when we create a view in constructor we can do this.index = viewIndex; and increment it viewIndex++. That way we can access the index and do whatever we want like remove, select but we just don't have to pass it in.

[priyank-p]

Left some more comments, I like how we get to change tab.webview.props.name -> tabs.props.name.

[akashnimare]

@kanishk98 let's try to find out which commit broke the app on macOS.

[kanishk98]

@akashnimare I'm doing that right now. I don't think it's something to do with Electron, though - this seems to stem from a bug in activateTab(). I'm trying to fix it right now, will text you and Vipul on czo if I find something.

[akashnimare]

@kanishk98 any luck in debugging?

[kanishk98]

any luck in debugging?

Yeah, the app breaks at commit 9c4a3de. I'm currently working to see where the Mac-specific problem is.

[kanishk98]

Ah okay, the solution is almost too simple:

In viewmanager.select(), we need to reverse the order of function calls from

this.fixBounds();
mainWindow.setBrowserView(view);

to

mainWindow.setBrowserView(view);
this.fixBounds();

@vsvipul can you make the changes? I just tried this out on a Mac - it should solve our problem.

[akashnimare]

@kanishk98 are you sure this works for you? Can you post a gif here?

[kanishk98]

Can you post a gif here?

I can't post a gif right now (it's not my Mac that I tested this on), but you can fetch my branch browserview onto your system and try it out. Do let me know if it doesn't work.

[akashnimare]

@vsvipul can you fix the merge conflicts?

[vsvipul]

@akashnimare working on it right now. Sorry for the late response. Was a little busy this week.

[vsvipul]

I am having some difficulty porting the new notification option to browser view.

[kanishk98]

I am having some difficulty porting the new notification option to browser view.

I think I can help with that. Can you tell me what the issue is?

[vsvipul]

@kanishk98 How do i set the enabled state in the rendered process for that setting because the current URL in browser view is in the main procesS?

[kanishk98]

I can think of the following methods:

1. Use a synchronous IPC call (probably the simplest, but I'd avoid this).
2. Save a loggedIn flag in domain.json for all orgs that you can read when deciding whether enabled is true or not. Said flag is updated when the URL of the view changes.
3. I'm a little doubtful about this one, but we can also maintain an instance variable in ViewManager that maps server URLs to loggedIn status. Updated in the same way as (2).

Since I'm also working on this right now, I'll try what works on my end and let you know. Text me on czo if you have any further questions.

[akashnimare]

@kanishk98, @vsvipul awesome. Let's try to merge this asap since it's a blocker for other PRs.

[vsvipul]

@akashnimare Apparently, rebasing it automatically messes up my whole branch. The title property which I added gets removed, function names get changed due to which many ipc Calls stop working. I'll have to do this commit by commit myself. So, it is going to take some time.

[akashnimare]

@vsvipul maybe reset the branch locally and git pull + rebase and fix merge conflicts? There are three files mainly where the conflicts thing happening.

[vsvipul]

@akashnimare Yeah, i did exactly that, but that still messes up a lot of stuff. I am doing it but will take some time.

[akashnimare]

@vsvipul let me know if you need help. Gotta merge this asap since a lot of other PRs are still pending.

[vsvipul]

@akashnimare As I am currently a little busy with academic work, I have given access to my branch to @kanishk98 a few days back. He is working on rebasing and migrating the notification option he added earlier.

[akashnimare]

Cool, thanks for the update!

[zulipbot]

Heads up @vsvipul, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/master branch and resolve your pull request's merge conflicts accordingly.

[vsvipul]

Closing this in favor of #831 .

[vsvipul]

Yes @andersk . Thanks. Updated.