Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
286943005:
Reducing CPU cycles of hidden/minimized browser windows (Closed)
|
Created:
6 years, 7 months ago by Mr4D (OOO till 08-26) Modified:
6 years, 7 months ago CC:
chromium-reviews, tfarina, ncarter (slow) Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionReleasing CPU resources of hidden/minimized browser windows
BUG=310365
TEST=browser_test & tested visually (taskamanger, running YouTube, deleting active&inactive hidden tabs, ..)
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=272662
Patch Set 1 #Patch Set 2 : Self nit #Patch Set 3 : . #
Total comments: 1
Patch Set 4 : Moved to WebContentsViewAura #
Total comments: 2
Patch Set 5 : Addressed #
Total comments: 7
Patch Set 6 : . #
Total comments: 9
Patch Set 7 : . #
Total comments: 4
Patch Set 8 : Addressed nits #Patch Set 9 : Fixing telemetry tests #Patch Set 10 : Fixing windows shutdown issue #
Messages
Total messages: 44 (0 generated)
I controlled the active web content with the visibility of the browser window. Please have a look!
This is the wrong place for this logic. If there are resources that can be released when a renderer is no longer visible it should be in the content side.
I hope this is more what you were looking for. Please have a look!
https://codereview.chromium.org/286943005/diff/40001/ui/views/controls/native... File ui/views/controls/native/native_view_host_aura.cc (right): https://codereview.chromium.org/286943005/diff/40001/ui/views/controls/native... ui/views/controls/native/native_view_host_aura.cc:115: void NativeViewHostAura::OnWindowVisibilityChanged(aura::Window* window, Again, I think this should be done at the content layer. If WebContents/RenderWidget want to do something special when an ancestor is hidden then it should install the necessary listeners to do that.
The reason for my previous fix was that there was already some logic in place to handle this case (WebContentsViewAura::OnWindowTargetVisibilityChanged() - only it only gets called when the window gets destroyed. To address that I was adding code to make sure that the observer fires. But I moved it now into WebContentsViewAura instead. There is already some observer framework in place for NPAPI applications, but it is highly OS specific and only gets set for non guest sessions. As such I added one more observer. My only concern is now, that there might be some app content (e.g. back page) which could be affected by this. Please have another look!
https://codereview.chromium.org/286943005/diff/60001/content/browser/web_cont... File content/browser/web_contents/web_contents_view_aura.cc (right): https://codereview.chromium.org/286943005/diff/60001/content/browser/web_cont... content/browser/web_contents/web_contents_view_aura.cc:1433: void WebContentsViewAura::OnWindowTargetVisibilityChanged(bool visible) { I don't think you want this anymore. https://codereview.chromium.org/286943005/diff/60001/content/browser/web_cont... content/browser/web_contents/web_contents_view_aura.cc:1562: if (!parent || parent->IsVisible()) I don't get why you're tracking visibility of the parent to determine whether this is visible here (and below). I see your comment, but that doesn't explain. What flash? What is causing the flast? I also suspect your changes may invoke WasShow (or WasHidden) multiple times in a row. I don't know that the rest of the code deals well with that. Lastly, how a test?
+ncarter to cc; I helped Stefan look at this today, and noticed that the hide-on-minimize behavior already happens on Windows. I just uploaded a browser test exercising this to https://codereview.chromium.org/293923002 -- feel free to merge that into this issue if it helps.
Added unit test, got rid of the check and added a test to not change the visibility multiple times. Please have a look!
https://codereview.chromium.org/286943005/diff/80001/chrome/browser/ui/views/... File chrome/browser/ui/views/frame/browser_view_browsertest.cc (right): https://codereview.chromium.org/286943005/diff/80001/chrome/browser/ui/views/... chrome/browser/ui/views/frame/browser_view_browsertest.cc:105: IN_PROC_BROWSER_TEST_F(BrowserViewTest, MinimizeHidesWebContents) { Test coverage should be in content. The test here should be is visibility set to false on minimize? I'm pretty sure we already have coverage of that in the aura side. https://codereview.chromium.org/286943005/diff/80001/content/browser/web_cont... File content/browser/web_contents/web_contents_view_aura.cc (right): https://codereview.chromium.org/286943005/diff/80001/content/browser/web_cont... content/browser/web_contents/web_contents_view_aura.cc:1586: if (IsInHierarchyBelow(window, window_.get())) Isn't this the same as window_->Contains(window) && window_ != window ?
look again. https://codereview.chromium.org/286943005/diff/80001/chrome/browser/ui/views/... File chrome/browser/ui/views/frame/browser_view_browsertest.cc (right): https://codereview.chromium.org/286943005/diff/80001/chrome/browser/ui/views/... chrome/browser/ui/views/frame/browser_view_browsertest.cc:105: IN_PROC_BROWSER_TEST_F(BrowserViewTest, MinimizeHidesWebContents) { I prefer 1:1 tests and not tests which imply that something else is in place which completes the test. If something later on changes, the proposed test will return passed but the real thing we want to test does still not work. Anyways. Done. https://codereview.chromium.org/286943005/diff/80001/content/browser/web_cont... File content/browser/web_contents/web_contents_view_aura.cc (right): https://codereview.chromium.org/286943005/diff/80001/content/browser/web_cont... content/browser/web_contents/web_contents_view_aura.cc:1586: if (IsInHierarchyBelow(window, window_.get())) On 2014/05/20 19:46:27, sky wrote: > Isn't this the same as window_->Contains(window) && window_ != window ? Done.
https://codereview.chromium.org/286943005/diff/80001/chrome/browser/ui/views/... File chrome/browser/ui/views/frame/browser_view_browsertest.cc (right): https://codereview.chromium.org/286943005/diff/80001/chrome/browser/ui/views/... chrome/browser/ui/views/frame/browser_view_browsertest.cc:105: IN_PROC_BROWSER_TEST_F(BrowserViewTest, MinimizeHidesWebContents) { On 2014/05/20 21:26:56, Mr4D wrote: > I prefer 1:1 tests and not tests which imply that something else is in place > which completes the test. If something later on changes, the proposed test will > return passed but the real thing we want to test does still not work. Anyways. > Done. Fwiw, I think this test can move to a content_browsertest and provide equivalent coverage. When I wrote it, I couldn't figure out how to minimize a window from a content_browsertest, but it looks like shell()->widget()->Minimize() should do the trick, and I think it'll still substantially exercise the same views codepath. Inside of content there's the advantage that you can get the WebContentsImpl and query other things besides. Since this test would fail without the rest of the patch, it seems that whatever the current coverage is, it's not sufficient? https://codereview.chromium.org/286943005/diff/80001/content/browser/web_cont... File content/browser/web_contents/web_contents_view_aura.h (right): https://codereview.chromium.org/286943005/diff/80001/content/browser/web_cont... content/browser/web_contents/web_contents_view_aura.h:46: public aura::WindowObserver { Why not extend the existing WebContentsViewAura::WindowObserver / window_observer_ helper ? It seems inconsistent for this class to be observing window_ both directly and via a helper.
https://codereview.chromium.org/286943005/diff/100001/content/browser/web_con... File content/browser/web_contents/web_contents_view_aura_browsertest.cc (right): https://codereview.chromium.org/286943005/diff/100001/content/browser/web_con... content/browser/web_contents/web_contents_view_aura_browsertest.cc:694: parent->Hide(); Does this actually minimize the parent? Wasn't the original bug that Hide() was never being called on the parent?
Please see comments. https://codereview.chromium.org/286943005/diff/80001/content/browser/web_cont... File content/browser/web_contents/web_contents_view_aura.h (right): https://codereview.chromium.org/286943005/diff/80001/content/browser/web_cont... content/browser/web_contents/web_contents_view_aura.h:46: public aura::WindowObserver { The existing observer is heavily Windows dependent and has many more functions (like that several window observers get set). Last but not least it is mentioned that most of that observer was meant for constrained windows and NPAPI (which will go away soon). That all said - if you want it to be in there I will add it there as well. https://codereview.chromium.org/286943005/diff/100001/content/browser/web_con... File content/browser/web_contents/web_contents_view_aura_browsertest.cc (right): https://codereview.chromium.org/286943005/diff/100001/content/browser/web_con... content/browser/web_contents/web_contents_view_aura_browsertest.cc:694: parent->Hide(); Well - If I understand sky's request correctly, we have a test that a minimize triggers a hide. As such only a test is required which proofs that the hide triggers a content hide as well.
https://codereview.chromium.org/286943005/diff/100001/content/browser/web_con... File content/browser/web_contents/web_contents_view_aura.cc (right): https://codereview.chromium.org/286943005/diff/100001/content/browser/web_con... content/browser/web_contents/web_contents_view_aura.cc:103: bool visibility) { visibility->visible https://codereview.chromium.org/286943005/diff/100001/content/browser/web_con... content/browser/web_contents/web_contents_view_aura.cc:736: window_->RemoveObserver(this); nit: move this above comment (since comment applies to window_.reset, not your new code. https://codereview.chromium.org/286943005/diff/100001/content/browser/web_con... File content/browser/web_contents/web_contents_view_aura_browsertest.cc (right): https://codereview.chromium.org/286943005/diff/100001/content/browser/web_con... content/browser/web_contents/web_contents_view_aura_browsertest.cc:691: aura::Window* content = web_contents->GetContentNativeView(); I'm surprised this works. web_contents->GetContentNativeView returns a descendant of the window, not an ancestor. You want an ancestor. https://codereview.chromium.org/286943005/diff/100001/content/browser/web_con... content/browser/web_contents/web_contents_view_aura_browsertest.cc:694: parent->Hide(); On 2014/05/20 21:48:56, Mr4D wrote: > Well - If I understand sky's request correctly, we have a test that a minimize > triggers a hide. As such only a test is required which proofs that the hide > triggers a content hide as well. Exactly.
Plase have another look! https://codereview.chromium.org/286943005/diff/100001/content/browser/web_con... File content/browser/web_contents/web_contents_view_aura.cc (right): https://codereview.chromium.org/286943005/diff/100001/content/browser/web_con... content/browser/web_contents/web_contents_view_aura.cc:103: bool visibility) { On 2014/05/20 23:33:51, sky wrote: > visibility->visible Done. https://codereview.chromium.org/286943005/diff/100001/content/browser/web_con... content/browser/web_contents/web_contents_view_aura.cc:736: window_->RemoveObserver(this); On 2014/05/20 23:33:51, sky wrote: > nit: move this above comment (since comment applies to window_.reset, not your > new code. Done. https://codereview.chromium.org/286943005/diff/100001/content/browser/web_con... File content/browser/web_contents/web_contents_view_aura_browsertest.cc (right): https://codereview.chromium.org/286943005/diff/100001/content/browser/web_con... content/browser/web_contents/web_contents_view_aura_browsertest.cc:691: aura::Window* content = web_contents->GetContentNativeView(); I am not using this window - I am using it's parent. For the sake of argument I should however even be able to go higher. Done.
LGTM https://codereview.chromium.org/286943005/diff/120001/content/browser/web_con... File content/browser/web_contents/web_contents_view_aura_browsertest.cc (right): https://codereview.chromium.org/286943005/diff/120001/content/browser/web_con... content/browser/web_contents/web_contents_view_aura_browsertest.cc:691: aura::Window* content = web_contents->GetContentNativeView()->parent(); My confusion here is is that what you care about is an ancestor of web_contents you GetContentsNativeView returns a child. Would this be cleaner if you did: web_contents->GetNativeView()->parent()?
lgtm with nit https://codereview.chromium.org/286943005/diff/120001/content/browser/web_con... File content/browser/web_contents/web_contents_view_aura.cc (right): https://codereview.chromium.org/286943005/diff/120001/content/browser/web_con... content/browser/web_contents/web_contents_view_aura.cc:104: if (visibile) { visibile -> visible (extra i)
Many thanks for your review! https://codereview.chromium.org/286943005/diff/120001/content/browser/web_con... File content/browser/web_contents/web_contents_view_aura.cc (right): https://codereview.chromium.org/286943005/diff/120001/content/browser/web_con... content/browser/web_contents/web_contents_view_aura.cc:104: if (visibile) { On 2014/05/21 21:11:03, ncarter wrote: > visibile -> visible (extra i) Done. https://codereview.chromium.org/286943005/diff/120001/content/browser/web_con... File content/browser/web_contents/web_contents_view_aura_browsertest.cc (right): https://codereview.chromium.org/286943005/diff/120001/content/browser/web_con... content/browser/web_contents/web_contents_view_aura_browsertest.cc:691: aura::Window* content = web_contents->GetContentNativeView()->parent(); Done.
The CQ bit was checked by skuhne@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/skuhne@chromium.org/286943005/140001
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...) win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...) win_gpu_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_gpu_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)
The CQ bit was checked by skuhne@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/skuhne@chromium.org/286943005/140001
The CQ bit was unchecked by skuhne@chromium.org
The CQ bit was checked by skuhne@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/skuhne@chromium.org/286943005/140001
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...) win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...) win_gpu_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_gpu_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)
The CQ bit was checked by skuhne@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/skuhne@chromium.org/286943005/160001
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
The CQ bit was checked by skuhne@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/skuhne@chromium.org/286943005/180001
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
The CQ bit was checked by skuhne@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/skuhne@chromium.org/286943005/180001
Message was sent while issue was closed.
Change committed as 272662 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
