Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
465363003:
ResourceScheduler get visual signal from RenderViewHostImpl (Closed)
|
Created:
6 years, 4 months ago by Zhen Wang Modified:
6 years, 3 months ago CC:
chromium-reviews, darin-cc_chromium.org, nasko+codewatch_chromium.org, jam, creis+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionResourceScheduler get visual signal from RenderViewHostImpl
BUG=
Committed: https://crrev.com/e0b2af8a59f556b9f0fbb774e380294b13871ffe
Cr-Commit-Position: refs/heads/master@{#291687}
Patch Set 1 #Patch Set 2 : #
Total comments: 8
Patch Set 3 : remove ResourceScheduler::OnVisibilityChanged #
Total comments: 6
Patch Set 4 : nit fix #
Total comments: 1
Patch Set 5 : Add VisualObserver #
Total comments: 4
Patch Set 6 : Reset client_ in VisualObserver properly. #Patch Set 7 : Get visual signal from RenderViewHostImpl #
Total comments: 4
Patch Set 8 : unit test fix #
Total comments: 2
Patch Set 9 : Do not throttle when should not #Messages
Total messages: 33 (0 generated)
You'll also want to replace current calls of scheduler_.OnVisibilityChanged to webcontents.WasShown/WasHidden in the existing unittests. https://codereview.chromium.org/465363003/diff/20001/content/browser/loader/r... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/465363003/diff/20001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:249: Observe(web_contents); Do you need to set the initial visibility on Client creation, or will WasShown always be called after Client creation? I thought it was the former, but it's been a while since I looked. Have you tested the case of the initial foreground tab to make sure it's marked as visible? https://codereview.chromium.org/465363003/diff/20001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:263: scheduler_->OnVisibilityChanged(child_id_, route_id_, true); This makes a longer loop than needed to set the information. Ie Client -> ResourceScheduler -> Client. Just call the Client::OnVisibilityChanged directly from here. You can just remove ResourceScheduler::OnVisibilityChanged. https://codereview.chromium.org/465363003/diff/20001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:268: scheduler_->OnVisibilityChanged(child_id_, route_id_, false); Same as above. https://codereview.chromium.org/465363003/diff/20001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:859: bool ResourceScheduler::ClientIsVisible(int child_id, int route_id) { nit: IsClientVisible instead.
Also, it will be easier to review if you upload your changes to this review instead of starting a new one again.
https://codereview.chromium.org/465363003/diff/20001/content/browser/loader/r... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/465363003/diff/20001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:249: Observe(web_contents); I set the visibility here now. Test added. On 2014/08/13 21:40:33, aiolos wrote: > Do you need to set the initial visibility on Client creation, or will WasShown > always be called after Client creation? I thought it was the former, but it's > been a while since I looked. Have you tested the case of the initial foreground > tab to make sure it's marked as visible? https://codereview.chromium.org/465363003/diff/20001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:263: scheduler_->OnVisibilityChanged(child_id_, route_id_, true); On 2014/08/13 21:40:33, aiolos wrote: > This makes a longer loop than needed to set the information. Ie Client -> > ResourceScheduler -> Client. Just call the Client::OnVisibilityChanged directly > from here. > > You can just remove ResourceScheduler::OnVisibilityChanged. Done. https://codereview.chromium.org/465363003/diff/20001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:268: scheduler_->OnVisibilityChanged(child_id_, route_id_, false); On 2014/08/13 21:40:33, aiolos wrote: > Same as above. Done. https://codereview.chromium.org/465363003/diff/20001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:859: bool ResourceScheduler::ClientIsVisible(int child_id, int route_id) { On 2014/08/13 21:40:33, aiolos wrote: > nit: IsClientVisible instead. Done.
Just a few small changes. https://codereview.chromium.org/465363003/diff/40001/content/browser/loader/r... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/465363003/diff/40001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:236: is_visible_(false), is_visible_ should just be set once here. https://codereview.chromium.org/465363003/diff/40001/content/browser/loader/r... File content/browser/loader/resource_scheduler.h (right): https://codereview.chromium.org/465363003/diff/40001/content/browser/loader/r... content/browser/loader/resource_scheduler.h:139: bool IsClientVisible(int child_id, int route_id); nit: Since this isn't used outside of the unittest, it might be best to add ForTesting to the end of the name. https://codereview.chromium.org/465363003/diff/40001/content/browser/loader/r... File content/browser/loader/resource_scheduler_unittest.cc (right): https://codereview.chromium.org/465363003/diff/40001/content/browser/loader/r... content/browser/loader/resource_scheduler_unittest.cc:171: web_contents_->WasHidden(); Either remove this line, or change it to WasShown() and remove line 184.
LGTM. Pulling Matt in for review.
Nit fix. By the way, I have seen several try bots fail, mostly for browser_tests. This seems persistent, so something must be wrong. But I don't really understand why it happened. Do you mind taking a look? Thanks! https://codereview.chromium.org/465363003/diff/40001/content/browser/loader/r... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/465363003/diff/40001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:236: is_visible_(false), On 2014/08/15 01:45:38, aiolos wrote: > is_visible_ should just be set once here. Done. https://codereview.chromium.org/465363003/diff/40001/content/browser/loader/r... File content/browser/loader/resource_scheduler.h (right): https://codereview.chromium.org/465363003/diff/40001/content/browser/loader/r... content/browser/loader/resource_scheduler.h:139: bool IsClientVisible(int child_id, int route_id); On 2014/08/15 01:45:38, aiolos wrote: > nit: Since this isn't used outside of the unittest, it might be best to add > ForTesting to the end of the name. Done. https://codereview.chromium.org/465363003/diff/40001/content/browser/loader/r... File content/browser/loader/resource_scheduler_unittest.cc (right): https://codereview.chromium.org/465363003/diff/40001/content/browser/loader/r... content/browser/loader/resource_scheduler_unittest.cc:171: web_contents_->WasHidden(); On 2014/08/15 01:45:38, aiolos wrote: > Either remove this line, or change it to WasShown() and remove line 184. Done.
+mmenke Hi Matt, this CL passes visual signal from web contents to ResourceScheduler. ptal.
https://codereview.chromium.org/465363003/diff/60001/content/browser/loader/r... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/465363003/diff/60001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:236: is_visible_(!web_contents->IsHidden()), It's not safe to muck with the web_contents here. We're on the IO thread, the WebContents lives on the UI thread. You're going to need some class that lives on the UI thread to observe the events, and pass visibility change events over to the IO thread.
On 2014/08/15 18:43:02, Zhen Wang wrote: > Nit fix. > > By the way, I have seen several try bots fail, mostly for browser_tests. This > seems persistent, so something must be wrong. But I don't really understand why > it happened. Do you mind taking a look? Thanks! The browser tests are failing without your patch as well as with it. For example, look at http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_... You'll notice that both browser_tests (with patch) and browser_tests are both red. If you click on the link to stdio for browser tests, you'll see the same tests listed as failing. That means you should try the bots again later when the tests are fixed. The compile failure on ios_dbg_simulator and ios_rel_device asked you to create a bug: 2014-08-14 15:41:01.883 xcodebuild[4807:6e07] DVTAssertions: Warning in /SourceCache/IDEXcode3ProjectSupport/IDEXcode3ProjectSupport-5067/Xcode3Core/LegacyProjects/Frameworks/DevToolsCore/DevToolsCore/SpecificationTypes/BuiltInSpecifications/Compilers/XCGccMakefileDependencies.m:76 Details: Failed to load dependencies output contents from ``/b/build/slave/ios_dbg_simulator/build/src/xcodebuild/content.build/Debug-iphonesimulator/content_browser.build/Objects-normal/i386/browser_main_loop.d''. Error: Error Domain=NSCocoaErrorDomain Code=260 "The file “browser_main_loop.d” couldn’t be opened because there is no such file." UserInfo=0x7f8995189450 {NSFilePath=/b/build/slave/ios_dbg_simulator/build/src/xcodebuild/content.build/Debug-iphonesimulator/content_browser.build/Objects-normal/i386/browser_main_loop.d, NSUnderlyingError=0x7f898e00e2e0 "The operation couldn’t be completed. No such file or directory"}. User info: { NSFilePath = "/b/build/slave/ios_dbg_simulator/build/src/xcodebuild/content.build/Debug-iphonesimulator/content_browser.build/Objects-normal/i386/browser_main_loop.d"; NSUnderlyingError = "Error Domain=NSPOSIXErrorDomain Code=2 \"The operation couldn\U2019t be completed. No such file or directory\""; }. Function: void XCGccMakefileDependenciesParsePathsFromRuleFile(NSString *__strong, void (^__strong)(NSString *__strong)) Thread: <NSThread: 0x7f89930334d0>{name = (null), num = 38} Please file a bug at http://bugreport.apple.com with this warning message and any useful information you can provide. You had another test fail in slave steps, which also isn't related to your patch.
I add visual observer, which observes web contents on UI thread and notify the client. ptal.
Can we just have the RenderViewHost pass visibility messages directly to the ResourceDispatcherHost, which then passes them to the scheduler, instead? Just like ResourceDispatcherHostImpl::OnRenderViewHostCreated does. You may have to override a RenderWidgetHostImpl funciton. Didn't suggest this earlier, because I didn't realize the REsourceDispatcherHost was passed messages directly by the RenderViewHost. https://codereview.chromium.org/465363003/diff/80001/content/browser/loader/r... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/465363003/diff/80001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:711: if (client_) So we ignore events if the visibility changes if we don't know the client yet? https://codereview.chromium.org/465363003/diff/80001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:716: true)); This isn't safe. The client may already have been deleted, and the message to destroy the VisualObserver currently on the way to the UI thread. Note that a WebContents can change RenderViewHosts.
> Can we just have the RenderViewHost pass visibility messages directly to the > ResourceDispatcherHost, which then passes them to the scheduler, instead? Just > like ResourceDispatcherHostImpl::OnRenderViewHostCreated does. You may have to > override a RenderWidgetHostImpl funciton. Do you mean getting the information from RenderWidgetHostImpl::WasHidden() and RenderWidgetHostImpl::WasShown()? I think that may work. I will go ahead to try that and let you know how it works later. Let me know if you think otherwise. And you can ignore my fix to reset client_ if we end up with passing information from RenderWidgetHostImpl. https://codereview.chromium.org/465363003/diff/80001/content/browser/loader/r... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/465363003/diff/80001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:711: if (client_) Yes. It may happen that the Client hasn't been created and associated with the VisualObserver when the WebContents change its visibility. The NULL pointer check will also help when the Client is deleted. See below. On 2014/08/19 14:38:27, mmenke wrote: > So we ignore events if the visibility changes if we don't know the client yet? https://codereview.chromium.org/465363003/diff/80001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:716: true)); I think you are right. I override OnRenderViewHostDeleted() in VisualObser now to reset client_ properly. My reasoning is as follows. Background: 1. WasShown() is called on UI thread. 2. The Client is deleted when ResourceScheduler::OnClientDeleted() is called, which is called by ResourceDispatcherHostImpl::OnRenderViewHostDeleted() on IO thread. And ResourceDispatcherHostImpl::OnRenderViewHostDeleted() is called by RenderViewHostImpl::~RenderViewHostImpl() on UI thread. Case 1: WasShown() is called before RenderViewHostImpl::~RenderViewHostImpl() on UI thread. In this case, ResourceScheduler::Client::OnVisibilityChanged() will be executed before ResourceScheduler::OnClientDeleted() on IO thread. The pointer client_ is still available. No problem. Case 2: RenderViewHostImpl::~RenderViewHostImpl() is called before WasShown() on UI thread. The destructor will post ResourceDispatcherHostImpl::OnRenderViewHostDeleted() to IO thread, which will in turn call ResourceScheduler::OnClientDeleted(). Before switching to IO thread, the destructor will immediately call WebContentsImpl::RenderViewDeleted(), which will call VisualObserver::RenderViewDeleted(). Then the pointer client_ will be reset to NULL. At this point, it is still in the destructor and WasShown() hasn't been called yet. So later, when WasShown() is called, the NULL pointer check will work. And eventually, the VisualObserver will be deleted after the Client is deleted. On 2014/08/19 14:38:27, mmenke wrote: > This isn't safe. The client may already have been deleted, and the message to > destroy the VisualObserver currently on the way to the UI thread. Note that a > WebContents can change RenderViewHosts.
On 2014/08/19 17:53:41, Zhen Wang wrote: > > Can we just have the RenderViewHost pass visibility messages directly to the > > ResourceDispatcherHost, which then passes them to the scheduler, instead? > Just > > like ResourceDispatcherHostImpl::OnRenderViewHostCreated does. You may have > to > > override a RenderWidgetHostImpl funciton. > > Do you mean getting the information from RenderWidgetHostImpl::WasHidden() and > RenderWidgetHostImpl::WasShown()? I think that may work. I will go ahead to try > that and let you know how it works later. Let me know if you think otherwise. > And you can ignore my fix to reset client_ if we end up with passing information > from RenderWidgetHostImpl. Yes, that's exactly what I mean. I think it will be simpler, and less regression prone. > https://codereview.chromium.org/465363003/diff/80001/content/browser/loader/r... > File content/browser/loader/resource_scheduler.cc (right): > > https://codereview.chromium.org/465363003/diff/80001/content/browser/loader/r... > content/browser/loader/resource_scheduler.cc:711: if (client_) > Yes. It may happen that the Client hasn't been created and associated with the > VisualObserver when the WebContents change its visibility. > > The NULL pointer check will also help when the Client is deleted. See below. > > On 2014/08/19 14:38:27, mmenke wrote: > > So we ignore events if the visibility changes if we don't know the client yet? > > https://codereview.chromium.org/465363003/diff/80001/content/browser/loader/r... > content/browser/loader/resource_scheduler.cc:716: true)); > I think you are right. I override OnRenderViewHostDeleted() in VisualObser now > to reset client_ properly. My reasoning is as follows. > > Background: > 1. WasShown() is called on UI thread. > 2. The Client is deleted when ResourceScheduler::OnClientDeleted() is called, > which is called by ResourceDispatcherHostImpl::OnRenderViewHostDeleted() on IO > thread. And ResourceDispatcherHostImpl::OnRenderViewHostDeleted() is called by > RenderViewHostImpl::~RenderViewHostImpl() on UI thread. > > Case 1: > WasShown() is called before RenderViewHostImpl::~RenderViewHostImpl() on UI > thread. > > In this case, ResourceScheduler::Client::OnVisibilityChanged() will be executed > before ResourceScheduler::OnClientDeleted() on IO thread. The pointer client_ is > still available. No problem. > > Case 2: > RenderViewHostImpl::~RenderViewHostImpl() is called before WasShown() on UI > thread. > > The destructor will post ResourceDispatcherHostImpl::OnRenderViewHostDeleted() > to IO thread, which will in turn call ResourceScheduler::OnClientDeleted(). > Before switching to IO thread, the destructor will immediately call > WebContentsImpl::RenderViewDeleted(), which will call > VisualObserver::RenderViewDeleted(). Then the pointer client_ will be reset to > NULL. At this point, it is still in the destructor and WasShown() hasn't been > called yet. So later, when WasShown() is called, the NULL pointer check will > work. And eventually, the VisualObserver will be deleted after the Client is > deleted. Quick comment: A RenderView is generally only deleted after the new one is created, so you'd have to check the client ID and routing ID with that approach. I think that we'd need quite a few very intricate tests to convince me there aren't any races here, while with the other approach, I don't think it's such a concern, though still be great if we could have tests that make sure everything's hooked up properly. > > On 2014/08/19 14:38:27, mmenke wrote: > > This isn't safe. The client may already have been deleted, and the message to > > destroy the VisualObserver currently on the way to the UI thread. Note that a > > WebContents can change RenderViewHosts.
I have updated the patch by getting visual signal from RenderViewHostImpl. ptal.
I like this much better, though it looks like this breaks some prerendering tests. https://codereview.chromium.org/465363003/diff/120001/content/browser/loader/... File content/browser/loader/resource_scheduler_unittest.cc (right): https://codereview.chromium.org/465363003/diff/120001/content/browser/loader/... content/browser/loader/resource_scheduler_unittest.cc:2129: // Check initial visibility This isn't initial visibility, since you've already sent wasshown/hidden events. Ideally, we'd create rvh1 as visible and rvh2 as hidden, but I don't think we can do that. Instead, and we look at rvh1 and rvh2 just after creation, and make sure that the scheduler starts out with the correct visibility values, whatever they may be? https://codereview.chromium.org/465363003/diff/120001/content/browser/loader/... content/browser/loader/resource_scheduler_unittest.cc:2135: // Flip the visibility and check again nit: End comments that are sentences (Or at least sentence-like) with periods (x3).
Unit test fixed. In patch set 8, I only modified the unittest file. But since I rebased against master to pass the compilation, there are noises in other files. The failure of the prerendering browser test may due to the following reason. In the failed PrerenderBrowserTest, the test will hang at WaitForASCIITitle(). Before this gets called, RenderWidgetHostImpl::WasHidden() is called once. Withoutout my patch, there is no effect on the client. With my patch, ResourceScheduler::Client::OnVisibilityChanged(false) will be called, which in turn calls UpdateThrottleState(). I guess this somehow affects the prerender test and then hangs at WaitForASCIITitle(). With my patch, if I remove "scheduler_->OnVisibilityChanged(child_id, route_id, false);" in ResourceDispatcherHostImpl::OnRenderViewHostWasHidden(), the prerender test will pass. Kari, is there any way to control the usage of ResourceScheduler in the tests? https://codereview.chromium.org/465363003/diff/120001/content/browser/loader/... File content/browser/loader/resource_scheduler_unittest.cc (right): https://codereview.chromium.org/465363003/diff/120001/content/browser/loader/... content/browser/loader/resource_scheduler_unittest.cc:2129: // Check initial visibility On 2014/08/20 20:21:43, mmenke wrote: > This isn't initial visibility, since you've already sent wasshown/hidden events. > Ideally, we'd create rvh1 as visible and rvh2 as hidden, but I don't think we > can do that. Instead, and we look at rvh1 and rvh2 just after creation, and > make sure that the scheduler starts out with the correct visibility values, > whatever they may be? Done. https://codereview.chromium.org/465363003/diff/120001/content/browser/loader/... content/browser/loader/resource_scheduler_unittest.cc:2135: // Flip the visibility and check again On 2014/08/20 20:21:43, mmenke wrote: > nit: End comments that are sentences (Or at least sentence-like) with periods > (x3). Done.
About the test failures: The prerender tests use our out-of-process (spawned) test server class, which is a really junky server - it's single threaded, and each socket has to be used and then closed in the order it was connected. This has caused problems in the past. This is presumably the cause of the failures. That having been said, I'm having trouble coming up with the exact order of requests which would run into issues with it in these tests - we should only connect sockets once requests are let through the scheduler, and we should always be using those sockets right after they're connected. On 2014/08/21 03:34:48, Zhen Wang wrote: > Unit test fixed. In patch set 8, I only modified the unittest file. But since I > rebased against master to pass the compilation, there are noises in other files. > > The failure of the prerendering browser test may due to the following reason. > > In the failed PrerenderBrowserTest, the test will hang at WaitForASCIITitle(). > Before this gets called, RenderWidgetHostImpl::WasHidden() is called once. > Withoutout my patch, there is no effect on the client. With my patch, > ResourceScheduler::Client::OnVisibilityChanged(false) will be called, which in > turn calls UpdateThrottleState(). I guess this somehow affects the prerender > test and then hangs at WaitForASCIITitle(). > > With my patch, if I remove "scheduler_->OnVisibilityChanged(child_id, route_id, > false);" in ResourceDispatcherHostImpl::OnRenderViewHostWasHidden(), the > prerender test will pass. > > Kari, is there any way to control the usage of ResourceScheduler in the tests? > > https://codereview.chromium.org/465363003/diff/120001/content/browser/loader/... > File content/browser/loader/resource_scheduler_unittest.cc (right): > > https://codereview.chromium.org/465363003/diff/120001/content/browser/loader/... > content/browser/loader/resource_scheduler_unittest.cc:2129: // Check initial > visibility > On 2014/08/20 20:21:43, mmenke wrote: > > This isn't initial visibility, since you've already sent wasshown/hidden > events. > > Ideally, we'd create rvh1 as visible and rvh2 as hidden, but I don't think we > > can do that. Instead, and we look at rvh1 and rvh2 just after creation, and > > make sure that the scheduler starts out with the correct visibility values, > > whatever they may be? > > Done. > > https://codereview.chromium.org/465363003/diff/120001/content/browser/loader/... > content/browser/loader/resource_scheduler_unittest.cc:2135: // Flip the > visibility and check again > On 2014/08/20 20:21:43, mmenke wrote: > > nit: End comments that are sentences (Or at least sentence-like) with periods > > (x3). > > Done.
Also, once the test failures are resolved, I'm happy with this. On 2014/08/21 14:14:26, mmenke wrote: > About the test failures: The prerender tests use our out-of-process (spawned) > test server class, which is a really junky server - it's single threaded, and > each socket has to be used and then closed in the order it was connected. This > has caused problems in the past. This is presumably the cause of the failures. > > That having been said, I'm having trouble coming up with the exact order of > requests which would run into issues with it in these tests - we should only > connect sockets once requests are let through the scheduler, and we should > always be using those sockets right after they're connected. > > On 2014/08/21 03:34:48, Zhen Wang wrote: > > Unit test fixed. In patch set 8, I only modified the unittest file. But since > I > > rebased against master to pass the compilation, there are noises in other > files. > > > > The failure of the prerendering browser test may due to the following reason. > > > > In the failed PrerenderBrowserTest, the test will hang at WaitForASCIITitle(). > > Before this gets called, RenderWidgetHostImpl::WasHidden() is called once. > > Withoutout my patch, there is no effect on the client. With my patch, > > ResourceScheduler::Client::OnVisibilityChanged(false) will be called, which in > > turn calls UpdateThrottleState(). I guess this somehow affects the prerender > > test and then hangs at WaitForASCIITitle(). > > > > With my patch, if I remove "scheduler_->OnVisibilityChanged(child_id, > route_id, > > false);" in ResourceDispatcherHostImpl::OnRenderViewHostWasHidden(), the > > prerender test will pass. > > > > Kari, is there any way to control the usage of ResourceScheduler in the tests? > > > > > https://codereview.chromium.org/465363003/diff/120001/content/browser/loader/... > > File content/browser/loader/resource_scheduler_unittest.cc (right): > > > > > https://codereview.chromium.org/465363003/diff/120001/content/browser/loader/... > > content/browser/loader/resource_scheduler_unittest.cc:2129: // Check initial > > visibility > > On 2014/08/20 20:21:43, mmenke wrote: > > > This isn't initial visibility, since you've already sent wasshown/hidden > > events. > > > Ideally, we'd create rvh1 as visible and rvh2 as hidden, but I don't think > we > > > can do that. Instead, and we look at rvh1 and rvh2 just after creation, and > > > make sure that the scheduler starts out with the correct visibility values, > > > whatever they may be? > > > > Done. > > > > > https://codereview.chromium.org/465363003/diff/120001/content/browser/loader/... > > content/browser/loader/resource_scheduler_unittest.cc:2135: // Flip the > > visibility and check again > > On 2014/08/20 20:21:43, mmenke wrote: > > > nit: End comments that are sentences (Or at least sentence-like) with > periods > > > (x3). > > > > Done.
Throttled Clients can change the order of requests since they will let SPDY, syncronous, and non-http(s) requests through before some of the high priority requests that would get issued at the same time in Unthrottled Clients. There is a bug that allow us to set Clients to Throttled when we shouldn't be yet (ie, when throttling isn't turned on.) This means those browser tests are going to fail when we turn throttling on, but it doesn't need to get fixed for this CL. This should fix it: https://codereview.chromium.org/465363003/diff/140001/content/browser/loader/... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/465363003/diff/140001/content/browser/loader/... content/browser/loader/resource_scheduler.cc:324: if (is_active() && !is_loaded_) { Add if (!scheduler_->should_throttle()) { SetThrottleState(UNTHROTTLED); } else ...
All tests pass with the fix in patch set 9. ptal. https://codereview.chromium.org/465363003/diff/140001/content/browser/loader/... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/465363003/diff/140001/content/browser/loader/... content/browser/loader/resource_scheduler.cc:324: if (is_active() && !is_loaded_) { Thanks! This works. On 2014/08/21 17:27:02, aiolos wrote: > Add > > if (!scheduler_->should_throttle()) { > SetThrottleState(UNTHROTTLED); > > } else ...
On 2014/08/21 21:37:50, Zhen Wang wrote: > All tests pass with the fix in patch set 9. ptal. > > https://codereview.chromium.org/465363003/diff/140001/content/browser/loader/... > File content/browser/loader/resource_scheduler.cc (right): > > https://codereview.chromium.org/465363003/diff/140001/content/browser/loader/... > content/browser/loader/resource_scheduler.cc:324: if (is_active() && > !is_loaded_) { > Thanks! This works. > > On 2014/08/21 17:27:02, aiolos wrote: > > Add > > > > if (!scheduler_->should_throttle()) { > > SetThrottleState(UNTHROTTLED); > > > > } else ... LGTM!
The CQ bit was checked by zhenw@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/zhenw@chromium.org/465363003/160001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Hi Jam, Can you take a look at the change in following files? I modified WasHidden() and WasShown() functions to pass the visual signal to ResourceScheduler. Thanks! - content/browser/renderer_host/render_view_host_impl.cc - content/browser/renderer_host/render_view_host_impl.h - content/browser/renderer_host/render_widget_host_impl.h Best -Zhen
lgtm
The CQ bit was checked by zhenw@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/zhenw@chromium.org/465363003/160001
Message was sent while issue was closed.
Committed patchset #9 (160001) as 2f8809be5a3e16605c248bba0d062e0f0117971f
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/e0b2af8a59f556b9f0fbb774e380294b13871ffe Cr-Commit-Position: refs/heads/master@{#291687} |
