From 15c53961696c186ee799e7bbbe6efc40d9eff128 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Thu, 14 Feb 2019 11:45:11 -0800 Subject: [PATCH] Re-added "Settings" panel to browser extension and (hopefully) fixed a lot of sources of error --- shells/browser/chrome/manifest.json | 2 +- shells/browser/firefox/manifest.json | 2 +- shells/browser/shared/build.js | 2 +- .../shared/{panel.html => elements.html} | 2 +- shells/browser/shared/src/main.js | 101 +++++++++++++++--- shells/browser/shared/src/panel.js | 63 ----------- shells/browser/shared/src/panels/elements.js | 3 + shells/browser/shared/src/panels/settings.js | 3 + shells/browser/shared/src/panels/utils.js | 53 +++++++++ shells/browser/shared/src/settings.js | 48 --------- shells/browser/shared/webpack.config.js | 4 +- shells/dev/src/devtools.js | 4 + src/backend/agent.js | 15 ++- src/devtools/store.js | 11 +- src/devtools/views/DevTools.js | 5 +- src/devtools/views/Element.js | 2 + src/devtools/views/TreeContext.js | 2 +- 17 files changed, 180 insertions(+), 142 deletions(-) rename shells/browser/shared/{panel.html => elements.html} (93%) delete mode 100644 shells/browser/shared/src/panel.js create mode 100644 shells/browser/shared/src/panels/elements.js create mode 100644 shells/browser/shared/src/panels/settings.js create mode 100644 shells/browser/shared/src/panels/utils.js delete mode 100644 shells/browser/shared/src/settings.js diff --git a/shells/browser/chrome/manifest.json b/shells/browser/chrome/manifest.json index 5b06474126..3b3cec6d91 100644 --- a/shells/browser/chrome/manifest.json +++ b/shells/browser/chrome/manifest.json @@ -28,8 +28,8 @@ "content_security_policy": "script-src 'self' 'unsafe-eval'; object-src 'self'", "web_accessible_resources": [ + "elements.html", "main.html", - "panel.html", "settings.html", "build/backend.js" ], diff --git a/shells/browser/firefox/manifest.json b/shells/browser/firefox/manifest.json index 824aa00520..2829d100e6 100644 --- a/shells/browser/firefox/manifest.json +++ b/shells/browser/firefox/manifest.json @@ -34,8 +34,8 @@ "content_security_policy": "script-src 'self' 'unsafe-eval'; object-src 'self'", "web_accessible_resources": [ + "elements.html", "main.html", - "panel.html", "settings.html", "build/backend.js" ], diff --git a/shells/browser/shared/build.js b/shells/browser/shared/build.js index dd57af36ac..b8107aa1ad 100644 --- a/shells/browser/shared/build.js +++ b/shells/browser/shared/build.js @@ -10,8 +10,8 @@ const { join } = require('path'); const STATIC_FILES = [ 'icons', 'popups', + 'elements.html', 'main.html', - 'panel.html', 'settings.html', ]; diff --git a/shells/browser/shared/panel.html b/shells/browser/shared/elements.html similarity index 93% rename from shells/browser/shared/panel.html rename to shells/browser/shared/elements.html index 60fd1bdf13..bbaa81a0f2 100644 --- a/shells/browser/shared/panel.html +++ b/shells/browser/shared/elements.html @@ -27,6 +27,6 @@
Unable to find React on the page.
- + diff --git a/shells/browser/shared/src/main.js b/shells/browser/shared/src/main.js index 67e4d36d1d..9703b59f46 100644 --- a/shells/browser/shared/src/main.js +++ b/shells/browser/shared/src/main.js @@ -1,5 +1,8 @@ /* global chrome */ +import Bridge from 'src/bridge'; +import Store from 'src/devtools/Store'; + let panelCreated = false; function createPanelIfReactLoaded() { @@ -9,37 +12,105 @@ function createPanelIfReactLoaded() { chrome.devtools.inspectedWindow.eval( 'window.__REACT_DEVTOOLS_GLOBAL_HOOK__ && window.__REACT_DEVTOOLS_GLOBAL_HOOK__.renderers.size > 0', - function(pageHasReact, err) { + function(pageHasReact, error) { if (!pageHasReact || panelCreated) { return; } - clearInterval(loadCheckInterval); panelCreated = true; - chrome.devtools.panels.create('⚛ Elements', '', 'panel.html', panel => { - panel.onShown.addListener(function(window) { - // TODO: When the user switches to the panel, check for an Elements tab selection. - }); - panel.onHidden.addListener(function() { - // TODO: Stop highlighting and stuff. - }); - }); - /* TODO Revisit this architecture; currently it causes duplicate agent/bridge traffic. + clearInterval(loadCheckInterval); + + let bridge = null; + let store = null; + let elementsPanel = null; + let settingsPanel = null; + + function initBridgeAndStore() { + let hasPortBeenDisconnected = false; + const port = chrome.runtime.connect({ + name: '' + chrome.devtools.inspectedWindow.tabId, + }); + port.onDisconnect.addListener(() => { + hasPortBeenDisconnected = true; + }); + + bridge = new Bridge({ + listen(fn) { + port.onMessage.addListener(message => fn(message)); + }, + send(event: string, payload: any, transferable?: Array) { + if (!hasPortBeenDisconnected) { + port.postMessage({ event, payload }, transferable); + } + }, + }); + + store = new Store(bridge); + + if (elementsPanel !== null) { + elementsPanel.injectBridgeAndStore(bridge, store); + } + if (settingsPanel !== null) { + settingsPanel.injectBridgeAndStore(bridge, store); + } + } + + initBridgeAndStore(); + + chrome.devtools.panels.create( + '⚛ Elements', + '', + 'elements.html', + panel => { + panel.onShown.addListener(panel => { + if (elementsPanel === null) { + panel.injectBridgeAndStore(bridge, store); + } + + elementsPanel = panel; + + // TODO: When the user switches to the panel, check for an Elements tab selection. + }); + panel.onHidden.addListener(() => { + // TODO: Stop highlighting and stuff. + }); + } + ); + chrome.devtools.panels.create( '⚛ Settings', '', 'settings.html', - panel => {} + panel => { + panel.onShown.addListener(panel => { + if (settingsPanel === null) { + panel.injectBridgeAndStore(bridge, store); + } + + settingsPanel = panel; + }); + } ); - */ + + chrome.devtools.network.onNavigated.removeListener(checkPageForReact); + + // Shutdown bridge and re-initialize DevTools panel when a new page is loaded. + chrome.devtools.network.onNavigated.addListener(function onNavigated() { + bridge.send('shutdown'); + + initBridgeAndStore(); + }); } ); } -chrome.devtools.network.onNavigated.addListener(function() { +// Load (or reload) the DevTools extension when the user navigates to a new page. +function checkPageForReact() { createPanelIfReactLoaded(); -}); +} + +chrome.devtools.network.onNavigated.addListener(checkPageForReact); // Check to see if React has loaded once per second in case React is added // after page load diff --git a/shells/browser/shared/src/panel.js b/shells/browser/shared/src/panel.js deleted file mode 100644 index 56780efe67..0000000000 --- a/shells/browser/shared/src/panel.js +++ /dev/null @@ -1,63 +0,0 @@ -/* global chrome */ - -import { createElement } from 'react'; -import { createRoot, flushSync } from 'react-dom'; -import Bridge from 'src/bridge'; -import DevTools from 'src/devtools/views/DevTools'; -import inject from './inject'; -import { getBrowserName, getBrowserTheme } from './utils'; - -const container = ((document.getElementById('container'): any): HTMLElement); - -function injectAndInit() { - let disconnected = false; - - const port = chrome.runtime.connect({ - name: '' + chrome.devtools.inspectedWindow.tabId, - }); - port.onDisconnect.addListener(() => { - disconnected = true; - }); - - const bridge = new Bridge({ - listen(fn) { - port.onMessage.addListener(message => fn(message)); - }, - send(event: string, payload: any, transferable?: Array) { - if (disconnected) { - return; - } - port.postMessage({ event, payload }, transferable); - }, - }); - - // Clear the "React not found" initial message before rendering. - container.innerHTML = ''; - - const root = createRoot(container); - root.render( - createElement(DevTools, { - bridge, - browserName: getBrowserName(), - browserTheme: getBrowserTheme(), - defaultTab: 'elements', - showTabBar: false, - }) - ); - - // Initialize the backend only once the DevTools frontend Store has been initialized. - // Otherwise the Store may miss important initial tree op codes. - inject(chrome.runtime.getURL('build/backend.js')); - - // Reload the DevTools extension when the user navigates to a new page. - function onNavigated() { - chrome.devtools.network.onNavigated.removeListener(onNavigated); - - bridge.send('shutdown'); - - flushSync(() => root.unmount(injectAndInit)); - } - chrome.devtools.network.onNavigated.addListener(onNavigated); -} - -injectAndInit(); diff --git a/shells/browser/shared/src/panels/elements.js b/shells/browser/shared/src/panels/elements.js new file mode 100644 index 0000000000..ef7f5e80df --- /dev/null +++ b/shells/browser/shared/src/panels/elements.js @@ -0,0 +1,3 @@ +import { createPanel } from './utils'; + +createPanel('elements'); diff --git a/shells/browser/shared/src/panels/settings.js b/shells/browser/shared/src/panels/settings.js new file mode 100644 index 0000000000..a8dd78b050 --- /dev/null +++ b/shells/browser/shared/src/panels/settings.js @@ -0,0 +1,3 @@ +import { createPanel } from './utils'; + +createPanel('settings'); diff --git a/shells/browser/shared/src/panels/utils.js b/shells/browser/shared/src/panels/utils.js new file mode 100644 index 0000000000..3863f734f5 --- /dev/null +++ b/shells/browser/shared/src/panels/utils.js @@ -0,0 +1,53 @@ +/* global chrome */ + +import { createElement } from 'react'; +import { createRoot, flushSync } from 'react-dom'; +import DevTools from 'src/devtools/views/DevTools'; +import inject from '../inject'; +import { getBrowserName, getBrowserTheme } from '../utils'; + +export function createPanel(defaultTab) { + let injectedBridge = null; + let injectedStore = null; + let root = null; + + // All DevTools panel share a single Bridge and Store instance. + // The main script will inject those shared instances using this method. + window.injectBridgeAndStore = (bridge, store) => { + injectedBridge = bridge; + injectedStore = store; + + if (root === null) { + injectAndInit(); + } else { + // It's easiest to recreate the DevTools panel (to clean up potential stale state). + // We can revisit this in the future as a small optimization. + flushSync(() => root.unmount(injectAndInit)); + } + }; + + function injectAndInit() { + const container = ((document.getElementById( + 'container' + ): any): HTMLElement); + + // Clear the "React not found" initial message before rendering. + container.innerHTML = ''; + + root = createRoot(container); + root.render( + createElement(DevTools, { + bridge: injectedBridge, + browserName: getBrowserName(), + browserTheme: getBrowserTheme(), + defaultTab, + showTabBar: false, + store: injectedStore, + }) + ); + + // Initialize the backend only once the DevTools frontend Store has been initialized. + // Otherwise the Store may miss important initial tree op codes. + inject(chrome.runtime.getURL('build/backend.js')); + } +} diff --git a/shells/browser/shared/src/settings.js b/shells/browser/shared/src/settings.js deleted file mode 100644 index d51dba755d..0000000000 --- a/shells/browser/shared/src/settings.js +++ /dev/null @@ -1,48 +0,0 @@ -/* global chrome */ - -import { createElement } from 'react'; -import { createRoot, flushSync } from 'react-dom'; -import Bridge from 'src/bridge'; -import DevTools from 'src/devtools/views/DevTools'; -import inject from './inject'; -import { getBrowserName, getBrowserTheme } from './utils'; - -const container = ((document.getElementById('container'): any): HTMLElement); - -function injectAndInit() { - // Noop Bridge for the Settings panel - const bridge = new Bridge({ - listen(fn) {}, - send(event: string, payload: any, transferable?: Array) {}, - }); - - // Clear the "React not found" initial message before rendering. - container.innerHTML = ''; - - const root = createRoot(container); - root.render( - createElement(DevTools, { - bridge, - browserName: getBrowserName(), - browserTheme: getBrowserTheme(), - defaultTab: 'settings', - showTabBar: false, - }) - ); - - // Initialize the backend only once the DevTools frontend Store has been initialized. - // Otherwise the Store may miss important initial tree op codes. - inject(chrome.runtime.getURL('build/backend.js')); - - // Reload the DevTools extension when the user navigates to a new page. - function onNavigated() { - chrome.devtools.network.onNavigated.removeListener(onNavigated); - - bridge.send('shutdown'); - - flushSync(() => root.unmount(injectAndInit)); - } - chrome.devtools.network.onNavigated.addListener(onNavigated); -} - -injectAndInit(); diff --git a/shells/browser/shared/webpack.config.js b/shells/browser/shared/webpack.config.js index 372889ab4b..0a304f4a64 100644 --- a/shells/browser/shared/webpack.config.js +++ b/shells/browser/shared/webpack.config.js @@ -16,8 +16,8 @@ module.exports = { contentScript: './src/contentScript.js', inject: './src/GlobalHook.js', main: './src/main.js', - panel: './src/panel.js', - settings: './src/settings.js', + elements: './src/panels/elements.js', + settings: './src/panels/settings.js', }, output: { path: __dirname + '/build', diff --git a/shells/dev/src/devtools.js b/shells/dev/src/devtools.js index d1a62aede6..accc6af827 100644 --- a/shells/dev/src/devtools.js +++ b/shells/dev/src/devtools.js @@ -6,6 +6,7 @@ import { createRoot } from 'react-dom'; import Bridge from 'src/bridge'; import { installHook } from 'src/hook'; import { initDevTools } from 'src/devtools'; +import Store from 'src/devtools/Store'; import DevTools from 'src/devtools/views/DevTools'; const iframe = ((document.getElementById('target'): any): HTMLIFrameElement); @@ -53,6 +54,8 @@ inject('./build/app.js', () => { cb(bridge); + const store = new Store(bridge); + const root = createRoot(container); const batch = root.createBatch(); batch.render( @@ -61,6 +64,7 @@ inject('./build/app.js', () => { browserName: 'Chrome', browserTheme: 'light', showTabBar: true, + store, }) ); batch.then(() => { diff --git a/src/backend/agent.js b/src/backend/agent.js index b23ff86d75..e14ec53c05 100644 --- a/src/backend/agent.js +++ b/src/backend/agent.js @@ -25,11 +25,11 @@ export default class Agent extends EventEmitter { addBridge(bridge: Bridge) { this._bridge = bridge; - bridge.addListener('shutdown', () => this.emit('shutdown')); - bridge.addListener('highlightElementInDOM', this.highlightElementInDOM); bridge.addListener('inspectElement', this.inspectElement); bridge.addListener('selectElement', this.selectElement); + bridge.addListener('shutdown', this.shutdown); + // TODO Listen to bridge for things like selection. // bridge.on('...'), this...); } @@ -94,8 +94,17 @@ export default class Agent extends EventEmitter { this._rendererInterfaces[rendererID] = rendererInterface; } + shutdown = () => { + this.emit('shutdown'); + }; + onHookOperations = (operations: Uint32Array) => { debug('onHookOperations', operations); - this._bridge.send('operations', operations, [operations.buffer]); + + // TODO The chrome.runtime does not currently support transferables; it forces JSON serialization. + // The Store has a fallback in place that parses the message as JSON if the type isn't an array. + // Sometimes using transferrables also cause Chrome or Firefox to throw "ArrayBuffer at index 0 is already neutered". + // this._bridge.send('operations', operations, [operations.buffer]); + this._bridge.send('operations', operations); }; } diff --git a/src/devtools/store.js b/src/devtools/store.js index 59465fd3e4..388ae24599 100644 --- a/src/devtools/store.js +++ b/src/devtools/store.js @@ -28,6 +28,8 @@ const debug = (methodName, ...args) => { * ContextProviders can subscribe to the Store for specific things they want to provide. */ export default class Store extends EventEmitter { + _bridge: Bridge; + // Map of ID to Element. // Elements are mutable (for now) to avoid excessive cloning during tree updates. _idToElement: Map = new Map(); @@ -52,8 +54,9 @@ export default class Store extends EventEmitter { debug('constructor', 'subscribing to Bridge'); - bridge.on('operations', this.onBridgeOperations); - bridge.on('shutdown', this.onBridgeShutdown); + this._bridge = bridge; + this._bridge.on('operations', this.onBridgeOperations); + this._bridge.on('shutdown', this.onBridgeShutdown); } get numElements(): number { @@ -402,8 +405,8 @@ export default class Store extends EventEmitter { onBridgeShutdown = () => { debug('onBridgeShutdown', 'unsubscribing from Bridge'); - bridge.off('operations', this.onBridgeOperations); - bridge.off('shutdown', this.onBridgeShutdown); + this._bridge.off('operations', this.onBridgeOperations); + this._bridge.off('shutdown', this.onBridgeShutdown); }; // DEBUG diff --git a/src/devtools/views/DevTools.js b/src/devtools/views/DevTools.js index 0c987d5c36..9b87c2dea8 100644 --- a/src/devtools/views/DevTools.js +++ b/src/devtools/views/DevTools.js @@ -1,6 +1,6 @@ // @flow -import React, { useMemo, useState } from 'react'; +import React, { useState } from 'react'; import Store from '../store'; import { BridgeContext, StoreContext } from './context'; import Elements from './Elements'; @@ -26,6 +26,7 @@ export type Props = {| defaultTab?: TabID, browserTheme: BrowserTheme, showTabBar?: boolean, + store: Store, |}; export default function DevTools({ @@ -34,8 +35,8 @@ export default function DevTools({ defaultTab = 'elements', browserTheme = 'light', showTabBar = false, + store, }: Props) { - const store = useMemo(() => new Store(bridge), []); const [tab, setTab] = useState(defaultTab); let tabElement; diff --git a/src/devtools/views/Element.js b/src/devtools/views/Element.js index fc25616693..5fe0bc8816 100644 --- a/src/devtools/views/Element.js +++ b/src/devtools/views/Element.js @@ -44,6 +44,8 @@ export default function ElementView({ index, style }: Props) { const isSelected = selectedElementID === id; const showDollarR = isSelected && type === ElementTypeClassOrFunction; + // TODO styles.SelectedElement is 100% width but it doesn't take horizontal overflow into account. + return (