Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1978343002:
[TabManager] Start function refactoring. (Closed)
|
Created:
4 years, 7 months ago by Anderson Silva Modified:
4 years, 7 months ago Reviewers:
Georges Khalil CC:
chromium-reviews, chrisha Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[TabManager] Start function refactoring.
BUG=605533
Committed: https://crrev.com/96fd742a6640c6186abf763f4ebda178051716f1
Cr-Commit-Position: refs/heads/master@{#394211}
Patch Set 1 #
Total comments: 8
Patch Set 2 : [TabManager] refactoring. #
Total comments: 4
Patch Set 3 : [TabManager] start function refactoring. #
Total comments: 8
Patch Set 4 : [TabManager] start function refactoring. #Patch Set 5 : [TabManager] Start function refactoring: dealing with different platforms. #
Total comments: 4
Patch Set 6 : #
Total comments: 2
Patch Set 7 : #
Dependent Patchsets: Messages
Total messages: 26 (8 generated)
Description was changed from ========== [TabManager] Start function refactoring. BUG=605533 ========== to ========== [TabManager] Start function refactoring. BUG=605533 ==========
andersoncss@google.com changed reviewers: + georgesak@chromium.org
PTAL
Awsome, first CL! :) https://codereview.chromium.org/1978343002/diff/1/chrome/browser/memory/tab_m... File chrome/browser/memory/tab_manager.cc (right): https://codereview.chromium.org/1978343002/diff/1/chrome/browser/memory/tab_m... chrome/browser/memory/tab_manager.cc:174: #endif Let's simplify this. For CHROMEOS, always return false. Otherwise, check the variation and return the value (so no need to explicitly check for WIN or MACOSX). https://codereview.chromium.org/1978343002/diff/1/chrome/browser/memory/tab_m... chrome/browser/memory/tab_manager.cc:197: #endif Pasted by mistake? https://codereview.chromium.org/1978343002/diff/1/chrome/browser/memory/tab_m... chrome/browser/memory/tab_manager.cc:200: discard_once_ = CheckMultipleDiscards(); The logic here is inverted, as discard_once_ would be true if the function return false. https://codereview.chromium.org/1978343002/diff/1/chrome/browser/memory/tab_m... File chrome/browser/memory/tab_manager.h (right): https://codereview.chromium.org/1978343002/diff/1/chrome/browser/memory/tab_m... chrome/browser/memory/tab_manager.h:81: bool CheckMultipleDiscards(); This should be private (don't forget to reorder the implementation as well). nit: Change function name to AllowMultipleDiscards. nit: Change comment to "Return true is tabs can be discarded more than once."
I've solved the problems in the first comments but I think it would be good to talk about an specific thing that I didn't get. Thank you https://codereview.chromium.org/1978343002/diff/1/chrome/browser/memory/tab_m... File chrome/browser/memory/tab_manager.cc (right): https://codereview.chromium.org/1978343002/diff/1/chrome/browser/memory/tab_m... chrome/browser/memory/tab_manager.cc:174: #endif On 2016/05/16 18:41:22, Georges Khalil wrote: > Let's simplify this. For CHROMEOS, always return false. Otherwise, check the > variation and return the value (so no need to explicitly check for WIN or > MACOSX). I've also thought about that. I didn't do it though because I was wondering about the other platforms such as iOS, Linux or Android. But if it's not an issue to those, then yes it's simpler that way. https://codereview.chromium.org/1978343002/diff/1/chrome/browser/memory/tab_m... chrome/browser/memory/tab_manager.cc:197: #endif On 2016/05/16 18:41:22, Georges Khalil wrote: > Pasted by mistake? No, that's actually because of the #if defined WIN or MAC up there. Maybe we'll end up removing these as well as we follow refactoring the other parts. https://codereview.chromium.org/1978343002/diff/1/chrome/browser/memory/tab_m... chrome/browser/memory/tab_manager.cc:200: discard_once_ = CheckMultipleDiscards(); On 2016/05/16 18:41:22, Georges Khalil wrote: > The logic here is inverted, as discard_once_ would be true if the function > return false. You're right. But then I think we might have an issue or I misunderstood the previous code. Let's sync in person. https://codereview.chromium.org/1978343002/diff/1/chrome/browser/memory/tab_m... File chrome/browser/memory/tab_manager.h (right): https://codereview.chromium.org/1978343002/diff/1/chrome/browser/memory/tab_m... chrome/browser/memory/tab_manager.h:81: bool CheckMultipleDiscards(); On 2016/05/16 18:41:22, Georges Khalil wrote: > This should be private (don't forget to reorder the implementation as well). > > nit: Change function name to AllowMultipleDiscards. > nit: Change comment to "Return true is tabs can be discarded more than once." Acknowledged.
Almost there! Don'T forget to merge to ToT. https://codereview.chromium.org/1978343002/diff/20001/chrome/browser/memory/t... File chrome/browser/memory/tab_manager.cc (right): https://codereview.chromium.org/1978343002/diff/20001/chrome/browser/memory/t... chrome/browser/memory/tab_manager.cc:852: bool TabManager::AllowMultipleDiscards() { Rename this to ShouldOnlyDiscardOnce and flip logic. https://codereview.chromium.org/1978343002/diff/20001/chrome/browser/memory/t... File chrome/browser/memory/tab_manager.h (right): https://codereview.chromium.org/1978343002/diff/20001/chrome/browser/memory/t... chrome/browser/memory/tab_manager.h:246: // "Return true if tabs can be discarded more than once. nit: Remove " and return -> returns.
Solved those issues as well. Let me know if we need to change something else. https://codereview.chromium.org/1978343002/diff/20001/chrome/browser/memory/t... File chrome/browser/memory/tab_manager.cc (right): https://codereview.chromium.org/1978343002/diff/20001/chrome/browser/memory/t... chrome/browser/memory/tab_manager.cc:852: bool TabManager::AllowMultipleDiscards() { On 2016/05/17 13:49:07, Georges Khalil wrote: > Rename this to ShouldOnlyDiscardOnce and flip logic. Acknowledged. https://codereview.chromium.org/1978343002/diff/20001/chrome/browser/memory/t... File chrome/browser/memory/tab_manager.h (right): https://codereview.chromium.org/1978343002/diff/20001/chrome/browser/memory/t... chrome/browser/memory/tab_manager.h:246: // "Return true if tabs can be discarded more than once. On 2016/05/17 13:49:07, Georges Khalil wrote: > nit: Remove " and return -> returns. Acknowledged.
One last round. https://codereview.chromium.org/1978343002/diff/40001/chrome/browser/memory/t... File chrome/browser/memory/tab_manager.cc (right): https://codereview.chromium.org/1978343002/diff/40001/chrome/browser/memory/t... chrome/browser/memory/tab_manager.cc:855: // On Chrome OS, tab manager is always started and tabs can be discarded more nit: change this comment to On Chrome OS, tabs can always be discarded more than once. https://codereview.chromium.org/1978343002/diff/40001/chrome/browser/memory/t... chrome/browser/memory/tab_manager.cc:858: #endif You need an #elif otherwise you'll get a warning on Chrome OS (unreachable code). https://codereview.chromium.org/1978343002/diff/40001/chrome/browser/memory/t... chrome/browser/memory/tab_manager.cc:862: return (allow_multiple_discards == "false"); This should be: return (allow_multiple_discards != "true"); That's because the default behavior is to discard only once so if the variation doesn't exist, we return true. Add a comment to that effect: On all other platforms, default to discarding only once unless otherwise specified by the variation parameter. https://codereview.chromium.org/1978343002/diff/40001/chrome/browser/memory/t... File chrome/browser/memory/tab_manager.h (right): https://codereview.chromium.org/1978343002/diff/40001/chrome/browser/memory/t... chrome/browser/memory/tab_manager.h:247: bool ShouldOnlyDiscardOnce(); nit: Finally, I think CanOnlyDiscardOnce makes more sense as the name. It matches the description as well.
There it go. Do not hesitate if something still need to be changed. It's good we go deep in details in the first CLs so I'll be good quicker ;) https://codereview.chromium.org/1978343002/diff/40001/chrome/browser/memory/t... File chrome/browser/memory/tab_manager.cc (right): https://codereview.chromium.org/1978343002/diff/40001/chrome/browser/memory/t... chrome/browser/memory/tab_manager.cc:855: // On Chrome OS, tab manager is always started and tabs can be discarded more On 2016/05/17 15:18:01, Georges Khalil wrote: > nit: change this comment to > On Chrome OS, tabs can always be discarded more than once. Acknowledged. https://codereview.chromium.org/1978343002/diff/40001/chrome/browser/memory/t... chrome/browser/memory/tab_manager.cc:858: #endif On 2016/05/17 15:18:01, Georges Khalil wrote: > You need an #elif otherwise you'll get a warning on Chrome OS (unreachable > code). Acknowledged. https://codereview.chromium.org/1978343002/diff/40001/chrome/browser/memory/t... chrome/browser/memory/tab_manager.cc:862: return (allow_multiple_discards == "false"); On 2016/05/17 15:18:01, Georges Khalil wrote: > This should be: > return (allow_multiple_discards != "true"); > > That's because the default behavior is to discard only once so if the variation > doesn't exist, we return true. Add a comment to that effect: > On all other platforms, default to discarding only once unless otherwise > specified by the variation parameter. Acknowledged. https://codereview.chromium.org/1978343002/diff/40001/chrome/browser/memory/t... File chrome/browser/memory/tab_manager.h (right): https://codereview.chromium.org/1978343002/diff/40001/chrome/browser/memory/t... chrome/browser/memory/tab_manager.h:247: bool ShouldOnlyDiscardOnce(); On 2016/05/17 15:18:01, Georges Khalil wrote: > nit: Finally, I think CanOnlyDiscardOnce makes more sense as the name. It > matches the description as well. Acknowledged.
LGTM!
The CQ bit was checked by andersoncss@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1978343002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1978343002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
I think it's something like this. Let me know what you think.
https://codereview.chromium.org/1978343002/diff/80001/chrome/browser/memory/t... File chrome/browser/memory/tab_manager.cc (right): https://codereview.chromium.org/1978343002/diff/80001/chrome/browser/memory/t... chrome/browser/memory/tab_manager.cc:856: // specified by the variation parameter. Add a TODO for linux. https://codereview.chromium.org/1978343002/diff/80001/chrome/browser/memory/t... chrome/browser/memory/tab_manager.cc:861: #elif defined(OS_CHROMEOS) Remove the defined.
Getting there. Seems like this Linux thing is a relevant issue for the maintenance of the codebase. https://codereview.chromium.org/1978343002/diff/80001/chrome/browser/memory/t... File chrome/browser/memory/tab_manager.cc (right): https://codereview.chromium.org/1978343002/diff/80001/chrome/browser/memory/t... chrome/browser/memory/tab_manager.cc:856: // specified by the variation parameter. On 2016/05/17 18:24:29, Georges Khalil wrote: > Add a TODO for linux. Acknowledged. https://codereview.chromium.org/1978343002/diff/80001/chrome/browser/memory/t... chrome/browser/memory/tab_manager.cc:861: #elif defined(OS_CHROMEOS) On 2016/05/17 18:24:29, Georges Khalil wrote: > Remove the defined. Acknowledged.
LGTM % 1 nit. https://codereview.chromium.org/1978343002/diff/100001/chrome/browser/memory/... File chrome/browser/memory/tab_manager.cc (right): https://codereview.chromium.org/1978343002/diff/100001/chrome/browser/memory/... chrome/browser/memory/tab_manager.cc:857: // TODO(georgesak): add condition for linux when TabManager is supported. nit: change this comment to Add Linux when automatic discarding is enabled for that platform.
Done. Will submit to CQ again. https://codereview.chromium.org/1978343002/diff/100001/chrome/browser/memory/... File chrome/browser/memory/tab_manager.cc (right): https://codereview.chromium.org/1978343002/diff/100001/chrome/browser/memory/... chrome/browser/memory/tab_manager.cc:857: // TODO(georgesak): add condition for linux when TabManager is supported. On 2016/05/17 18:43:16, Georges Khalil wrote: > nit: change this comment to > Add Linux when automatic discarding is enabled for that platform. Acknowledged.
The CQ bit was checked by andersoncss@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from georgesak@chromium.org Link to the patchset: https://codereview.chromium.org/1978343002/#ps120001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1978343002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1978343002/120001
Message was sent while issue was closed.
Description was changed from ========== [TabManager] Start function refactoring. BUG=605533 ========== to ========== [TabManager] Start function refactoring. BUG=605533 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== [TabManager] Start function refactoring. BUG=605533 ========== to ========== [TabManager] Start function refactoring. BUG=605533 Committed: https://crrev.com/96fd742a6640c6186abf763f4ebda178051716f1 Cr-Commit-Position: refs/heads/master@{#394211} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/96fd742a6640c6186abf763f4ebda178051716f1 Cr-Commit-Position: refs/heads/master@{#394211} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
