Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2711093002:
Purge once random minutes(between 30min and 60min) after backgrounded. (Closed)
DescriptionPurge once random minutes(between 30min and 60min) after backgrounded.
Design doc: https://docs.google.com/document/d/1L5qRmCwjidiZWKmQRiuqMjZivBWV7XSW9RTypg9XxlI/edit?usp=sharing
BUG=635419
Review-Url: https://codereview.chromium.org/2711093002
Cr-Commit-Position: refs/heads/master@{#457386}
Committed: https://chromium.googlesource.com/chromium/src/+/e8014f980b6380e690b2e386ac8f39d228419e2f
Patch Set 1 #Patch Set 2 : s/PurgeAndSuspendState/PurgeState/g #Patch Set 3 : s/RUNNING/NOT_PURGED/g #
Total comments: 45
Patch Set 4 : Fixed. #
Total comments: 21
Patch Set 5 : Added a browser test. #
Total comments: 19
Patch Set 6 : Updated. #
Total comments: 2
Patch Set 7 : Rebased #
Total comments: 9
Patch Set 8 : Fixed #
Messages
Total messages: 80 (58 generated)
The CQ bit was checked by tasak@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by tasak@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by tasak@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by tasak@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #4 (id:60001) has been deleted
Patchset #3 (id:40001) has been deleted
Patchset #2 (id:20001) has been deleted
The CQ bit was checked by tasak@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Purge once 30min+random(0~30min) after backgrounded. BUG= ========== to ========== Purge once random X minutes in [30min, 60min) after backgrounded. BUG= ==========
Description was changed from ========== Purge once random X minutes in [30min, 60min) after backgrounded. BUG= ========== to ========== Purge once random X minutes in [30min, 60min) after backgrounded. Design doc: https://docs.google.com/document/d/1L5qRmCwjidiZWKmQRiuqMjZivBWV7XSW9RTypg9Xx... BUG= ==========
Description was changed from ========== Purge once random X minutes in [30min, 60min) after backgrounded. Design doc: https://docs.google.com/document/d/1L5qRmCwjidiZWKmQRiuqMjZivBWV7XSW9RTypg9Xx... BUG= ========== to ========== Purge once random X minutes in [30min, 60min) after backgrounded. Design doc: https://docs.google.com/document/d/1L5qRmCwjidiZWKmQRiuqMjZivBWV7XSW9RTypg9Xx... BUG=635419 ==========
Description was changed from ========== Purge once random X minutes in [30min, 60min) after backgrounded. Design doc: https://docs.google.com/document/d/1L5qRmCwjidiZWKmQRiuqMjZivBWV7XSW9RTypg9Xx... BUG=635419 ========== to ========== Purge once random minutes(between 30min and 60min) after backgrounded. Design doc: https://docs.google.com/document/d/1L5qRmCwjidiZWKmQRiuqMjZivBWV7XSW9RTypg9Xx... BUG=635419 ==========
tasak@google.com changed reviewers: + chrisha@chromium.org, haraken@chromium.org, shrike@chromium.org, wez@chromium.org
Would you review this CL?
https://codereview.chromium.org/2711093002/diff/100001/chrome/browser/memory/... File chrome/browser/memory/tab_manager.cc (right): https://codereview.chromium.org/2711093002/diff/100001/chrome/browser/memory/... chrome/browser/memory/tab_manager.cc:91: const unsigned int kRangeOfTimeToFirstPurgeInMinutes = 30; How is this a range? Or is it implicitly lower bounded by zero?
https://codereview.chromium.org/2711093002/diff/100001/chrome/browser/memory/... File chrome/browser/memory/tab_manager.cc (right): https://codereview.chromium.org/2711093002/diff/100001/chrome/browser/memory/... chrome/browser/memory/tab_manager.cc:90: // backgrounded and in-active. This comment is pretty confusingly worded! Would it be clearer to express this as a "maximum" time-to-first-purge, and the existing time-to-first-purge as a minimum? Rather than as a minimum + range? Also, do you need this value to be visible to tests? Surely tests need to know the maximum time-to-first-suspend, so they can fast-forward time and be sure to reach the suspended state? https://codereview.chromium.org/2711093002/diff/100001/chrome/browser/memory/... chrome/browser/memory/tab_manager.cc:688: // cache memory and suspend tabs which becomes and keeps backgrounded for a nit: Looks like this comment regarding purging is missing some words? https://codereview.chromium.org/2711093002/diff/100001/chrome/browser/memory/... chrome/browser/memory/tab_manager.cc:723: DCHECK(content); You don't need a DCHECK here, since you're about to de-reference |content|, which will crash cleanly if it's null. GetWebContentsData() will already DCHECK(contents) (twice!) in any case; those should probably be removed as well (not in this CL, though) since they're similarly redundant. https://codereview.chromium.org/2711093002/diff/100001/chrome/browser/memory/... chrome/browser/memory/tab_manager.cc:728: DCHECK(state == NOT_PURGED); This is a tautologous DCHECK, since there are only two possible states; you should change this code to use a switch(state) so that later addition of some new PurgeState value will be caught at compile-time if this code isn't also updated. https://codereview.chromium.org/2711093002/diff/100001/chrome/browser/memory/... chrome/browser/memory/tab_manager.cc:729: auto time_passed = There's no need to use |auto| here; it makes the code less readable since the reader has to either know, or guess, that the value will probably be base::TimeDelta, rather than e.g. an int holding a number of milliseconds. https://codereview.chromium.org/2711093002/diff/100001/chrome/browser/memory/... chrome/browser/memory/tab_manager.cc:757: // timers if we want necessary and sufficient signals. It's not clear what this (pre-existing) comment is trying to express, but it seems to conflict with the |continue| immediately preceding it - should the if() continue be removed, or the comment be updated? https://codereview.chromium.org/2711093002/diff/100001/chrome/browser/memory/... chrome/browser/memory/tab_manager.cc:759: DCHECK(next_state == PURGED); As noted in the previous function, you can replace the if()continue with a switch(next_state) with the two cases, to get compile-time checking that you must be in the correct state (though see my comment re the comment above vs the if() being inconsistent). However, since GetNextPurgeState() never transitions from PURGED->NOT_PURGED (that happens separately in response to activity) and since GetNextPurgeState() already fetches the current state to shortcut the PURGED case, I'd suggest change it to something like: bool purge_now = ShouldPurgeAtTime(current_time); if (!purge_now) continue // Do purge stuff. SetPurgeState(PURGED) PurgeAndSuspend() which would at least avoid making duplicate calls to GetPurgeState(). https://codereview.chromium.org/2711093002/diff/100001/chrome/browser/memory/... chrome/browser/memory/tab_manager.cc:865: // kRangeOfTimeToFirstPurgeInMinutes to invoke purge. I think the more important thing to get across in this comment is that we are re-setting the time-to-purge to a new random value every time a tab becomes active (otherwise it's a little surprising to see us setting teh time-to-suspend when the tab becomes active, rather than when it becomes inactive) https://codereview.chromium.org/2711093002/diff/100001/chrome/browser/memory/... chrome/browser/memory/tab_manager.cc:870: GetWebContentsData(new_contents)->SetTimeToFirstPurge(time_to_first_purge); nit: I'd suggest calling this SetTimeToPurge(), so you can re-use it if you want to do periodic purges; even if purge is a one-shot operation, it's still a reasonable name. https://codereview.chromium.org/2711093002/diff/100001/chrome/browser/memory/... chrome/browser/memory/tab_manager.cc:875: GetWebContentsData(old_contents)->SetLastInactiveTime(NowTicks()); Again, this is pre-existing code, but shouldn't this be SetLast*Active*Time()? Surely this is the last time the old tab was _active_? https://codereview.chromium.org/2711093002/diff/100001/chrome/browser/memory/... File chrome/browser/memory/tab_manager.h (right): https://codereview.chromium.org/2711093002/diff/100001/chrome/browser/memory/... chrome/browser/memory/tab_manager.h:265: // - When ActiveTabChaged, the newly activated tab's state will be NOT_PURGED. typo: ActiveTabChanged The naming here implies that purging is a one-off operation, i.e. that we purge one, at the point of switching between states - but I think we actually purge every 20 minutes? https://codereview.chromium.org/2711093002/diff/100001/chrome/browser/memory/... chrome/browser/memory/tab_manager.h:266: enum PurgeState { Note that as per style-guide, this and the other enums and types defined in this private section should preferably come before function and method definitions. https://codereview.chromium.org/2711093002/diff/100001/chrome/browser/memory/... chrome/browser/memory/tab_manager.h:271: content::WebContents* GetWebContentsById(int64_t tab_contents_id) const; Why is this getter placed randomly in-between the enum typedef and GetNextPurgeState()? https://codereview.chromium.org/2711093002/diff/100001/chrome/browser/memory/... chrome/browser/memory/tab_manager.h:273: // Returns the next state of the purge and suspend. The function is called GetNextPurgeState() so the reader can see that it returns the next state - this comment should clarify what that _means_, and how it is used. e.g. "Returns PURGED if |content| should be asked to purge memory at time |current_time|". Why do we need a |current_time| parameter - why not use Now()? If the point is that we sometimes pass in something other than |current_time| then rename it to just |time| or similar? https://codereview.chromium.org/2711093002/diff/100001/chrome/browser/memory/... chrome/browser/memory/tab_manager.h:277: // Purges renderers in backgrounded tabs. Again, the reader can tell that from the name; what's important is what that means, and how it happens, e.g. is this called periodically by some internal timer? Or is it called whenever a tab state changes? Also, the comment is not strictly accurate - this function only purges tabs which are not already in the PURGED state. https://codereview.chromium.org/2711093002/diff/100001/chrome/browser/memory/... chrome/browser/memory/tab_manager.h:362: base::TimeDelta time_to_first_purge_; This doesn't change over the lifetime of the TabManager, so you could initialize it in the constructor and make the member const. https://codereview.chromium.org/2711093002/diff/100001/chrome/browser/memory/... File chrome/browser/memory/tab_manager_web_contents_data.h (right): https://codereview.chromium.org/2711093002/diff/100001/chrome/browser/memory/... chrome/browser/memory/tab_manager_web_contents_data.h:88: void SetTimeToFirstPurge(const base::TimeDelta& time_to_first_purge); since these are simple setter & getter, they can be defined inline: void set_time_to_purge(const ...& ...) { time_to_purge_ = ...; } https://codereview.chromium.org/2711093002/diff/100001/chrome/browser/memory/... chrome/browser/memory/tab_manager_web_contents_data.h:96: TabManager::PurgeState GetPurgeState() const; Why not put the setter and getter for PurgeState next to one another? https://codereview.chromium.org/2711093002/diff/100001/chrome/browser/memory/... chrome/browser/memory/tab_manager_web_contents_data.h:144: PurgeState purge_state_; Putting these fields here, rather than in tab_data_ means they won't be copied by CopyState() - is that deliberate? If so then a comment would be good here.
The CQ bit was checked by tasak@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #4 (id:120001) has been deleted
The CQ bit was checked by tasak@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thank you for review. https://codereview.chromium.org/2711093002/diff/100001/chrome/browser/memory/... File chrome/browser/memory/tab_manager.cc (right): https://codereview.chromium.org/2711093002/diff/100001/chrome/browser/memory/... chrome/browser/memory/tab_manager.cc:90: // backgrounded and in-active. On 2017/02/27 22:20:55, Wez wrote: > This comment is pretty confusingly worded! > > Would it be clearer to express this as a "maximum" time-to-first-purge, and the > existing time-to-first-purge as a minimum? Rather than as a minimum + range? > > Also, do you need this value to be visible to tests? Surely tests need to know > the maximum time-to-first-suspend, so they can fast-forward time and be sure to > reach the suspended state? Done. https://codereview.chromium.org/2711093002/diff/100001/chrome/browser/memory/... chrome/browser/memory/tab_manager.cc:91: const unsigned int kRangeOfTimeToFirstPurgeInMinutes = 30; On 2017/02/27 13:31:06, chrisha wrote: > How is this a range? Or is it implicitly lower bounded by zero? Done. https://codereview.chromium.org/2711093002/diff/100001/chrome/browser/memory/... chrome/browser/memory/tab_manager.cc:688: // cache memory and suspend tabs which becomes and keeps backgrounded for a On 2017/02/27 22:20:55, Wez wrote: > nit: Looks like this comment regarding purging is missing some words? Done. https://codereview.chromium.org/2711093002/diff/100001/chrome/browser/memory/... chrome/browser/memory/tab_manager.cc:707: } Now we don't suspend any renderers. So we can remove this code. However I'm not sure whether I should remove now or not... I should just add TODO(tasak)? https://codereview.chromium.org/2711093002/diff/100001/chrome/browser/memory/... chrome/browser/memory/tab_manager.cc:723: DCHECK(content); On 2017/02/27 22:20:55, Wez wrote: > You don't need a DCHECK here, since you're about to de-reference |content|, > which will crash cleanly if it's null. > > GetWebContentsData() will already DCHECK(contents) (twice!) in any case; those > should probably be removed as well (not in this CL, though) since they're > similarly redundant. Done. https://codereview.chromium.org/2711093002/diff/100001/chrome/browser/memory/... chrome/browser/memory/tab_manager.cc:728: DCHECK(state == NOT_PURGED); On 2017/02/27 22:20:55, Wez wrote: > This is a tautologous DCHECK, since there are only two possible states; you > should change this code to use a switch(state) so that later addition of some > new PurgeState value will be caught at compile-time if this code isn't also > updated. Done. https://codereview.chromium.org/2711093002/diff/100001/chrome/browser/memory/... chrome/browser/memory/tab_manager.cc:729: auto time_passed = On 2017/02/27 22:20:55, Wez wrote: > There's no need to use |auto| here; it makes the code less readable since the > reader has to either know, or guess, that the value will probably be > base::TimeDelta, rather than e.g. an int holding a number of milliseconds. Done. https://codereview.chromium.org/2711093002/diff/100001/chrome/browser/memory/... chrome/browser/memory/tab_manager.cc:757: // timers if we want necessary and sufficient signals. On 2017/02/27 22:20:55, Wez wrote: > It's not clear what this (pre-existing) comment is trying to express, but it > seems to conflict with the |continue| immediately preceding it - should the if() > continue be removed, or the comment be updated? Done. https://codereview.chromium.org/2711093002/diff/100001/chrome/browser/memory/... chrome/browser/memory/tab_manager.cc:759: DCHECK(next_state == PURGED); On 2017/02/27 22:20:55, Wez wrote: > As noted in the previous function, you can replace the if()continue with a > switch(next_state) with the two cases, to get compile-time checking that you > must be in the correct state (though see my comment re the comment above vs the > if() being inconsistent). > > However, since GetNextPurgeState() never transitions from PURGED->NOT_PURGED > (that happens separately in response to activity) and since GetNextPurgeState() > already fetches the current state to shortcut the PURGED case, I'd suggest > change it to something like: > > bool purge_now = ShouldPurgeAtTime(current_time); > if (!purge_now) > continue > > // Do purge stuff. > SetPurgeState(PURGED) > PurgeAndSuspend() > > which would at least avoid making duplicate calls to GetPurgeState(). Done. https://codereview.chromium.org/2711093002/diff/100001/chrome/browser/memory/... chrome/browser/memory/tab_manager.cc:865: // kRangeOfTimeToFirstPurgeInMinutes to invoke purge. On 2017/02/27 22:20:55, Wez wrote: > I think the more important thing to get across in this comment is that we are > re-setting the time-to-purge to a new random value every time a tab becomes > active (otherwise it's a little surprising to see us setting teh time-to-suspend > when the tab becomes active, rather than when it becomes inactive) Moved the code into the statement: if(old_contents) { ... }, and added the code to TabInsertedAt. https://codereview.chromium.org/2711093002/diff/100001/chrome/browser/memory/... chrome/browser/memory/tab_manager.cc:870: GetWebContentsData(new_contents)->SetTimeToFirstPurge(time_to_first_purge); On 2017/02/27 22:20:55, Wez wrote: > nit: I'd suggest calling this SetTimeToPurge(), so you can re-use it if you want > to do periodic purges; even if purge is a one-shot operation, it's still a > reasonable name. Done. https://codereview.chromium.org/2711093002/diff/100001/chrome/browser/memory/... chrome/browser/memory/tab_manager.cc:875: GetWebContentsData(old_contents)->SetLastInactiveTime(NowTicks()); On 2017/02/27 22:20:55, Wez wrote: > Again, this is pre-existing code, but shouldn't this be SetLast*Active*Time()? > Surely this is the last time the old tab was _active_? As far as I understand, new_contents is a tab which has been just activated, and old_contents is a tab which was active and has been just deactivated. So old_contents's LastInactiveTime is updated here. https://codereview.chromium.org/2711093002/diff/100001/chrome/browser/memory/... File chrome/browser/memory/tab_manager.h (right): https://codereview.chromium.org/2711093002/diff/100001/chrome/browser/memory/... chrome/browser/memory/tab_manager.h:265: // - When ActiveTabChaged, the newly activated tab's state will be NOT_PURGED. On 2017/02/27 22:20:56, Wez wrote: > typo: ActiveTabChanged > > The naming here implies that purging is a one-off operation, i.e. that we purge > one, at the point of switching between states - but I think we actually purge > every 20 minutes? The new code doesn't purge every 20 minutes, because I think, ActiveTabChanged is invoked when a user moves a background tab to foreground. https://codereview.chromium.org/2711093002/diff/100001/chrome/browser/memory/... chrome/browser/memory/tab_manager.h:266: enum PurgeState { On 2017/02/27 22:20:56, Wez wrote: > Note that as per style-guide, this and the other enums and types defined in this > private section should preferably come before function and method definitions. Removed. https://codereview.chromium.org/2711093002/diff/100001/chrome/browser/memory/... chrome/browser/memory/tab_manager.h:271: content::WebContents* GetWebContentsById(int64_t tab_contents_id) const; On 2017/02/27 22:20:55, Wez wrote: > Why is this getter placed randomly in-between the enum typedef and > GetNextPurgeState()? Removed. https://codereview.chromium.org/2711093002/diff/100001/chrome/browser/memory/... chrome/browser/memory/tab_manager.h:273: // Returns the next state of the purge and suspend. Replaced the method with ShouldPurgeAtTime. On 2017/02/27 22:20:55, Wez wrote: > The function is called GetNextPurgeState() so the reader can see that it returns > the next state - this comment should clarify what that _means_, and how it is > used. > > e.g. "Returns PURGED if |content| should be asked to purge memory at time > |current_time|". > > Why do we need a |current_time| parameter - why not use Now()? If the point is > that we sometimes pass in something other than |current_time| then rename it to > just |time| or similar? For testing. If we use Now() in the method, it is difficult to pass test_clock. https://codereview.chromium.org/2711093002/diff/100001/chrome/browser/memory/... chrome/browser/memory/tab_manager.h:277: // Purges renderers in backgrounded tabs. On 2017/02/27 22:20:56, Wez wrote: > Again, the reader can tell that from the name; what's important is what that > means, and how it happens, e.g. is this called periodically by some internal > timer? Or is it called whenever a tab state changes? > > Also, the comment is not strictly accurate - this function only purges tabs > which are not already in the PURGED state. Done. https://codereview.chromium.org/2711093002/diff/100001/chrome/browser/memory/... chrome/browser/memory/tab_manager.h:362: base::TimeDelta time_to_first_purge_; On 2017/02/27 22:20:56, Wez wrote: > This doesn't change over the lifetime of the TabManager, so you could initialize > it in the constructor and make the member const. So is it possible to initialize the value by using GetVariationParamValue and StringToUint in the constructor? As far as I understand, TabManager::Start() is invoked at almost the same time as TabManager is created. I think, it is ok to initialize the value in Start(). https://codereview.chromium.org/2711093002/diff/100001/chrome/browser/memory/... File chrome/browser/memory/tab_manager_web_contents_data.h (right): https://codereview.chromium.org/2711093002/diff/100001/chrome/browser/memory/... chrome/browser/memory/tab_manager_web_contents_data.h:88: void SetTimeToFirstPurge(const base::TimeDelta& time_to_first_purge); On 2017/02/27 22:20:56, Wez wrote: > since these are simple setter & getter, they can be defined inline: > > void set_time_to_purge(const ...& ...) { time_to_purge_ = ...; } Done. https://codereview.chromium.org/2711093002/diff/100001/chrome/browser/memory/... chrome/browser/memory/tab_manager_web_contents_data.h:96: TabManager::PurgeState GetPurgeState() const; On 2017/02/27 22:20:56, Wez wrote: > Why not put the setter and getter for PurgeState next to one another? Done. https://codereview.chromium.org/2711093002/diff/100001/chrome/browser/memory/... chrome/browser/memory/tab_manager_web_contents_data.h:144: PurgeState purge_state_; On 2017/02/27 22:20:56, Wez wrote: > Putting these fields here, rather than in tab_data_ means they won't be copied > by CopyState() - is that deliberate? If so then a comment would be good here. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2711093002/diff/100001/chrome/browser/memory/... File chrome/browser/memory/tab_manager.h (right): https://codereview.chromium.org/2711093002/diff/100001/chrome/browser/memory/... chrome/browser/memory/tab_manager.h:265: // - When ActiveTabChaged, the newly activated tab's state will be NOT_PURGED. On 2017/02/28 09:45:37, tasak wrote: > On 2017/02/27 22:20:56, Wez wrote: > > typo: ActiveTabChanged > > > > The naming here implies that purging is a one-off operation, i.e. that we > purge > > one, at the point of switching between states - but I think we actually purge > > every 20 minutes? > > The new code doesn't purge every 20 minutes, because I think, ActiveTabChanged > is invoked when a user moves a background tab to foreground. I thought the idea was to re-purge each background tab every 20 minutes or so, though? https://codereview.chromium.org/2711093002/diff/100001/chrome/browser/memory/... chrome/browser/memory/tab_manager.h:273: // Returns the next state of the purge and suspend. On 2017/02/28 09:45:37, tasak wrote: > On 2017/02/27 22:20:55, Wez wrote: > > Why do we need a |current_time| parameter - why not use Now()? If the point is > > that we sometimes pass in something other than |current_time| then rename it > to > > just |time| or similar? > > For testing. If we use Now() in the method, it is difficult to pass test_clock. The class has a set_test_tick_clock() method, though, so surely we just set that, and then call this function after having fast-forwarded the clock? https://codereview.chromium.org/2711093002/diff/140001/chrome/browser/memory/... File chrome/browser/memory/tab_manager.h (right): https://codereview.chromium.org/2711093002/diff/140001/chrome/browser/memory/... chrome/browser/memory/tab_manager.h:150: // Returns true when a given renderer can be purged when it is backgrounded. nit: Suggest ".. if the specified rendered is eligible for purging." Also note that the name implies that it's invalid to call this for a non-backgrounded renderer - is that the case? https://codereview.chromium.org/2711093002/diff/140001/chrome/browser/memory/... chrome/browser/memory/tab_manager.h:265: // value is min_time_to_purge + kRangeBetweenMinAndMaxTimeToPurge. kRangeBetween... isn't defined anywhere in this header. It's also not clear why this function needs to be public; IMO it would be better to expose MinTimeUntilPurge and MaxTimeUntilPurge instead - you can then have one test which lets the test choose, and ensures it's within that range, and a separate test which uses a fixed value to verify that the should-purge result changes correctly. https://codereview.chromium.org/2711093002/diff/140001/chrome/browser/memory/... File chrome/browser/memory/tab_manager_unittest.cc (right): https://codereview.chromium.org/2711093002/diff/140001/chrome/browser/memory/... chrome/browser/memory/tab_manager_unittest.cc:675: TEST_F(TabManagerTest, NextPurgeState) { NextPurgeState was the name of the function - you really want the test name to describe what you're testing. In this case I think you want two separate tests: 1. Do we choose an appropriate time to purge? - If I notify TabManager that a tab was backgrounded, does it get a time-to-purge in the right default range? 2. Do we respect the configured time-to-purge? - If I set a particular time-to-purge, does the ShouldPurge result change at the right time? https://codereview.chromium.org/2711093002/diff/140001/chrome/browser/memory/... chrome/browser/memory/tab_manager_unittest.cc:684: base::SimpleTestTickClock test_clock; You probably want to tab_manager.set_test_tick_clock() with this here. https://codereview.chromium.org/2711093002/diff/140001/chrome/browser/memory/... File chrome/browser/memory/tab_manager_web_contents_data.h (right): https://codereview.chromium.org/2711093002/diff/140001/chrome/browser/memory/... chrome/browser/memory/tab_manager_web_contents_data.h:85: void SetPurgeState(bool state) { purge_state_ = state; } Similar to below, this can be set_is_purged() https://codereview.chromium.org/2711093002/diff/140001/chrome/browser/memory/... chrome/browser/memory/tab_manager_web_contents_data.h:90: bool IsPurged() const { return purge_state_; } This can be hacker_case is_purged() https://codereview.chromium.org/2711093002/diff/140001/chrome/browser/memory/... chrome/browser/memory/tab_manager_web_contents_data.h:93: void SetTimeToPurge(const base::TimeDelta& time_to_purge) { These can be hacker_case setter and getter. https://codereview.chromium.org/2711093002/diff/140001/chrome/browser/memory/... chrome/browser/memory/tab_manager_web_contents_data.h:146: // Initially purge_state_ is false. This isn't very clear; I think you want to say something like: "True if the tab has been purged since it was last active, and should therefore not be purged again." The rest of the detail of the algorithm doesn't belong here; the only detail to keep here would be: "When a discarded tab is restored, we set |is_purged_=true|, since purging the newly-created renderer doesn't make sense." Although note that your comment "we make purge_state_ true" seems to contradict the .cc, in which you set it to false. https://codereview.chromium.org/2711093002/diff/140001/chrome/browser/memory/... chrome/browser/memory/tab_manager_web_contents_data.h:153: bool purge_state_; Rename this is_purged_, to match the getter & setter naming.
The CQ bit was checked by tasak@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #5 (id:160001) has been deleted
The CQ bit was checked by tasak@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thank you for review. https://codereview.chromium.org/2711093002/diff/100001/chrome/browser/memory/... File chrome/browser/memory/tab_manager.h (right): https://codereview.chromium.org/2711093002/diff/100001/chrome/browser/memory/... chrome/browser/memory/tab_manager.h:265: // - When ActiveTabChaged, the newly activated tab's state will be NOT_PURGED. On 2017/03/02 02:28:32, Wez wrote: > On 2017/02/28 09:45:37, tasak wrote: > > On 2017/02/27 22:20:56, Wez wrote: > > > typo: ActiveTabChanged > > > > > > The naming here implies that purging is a one-off operation, i.e. that we > > purge > > > one, at the point of switching between states - but I think we actually > purge > > > every 20 minutes? > > > > The new code doesn't purge every 20 minutes, because I think, ActiveTabChanged > > is invoked when a user moves a background tab to foreground. > > I thought the idea was to re-purge each background tab every 20 minutes or so, > though? As far as I understand, we have a strong concern that re-purge occurs under critical memory pressure situation. I agree that 1 purge also occurs under the same situation, but next purge doesn't. So I would like to start from 1 purge implementation. https://codereview.chromium.org/2711093002/diff/100001/chrome/browser/memory/... chrome/browser/memory/tab_manager.h:273: // Returns the next state of the purge and suspend. On 2017/03/02 02:28:32, Wez wrote: > On 2017/02/28 09:45:37, tasak wrote: > > On 2017/02/27 22:20:55, Wez wrote: > > > Why do we need a |current_time| parameter - why not use Now()? If the point > is > > > that we sometimes pass in something other than |current_time| then rename it > > to > > > just |time| or similar? > > > > For testing. If we use Now() in the method, it is difficult to pass > test_clock. > > The class has a set_test_tick_clock() method, though, so surely we just set > that, and then call this function after having fast-forwarded the clock? I see. Done. https://codereview.chromium.org/2711093002/diff/140001/chrome/browser/memory/... File chrome/browser/memory/tab_manager.h (right): https://codereview.chromium.org/2711093002/diff/140001/chrome/browser/memory/... chrome/browser/memory/tab_manager.h:150: // Returns true when a given renderer can be purged when it is backgrounded. On 2017/03/02 02:28:32, Wez wrote: > nit: Suggest ".. if the specified rendered is eligible for purging." Done. > Also note that the name implies that it's invalid to call this for a > non-backgrounded renderer - is that the case? I think, the name implies that the method doesn't check whether the renderer is background one or not. We need to move the check into the method? So we could rename the name into CanPurgeRenderer or something. https://codereview.chromium.org/2711093002/diff/140001/chrome/browser/memory/... chrome/browser/memory/tab_manager.h:265: // value is min_time_to_purge + kRangeBetweenMinAndMaxTimeToPurge. On 2017/03/02 02:28:32, Wez wrote: > kRangeBetween... isn't defined anywhere in this header. > > It's also not clear why this function needs to be public; IMO it would be better > to expose MinTimeUntilPurge and MaxTimeUntilPurge instead - you can then have > one test which lets the test choose, and ensures it's within that range, and a > separate test which uses a fixed value to verify that the should-purge result > changes correctly. I would like to confirm your suggestion. So: - The GetTimeToPurge is a private method. So what the "public" means? - What MinTimeUntilPurge and MaxTimeUntilPurge mean? They are default TimeUntilPurge? Or could be updated by finch parameter? - If we test GetTimeToPurge and use fixed value for ShouldPurgeAtDefaultTime test (the old name is NextPurgeState), we could test enough? - MaxTimeUntilPurge is also updated by finch parameter? If so, we should plan to change MaxTimeUntilPurge at the future finch experiment? (I think, the range is fixed value, i.e. 30 minutes. I have no plan to test the range at the future finch experiment.) https://codereview.chromium.org/2711093002/diff/140001/chrome/browser/memory/... File chrome/browser/memory/tab_manager_unittest.cc (right): https://codereview.chromium.org/2711093002/diff/140001/chrome/browser/memory/... chrome/browser/memory/tab_manager_unittest.cc:675: TEST_F(TabManagerTest, NextPurgeState) { On 2017/03/02 02:28:32, Wez wrote: > NextPurgeState was the name of the function - you really want the test name to > describe what you're testing. Renamed to be ShouldPurgeAtDefaultTime. > In this case I think you want two separate tests: > > 1. Do we choose an appropriate time to purge? > - If I notify TabManager that a tab was backgrounded, does it get a > time-to-purge in the right default range? I added a unit test to see GetTimeToPurge. > 2. Do we respect the configured time-to-purge? > - If I set a particular time-to-purge, does the ShouldPurge result change at the > right time? I updated ShouldPurgedAtDefaultTime. I also added a new browser test. https://codereview.chromium.org/2711093002/diff/140001/chrome/browser/memory/... chrome/browser/memory/tab_manager_unittest.cc:684: base::SimpleTestTickClock test_clock; On 2017/03/02 02:28:32, Wez wrote: > You probably want to tab_manager.set_test_tick_clock() with this here. Done. https://codereview.chromium.org/2711093002/diff/140001/chrome/browser/memory/... File chrome/browser/memory/tab_manager_web_contents_data.h (right): https://codereview.chromium.org/2711093002/diff/140001/chrome/browser/memory/... chrome/browser/memory/tab_manager_web_contents_data.h:85: void SetPurgeState(bool state) { purge_state_ = state; } On 2017/03/02 02:28:32, Wez wrote: > Similar to below, this can be set_is_purged() Done. https://codereview.chromium.org/2711093002/diff/140001/chrome/browser/memory/... chrome/browser/memory/tab_manager_web_contents_data.h:90: bool IsPurged() const { return purge_state_; } On 2017/03/02 02:28:33, Wez wrote: > This can be hacker_case is_purged() Done. https://codereview.chromium.org/2711093002/diff/140001/chrome/browser/memory/... chrome/browser/memory/tab_manager_web_contents_data.h:93: void SetTimeToPurge(const base::TimeDelta& time_to_purge) { On 2017/03/02 02:28:32, Wez wrote: > These can be hacker_case setter and getter. Done. https://codereview.chromium.org/2711093002/diff/140001/chrome/browser/memory/... chrome/browser/memory/tab_manager_web_contents_data.h:146: // Initially purge_state_ is false. On 2017/03/02 02:28:32, Wez wrote: > This isn't very clear; I think you want to say something like: > > "True if the tab has been purged since it was last active, and should therefore > not be purged again." > > The rest of the detail of the algorithm doesn't belong here; the only detail to > keep here would be: > Done. > "When a discarded tab is restored, we set |is_purged_=true|, since purging the > newly-created renderer doesn't make sense." I think, when a restored tab is activated, ActiveTabChanged will be invoked. So is_purged_ will updated. > Although note that your comment "we make purge_state_ true" seems to contradict > the .cc, in which you set it to false. I set new_contents' is_purged_ to false in ActiveTabChanged. Do you mean the set_is_purged(false)? Since the tab is activated, it is ok to invoke Purge when the tab is backgrounded, I think. Because active & foreground tabs will be never purged. Your suggestion is that we should do for old_contents? Either is fine. https://codereview.chromium.org/2711093002/diff/140001/chrome/browser/memory/... chrome/browser/memory/tab_manager_web_contents_data.h:153: bool purge_state_; On 2017/03/02 02:28:33, Wez wrote: > Rename this is_purged_, to match the getter & setter naming. Done.
Apologies for the delayed review, and thanks for addressing my comments - just a few remaining bits to iron out. https://codereview.chromium.org/2711093002/diff/140001/chrome/browser/memory/... File chrome/browser/memory/tab_manager.h (right): https://codereview.chromium.org/2711093002/diff/140001/chrome/browser/memory/... chrome/browser/memory/tab_manager.h:265: // value is min_time_to_purge + kRangeBetweenMinAndMaxTimeToPurge. On 2017/03/02 09:22:47, tasak wrote: > So: > - The GetTimeToPurge is a private method. So what the "public" means? Sorry; I meant why does it need to be a member function, rather than a static function in the .cc file? > - What MinTimeUntilPurge and MaxTimeUntilPurge mean? They are default > TimeUntilPurge? Or could be updated by finch parameter? I'm suggesting that rather than TimeToPurge and RangeOfTimeToPurge you have MinTimeToPurge and MaxTimeToPurge as the parameters to the purge-timing logic; the implementation is basically the same, but it's easier to talk about the minimum and maximum time-to-purge. > - If we test GetTimeToPurge and use fixed value for ShouldPurgeAtDefaultTime > test (the old name is NextPurgeState), we could test enough? Yes, I think that's what I was suggesting; the two tests are: 1. Given the configured Min and Max time-to-purge, do we choose a value that is *sensible* - basically a sanity-check for whether we accidentally set it to 30 hours again! 2. Given a test-specified max and min, do we see the process purged when we expect it to be? (i.e. definitely not purged until min, and definitely is purged after max) > - MaxTimeUntilPurge is also updated by finch parameter? If so, we should plan to > change MaxTimeUntilPurge at the future finch experiment? > (I think, the range is fixed value, i.e. 30 minutes. I have no plan to test > the range at the future finch experiment.) If you prefer to have a single parameter then I'd suggest: - Have TimeToPurge as that parameter's name, - When choosing the actual time-to-purge, choose a value in the range TimeToPurge..2*TimeToPurge. i.e. if we set time-to-purge to 30m then it'll purge between 30- and 60- minutes after backgrounding. If we set it to 5m then the range will be 5m-10m. Make sense? https://codereview.chromium.org/2711093002/diff/140001/chrome/browser/memory/... File chrome/browser/memory/tab_manager_unittest.cc (right): https://codereview.chromium.org/2711093002/diff/140001/chrome/browser/memory/... chrome/browser/memory/tab_manager_unittest.cc:675: TEST_F(TabManagerTest, NextPurgeState) { On 2017/03/02 09:22:47, tasak wrote: > On 2017/03/02 02:28:32, Wez wrote: > > NextPurgeState was the name of the function - you really want the test name to > > describe what you're testing. > > Renamed to be ShouldPurgeAtDefaultTime. > > > In this case I think you want two separate tests: > > > > 1. Do we choose an appropriate time to purge? > > - If I notify TabManager that a tab was backgrounded, does it get a > > time-to-purge in the right default range? > > I added a unit test to see GetTimeToPurge. > > > 2. Do we respect the configured time-to-purge? > > - If I set a particular time-to-purge, does the ShouldPurge result change at > the > > right time? > > I updated ShouldPurgedAtDefaultTime. > > I also added a new browser test. Acknowledged. https://codereview.chromium.org/2711093002/diff/180001/chrome/browser/memory/... File chrome/browser/memory/tab_manager.h (right): https://codereview.chromium.org/2711093002/diff/180001/chrome/browser/memory/... chrome/browser/memory/tab_manager.h:271: bool ShouldPurgeAtTime(content::WebContents* content) const; ShouldPurgeNow()? Suggest: Returns true if the tab specified by |content| is now eligible to have its memory purged. https://codereview.chromium.org/2711093002/diff/180001/chrome/browser/memory/... File chrome/browser/memory/tab_manager_browsertest.cc (right): https://codereview.chromium.org/2711093002/diff/180001/chrome/browser/memory/... chrome/browser/memory/tab_manager_browsertest.cc:484: IN_PROC_BROWSER_TEST_F(TabManagerTest, PurgeBackgroundRenderer) { Looks good! https://codereview.chromium.org/2711093002/diff/180001/chrome/browser/memory/... chrome/browser/memory/tab_manager_browsertest.cc:526: // The time-to-purge initialied at ActiveTabChanged should be in the typo: initialized https://codereview.chromium.org/2711093002/diff/180001/chrome/browser/memory/... chrome/browser/memory/tab_manager_browsertest.cc:565: // Since tab1 is kept in-active and background for more than nit: in-active -> inactive https://codereview.chromium.org/2711093002/diff/180001/chrome/browser/memory/... chrome/browser/memory/tab_manager_browsertest.cc:571: // Advance the clock for 10 minutes. nit: Remove "for 10 minutes", since someone might change the timing in the test for some reason ;) https://codereview.chromium.org/2711093002/diff/180001/chrome/browser/memory/... chrome/browser/memory/tab_manager_browsertest.cc:575: // Since tab2 is kept in-active and background for more than in-active -> inactive https://codereview.chromium.org/2711093002/diff/180001/chrome/browser/memory/... File chrome/browser/memory/tab_manager_unittest.cc (right): https://codereview.chromium.org/2711093002/diff/180001/chrome/browser/memory/... chrome/browser/memory/tab_manager_unittest.cc:714: test_clock.Advance(base::TimeDelta::FromMinutes(60 * 24)); You can use FromHours(24) here. https://codereview.chromium.org/2711093002/diff/180001/chrome/browser/memory/... chrome/browser/memory/tab_manager_unittest.cc:738: tab_manager.GetWebContentsData(tab2)->set_is_purged(true); You're not actually going through the normal purge flow; you might prefer to configure the tab to purge after one minute, Advance() time, verify that it purges, and then signal that it was active and verify that it is no longer purged, for example. https://codereview.chromium.org/2711093002/diff/180001/chrome/browser/memory/... chrome/browser/memory/tab_manager_unittest.cc:748: EXPECT_TRUE(tabstrip.empty()); nit: Do you need to expect this? Isn't it part of the CloseAllTabs() API contract? https://codereview.chromium.org/2711093002/diff/180001/chrome/browser/memory/... File chrome/browser/memory/tab_manager_web_contents_data.h (right): https://codereview.chromium.org/2711093002/diff/180001/chrome/browser/memory/... chrome/browser/memory/tab_manager_web_contents_data.h:148: // detail to keep here would be: I would actually remove these transition descriptions entirely, and document them alongside the code that implements the logic e.g. at ShouldPurgeNow()
The CQ bit was checked by tasak@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by tasak@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #6 (id:200001) has been deleted
Thank you for review. https://codereview.chromium.org/2711093002/diff/140001/chrome/browser/memory/... File chrome/browser/memory/tab_manager.h (right): https://codereview.chromium.org/2711093002/diff/140001/chrome/browser/memory/... chrome/browser/memory/tab_manager.h:265: // value is min_time_to_purge + kRangeBetweenMinAndMaxTimeToPurge. On 2017/03/03 21:06:51, Wez wrote: range at the future finch experiment.) > > If you prefer to have a single parameter then I'd suggest: > > - Have TimeToPurge as that parameter's name, > - When choosing the actual time-to-purge, choose a value in the range > TimeToPurge..2*TimeToPurge. > > i.e. if we set time-to-purge to 30m then it'll purge between 30- and 60- minutes > after backgrounding. If we set it to 5m then the range will be 5m-10m. > > Make sense? Yeah, this looks good idea. I tried this. So the reason why I prefer a single parameter is that I have no idea about how to measure user experience changes when changing [min..max-time-to-purge]. We know, at least, the range should not be small. https://codereview.chromium.org/2711093002/diff/180001/chrome/browser/memory/... File chrome/browser/memory/tab_manager.h (right): https://codereview.chromium.org/2711093002/diff/180001/chrome/browser/memory/... chrome/browser/memory/tab_manager.h:271: bool ShouldPurgeAtTime(content::WebContents* content) const; On 2017/03/03 21:06:51, Wez wrote: > ShouldPurgeNow()? > > Suggest: Returns true if the tab specified by |content| is now eligible to have > its memory purged. Done. https://codereview.chromium.org/2711093002/diff/180001/chrome/browser/memory/... File chrome/browser/memory/tab_manager_browsertest.cc (right): https://codereview.chromium.org/2711093002/diff/180001/chrome/browser/memory/... chrome/browser/memory/tab_manager_browsertest.cc:526: // The time-to-purge initialied at ActiveTabChanged should be in the On 2017/03/03 21:06:51, Wez wrote: > typo: initialized Done. https://codereview.chromium.org/2711093002/diff/180001/chrome/browser/memory/... chrome/browser/memory/tab_manager_browsertest.cc:565: // Since tab1 is kept in-active and background for more than On 2017/03/03 21:06:51, Wez wrote: > nit: in-active -> inactive Done. https://codereview.chromium.org/2711093002/diff/180001/chrome/browser/memory/... chrome/browser/memory/tab_manager_browsertest.cc:571: // Advance the clock for 10 minutes. On 2017/03/03 21:06:51, Wez wrote: > nit: Remove "for 10 minutes", since someone might change the timing in the test > for some reason ;) Done. https://codereview.chromium.org/2711093002/diff/180001/chrome/browser/memory/... chrome/browser/memory/tab_manager_browsertest.cc:575: // Since tab2 is kept in-active and background for more than On 2017/03/03 21:06:51, Wez wrote: > in-active -> inactive Done. https://codereview.chromium.org/2711093002/diff/180001/chrome/browser/memory/... File chrome/browser/memory/tab_manager_unittest.cc (right): https://codereview.chromium.org/2711093002/diff/180001/chrome/browser/memory/... chrome/browser/memory/tab_manager_unittest.cc:714: test_clock.Advance(base::TimeDelta::FromMinutes(60 * 24)); On 2017/03/03 21:06:51, Wez wrote: > You can use FromHours(24) here. Done. https://codereview.chromium.org/2711093002/diff/180001/chrome/browser/memory/... chrome/browser/memory/tab_manager_unittest.cc:738: tab_manager.GetWebContentsData(tab2)->set_is_purged(true); On 2017/03/03 21:06:51, Wez wrote: > You're not actually going through the normal purge flow; you might prefer to > configure the tab to purge after one minute, Advance() time, verify that it > purges, and then signal that it was active and verify that it is no longer > purged, for example. Done. However, what I want to do here is just checking if "TabManager::ActiveTabChanged" is invoked and if set_is_purged(false) is invoked. https://codereview.chromium.org/2711093002/diff/180001/chrome/browser/memory/... chrome/browser/memory/tab_manager_unittest.cc:748: EXPECT_TRUE(tabstrip.empty()); On 2017/03/03 21:06:51, Wez wrote: > nit: Do you need to expect this? Isn't it part of the CloseAllTabs() API > contract? Removed. https://codereview.chromium.org/2711093002/diff/180001/chrome/browser/memory/... File chrome/browser/memory/tab_manager_web_contents_data.h (right): https://codereview.chromium.org/2711093002/diff/180001/chrome/browser/memory/... chrome/browser/memory/tab_manager_web_contents_data.h:148: // detail to keep here would be: On 2017/03/03 21:06:51, Wez wrote: > I would actually remove these transition descriptions entirely, and document > them alongside the code that implements the logic e.g. at ShouldPurgeNow() I moved the document to TabManager::ActiveTabChanged, and TabManager::PurgeBackgroundedTabsIfNeeded. https://codereview.chromium.org/2711093002/diff/220001/chrome/browser/memory/... File chrome/browser/memory/tab_manager.cc (right): https://codereview.chromium.org/2711093002/diff/220001/chrome/browser/memory/... chrome/browser/memory/tab_manager.cc:229: TabStripModel** model) const { I made FindTabStripModelById TabManager's method, because I want the function to see test_tab_strip_models_. So we are able to use CanSuspendBackgroundRenderer in TabManagerTest.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2711093002/diff/220001/chrome/browser/memory/... File chrome/browser/memory/tab_manager.cc (right): https://codereview.chromium.org/2711093002/diff/220001/chrome/browser/memory/... chrome/browser/memory/tab_manager.cc:229: TabStripModel** model) const { On 2017/03/06 10:30:19, tasak wrote: > I made FindTabStripModelById TabManager's method, because I want the function to > see test_tab_strip_models_. > So we are able to use CanSuspendBackgroundRenderer in TabManagerTest. Acknowledged.
Sorry about the delay in getting back to this.
Thank you for review, wez. chrisha, would you review this CL?
The CQ bit was checked by tasak@google.com to run a CQ dry run
chrisha@, would you review this CL?
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/03/15 02:47:39, tasak wrote: > chrisha@, would you review this CL? Chris is ooo for a while. Shall we ask another reviewer? It's important to land this CL and start an experiment soon.
On 2017/03/15 07:36:08, haraken wrote: > On 2017/03/15 02:47:39, tasak wrote: > > chrisha@, would you review this CL? > > Chris is ooo for a while. Shall we ask another reviewer? It's important to land > this CL and start an experiment soon. I see. Thank you, haraken@.
tasak@google.com changed reviewers: + georgesak@chromium.org
georgesak@, would you review this CL? I need lgtm of chrome/browser/memory's OWNER.
On 2017/03/15 10:10:39, tasak wrote: > georgesak@, would you review this CL? > > I need lgtm of chrome/browser/memory's OWNER. Actually georgesak@ left memory coordination things... Maybe we should add us to browser/memory/OWNERs.
On 2017/03/15 10:22:45, haraken wrote: > On 2017/03/15 10:10:39, tasak wrote: > > georgesak@, would you review this CL? > > > > I need lgtm of chrome/browser/memory's OWNER. > > Actually georgesak@ left memory coordination things... Maybe we should add us to > browser/memory/OWNERs. Given wez@'s thorough review, maybe we can ask some chrome/browser/ owner to rubberstamp it.
Logic and general layout all looks good to me. Some nits around constants and a few typos. https://codereview.chromium.org/2711093002/diff/240001/chrome/browser/memory/... File chrome/browser/memory/tab_manager.cc (right): https://codereview.chromium.org/2711093002/diff/240001/chrome/browser/memory/... chrome/browser/memory/tab_manager.cc:723: min_time_to_purge.InSeconds(), min_time_to_purge.InSeconds() * 2)); I would prefer an explicit max_time_to_purge constant as well... or some other constant that replaces the "2" magic number here. https://codereview.chromium.org/2711093002/diff/240001/chrome/browser/memory/... chrome/browser/memory/tab_manager.cc:860: // When ActiveTabChaged, |new_contents| purged state changes to be false. ActiveTabChanged* https://codereview.chromium.org/2711093002/diff/240001/chrome/browser/memory/... File chrome/browser/memory/tab_manager.h (right): https://codereview.chromium.org/2711093002/diff/240001/chrome/browser/memory/... chrome/browser/memory/tab_manager.h:151: // rendered is eligible for purging. renderer* https://codereview.chromium.org/2711093002/diff/240001/chrome/browser/memory/... File chrome/browser/memory/tab_manager_browsertest.cc (right): https://codereview.chromium.org/2711093002/diff/240001/chrome/browser/memory/... chrome/browser/memory/tab_manager_browsertest.cc:529: base::TimeDelta::FromMinutes(30)); You should be referencing the class constants here so that this unittest doesn't have to be manually updated if those constants change. Also: please have a "min" and a "max" constant. https://codereview.chromium.org/2711093002/diff/240001/chrome/browser/memory/... File chrome/browser/memory/tab_manager_unittest.cc (right): https://codereview.chromium.org/2711093002/diff/240001/chrome/browser/memory/... chrome/browser/memory/tab_manager_unittest.cc:682: EXPECT_LT(time_to_purge, base::TimeDelta::FromMinutes(60)); Ditto for using constants and not magic numbers.
(I don't need another round of review for the changes I suggested, so lgtm!)
The CQ bit was checked by tasak@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thank you for review. https://codereview.chromium.org/2711093002/diff/240001/chrome/browser/memory/... File chrome/browser/memory/tab_manager.cc (right): https://codereview.chromium.org/2711093002/diff/240001/chrome/browser/memory/... chrome/browser/memory/tab_manager.cc:860: // When ActiveTabChaged, |new_contents| purged state changes to be false. On 2017/03/15 17:42:41, chrisha wrote: > ActiveTabChanged* Done. https://codereview.chromium.org/2711093002/diff/240001/chrome/browser/memory/... File chrome/browser/memory/tab_manager.h (right): https://codereview.chromium.org/2711093002/diff/240001/chrome/browser/memory/... chrome/browser/memory/tab_manager.h:151: // rendered is eligible for purging. On 2017/03/15 17:42:41, chrisha wrote: > renderer* Done. https://codereview.chromium.org/2711093002/diff/240001/chrome/browser/memory/... File chrome/browser/memory/tab_manager_browsertest.cc (right): https://codereview.chromium.org/2711093002/diff/240001/chrome/browser/memory/... chrome/browser/memory/tab_manager_browsertest.cc:529: base::TimeDelta::FromMinutes(30)); On 2017/03/15 17:42:41, chrisha wrote: > You should be referencing the class constants here so that this unittest doesn't > have to be manually updated if those constants change. > > Also: please have a "min" and a "max" constant. I know. What I really want to do here is that "manually updating the constants". Because time-to-purge default value (kDefaultPurgeAndSuspendTime) was 30hours (for about 3 months!) and no test covers. If I use such kDefault... value here, I think, it is not able to see whether the default value is wrong (e.g. 30 hours) or not. https://codereview.chromium.org/2711093002/diff/240001/chrome/browser/memory/... File chrome/browser/memory/tab_manager_unittest.cc (right): https://codereview.chromium.org/2711093002/diff/240001/chrome/browser/memory/... chrome/browser/memory/tab_manager_unittest.cc:682: EXPECT_LT(time_to_purge, base::TimeDelta::FromMinutes(60)); On 2017/03/15 17:42:41, chrisha wrote: > Ditto for using constants and not magic numbers. I would like to use magic numbers here to check the default constants.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by tasak@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from wez@chromium.org, chrisha@chromium.org Link to the patchset: https://codereview.chromium.org/2711093002/#ps260001 (title: "Fixed")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 260001, "attempt_start_ts": 1489653609535940,
"parent_rev": "ff9efad9c23a0623f055677bec071f4007b290e1", "commit_rev":
"e8014f980b6380e690b2e386ac8f39d228419e2f"}
Message was sent while issue was closed.
Description was changed from ========== Purge once random minutes(between 30min and 60min) after backgrounded. Design doc: https://docs.google.com/document/d/1L5qRmCwjidiZWKmQRiuqMjZivBWV7XSW9RTypg9Xx... BUG=635419 ========== to ========== Purge once random minutes(between 30min and 60min) after backgrounded. Design doc: https://docs.google.com/document/d/1L5qRmCwjidiZWKmQRiuqMjZivBWV7XSW9RTypg9Xx... BUG=635419 Review-Url: https://codereview.chromium.org/2711093002 Cr-Commit-Position: refs/heads/master@{#457386} Committed: https://chromium.googlesource.com/chromium/src/+/e8014f980b6380e690b2e386ac8f... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:260001) as https://chromium.googlesource.com/chromium/src/+/e8014f980b6380e690b2e386ac8f... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
