{"action":"closed","issue":{"url":"https://api.github.com/repos/brave/browser-laptop/issues/13216","repository_url":"https://api.github.com/repos/brave/browser-laptop","labels_url":"https://api.github.com/repos/brave/browser-laptop/issues/13216/labels{/name}","comments_url":"https://api.github.com/repos/brave/browser-laptop/issues/13216/comments","events_url":"https://api.github.com/repos/brave/browser-laptop/issues/13216/events","html_url":"https://github.com/brave/browser-laptop/issues/13216","id":298835991,"number":13216,"title":"Enhance performance by attaching only active tabs to a webview (single-webview)","user":{"login":"petemill","id":741836,"avatar_url":"https://avatars1.githubusercontent.com/u/741836?v=4","gravatar_id":"","url":"https://api.github.com/users/petemill","html_url":"https://github.com/petemill","followers_url":"https://api.github.com/users/petemill/followers","following_url":"https://api.github.com/users/petemill/following{/other_user}","gists_url":"https://api.github.com/users/petemill/gists{/gist_id}","starred_url":"https://api.github.com/users/petemill/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/petemill/subscriptions","organizations_url":"https://api.github.com/users/petemill/orgs","repos_url":"https://api.github.com/users/petemill/repos","events_url":"https://api.github.com/users/petemill/events{/privacy}","received_events_url":"https://api.github.com/users/petemill/received_events","type":"User","site_admin":false},"labels":[{"id":732347942,"url":"https://api.github.com/repos/brave/browser-laptop/labels/initiative/perf","name":"initiative/perf","color":"fbca04","default":false},{"id":435742051,"url":"https://api.github.com/repos/brave/browser-laptop/labels/needs-investigation","name":"needs-investigation","color":"fbca04","default":false},{"id":690557754,"url":"https://api.github.com/repos/brave/browser-laptop/labels/priority/P1","name":"priority/P1","color":"b60205","default":false}],"state":"closed","locked":false,"assignee":{"login":"bridiver","id":34129,"avatar_url":"https://avatars3.githubusercontent.com/u/34129?v=4","gravatar_id":"","url":"https://api.github.com/users/bridiver","html_url":"https://github.com/bridiver","followers_url":"https://api.github.com/users/bridiver/followers","following_url":"https://api.github.com/users/bridiver/following{/other_user}","gists_url":"https://api.github.com/users/bridiver/gists{/gist_id}","starred_url":"https://api.github.com/users/bridiver/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/bridiver/subscriptions","organizations_url":"https://api.github.com/users/bridiver/orgs","repos_url":"https://api.github.com/users/bridiver/repos","events_url":"https://api.github.com/users/bridiver/events{/privacy}","received_events_url":"https://api.github.com/users/bridiver/received_events","type":"User","site_admin":false},"assignees":[{"login":"bridiver","id":34129,"avatar_url":"https://avatars3.githubusercontent.com/u/34129?v=4","gravatar_id":"","url":"https://api.github.com/users/bridiver","html_url":"https://github.com/bridiver","followers_url":"https://api.github.com/users/bridiver/followers","following_url":"https://api.github.com/users/bridiver/following{/other_user}","gists_url":"https://api.github.com/users/bridiver/gists{/gist_id}","starred_url":"https://api.github.com/users/bridiver/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/bridiver/subscriptions","organizations_url":"https://api.github.com/users/bridiver/orgs","repos_url":"https://api.github.com/users/bridiver/repos","events_url":"https://api.github.com/users/bridiver/events{/privacy}","received_events_url":"https://api.github.com/users/bridiver/received_events","type":"User","site_admin":false},{"login":"petemill","id":741836,"avatar_url":"https://avatars1.githubusercontent.com/u/741836?v=4","gravatar_id":"","url":"https://api.github.com/users/petemill","html_url":"https://github.com/petemill","followers_url":"https://api.github.com/users/petemill/followers","following_url":"https://api.github.com/users/petemill/following{/other_user}","gists_url":"https://api.github.com/users/petemill/gists{/gist_id}","starred_url":"https://api.github.com/users/petemill/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/petemill/subscriptions","organizations_url":"https://api.github.com/users/petemill/orgs","repos_url":"https://api.github.com/users/petemill/repos","events_url":"https://api.github.com/users/petemill/events{/privacy}","received_events_url":"https://api.github.com/users/petemill/received_events","type":"User","site_admin":false}],"milestone":{"url":"https://api.github.com/repos/brave/browser-laptop/milestones/109","html_url":"https://github.com/brave/browser-laptop/milestone/109","labels_url":"https://api.github.com/repos/brave/browser-laptop/milestones/109/labels","id":3246523,"number":109,"title":"0.22.x Release 3 (Beta channel)","description":"Single-webview","creator":{"login":"bsclifton","id":4733304,"avatar_url":"https://avatars3.githubusercontent.com/u/4733304?v=4","gravatar_id":"","url":"https://api.github.com/users/bsclifton","html_url":"https://github.com/bsclifton","followers_url":"https://api.github.com/users/bsclifton/followers","following_url":"https://api.github.com/users/bsclifton/following{/other_user}","gists_url":"https://api.github.com/users/bsclifton/gists{/gist_id}","starred_url":"https://api.github.com/users/bsclifton/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/bsclifton/subscriptions","organizations_url":"https://api.github.com/users/bsclifton/orgs","repos_url":"https://api.github.com/users/bsclifton/repos","events_url":"https://api.github.com/users/bsclifton/events{/privacy}","received_events_url":"https://api.github.com/users/bsclifton/received_events","type":"User","site_admin":false},"open_issues":8,"closed_issues":150,"state":"open","created_at":"2018-04-05T23:58:53Z","updated_at":"2018-05-11T17:06:24Z","due_on":null,"closed_at":null},"comments":1,"created_at":"2018-02-21T04:10:26Z","updated_at":"2018-05-11T17:06:24Z","closed_at":"2018-05-11T17:06:24Z","author_association":"MEMBER","body":"## Description\r\nAt the moment, a `<webview>` element is created for each open tab in a window. This creates the following issues:\r\n- Additional DOM elements\r\n- `<webview>` overhead in muon - it's a plugin that has rendering overhead - the guestview assumes that a guest (attached to a webview) is visible, even if we make the <webview> invisible through css\r\n- Attach events firing for all tabs at the same time seems to create an event storm that creates even further performance impact by each event creating one or more redux actions, which then may also attempt to make render changes.\r\n\r\nIn order to fix this, we aim to only have a single webview per active / previewed tab, switching the guest contents that are attached to the webview when the active or previewed tab changes.\r\n\r\nThis will mainly fix startup with a profile containing many tabs, but should also improve general responsiveness and possible memory usage.\r\n\r\n## Steps to reproduce:\r\n### Manual\r\nStartup with a profile containing 4 windows with 150 tabs in each window. All tabs marked as unloaded: true (which is enforced in Brave on shutdown for all saved tabs)\r\n\r\n#### Expected result\r\n- App starts within a few seconds\r\n- Only the active tab loads its content\r\n\r\n#### Actual result\r\n- App takes > 5 minutes to load\r\n- Only the active tab loads its content\r\n\r\n### Automated perf test from #11424 \r\nAutomated test (feature/perf-tests branch): https://github.com/brave/browser-laptop/pull/10517/commits/648bafdaaf1308d966d415b98beba5e22664d457#diff-e5c305031ff191e0f8d1fb021abeb435\r\n\r\n  1. Start Brave\r\n  2. Open 100 tabs each with unique URLs\r\n  3. Close Brave\r\n  4. Open Brave. It should remember the 100 tabs.\r\n  5. Focus the URL bar, type a URL and hit enter to navigate.\r\n\r\n**Actual result:**\r\nMuch slower with many tabs\r\n\r\n**Expected result:**\r\nSimilar speed to a fresh profile\r\n\r\n## Prerequisites\r\n\r\n- [x] Ability to receive events for a tab in the render process without a webview.\r\nThis allows us to not move every bit of content functionality to the browser process from the renderer process which, although will be beneficial for performance too, will be too big of a refactor to do at once.\r\n**Status:** Working implementation in [muon/single-webview2](https://github.com/brave/muon/compare/single-webview2) with [petemill/browser-laptop/single-webview2](https://github.com/brave/browser-laptop/compare/single-webview2...petemill:single-webview2?expand=1)\r\n\r\n\r\n\r\n## TODO\r\n\r\n### Bugs\r\n\r\n#### Content-related\r\n\r\n- [x] Ability to load contents before tabs are attached to a GuestViewContainer.\r\nCurrently, if a tab does not have a `<webview>`, it will not proceed with loading in the background. This is the same reason that in the current version of browser-laptop, there is a delay in loading background tabs whilst the frame/<webview> is created and attached to. In the meantime 'Untitled' is displayed as the tab title. The WebContents SetOwnerWindow method is not called. Perhaps the contents are not activated until they are attached (`guest_view_base.cc::ActivateContents` checks if is attached)? It will also not fire its updated events when made active (although it will fire the `set-active` event, which we previously were not relying on). We need to allow tab's without `<webviews>` to load, whilst preserving lazy-load functionality via the `discarded: true` flag in a tab's `createProperties`, (e.g. background ctrl-click tabs)\r\n\r\n\r\n- [x] Background tabs remain active but do not continue processing requests or finishing navigation.\r\n        - e.g. a tab which console.logs and reloads on a setTimeout, javascript will continue to be processed whilst tab is not attached to a `<webview>` and `load-start` & `'did-get-response-details'` events will be received for tab, but not until the tab is re-attached will the tab send `navigation-entry-commited` & `did-navigate` & `dom-ready`, etc events\r\n        - e.g. a tab which you switch to and whilst still loading switch away. The loading spinner will never stop because we don't receive those events.\r\n\r\n\r\n - [x] tab.discard() does not destroy/replace unattached tab contents\r\nTab content memory will not be released if the tab is discarded whilst it is detached. When it is eventually re-attached (e.g. when it is made active) the tab will immediately discard at that point and then load again, which kind of defeats the purpose.\r\n\r\n- [x] many tabs startup crash: `[36162:775:0322/105702.875384:FATAL:navigator_impl.cc(156)] Check failed: 0.`\r\n\r\n- [x] move never-displayed tabs to a new window\r\n\r\n- [x] Background tab theme color is incorrect\r\n  - workaround: wait for first window resize event\r\n\r\n- [x] Lazy-load tabs are loaded initially then reloaded on first visit\r\n\r\n- [ ] background loaded tabs have initial DOM window inner{Width,Height} size of 0x0 (webcompat) (minor)\r\n    - `RenderWidgetHostViewGuest::GetViewBounds` returns `0 x 0` size\r\n      `WebContentsImpl::GetSizeForNewRenderView` returns the default `300 x 300` size\r\n- [x] detach then move tab to new window crash (check if fixed)\r\n\r\n- [ ] clone tab (check if works)\r\n\r\n#### Webview attaching / painting issues\r\n\r\n- [x] Webview cannot attach anything else if `will-destroy` of contents has been emitted whilst attached to it via guest\r\n    For all subsequent `<webview>.attachGuest()` calls, those contents do not get attach events, even though web view / guest view container thinks it is attached successfully.\r\n    Perhaps something else is being destroyed and not re-created / reset, inside browser_plugin_guest or browser_plugin_embedder.\r\n    A new `<webview>` can attach to the tabs fine.\r\n    ~**Workaround:** Another reason to use a new `<webview>` each time a tab is made active~\r\n- [x] Very often, when a guest attaches to a <webview> it has already been attached to previously, the webview is white and *will never paint, even when re-paint forced (hide and show in js)*\r\n    ~**Workaround:** (see `webviewDisplay.js`) Create and use a new <webview> each time we want to attach and display a guest.~\r\n\r\n- [x] harden race conditions for rapid attach / detach / destroy (rapid cmd-w, tab switching, detaching)\r\n\r\n- [x] GuestView receives no events when not attached to webview, and duplicate events for guest when re-attached\r\n   **Solution:** fixed in muon with https://github.com/brave/muon/commit/2fbabc61b64338c6b0549cf67c4a72030b977ab6 (new `set-window` event for assigning to first / subsequent window IDs, similar to `add-new-contents`), see https://github.com/brave/muon/issues/515 **requires review from @bridiver**\r\n\r\n- [ ] Removing <webview> from DOM destroys most-recently-attached WebContents, even if detached\r\n  ~**Solution:** fixed in muon with https://github.com/brave/muon/commit/32ebe0b1dbeb62e99fd00222c137afd1adf36025 (**requires review with @bridiver**)~ <-- reverted\r\n  ~Modified `guest_view_container` not to call `guest_view.destroy()` on the last attached guest (`detach()` does not remove `this.guest`)\r\n  We still want the guest to be destroyed when the web contents is, and that's ok because it's done in guest_view_base.cc WebContentsDestroyed()~\r\n  **Status:** Not a big blocker if we're not re-creating webview every active tab change.\r\n\r\n#### Solved by webview pool (2 webviews)\r\n\r\n- [x] Very often, detaching a guest, attaching another guest and then re-attaching the original guest will result in a white <webview>, even if its to a different / new <webview> that has never been attached to before. Forcing a paint by resizing the <webview> or changing a css style attribute and then back again fixes this (although causing another flicker which we hide).\r\n    **Workaround:** (see `webviewDisplay.js`) AttachGuest -> Hide <webview> -> Show <webview>. Unfortunate since this takes an additional > 50ms before we are sure the frame is visible.\r\n\r\n- [x] <webview> has brief white flash between attaches\r\n    **Workaround:** (see `webviewDisplay.js`) Use 2 single-webviews, only hide the currently attached webview when the newly attached webview is attached.\r\n\r\n\r\n#### State-related\r\n\r\n- [x] webview focus timing (especially re-open closed tab)\r\n- [x] tab title / loading status if events happen before event handler can be attached\r\n    - solution, handle all tab events immediately on web contents creation, don't wait for react render cycle\r\n\r\n\r\n\r\n### Implementation\r\n- [x] active frame display\r\n\r\n- [x] implement tab preview\r\n\r\n- [x] Ability to move tabs between windows, even if they have not had a guest attached to their assigned window yet. @bridiver agress we implement something similar to [WebExtension's `tab.move(tabId, { windowId, index })`](https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs/move): https://github.com/brave/muon/issues/489.\r\n    **Workaround:** Working workaround implemented via using new 'set-window-id' event to remove frame from departing window, and bypassing the `tab.detach()` call if `!tab.attached`. The tab 'gets' to the new window since we always make a moved frame active, so it gets attached to a `<webview>`. Not ideal since a move to a new window could happen internally to muon, but works for basic browser-laptop tab moving.\r\n\r\n\r\n\r\n### Cleanup\r\n- [x] Tidy up `frame.js` - consider non-React class, and do not use fake DOM element as a stand-in for eventemitter.\r\n- [ ] remove logging / put behind flag\r\n- [x] consider `tab-detached-at` should be emitted by embedder contents not tab\r\n\r\n## Testing\r\n\r\n### Areas to focus\r\nFocus areas for testing are contained on https://github.com/brave/browser-laptop/issues/13216 but essentially:\r\n- Tab creation, switching, closing and restore on restart\r\n- webcompat of various sites, especially for tabs loading in background through cmd/ctrl-click links as well as foreground loading)\r\n- entry and exit of fullscreen via on-site html buttons / keyboard shortcut (<keyb>esc</keyb>), and switching away from tab\r\n- origin settings (exceptions for flash / noscript when navigating to different origins in the same tab, as well as allow-once vs allow-always for all origin settings inc fullscreen https://github.com/brave/browser-laptop/pull/9475)\r\n- widevine popup (background-loaded tabs especially)\r\n- Autofill\r\n- Brave Shield settings\r\n- Ledger tab activity time logging\r\n- SSL (+EV) certificate and lock icon status"}}
{"actor":{"display_login":"petemill"}}