Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
393163003:
Add coalescing timer to ResourceScheduler. CL 2 from BUG=128035. (Closed)
|
Created:
6 years, 5 months ago by aiolos (Not reviewing) Modified:
6 years, 4 months ago Reviewers:
mmenke CC:
chromium-reviews, darin-cc_chromium.org, jam Base URL:
https://chromium.googlesource.com/chromium/src.git@rsblank Project:
chromium Visibility:
Public. |
DescriptionAdd coalescing timer to ResourceScheduler. CL 2 from BUG=128035
.
This is a continuation of https://codereview.chromium.org/357583003
BUG=128035
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=286372
Patch Set 1 #
Total comments: 12
Patch Set 2 : Review changes #
Total comments: 15
Patch Set 3 : More tests and nits. #
Total comments: 14
Patch Set 4 : Nits and reviewer suggestions. #
Total comments: 4
Patch Set 5 : Nits and test change. #
Total comments: 1
Patch Set 6 : Spelling nit. #
Messages
Total messages: 30 (0 generated)
https://codereview.chromium.org/393163003/diff/1/content/browser/loader/resou... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/393163003/diff/1/content/browser/loader/resou... content/browser/loader/resource_scheduler.cc:332: } We don't need to have the timer running when we don't have coalesced Clients (Good example: user with one tab open). To avoid battery usage in those cases, keep track of the coalesced Clients the same way we keep track of active and loading Clients. https://codereview.chromium.org/393163003/diff/1/content/browser/loader/resou... content/browser/loader/resource_scheduler.cc:863: void ResourceScheduler::IncrementCoalescedClients() { I wondered about adding tests to check that the timer is running iff there are coalesced Clients, but didn't know if that was too much of an implementation detail to have in the unittests based on one of your comments from the last CL. Thoughts? https://codereview.chromium.org/393163003/diff/1/content/browser/loader/resou... File content/browser/loader/resource_scheduler.h (left): https://codereview.chromium.org/393163003/diff/1/content/browser/loader/resou... content/browser/loader/resource_scheduler.h:142: void DecrementActiveClientsLoading(); The comments were in the wrong order. If you want this broken out into another CL, let me know.
Looks pretty good https://codereview.chromium.org/393163003/diff/1/content/browser/loader/resou... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/393163003/diff/1/content/browser/loader/resou... content/browser/loader/resource_scheduler.cc:332: } On 2014/07/15 23:54:58, aiolos wrote: > We don't need to have the timer running when we don't have coalesced Clients > (Good example: user with one tab open). To avoid battery usage in those cases, > keep track of the coalesced Clients the same way we keep track of active and > loading Clients. We could be even more careful with this - if no coalesced clients have any pending resource requests, we also don't need it running. No need to worry about it in this CL, just a suggestion for the future. https://codereview.chromium.org/393163003/diff/1/content/browser/loader/resou... content/browser/loader/resource_scheduler.cc:647: coalescing_timer_(new base::Timer(true, true)) { Suggest labelling the two arguments, like: new base::timer(true /* retain_user_task */, .... As-is, it's difficult to tell that it's a repeating timer and not a oneshot timer (The latter are more common, in my experience, so easy to assume it's one of them as well). Could also optionally add "repeating" to the name of the timer, to make it clearer, or add a comment where it's declared. https://codereview.chromium.org/393163003/diff/1/content/browser/loader/resou... content/browser/loader/resource_scheduler.cc:863: void ResourceScheduler::IncrementCoalescedClients() { On 2014/07/15 23:54:58, aiolos wrote: > I wondered about adding tests to check that the timer is running iff there are > coalesced Clients, but didn't know if that was too much of an implementation > detail to have in the unittests based on one of your comments from the last CL. > Thoughts? It is a bit of a transparent implementation detail, but I think it's worth testing, since it has real-world performance implications, and regressions are very likely to go unnoticed. https://codereview.chromium.org/393163003/diff/1/content/browser/loader/resou... content/browser/loader/resource_scheduler.cc:884: size_t ResourceScheduler::CountCoalescedClients() { nit: This should be const (As should CountActiveClientsLoading()) https://codereview.chromium.org/393163003/diff/1/content/browser/loader/resou... content/browser/loader/resource_scheduler.cc:897: void ResourceScheduler::LoadCoalescedRequests() { nit: Should use a similar naming scheme here as in OnLoadingActiveClientsStateChanged, given the parallels in code structure (Also think the name doesn't make it clear enough it affects all requests, so if you go with what they do for both functions, instead of when they're called, suggest adding "ForAllClients" or somesuch to the names). https://codereview.chromium.org/393163003/diff/1/content/browser/loader/resou... File content/browser/loader/resource_scheduler.h (left): https://codereview.chromium.org/393163003/diff/1/content/browser/loader/resou... content/browser/loader/resource_scheduler.h:142: void DecrementActiveClientsLoading(); On 2014/07/15 23:54:58, aiolos wrote: > The comments were in the wrong order. If you want this broken out into another > CL, let me know. I'm fine with the change in this CL. https://codereview.chromium.org/393163003/diff/1/content/browser/loader/resou... File content/browser/loader/resource_scheduler.h (right): https://codereview.chromium.org/393163003/diff/1/content/browser/loader/resou... content/browser/loader/resource_scheduler.h:148: // potential to be re-entarant. nit: While you're here, this should be "re-entrant", and "Calls" shouldn't be capitalized. https://codereview.chromium.org/393163003/diff/1/content/browser/loader/resou... content/browser/loader/resource_scheduler.h:151: // Caled when a Client stops being active loading. nit: "Called". Also, grammar isn't quite right. Maybe "Called when an active and loading client becomes either inactive or completes loading." Could update the other comment to match, too.
Oh, and should land a fix for the bug the memory bots found before landing this.
https://codereview.chromium.org/393163003/diff/1/content/browser/loader/resou... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/393163003/diff/1/content/browser/loader/resou... content/browser/loader/resource_scheduler.cc:897: void ResourceScheduler::LoadCoalescedRequests() { On 2014/07/16 17:01:34, mmenke wrote: > nit: Should use a similar naming scheme here as in > OnLoadingActiveClientsStateChanged, given the parallels in code structure (Also > think the name doesn't make it clear enough it affects all requests, so if you > go with what they do for both functions, instead of when they're called, suggest > adding "ForAllClients" or somesuch to the names). The code structures are parallel, but when/why they are called isn't. OnLoadingActiveClientsStateChanged is called when the first client to becomes active and loading enters that state, and when the last active and loading client leaves that state. It can either throttle or unthrottle clients in other states, depending on whether the first or last active and loading client changed state. Added "ForAllClients" to this call. LoadCoalescedRequests on the other hand is called specifically to load coalesced requests. It won't effect non-coalesced clients. It is called when the first client enters the coalesced state, and will be called when we get notified of the network becoming active. It won't be called when the last client leaves the coalesced state. I'd rather keep this different from OnLoadingActiveClientsStateChangedForAllClients. I changed it to LoadRequestsForAllCoalescedClients.
https://codereview.chromium.org/393163003/diff/20001/content/browser/loader/r... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/393163003/diff/20001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:282: // Reset state to default values for client deletion. Should mention this must be called prior to deletion. Maybe also rename this ResetStateForDeletion? https://codereview.chromium.org/393163003/diff/20001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:283: void ResetState() { Why can't you just do this on destruction? I seem to recall some re-entrancy related reason, but I'm having trouble figuring out what it was. https://codereview.chromium.org/393163003/diff/20001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:287: UpdateThrottleState(); This strikes me as regression-prone logic. We're relying on a non-visible, non-audible, unloaded client never to be in a state that the ResourceScheduler tracks. If that changes, things break, and it's hard to have DCHECKs that verify that never changes. You had talked about adding a paused state...Maybe add something like that and just use it for state on destruction, for now? Open to other ideas. https://codereview.chromium.org/393163003/diff/20001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:411: void LoadRequestsForAllCoalescedClients() { Undo this rename. https://codereview.chromium.org/393163003/diff/20001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:752: } Should this be done before changing the state? https://codereview.chromium.org/393163003/diff/20001/content/browser/loader/r... File content/browser/loader/resource_scheduler.h (right): https://codereview.chromium.org/393163003/diff/20001/content/browser/loader/r... content/browser/loader/resource_scheduler.h:75: DCHECK(timer); Probably shouldn't have DCHECKs in header files. It's a heavier weight from an inlining perspective, and no one ever remembers to include logging.h, so you end up either having include-what-you-use issues yourself, or encouraging other files to have them. https://codereview.chromium.org/393163003/diff/20001/content/browser/loader/r... content/browser/loader/resource_scheduler.h:77: coalescing_timer_.reset(timer.release()); Don't need two calls. coalescing_timer_.reset(timer.release()); both deletes the old one and moves the new one. coalescing_timer_ = timer.Pass(); also works, not sure if one is preferred over the other. https://codereview.chromium.org/393163003/diff/20001/content/browser/loader/r... content/browser/loader/resource_scheduler.h:189: /* This is a repeating timer to initiate requests on COALESCED Clients. */ nit: Generally should use C++-style comments. https://codereview.chromium.org/393163003/diff/20001/content/browser/loader/r... File content/browser/loader/resource_scheduler_unittest.cc (right): https://codereview.chromium.org/393163003/diff/20001/content/browser/loader/r... content/browser/loader/resource_scheduler_unittest.cc:1838: // To avoid errors on test tear down. nit: Style guide encourages complete sentences. Just add a "Needed" or "This is needed" in front is fine. https://codereview.chromium.org/393163003/diff/20001/content/browser/loader/r... content/browser/loader/resource_scheduler_unittest.cc:1997: } Good job on the tests, but two cases we don't seem to have: On client goes from COALESCED -> ACTIVE_AND_LOADING. Need to make sure that both the decrementing number of coalesced clients takes place (Starting the timer), and ACTIVE_AND_LOADING updates that counter, making unthrottling loading background clients. And the opposite as well. I think we handle these double updates correctly, but seems like a potential point of regression.
https://codereview.chromium.org/393163003/diff/20001/content/browser/loader/r... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/393163003/diff/20001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:283: void ResetState() { On 2014/07/25 16:33:27, mmenke wrote: > Why can't you just do this on destruction? I seem to recall some re-entrancy > related reason, but I'm having trouble figuring out what it was. Good point. https://codereview.chromium.org/393163003/diff/20001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:287: UpdateThrottleState(); On 2014/07/25 16:33:27, mmenke wrote: > This strikes me as regression-prone logic. We're relying on a non-visible, > non-audible, unloaded client never to be in a state that the ResourceScheduler > tracks. If that changes, things break, and it's hard to have DCHECKs that > verify that never changes. > > You had talked about adding a paused state...Maybe add something like that and > just use it for state on destruction, for now? Open to other ideas. I'd considered doing that. I was planning on adding that in the patch limiting the number of throttled tabs, to keep this CL smaller. But putting it in now works too. I assume you'd like tests for paused state added at the same time?
https://codereview.chromium.org/393163003/diff/20001/content/browser/loader/r... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/393163003/diff/20001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:287: UpdateThrottleState(); On 2014/07/25 16:54:50, aiolos wrote: > On 2014/07/25 16:33:27, mmenke wrote: > > This strikes me as regression-prone logic. We're relying on a non-visible, > > non-audible, unloaded client never to be in a state that the ResourceScheduler > > tracks. If that changes, things break, and it's hard to have DCHECKs that > > verify that never changes. > > > > You had talked about adding a paused state...Maybe add something like that and > > just use it for state on destruction, for now? Open to other ideas. > > I'd considered doing that. I was planning on adding that in the patch limiting > the number of throttled tabs, to keep this CL smaller. But putting it in now > works too. I assume you'd like tests for paused state added at the same time? If we just use it for shutdown for now, and then more broadly later, I'm fine with the current shutdown tests for the moment. Also, don't think we'll need quite as many tests for pausing as we have for the current behaviors - just one or two tests pausing and unpausing should probably be enough.
https://codereview.chromium.org/393163003/diff/20001/content/browser/loader/r... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/393163003/diff/20001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:287: UpdateThrottleState(); On 2014/07/25 17:02:41, mmenke wrote: > On 2014/07/25 16:54:50, aiolos wrote: > > On 2014/07/25 16:33:27, mmenke wrote: > > > This strikes me as regression-prone logic. We're relying on a non-visible, > > > non-audible, unloaded client never to be in a state that the > ResourceScheduler > > > tracks. If that changes, things break, and it's hard to have DCHECKs that > > > verify that never changes. > > > > > > You had talked about adding a paused state...Maybe add something like that > and > > > just use it for state on destruction, for now? Open to other ideas. > > > > I'd considered doing that. I was planning on adding that in the patch limiting > > the number of throttled tabs, to keep this CL smaller. But putting it in now > > works too. I assume you'd like tests for paused state added at the same time? > > If we just use it for shutdown for now, and then more broadly later, I'm fine > with the current shutdown tests for the moment. > > Also, don't think we'll need quite as many tests for pausing as we have for the > current behaviors - just one or two tests pausing and unpausing should probably > be enough. Agreed. Although shouldn't sync requests get issued even for paused Clients so we don't hang the render? I assume we should have a test for that as well.
https://codereview.chromium.org/393163003/diff/20001/content/browser/loader/r... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/393163003/diff/20001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:287: UpdateThrottleState(); On 2014/07/25 17:12:58, aiolos wrote: > On 2014/07/25 17:02:41, mmenke wrote: > > On 2014/07/25 16:54:50, aiolos wrote: > > > On 2014/07/25 16:33:27, mmenke wrote: > > > > This strikes me as regression-prone logic. We're relying on a > non-visible, > > > > non-audible, unloaded client never to be in a state that the > > ResourceScheduler > > > > tracks. If that changes, things break, and it's hard to have DCHECKs that > > > > verify that never changes. > > > > > > > > You had talked about adding a paused state...Maybe add something like that > > and > > > > just use it for state on destruction, for now? Open to other ideas. > > > > > > I'd considered doing that. I was planning on adding that in the patch > limiting > > > the number of throttled tabs, to keep this CL smaller. But putting it in now > > > works too. I assume you'd like tests for paused state added at the same > time? > > > > If we just use it for shutdown for now, and then more broadly later, I'm fine > > with the current shutdown tests for the moment. > > > > Also, don't think we'll need quite as many tests for pausing as we have for > the > > current behaviors - just one or two tests pausing and unpausing should > probably > > be enough. > > Agreed. Although shouldn't sync requests get issued even for paused Clients so > we don't hang the render? I assume we should have a test for that as well. Good point - a test for that definitely makes sense.
On 2014/07/25 17:22:20, mmenke wrote: > https://codereview.chromium.org/393163003/diff/20001/content/browser/loader/r... > File content/browser/loader/resource_scheduler.cc (right): > > https://codereview.chromium.org/393163003/diff/20001/content/browser/loader/r... > content/browser/loader/resource_scheduler.cc:287: UpdateThrottleState(); > On 2014/07/25 17:12:58, aiolos wrote: > > On 2014/07/25 17:02:41, mmenke wrote: > > > On 2014/07/25 16:54:50, aiolos wrote: > > > > On 2014/07/25 16:33:27, mmenke wrote: > > > > > This strikes me as regression-prone logic. We're relying on a > > non-visible, > > > > > non-audible, unloaded client never to be in a state that the > > > ResourceScheduler > > > > > tracks. If that changes, things break, and it's hard to have DCHECKs > that > > > > > verify that never changes. > > > > > > > > > > You had talked about adding a paused state...Maybe add something like > that > > > and > > > > > just use it for state on destruction, for now? Open to other ideas. > > > > > > > > I'd considered doing that. I was planning on adding that in the patch > > limiting > > > > the number of throttled tabs, to keep this CL smaller. But putting it in > now > > > > works too. I assume you'd like tests for paused state added at the same > > time? > > > > > > If we just use it for shutdown for now, and then more broadly later, I'm > fine > > > with the current shutdown tests for the moment. > > > > > > Also, don't think we'll need quite as many tests for pausing as we have for > > the > > > current behaviors - just one or two tests pausing and unpausing should > > probably > > > be enough. > > > > Agreed. Although shouldn't sync requests get issued even for paused Clients so > > we don't hang the render? I assume we should have a test for that as well. > > Good point - a test for that definitely makes sense. I opted to leave out the PAUSED scheduling logic until the CL to limit the number of throttled tabs goes in since I didn't want to add untested code. Since Clients only enter this state after their requests are canceled, right before they get deleted, it seemed fine to have them just get default behavior. (Which is the same as UNTHROTTLED/ACTIVE_AND_LOADING/the current Client behavior.) Thoughts?
Just minor comments. https://codereview.chromium.org/393163003/diff/40001/content/browser/loader/r... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/393163003/diff/40001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:246: // correct count of important types of clients. nit: I think "important" is a bit weird here. Maybe "various" or "relevant"? (Admittedly, the latter can be a synonym for important). https://codereview.chromium.org/393163003/diff/40001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:856: client->UpdateThrottleState(false /* should_pause */); This unpauses all clients. While not yet a bug, it seems potentially problematic. My suggestion: Add a function: Client::SetPaused(bool is_paused) { is_paused_ = is_paused; UpdateThrottleState(); } And add a check for is_paused_ in update throttle state. Open to other ideas. https://codereview.chromium.org/393163003/diff/40001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:876: DCHECK_EQ(coalesced_clients_, CountCoalescedClients()); DCHECK(should_coalesce_), maybe? Could also put that in some of these other functions, I believe. https://codereview.chromium.org/393163003/diff/40001/content/browser/loader/r... File content/browser/loader/resource_scheduler_unittest.cc (right): https://codereview.chromium.org/393163003/diff/40001/content/browser/loader/r... content/browser/loader/resource_scheduler_unittest.cc:142: mock_timer_ = new base::MockTimer(true, true); nit: Could put this in the initializer list. https://codereview.chromium.org/393163003/diff/40001/content/browser/loader/r... content/browser/loader/resource_scheduler_unittest.cc:145: // TODO(aiolos): remove when throttling and coalescing have both landed nit: Capitalize + add period. https://codereview.chromium.org/393163003/diff/40001/content/browser/loader/r... content/browser/loader/resource_scheduler_unittest.cc:1954: CoalescedClientBecomesVisibleAndLoadingStopsTimer) { optional: Not sure this test gets us anything the test two above does not. Suggest either getting rid of it, or moving it above that test.
On 2014/07/26 00:16:33, aiolos wrote: > On 2014/07/25 17:22:20, mmenke wrote: > > > https://codereview.chromium.org/393163003/diff/20001/content/browser/loader/r... > > File content/browser/loader/resource_scheduler.cc (right): > > > > > https://codereview.chromium.org/393163003/diff/20001/content/browser/loader/r... > > content/browser/loader/resource_scheduler.cc:287: UpdateThrottleState(); > > On 2014/07/25 17:12:58, aiolos wrote: > > > On 2014/07/25 17:02:41, mmenke wrote: > > > > On 2014/07/25 16:54:50, aiolos wrote: > > > > > On 2014/07/25 16:33:27, mmenke wrote: > > > > > > This strikes me as regression-prone logic. We're relying on a > > > non-visible, > > > > > > non-audible, unloaded client never to be in a state that the > > > > ResourceScheduler > > > > > > tracks. If that changes, things break, and it's hard to have DCHECKs > > that > > > > > > verify that never changes. > > > > > > > > > > > > You had talked about adding a paused state...Maybe add something like > > that > > > > and > > > > > > just use it for state on destruction, for now? Open to other ideas. > > > > > > > > > > I'd considered doing that. I was planning on adding that in the patch > > > limiting > > > > > the number of throttled tabs, to keep this CL smaller. But putting it in > > now > > > > > works too. I assume you'd like tests for paused state added at the same > > > time? > > > > > > > > If we just use it for shutdown for now, and then more broadly later, I'm > > fine > > > > with the current shutdown tests for the moment. > > > > > > > > Also, don't think we'll need quite as many tests for pausing as we have > for > > > the > > > > current behaviors - just one or two tests pausing and unpausing should > > > probably > > > > be enough. > > > > > > Agreed. Although shouldn't sync requests get issued even for paused Clients > so > > > we don't hang the render? I assume we should have a test for that as well. > > > > Good point - a test for that definitely makes sense. > > I opted to leave out the PAUSED scheduling logic until the CL to limit the > number of throttled tabs goes in since I didn't want to add untested code. Since > Clients only enter this state after their requests are canceled, right before > they get deleted, it seemed fine to have them just get default behavior. (Which > is the same as UNTHROTTLED/ACTIVE_AND_LOADING/the current Client behavior.) > Thoughts? Oh, and I'm fine with not fully hooking up the PAUSED state in this CL. Should add a TODO about it, though, explaining the current behavior. Suggest putting it just before the enum, though also makes sense in the scheduling logic.
https://codereview.chromium.org/393163003/diff/40001/content/browser/loader/r... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/393163003/diff/40001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:246: // correct count of important types of clients. On 2014/07/29 15:43:26, mmenke wrote: > nit: I think "important" is a bit weird here. Maybe "various" or "relevant"? > (Admittedly, the latter can be a synonym for important). Done. https://codereview.chromium.org/393163003/diff/40001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:856: client->UpdateThrottleState(false /* should_pause */); On 2014/07/29 15:43:26, mmenke wrote: > This unpauses all clients. While not yet a bug, it seems potentially > problematic. > > My suggestion: > > Add a function: > > Client::SetPaused(bool is_paused) { > is_paused_ = is_paused; > UpdateThrottleState(); > } > > And add a check for is_paused_ in update throttle state. Open to other ideas. Done. https://codereview.chromium.org/393163003/diff/40001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:876: DCHECK_EQ(coalesced_clients_, CountCoalescedClients()); On 2014/07/29 15:43:26, mmenke wrote: > DCHECK(should_coalesce_), maybe? > > Could also put that in some of these other functions, I believe. Done. https://codereview.chromium.org/393163003/diff/40001/content/browser/loader/r... File content/browser/loader/resource_scheduler_unittest.cc (right): https://codereview.chromium.org/393163003/diff/40001/content/browser/loader/r... content/browser/loader/resource_scheduler_unittest.cc:142: mock_timer_ = new base::MockTimer(true, true); On 2014/07/29 15:43:26, mmenke wrote: > nit: Could put this in the initializer list. Done. https://codereview.chromium.org/393163003/diff/40001/content/browser/loader/r... content/browser/loader/resource_scheduler_unittest.cc:145: // TODO(aiolos): remove when throttling and coalescing have both landed On 2014/07/29 15:43:26, mmenke wrote: > nit: Capitalize + add period. Done. https://codereview.chromium.org/393163003/diff/40001/content/browser/loader/r... content/browser/loader/resource_scheduler_unittest.cc:1954: CoalescedClientBecomesVisibleAndLoadingStopsTimer) { On 2014/07/29 15:43:26, mmenke wrote: > optional: Not sure this test gets us anything the test two above does not. > Suggest either getting rid of it, or moving it above that test. There are two ways to go from being COALESCED (background and loaded) to being ACTIVE_AND_LOADING (foreground and loading). One is for it to become observable and start loading again, the other is for it to start loading and then become observable. I figured that I should test both cases. Moved this test before the other, and changed this one to test an audibility change instead of a visibility change. Those paths are similar enough and tested enough that I believe we shouldn't need separate tests for all four combinations. Thoughts?
https://codereview.chromium.org/393163003/diff/40001/content/browser/loader/r... File content/browser/loader/resource_scheduler_unittest.cc (right): https://codereview.chromium.org/393163003/diff/40001/content/browser/loader/r... content/browser/loader/resource_scheduler_unittest.cc:1954: CoalescedClientBecomesVisibleAndLoadingStopsTimer) { On 2014/07/29 17:58:51, aiolos wrote: > On 2014/07/29 15:43:26, mmenke wrote: > > optional: Not sure this test gets us anything the test two above does not. > > Suggest either getting rid of it, or moving it above that test. > > There are two ways to go from being COALESCED (background and loaded) to being > ACTIVE_AND_LOADING (foreground and loading). One is for it to become observable > and start loading again, the other is for it to start loading and then become > observable. I figured that I should test both cases. Moved this test before the > other, and changed this one to test an audibility change instead of a visibility > change. Those paths are similar enough and tested enough that I believe we > shouldn't need separate tests for all four combinations. Thoughts? The request becomes uncoalesced on either of those two transitions, I don't think the second one is interesting (And we already have tests for UNTHROTTLED to ACTIVE_AND_LOADING). However, I leave the decision of whether to keep the test up to you. https://codereview.chromium.org/393163003/diff/60001/content/browser/loader/r... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/393163003/diff/60001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:247: // correct count of important types of clients. On 2014/07/29 17:58:51, aiolos wrote: > On 2014/07/29 15:43:26, mmenke wrote: > > nit: I think "important" is a bit weird here. Maybe "various" or "relevant"? > > > (Admittedly, the latter can be a synonym for important). > > Done. nit: You didn't change the wording (If you prefer it as-is, I'm fine with that, just thought one of the other options would be a bit clearer). https://codereview.chromium.org/393163003/diff/60001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:411: if (throttle_state_ != PAUSED) { This should probably go before LoadAnyStartablePendingRequests.
Oh, and LGTM
https://codereview.chromium.org/393163003/diff/40001/content/browser/loader/r... File content/browser/loader/resource_scheduler_unittest.cc (right): https://codereview.chromium.org/393163003/diff/40001/content/browser/loader/r... content/browser/loader/resource_scheduler_unittest.cc:1954: CoalescedClientBecomesVisibleAndLoadingStopsTimer) { On 2014/07/29 18:07:22, mmenke wrote: > On 2014/07/29 17:58:51, aiolos wrote: > > On 2014/07/29 15:43:26, mmenke wrote: > > > optional: Not sure this test gets us anything the test two above does not. > > > Suggest either getting rid of it, or moving it above that test. > > > > There are two ways to go from being COALESCED (background and loaded) to being > > ACTIVE_AND_LOADING (foreground and loading). One is for it to become > observable > > and start loading again, the other is for it to start loading and then become > > observable. I figured that I should test both cases. Moved this test before > the > > other, and changed this one to test an audibility change instead of a > visibility > > change. Those paths are similar enough and tested enough that I believe we > > shouldn't need separate tests for all four combinations. Thoughts? > > The request becomes uncoalesced on either of those two transitions, I don't > think the second one is interesting (And we already have tests for UNTHROTTLED > to ACTIVE_AND_LOADING). However, I leave the decision of whether to keep the > test up to you. True. I kept the test, but changed it so that it's just checking for the audibility change. Let me know if you disagree. https://codereview.chromium.org/393163003/diff/60001/content/browser/loader/r... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/393163003/diff/60001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:247: // correct count of important types of clients. On 2014/07/29 18:07:22, mmenke wrote: > On 2014/07/29 17:58:51, aiolos wrote: > > On 2014/07/29 15:43:26, mmenke wrote: > > > nit: I think "important" is a bit weird here. Maybe "various" or > "relevant"? > > > > > (Admittedly, the latter can be a synonym for important). > > > > Done. > > nit: You didn't change the wording (If you prefer it as-is, I'm fine with that, > just thought one of the other options would be a bit clearer). Thanks for catching that. It is now changed. https://codereview.chromium.org/393163003/diff/60001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:411: if (throttle_state_ != PAUSED) { On 2014/07/29 18:07:22, mmenke wrote: > This should probably go before LoadAnyStartablePendingRequests. Done.
And still LGTM https://codereview.chromium.org/393163003/diff/80001/content/browser/loader/r... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/393163003/diff/80001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:247: // correct count of relavent types of clients. nit: relevant
The CQ bit was checked by aiolos@chromium.org
The CQ bit was unchecked by aiolos@chromium.org
The CQ bit was checked by aiolos@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/aiolos@chromium.org/393163003/100001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_chromium_clang_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_clang_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by aiolos@chromium.org
On 2014/07/29 22:37:51, I haz the power (commit-bot) wrote: > Try jobs failed on following builders: > linux_chromium_clang_dbg on tryserver.chromium.linux > (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) This is due to the movement of a blink file.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/aiolos@chromium.org/393163003/100001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_chromium_clang_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Message was sent while issue was closed.
Change committed as 286372 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
