Early Draft Discussion: Delegate popup creation + action handling

[pnegahdar]

This is a super early draft of a few changes to the popup/browserActions that helped me integrate it a bit. Still need to figure out what to do with a few stale pieces now.

Major changes:

• Added getStateWithIcons that returns the state with the base64 URI for the icon of the avatar. This gives a good workdaround for https://github.com/samuelmaddock/electron-browser-shell/issues/41
• Added ChromeExtensionImpl.createPopup?(details: PopupViewOptions): void . This way you can handle making your own popup instead of using the PopupView
• Added ChromeExtensionImpl.onBrowserActionUpdate?(details: BrowserActionState): void. This way you can watch for icon/callback changes and construct your own UI without using the customElement
• Added ElectronChromeExtensions.activateExtension to manually trigger click/contextmenu if you created your own elements.

Before I cleanup the code I want to make sure I'm on the right path. What are your thoughts on this?

---

✅ By sending this pull request, I agree to the Contributor License Agreement of this project.

[samuelmaddock]

I think the idea of exposing an API for creating custom UIs is fine. This is obviously more work though for users of the library. Are there certain features lacking from the existing <browser-action-list> and popup which require you to take this approach?

Regarding extending ChromeExtensionImpl, these methods were meant to provide implementations for specific chrome.* extension APIs. Events emitted from the ElectronChromeExtensions class are another alternative.

'browser-action-popup-created' is an event which is emitted when the PopupView is created. You could call popupView.destroy() to effectively prevent it and replace it with your own.

const extensions = new ElectronChromeExtensions({});
extensions.on('browser-action-popup-created', (popupView) => {
  popupView.destroy();
  // Create custom popup here
});

I would be in favor of a design like the following:

class ElectronChromeExtensions {
  /** Called prior to popup being created. Calling `event.preventDefault()` will prevent popup creation. */
  on('will-create-popup', (event: Event, details: PopupViewOptions) => void);
  
  /** Called after popup has been created. */
  on('did-create-popup', (popupView: PopupView) => void);
}

[pnegahdar]

I think the idea of exposing an API for creating custom UIs is fine. This is obviously more work though for users of the library.

Probably true. In our case we already have a lot of the piping in there already which made handling it ourselves super straight forward (i.e. ipc events with icon images into our state manager and render them in React).

Are there certain features lacking from the existing and popup which require you to take this approach?

Three main reasons:

1 - UI customizations beyond CSS. Also note that, as is, it would be impossible to implement something more complex like Chrome's extension pinning and extension menu. 2 - the injecting the customElement felt weird to me given the browserActions can be completely isolated from the session where the extension is bound to, which led to things like #41 3 - It felt like a natural extension to the current API

I think (3) is a big point because the core needs to export like 2 more methods (popup-creation and context menu creation) to be very standalone and tight. Right now it feels like it was design a bit to satisfy building electron-browser-shell.

Tangentially and interestingly, what surprised me is how the core electron-chrome-extensions lib doesn't even need to be too coupled with Electron. Its as though you can even pass it just a few APIs (e.g. executeJS, setPreloads) and have chrome extension support in other webviews types too (e.g. Safari). The dream API for the kernel, for me, then would be:

new Extension(
    session_apis: { executeJavascript, setPreloads.... }, 
    chrome_apis: {... builtinDefaultChromeApiImpls, ...anyCustomOverrides},  // most setups wont use this
    implementation_handlers: {createTab, selectTab, ... createPopup, createContextMenu  } // while these are used by the chrome_apis they aren't necessarily chrome apis
) 

Then you can have a small lib that uses that to implement <browser-action-list>/PopupView/etc for usages like the browser-shell and by the looks of it electron-chrome-extensions would shed a lot of weight!

This also clarifies my confusion around ChromeExtensionImpl, which felt to me less like low level implementations of the chrome* apis, but, rather, small handlers you need to pass in for the default library to satisfy the chrome apis (in that case, createPopup/createContextMenu would also be fitting).

Re popups: agreed! that looks better.

With all that said, I have enough to get running on the fork, so I don't want to force the APIs to go somewhere they don't want to go. Let me know how best to proceed -- "do nothing for now" is fair!