Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(17)

Issue 577533003: Revert of When we switch tabs in chrome, the tab being switched away from gets hidden/shown/hidden. (Closed)

Created:
6 years, 3 months ago by nhiroki
Modified:
6 years, 3 months ago
Reviewers:
ananta, sky
CC:
chromium-reviews, tfarina
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Project:
chromium
Visibility:
Public.

Description

Revert of When we switch tabs in chrome, the tab being switched away from gets hidden/shown/hidden. (patchset #14 id:260001 of https://codereview.chromium.org/564553002/) Reason for revert: This seems to cause memory leaks in WebViewUnitTest.TestWebViewAttachDetachWebContents on Linux(ASan/LSan) and ChromiumOS(ASan/LSan) http://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%20LSan%20Tests%20%282%29/builds/7468 Original issue's description: > When we switch tabs in chrome, the tab being switched away from gets hidden/shown/hidden. > > This occurs in the NativeViewHostAura::NativeViewDetaching code path where we first remove the > clipping window which is the intermediate parent of the web contents view. The clipping window > is hidden which causes the RWHVA::Hide function to get called which initiates the hiding sequence. > Then the web contents view is reparented to the main view which is still visible. Now the RWHVA::Show > function is called which initiates the show sequence. Eventually the main view is hidden, which then > initiates the hide sequence. > > Addressed this with the following changes. > 1. WebView::AttachWebContents and WebView::DetachWebContents > now show and hide the webcontents native view. The > WebContents is shown and hidden as before in > WebContentsNativeViewAura::OnWindowVisibilityChanged. > > 2. Removed the WebContentsNativeViewAura::OnWindowParentChanged function. > This function was present to show and hide the webcontents if the window was visible. > This should not be needed with the change in #1 above. > > 3. Added a new file webview_unittest.cc. This contains the unittest WebViewUnitTest.TestWebViewAttachDetachWebContents > This is run as part of unit_tests.exe. > > BUG=412989 > R=sky > > Committed: https://crrev.com/99941773a742f62892fc9aad1a1ebfb7cc967164 > Cr-Commit-Position: refs/heads/master@{#294962} TBR=sky@chromium.org,ananta@chromium.org NOTREECHECKS=true NOTRY=true BUG=412989 Committed: https://crrev.com/e467a7139b660a5fddc72012cdc6628de9fa9f25 Cr-Commit-Position: refs/heads/master@{#295020}

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+15 lines, -182 lines) Patch
M chrome/chrome_tests_unit.gypi View 1 chunk +0 lines, -4 lines 0 comments Download
M content/browser/web_contents/web_contents_view_aura.h View 1 chunk +2 lines, -0 lines 0 comments Download
M content/browser/web_contents/web_contents_view_aura.cc View 1 chunk +13 lines, -0 lines 0 comments Download
M ui/views/controls/webview/DEPS View 1 chunk +0 lines, -2 lines 0 comments Download
M ui/views/controls/webview/webview.cc View 3 chunks +0 lines, -12 lines 0 comments Download
M ui/views/controls/webview/webview.gyp View 1 chunk +0 lines, -1 line 0 comments Download
D ui/views/controls/webview/webview_unittest.cc View 1 chunk +0 lines, -163 lines 0 comments Download

Messages

Total messages: 4 (0 generated)
nhiroki
Created Revert of When we switch tabs in chrome, the tab being switched away from ...
6 years, 3 months ago (2014-09-16 06:23:46 UTC) #1
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/577533003/1
6 years, 3 months ago (2014-09-16 06:24:50 UTC) #2
commit-bot: I haz the power
Committed patchset #1 (id:1) as 734782a55f1cf8065614e781b4d6720ac152022a
6 years, 3 months ago (2014-09-16 06:26:05 UTC) #3
commit-bot: I haz the power
6 years, 3 months ago (2014-09-16 06:27:17 UTC) #4
Message was sent while issue was closed.
Patchset 1 (id:??) landed as
https://crrev.com/e467a7139b660a5fddc72012cdc6628de9fa9f25
Cr-Commit-Position: refs/heads/master@{#295020}

Powered by Google App Engine
This is Rietveld 408576698