Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2095413002:
TabManagerDelegate: Better prioritize ARC processes. (Closed)
|
Created:
4 years, 5 months ago by cylee1 Modified:
4 years, 4 months ago CC:
Aaron Boodman, abarth-chromium, ben+mojo_chromium.org, chromium-reviews, darin (slow to review), davemoore+watch_chromium.org, elijahtaylor+arcwatch_chromium.org, hidehiko+watch_chromium.org, lhchavez+watch_chromium.org, oshima+watch_chromium.org, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yusukes+watch_chromium.org, yzshen+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionTabManagerDelegate: Better prioritize ARC processes.
Add new fields "is_focused" and "last_active_time" in struct RunningAppProcessInfo of process.mojom so Chrome knows how to prioritize Android processes better.
BUG=625851
TEST=minnie
Committed: https://crrev.com/c2e58df8d48e216450dd83af783a3e5d97a6e329
Cr-Commit-Position: refs/heads/master@{#407444}
Patch Set 1 #
Total comments: 18
Patch Set 2 : TabManagerDelegate: Better prioritize ARC processes. #
Total comments: 14
Patch Set 3 : Add new field is_focused in process.mojom. Handle review comments. #Patch Set 4 : minor fixes #
Total comments: 45
Patch Set 5 : more review comment #
Total comments: 4
Patch Set 6 : more review commments.. #Patch Set 7 : more review commments.. #
Total comments: 4
Patch Set 8 : final review comment? #Patch Set 9 : revert to move assignment #Patch Set 10 : sync to ToT #Messages
Total messages: 44 (19 generated)
cylee@chromium.org changed reviewers: + georgesak@chromium.org, rickyz@chromium.org, yusukes@chromium.org
lhchavez@chromium.org changed reviewers: + lhchavez@chromium.org
drive-by review of the .mojom change. https://codereview.chromium.org/2095413002/diff/1/components/arc/common/proce... File components/arc/common/process.mojom (right): https://codereview.chromium.org/2095413002/diff/1/components/arc/common/proce... components/arc/common/process.mojom:77: // This struct is a subset of android.app.ActivityManager.RunningAppProcessInfo. This is not true anymore. https://codereview.chromium.org/2095413002/diff/1/components/arc/common/proce... components/arc/common/process.mojom:96: // Last time the process is active. Milliseconds since boot. nit: "process was active". Also, please mention if this is wall or monotonic time.
https://codereview.chromium.org/2095413002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_process.h (right): https://codereview.chromium.org/2095413002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_process.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. Please file a placeholder bug (http://crbug.com/new) and use the bug for the BUG= line. https://codereview.chromium.org/2095413002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_process.h:10: #include <stdint.h> ? https://codereview.chromium.org/2095413002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_process.h:23: int adj, Can you use a more descriptive name for this? https://google.github.io/styleguide/cppguide.html#General_Naming_Rules says "eschew abbreviation." https://codereview.chromium.org/2095413002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_process.h:44: int adj_; Please document what this is. https://codereview.chromium.org/2095413002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_process.h:45: int64_t last_activity_time_; same. (what is the epoch of the time btw?) https://codereview.chromium.org/2095413002/diff/1/components/arc/common/proce... File components/arc/common/process.mojom (right): https://codereview.chromium.org/2095413002/diff/1/components/arc/common/proce... components/arc/common/process.mojom:86: // TODO(cylee): Deprecate it if nobody uses it. I think https://codereview.chromium.org/2059483003 uses the field/enum. https://codereview.chromium.org/2095413002/diff/1/components/arc/common/proce... components/arc/common/process.mojom:94: [MinVersion=5] int32 adj; I understand AMS calls this "adj" but in Chromium, can you use a more descriptive name? Most Chromium developers are not familiar with Android internals. https://google.github.io/styleguide/cppguide.html#General_Naming_Rules
LGTM for chrome/browser/memory % nits. Code looks cleaner, nice! https://codereview.chromium.org/2095413002/diff/20001/chrome/browser/memory/t... File chrome/browser/memory/tab_manager_delegate_chromeos.cc (right): https://codereview.chromium.org/2095413002/diff/20001/chrome/browser/memory/t... chrome/browser/memory/tab_manager_delegate_chromeos.cc:565: // Persistent an system android processes has negative adj value. nits: - an -> and - has -> have https://codereview.chromium.org/2095413002/diff/20001/chrome/browser/memory/t... File chrome/browser/memory/tab_manager_delegate_chromeos.h (right): https://codereview.chromium.org/2095413002/diff/20001/chrome/browser/memory/t... chrome/browser/memory/tab_manager_delegate_chromeos.h:217: : tab_(tab), app_(NULL), process_type_(GetProcessType()) { nit: NULL -> nullptr. https://codereview.chromium.org/2095413002/diff/20001/chrome/browser/memory/t... chrome/browser/memory/tab_manager_delegate_chromeos.h:220: : tab_(NULL), app_(app), process_type_(GetProcessType()) { nit: same as above. https://codereview.chromium.org/2095413002/diff/20001/chrome/browser/memory/t... chrome/browser/memory/tab_manager_delegate_chromeos.h:249: } else if (app()->adj() == ArcProcessAdjLevel::VISIBLE_APP_ADJ) { nit: No need for else here. Also allows the removal of curly braces. https://codereview.chromium.org/2095413002/diff/20001/chrome/browser/memory/t... chrome/browser/memory/tab_manager_delegate_chromeos.h:255: return ProcessType::FOCUSED_TAB; nit: no braces.
mojom lgtm
https://codereview.chromium.org/2095413002/diff/20001/chrome/browser/memory/t... File chrome/browser/memory/tab_manager_delegate_chromeos.h (right): https://codereview.chromium.org/2095413002/diff/20001/chrome/browser/memory/t... chrome/browser/memory/tab_manager_delegate_chromeos.h:228: if (app()->adj() != rhs.app()->adj()) You can replace all this by: // Larger adj value means lower priority as defined in Android. return std::tie(-app()->adj(), app()->last_activity_time()) > std::tie(-rhs.app()->adj(), rhs.app()->last_activity_time()); https://codereview.chromium.org/2095413002/diff/20001/components/arc/common/p... File components/arc/common/process.mojom (right): https://codereview.chromium.org/2095413002/diff/20001/components/arc/common/p... components/arc/common/process.mojom:77: // This struct is a subset of android.app.ActivityManager.RunningAppProcessInfo. You should probably remove this comment.
I want to on hold the CL for a moment because of Dianne Hackborn's comment at https://googleplex-android-review.git.corp.google.com/#/c/1179369/1 She's against passing raw adj valuel to chrome. I'm discussing with her. https://codereview.chromium.org/2095413002/diff/1/components/arc/common/proce... File components/arc/common/process.mojom (right): https://codereview.chromium.org/2095413002/diff/1/components/arc/common/proce... components/arc/common/process.mojom:77: // This struct is a subset of android.app.ActivityManager.RunningAppProcessInfo. On 2016/06/27 16:18:07, Luis Héctor Chávez wrote: > This is not true anymore. It's still true since I added the 2 attributes to RunningAppProcessInfo in another CL (sent for review). https://codereview.chromium.org/2095413002/diff/1/components/arc/common/proce... components/arc/common/process.mojom:96: // Last time the process is active. Milliseconds since boot. On 2016/06/27 16:18:07, Luis Héctor Chávez wrote: > nit: "process was active". Also, please mention if this is wall or monotonic > time. Done.
Description was changed from ========== TabManagerDelegate: Better prioritize ARC processes. Use Android adj level and lastActiveTime (instead of process state) to prioritize ARC processes. BUG=b:29576205 TEST=minnie ========== to ========== TabManagerDelegate: Better prioritize ARC processes. Use Android adj level and lastActiveTime (instead of process state) to prioritize ARC processes. BUG=625851 TEST=minnie ==========
Description was changed from ========== TabManagerDelegate: Better prioritize ARC processes. Use Android adj level and lastActiveTime (instead of process state) to prioritize ARC processes. BUG=625851 TEST=minnie ========== to ========== TabManagerDelegate: Better prioritize ARC processes. Add new fields "is_focused" and "last_active_time" in struct RunningAppProcessInfo of process.mojom so Chrome knows how to prioritize Android processes better. BUG=625851 TEST=minnie ==========
Hi reviewers, Android side CLs are done (submitting). In summary there're 2 fields "is_focused" and "last_activity_time" added in the IPC interface process.mojom. I think we can continue the review process here. Thanks. https://codereview.chromium.org/2095413002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_process.h (right): https://codereview.chromium.org/2095413002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_process.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2016/06/27 16:40:51, Yusuke Sato wrote: > Please file a placeholder bug (http://crbug.com/new) and use the bug for the > BUG= line. Done. https://codereview.chromium.org/2095413002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_process.h:10: On 2016/06/27 16:40:50, Yusuke Sato wrote: > #include <stdint.h> ? Done. https://codereview.chromium.org/2095413002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_process.h:23: int adj, On 2016/06/27 16:40:50, Yusuke Sato wrote: > Can you use a more descriptive name for this? > > https://google.github.io/styleguide/cppguide.html#General_Naming_Rules says > "eschew abbreviation." The parameter is replaced by is_focused. https://codereview.chromium.org/2095413002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_process.h:44: int adj_; On 2016/06/27 16:40:50, Yusuke Sato wrote: > Please document what this is. Replaced. https://codereview.chromium.org/2095413002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_process.h:45: int64_t last_activity_time_; On 2016/06/27 16:40:51, Yusuke Sato wrote: > same. (what is the epoch of the time btw?) Done. https://codereview.chromium.org/2095413002/diff/1/components/arc/common/proce... File components/arc/common/process.mojom (right): https://codereview.chromium.org/2095413002/diff/1/components/arc/common/proce... components/arc/common/process.mojom:86: // TODO(cylee): Deprecate it if nobody uses it. On 2016/06/27 16:40:51, Yusuke Sato wrote: > I think https://codereview.chromium.org/2059483003 uses the field/enum. I see. In my first implementation I should change the code there as well. But in the new implementation process_state is remained. https://codereview.chromium.org/2095413002/diff/1/components/arc/common/proce... components/arc/common/process.mojom:94: [MinVersion=5] int32 adj; On 2016/06/27 16:40:51, Yusuke Sato wrote: > I understand AMS calls this "adj" but in Chromium, can you use a more > descriptive name? Most Chromium developers are not familiar with Android > internals. > > https://google.github.io/styleguide/cppguide.html#General_Naming_Rules > Changed. https://codereview.chromium.org/2095413002/diff/20001/chrome/browser/memory/t... File chrome/browser/memory/tab_manager_delegate_chromeos.cc (right): https://codereview.chromium.org/2095413002/diff/20001/chrome/browser/memory/t... chrome/browser/memory/tab_manager_delegate_chromeos.cc:565: // Persistent an system android processes has negative adj value. On 2016/06/27 20:15:04, Georges Khalil wrote: > nits: > - an -> and > - has -> have Done. https://codereview.chromium.org/2095413002/diff/20001/chrome/browser/memory/t... File chrome/browser/memory/tab_manager_delegate_chromeos.h (right): https://codereview.chromium.org/2095413002/diff/20001/chrome/browser/memory/t... chrome/browser/memory/tab_manager_delegate_chromeos.h:217: : tab_(tab), app_(NULL), process_type_(GetProcessType()) { On 2016/06/27 20:15:05, Georges Khalil wrote: > nit: NULL -> nullptr. Done. https://codereview.chromium.org/2095413002/diff/20001/chrome/browser/memory/t... chrome/browser/memory/tab_manager_delegate_chromeos.h:220: : tab_(NULL), app_(app), process_type_(GetProcessType()) { On 2016/06/27 20:15:05, Georges Khalil wrote: > nit: same as above. Done. https://codereview.chromium.org/2095413002/diff/20001/chrome/browser/memory/t... chrome/browser/memory/tab_manager_delegate_chromeos.h:228: if (app()->adj() != rhs.app()->adj()) On 2016/06/28 15:43:53, Luis Héctor Chávez wrote: > You can replace all this by: > > // Larger adj value means lower priority as defined in Android. > return std::tie(-app()->adj(), app()->last_activity_time()) > > std::tie(-rhs.app()->adj(), rhs.app()->last_activity_time()); tie() requires non-const reference since it expects lvalue (so can be used as unpack). make_pair() seems to work as intended. http://stackoverflow.com/questions/5367707/invalid-initialization-error https://codereview.chromium.org/2095413002/diff/20001/chrome/browser/memory/t... chrome/browser/memory/tab_manager_delegate_chromeos.h:249: } else if (app()->adj() == ArcProcessAdjLevel::VISIBLE_APP_ADJ) { On 2016/06/27 20:15:05, Georges Khalil wrote: > nit: No need for else here. Also allows the removal of curly braces. Done. https://codereview.chromium.org/2095413002/diff/20001/chrome/browser/memory/t... chrome/browser/memory/tab_manager_delegate_chromeos.h:255: return ProcessType::FOCUSED_TAB; On 2016/06/27 20:15:05, Georges Khalil wrote: > nit: no braces. Done. https://codereview.chromium.org/2095413002/diff/20001/components/arc/common/p... File components/arc/common/process.mojom (right): https://codereview.chromium.org/2095413002/diff/20001/components/arc/common/p... components/arc/common/process.mojom:77: // This struct is a subset of android.app.ActivityManager.RunningAppProcessInfo. On 2016/06/28 15:43:53, Luis Héctor Chávez wrote: > You should probably remove this comment. Replied in the previous patch set. It's still a subset of that structure. Should I remove it ?
The CQ bit was checked by cylee@chromium.org 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...
On 2016/07/14 19:15:25, cylee1 wrote: > Hi reviewers, > Android side CLs are done (submitting). > In summary there're 2 fields "is_focused" and "last_activity_time" added in > the IPC interface process.mojom. I think we can continue the review process > here. Android side CLs should always be submitted after Chrome-side changes. > > Thanks. > > https://codereview.chromium.org/2095413002/diff/1/chrome/browser/chromeos/arc... > File chrome/browser/chromeos/arc/arc_process.h (right): > > https://codereview.chromium.org/2095413002/diff/1/chrome/browser/chromeos/arc... > chrome/browser/chromeos/arc/arc_process.h:1: // Copyright 2016 The Chromium > Authors. All rights reserved. > On 2016/06/27 16:40:51, Yusuke Sato wrote: > > Please file a placeholder bug (http://crbug.com/new) and use the bug for the > > BUG= line. > > Done. > > https://codereview.chromium.org/2095413002/diff/1/chrome/browser/chromeos/arc... > chrome/browser/chromeos/arc/arc_process.h:10: > On 2016/06/27 16:40:50, Yusuke Sato wrote: > > #include <stdint.h> ? > > Done. > > https://codereview.chromium.org/2095413002/diff/1/chrome/browser/chromeos/arc... > chrome/browser/chromeos/arc/arc_process.h:23: int adj, > On 2016/06/27 16:40:50, Yusuke Sato wrote: > > Can you use a more descriptive name for this? > > > > https://google.github.io/styleguide/cppguide.html#General_Naming_Rules says > > "eschew abbreviation." > > The parameter is replaced by is_focused. > > https://codereview.chromium.org/2095413002/diff/1/chrome/browser/chromeos/arc... > chrome/browser/chromeos/arc/arc_process.h:44: int adj_; > On 2016/06/27 16:40:50, Yusuke Sato wrote: > > Please document what this is. > > Replaced. > > https://codereview.chromium.org/2095413002/diff/1/chrome/browser/chromeos/arc... > chrome/browser/chromeos/arc/arc_process.h:45: int64_t last_activity_time_; > On 2016/06/27 16:40:51, Yusuke Sato wrote: > > same. (what is the epoch of the time btw?) > > Done. > > https://codereview.chromium.org/2095413002/diff/1/components/arc/common/proce... > File components/arc/common/process.mojom (right): > > https://codereview.chromium.org/2095413002/diff/1/components/arc/common/proce... > components/arc/common/process.mojom:86: // TODO(cylee): Deprecate it if nobody > uses it. > On 2016/06/27 16:40:51, Yusuke Sato wrote: > > I think https://codereview.chromium.org/2059483003 uses the field/enum. > > I see. In my first implementation I should change the code there as well. > But in the new implementation process_state is remained. > > https://codereview.chromium.org/2095413002/diff/1/components/arc/common/proce... > components/arc/common/process.mojom:94: [MinVersion=5] int32 adj; > On 2016/06/27 16:40:51, Yusuke Sato wrote: > > I understand AMS calls this "adj" but in Chromium, can you use a more > > descriptive name? Most Chromium developers are not familiar with Android > > internals. > > > > https://google.github.io/styleguide/cppguide.html#General_Naming_Rules > > > > Changed. > > https://codereview.chromium.org/2095413002/diff/20001/chrome/browser/memory/t... > File chrome/browser/memory/tab_manager_delegate_chromeos.cc (right): > > https://codereview.chromium.org/2095413002/diff/20001/chrome/browser/memory/t... > chrome/browser/memory/tab_manager_delegate_chromeos.cc:565: // Persistent an > system android processes has negative adj value. > On 2016/06/27 20:15:04, Georges Khalil wrote: > > nits: > > - an -> and > > - has -> have > > Done. > > https://codereview.chromium.org/2095413002/diff/20001/chrome/browser/memory/t... > File chrome/browser/memory/tab_manager_delegate_chromeos.h (right): > > https://codereview.chromium.org/2095413002/diff/20001/chrome/browser/memory/t... > chrome/browser/memory/tab_manager_delegate_chromeos.h:217: : tab_(tab), > app_(NULL), process_type_(GetProcessType()) { > On 2016/06/27 20:15:05, Georges Khalil wrote: > > nit: NULL -> nullptr. > > Done. > > https://codereview.chromium.org/2095413002/diff/20001/chrome/browser/memory/t... > chrome/browser/memory/tab_manager_delegate_chromeos.h:220: : tab_(NULL), > app_(app), process_type_(GetProcessType()) { > On 2016/06/27 20:15:05, Georges Khalil wrote: > > nit: same as above. > > Done. > > https://codereview.chromium.org/2095413002/diff/20001/chrome/browser/memory/t... > chrome/browser/memory/tab_manager_delegate_chromeos.h:228: if (app()->adj() != > rhs.app()->adj()) > On 2016/06/28 15:43:53, Luis Héctor Chávez wrote: > > You can replace all this by: > > > > // Larger adj value means lower priority as defined in Android. > > return std::tie(-app()->adj(), app()->last_activity_time()) > > > std::tie(-rhs.app()->adj(), rhs.app()->last_activity_time()); > > tie() requires non-const reference since it expects lvalue (so can be used as > unpack). make_pair() seems to work as intended. > http://stackoverflow.com/questions/5367707/invalid-initialization-error > > https://codereview.chromium.org/2095413002/diff/20001/chrome/browser/memory/t... > chrome/browser/memory/tab_manager_delegate_chromeos.h:249: } else if > (app()->adj() == ArcProcessAdjLevel::VISIBLE_APP_ADJ) { > On 2016/06/27 20:15:05, Georges Khalil wrote: > > nit: No need for else here. Also allows the removal of curly braces. > > Done. > > https://codereview.chromium.org/2095413002/diff/20001/chrome/browser/memory/t... > chrome/browser/memory/tab_manager_delegate_chromeos.h:255: return > ProcessType::FOCUSED_TAB; > On 2016/06/27 20:15:05, Georges Khalil wrote: > > nit: no braces. > > Done. > > https://codereview.chromium.org/2095413002/diff/20001/components/arc/common/p... > File components/arc/common/process.mojom (right): > > https://codereview.chromium.org/2095413002/diff/20001/components/arc/common/p... > components/arc/common/process.mojom:77: // This struct is a subset of > android.app.ActivityManager.RunningAppProcessInfo. > On 2016/06/28 15:43:53, Luis Héctor Chávez wrote: > > You should probably remove this comment. > > Replied in the previous patch set. It's still a subset of that structure. Should > I remove it ?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
mostly style comments: https://codereview.chromium.org/2095413002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_process_service.cc (right): https://codereview.chromium.org/2095413002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_process_service.cc:175: entry->process_state, entry->is_focused, Have you run 'git cl format'? Please do if you haven't. https://codereview.chromium.org/2095413002/diff/60001/chrome/browser/memory/t... File chrome/browser/memory/tab_manager.h (right): https://codereview.chromium.org/2095413002/diff/60001/chrome/browser/memory/t... chrome/browser/memory/tab_manager.h:133: static bool CompareTabStats(TabStats first, TabStats second); I know this is not your code, but isn't it better to use const TabStats& ? The struct is large. https://codereview.chromium.org/2095413002/diff/60001/chrome/browser/memory/t... File chrome/browser/memory/tab_manager_delegate_chromeos.cc (right): https://codereview.chromium.org/2095413002/diff/60001/chrome/browser/memory/t... chrome/browser/memory/tab_manager_delegate_chromeos.cc:557: static const char kAppLauncherProcessName[] = "org.chromium.arc.applauncher"; constexpr https://codereview.chromium.org/2095413002/diff/60001/chrome/browser/memory/t... chrome/browser/memory/tab_manager_delegate_chromeos.cc:612: for (auto it = candidates.rbegin(); it != candidates.rend(); ++it) { const auto& it probably? https://codereview.chromium.org/2095413002/diff/60001/chrome/browser/memory/t... chrome/browser/memory/tab_manager_delegate_chromeos.cc:671: for (auto it = candidates.begin(); it != candidates.end(); ++it) { same https://codereview.chromium.org/2095413002/diff/60001/chrome/browser/memory/t... File chrome/browser/memory/tab_manager_delegate_chromeos.h (right): https://codereview.chromium.org/2095413002/diff/60001/chrome/browser/memory/t... chrome/browser/memory/tab_manager_delegate_chromeos.h:34: enum ProcessType { enum class ? https://codereview.chromium.org/2095413002/diff/60001/chrome/browser/memory/t... chrome/browser/memory/tab_manager_delegate_chromeos.h:220: } DCHECK(tab_)? https://codereview.chromium.org/2095413002/diff/60001/chrome/browser/memory/t... chrome/browser/memory/tab_manager_delegate_chromeos.h:223: } DCHECK(app_)? https://codereview.chromium.org/2095413002/diff/60001/chrome/browser/memory/t... chrome/browser/memory/tab_manager_delegate_chromeos.h:226: bool operator<(const Candidate& rhs) const { Can you move the implementation to .cc? The implementation is not longer one-liner. https://codereview.chromium.org/2095413002/diff/60001/chrome/browser/memory/t... chrome/browser/memory/tab_manager_delegate_chromeos.h:234: return std::make_pair(app()->process_state(), What about adding CompareAppStats method to the app class (just like line 239?) https://codereview.chromium.org/2095413002/diff/60001/chrome/browser/memory/t... chrome/browser/memory/tab_manager_delegate_chromeos.h:252: ProcessType GetProcessType() { * This can be const. * Or better, can you make this 'static ProcessType GetProcessType(const TabStats* tab, const arc::ArcProcess* app);'? Having process_type() and GetProcessType() with the same signature is very confusing to me. https://codereview.chromium.org/2095413002/diff/60001/chrome/browser/memory/t... chrome/browser/memory/tab_manager_delegate_chromeos.h:270: ProcessType process_type_; can be const too. https://codereview.chromium.org/2095413002/diff/60001/chrome/browser/memory/t... chrome/browser/memory/tab_manager_delegate_chromeos.h:271: }; DISALLOW_COPY_AND_ASSIGN is missing. https://codereview.chromium.org/2095413002/diff/60001/chrome/browser/memory/t... File chrome/browser/memory/tab_manager_delegate_chromeos_unittest.cc (right): https://codereview.chromium.org/2095413002/diff/60001/chrome/browser/memory/t... chrome/browser/memory/tab_manager_delegate_chromeos_unittest.cc:31: typedef testing::Test TabManagerDelegateTest; using https://codereview.chromium.org/2095413002/diff/60001/chrome/browser/memory/t... chrome/browser/memory/tab_manager_delegate_chromeos_unittest.cc:34: bool IS_FOCUSED = true; constexpr bool kIsFocused = true; https://codereview.chromium.org/2095413002/diff/60001/chrome/browser/memory/t... chrome/browser/memory/tab_manager_delegate_chromeos_unittest.cc:35: bool NOT_FOCUSED = false; same
more nits https://codereview.chromium.org/2095413002/diff/60001/chrome/browser/memory/t... File chrome/browser/memory/tab_manager_delegate_chromeos.h (right): https://codereview.chromium.org/2095413002/diff/60001/chrome/browser/memory/t... chrome/browser/memory/tab_manager_delegate_chromeos.h:129: // Get the list of candidates to kill, sorted by importance descendingly. nit: descending importance
https://codereview.chromium.org/2095413002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_process_service.cc (right): https://codereview.chromium.org/2095413002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_process_service.cc:175: entry->process_state, entry->is_focused, On 2016/07/15 00:55:05, Yusuke Sato wrote: > Have you run 'git cl format'? Please do if you haven't. Done. https://codereview.chromium.org/2095413002/diff/60001/chrome/browser/memory/t... File chrome/browser/memory/tab_manager.h (right): https://codereview.chromium.org/2095413002/diff/60001/chrome/browser/memory/t... chrome/browser/memory/tab_manager.h:133: static bool CompareTabStats(TabStats first, TabStats second); On 2016/07/15 00:55:05, Yusuke Sato wrote: > I know this is not your code, but isn't it better to use > > const TabStats& > > ? The struct is large. Done. https://codereview.chromium.org/2095413002/diff/60001/chrome/browser/memory/t... File chrome/browser/memory/tab_manager_delegate_chromeos.cc (right): https://codereview.chromium.org/2095413002/diff/60001/chrome/browser/memory/t... chrome/browser/memory/tab_manager_delegate_chromeos.cc:557: static const char kAppLauncherProcessName[] = "org.chromium.arc.applauncher"; On 2016/07/15 00:55:05, Yusuke Sato wrote: > constexpr Done. https://codereview.chromium.org/2095413002/diff/60001/chrome/browser/memory/t... chrome/browser/memory/tab_manager_delegate_chromeos.cc:612: for (auto it = candidates.rbegin(); it != candidates.rend(); ++it) { On 2016/07/15 00:55:05, Yusuke Sato wrote: > const auto& it > > probably? No it will make "it" const so I cant ++it candidates is of type const vector<> so is should returns const_iterator() by default. I think it means auto derives to const_iterator already. http://www.cplusplus.com/reference/vector/vector/begin/ "If the vector object is const-qualified, the function returns a const_iterator. Otherwise, it returns an iterator." https://codereview.chromium.org/2095413002/diff/60001/chrome/browser/memory/t... chrome/browser/memory/tab_manager_delegate_chromeos.cc:671: for (auto it = candidates.begin(); it != candidates.end(); ++it) { On 2016/07/15 00:55:05, Yusuke Sato wrote: > same same. https://codereview.chromium.org/2095413002/diff/60001/chrome/browser/memory/t... File chrome/browser/memory/tab_manager_delegate_chromeos.h (right): https://codereview.chromium.org/2095413002/diff/60001/chrome/browser/memory/t... chrome/browser/memory/tab_manager_delegate_chromeos.h:34: enum ProcessType { On 2016/07/15 00:55:05, Yusuke Sato wrote: > enum class > > ? Yes I can make it so, at the expense of overloading operator<< because I cout << process_type somewhere. Or I'll encounter the error http://stackoverflow.com/questions/11421432/how-can-i-output-the-value-of-an-... which I need to use underlying_type to resolve otherwise. https://codereview.chromium.org/2095413002/diff/60001/chrome/browser/memory/t... chrome/browser/memory/tab_manager_delegate_chromeos.h:129: // Get the list of candidates to kill, sorted by importance descendingly. On 2016/07/15 01:29:24, Luis Héctor Chávez wrote: > nit: descending importance Done. https://codereview.chromium.org/2095413002/diff/60001/chrome/browser/memory/t... chrome/browser/memory/tab_manager_delegate_chromeos.h:220: } On 2016/07/15 00:55:05, Yusuke Sato wrote: > DCHECK(tab_)? Done. https://codereview.chromium.org/2095413002/diff/60001/chrome/browser/memory/t... chrome/browser/memory/tab_manager_delegate_chromeos.h:223: } On 2016/07/15 00:55:05, Yusuke Sato wrote: > DCHECK(app_)? Done. https://codereview.chromium.org/2095413002/diff/60001/chrome/browser/memory/t... chrome/browser/memory/tab_manager_delegate_chromeos.h:226: bool operator<(const Candidate& rhs) const { On 2016/07/15 00:55:05, Yusuke Sato wrote: > Can you move the implementation to .cc? The implementation is not longer > one-liner. Done. https://codereview.chromium.org/2095413002/diff/60001/chrome/browser/memory/t... chrome/browser/memory/tab_manager_delegate_chromeos.h:234: return std::make_pair(app()->process_state(), On 2016/07/15 00:55:05, Yusuke Sato wrote: > What about adding CompareAppStats method to the app class (just like line 239?) Moved to arc_process.cc . https://codereview.chromium.org/2095413002/diff/60001/chrome/browser/memory/t... chrome/browser/memory/tab_manager_delegate_chromeos.h:252: ProcessType GetProcessType() { On 2016/07/15 00:55:05, Yusuke Sato wrote: > * This can be const. > * Or better, can you make this 'static ProcessType GetProcessType(const > TabStats* tab, const arc::ArcProcess* app);'? Having process_type() and > GetProcessType() with the same signature is very confusing to me. Can I just use a const ? Passing all its member so another static function doesn't look scalable to me. (Imagine if process type depends on other members in the future) https://codereview.chromium.org/2095413002/diff/60001/chrome/browser/memory/t... chrome/browser/memory/tab_manager_delegate_chromeos.h:270: ProcessType process_type_; On 2016/07/15 00:55:05, Yusuke Sato wrote: > can be const too. Because this class must allow copy and assign (used in STL vector, see below), it should not be defined const. http://stackoverflow.com/questions/15030972/type-non-static-const-member-cant... https://codereview.chromium.org/2095413002/diff/60001/chrome/browser/memory/t... chrome/browser/memory/tab_manager_delegate_chromeos.h:271: }; On 2016/07/15 00:55:05, Yusuke Sato wrote: > DISALLOW_COPY_AND_ASSIGN is missing. This is used in a STL vector so it needs to allow copy and assign https://codereview.chromium.org/2095413002/diff/60001/chrome/browser/memory/t... File chrome/browser/memory/tab_manager_delegate_chromeos_unittest.cc (right): https://codereview.chromium.org/2095413002/diff/60001/chrome/browser/memory/t... chrome/browser/memory/tab_manager_delegate_chromeos_unittest.cc:31: typedef testing::Test TabManagerDelegateTest; On 2016/07/15 00:55:06, Yusuke Sato wrote: > using Done. https://codereview.chromium.org/2095413002/diff/60001/chrome/browser/memory/t... chrome/browser/memory/tab_manager_delegate_chromeos_unittest.cc:34: bool IS_FOCUSED = true; On 2016/07/15 00:55:05, Yusuke Sato wrote: > constexpr bool kIsFocused = true; Done. https://codereview.chromium.org/2095413002/diff/60001/chrome/browser/memory/t... chrome/browser/memory/tab_manager_delegate_chromeos_unittest.cc:35: bool NOT_FOCUSED = false; On 2016/07/15 00:55:06, Yusuke Sato wrote: > same Done.
https://codereview.chromium.org/2095413002/diff/60001/chrome/browser/memory/t... File chrome/browser/memory/tab_manager_delegate_chromeos.h (right): https://codereview.chromium.org/2095413002/diff/60001/chrome/browser/memory/t... chrome/browser/memory/tab_manager_delegate_chromeos.h:34: enum ProcessType { On 2016/07/15 17:31:13, cylee1 wrote: > On 2016/07/15 00:55:05, Yusuke Sato wrote: > > enum class > > > > ? > > Yes I can make it so, at the expense of overloading operator<< because I cout << > process_type somewhere. > Or I'll encounter the error > http://stackoverflow.com/questions/11421432/how-can-i-output-the-value-of-an-... > which I need to use underlying_type to resolve otherwise. I think it's fine to overload operator<<. You are already doing it for Candidate and it will improve readability of logs. https://codereview.chromium.org/2095413002/diff/60001/chrome/browser/memory/t... chrome/browser/memory/tab_manager_delegate_chromeos.h:252: ProcessType GetProcessType() { On 2016/07/15 17:31:13, cylee1 wrote: > On 2016/07/15 00:55:05, Yusuke Sato wrote: > > * This can be const. > > * Or better, can you make this 'static ProcessType GetProcessType(const > > TabStats* tab, const arc::ArcProcess* app);'? Having process_type() and > > GetProcessType() with the same signature is very confusing to me. > > Can I just use a const ? Passing all its member so another static function > doesn't look scalable to me. > (Imagine if process type depends on other members in the future) Can you at the very least call this GetProcessTypeInternal()? I'll defer the final decision on this to yusukes@. https://codereview.chromium.org/2095413002/diff/60001/chrome/browser/memory/t... chrome/browser/memory/tab_manager_delegate_chromeos.h:271: }; On 2016/07/15 17:31:13, cylee1 wrote: > On 2016/07/15 00:55:05, Yusuke Sato wrote: > > DISALLOW_COPY_AND_ASSIGN is missing. > > This is used in a STL vector so it needs to allow copy and assign non-copyable classes can be used in STL vectors. The only caveat is that you are using std::sort(), which needs an explicit move-assignment constructor. If you add Candidate(Candidate&&) = default; Candidate& operator=(Candidate&& other) { // This should go in the .cc. tab_ = std::move(other.tab_); app_ = std::move(other.app_); process_type_ = std::move(other.process_type_); return *this; } you should be fine. Due to the |process_type_| assignment, it indeed cannot be const. https://codereview.chromium.org/2095413002/diff/80001/chrome/browser/memory/t... File chrome/browser/memory/tab_manager_delegate_chromeos.cc (right): https://codereview.chromium.org/2095413002/diff/80001/chrome/browser/memory/t... chrome/browser/memory/tab_manager_delegate_chromeos.cc:89: out = "FOCUSED_APP/FOCUSED_TAB"; nit: return os << "FOCUSED_APP/FOCUSED_TAB"; that avoids unnecessary copies. https://codereview.chromium.org/2095413002/diff/80001/chrome/browser/memory/t... chrome/browser/memory/tab_manager_delegate_chromeos.cc:665: // The list is sorted by importance descendingly, so we go through the list nit: descending importance
https://codereview.chromium.org/2095413002/diff/60001/chrome/browser/memory/t... File chrome/browser/memory/tab_manager_delegate_chromeos.h (right): https://codereview.chromium.org/2095413002/diff/60001/chrome/browser/memory/t... chrome/browser/memory/tab_manager_delegate_chromeos.h:252: ProcessType GetProcessType() { On 2016/07/15 18:10:06, Luis Héctor Chávez wrote: > On 2016/07/15 17:31:13, cylee1 wrote: > > On 2016/07/15 00:55:05, Yusuke Sato wrote: > > > * This can be const. > > > * Or better, can you make this 'static ProcessType GetProcessType(const > > > TabStats* tab, const arc::ArcProcess* app);'? Having process_type() and > > > GetProcessType() with the same signature is very confusing to me. > > > > Can I just use a const ? Passing all its member so another static function > > doesn't look scalable to me. > > (Imagine if process type depends on other members in the future) > > Can you at the very least call this GetProcessTypeInternal()? I'll defer the > final decision on this to yusukes@. Yeah, please rename it then.
https://codereview.chromium.org/2095413002/diff/60001/chrome/browser/memory/t... File chrome/browser/memory/tab_manager_delegate_chromeos.h (right): https://codereview.chromium.org/2095413002/diff/60001/chrome/browser/memory/t... chrome/browser/memory/tab_manager_delegate_chromeos.h:34: enum ProcessType { On 2016/07/15 18:10:06, Luis Héctor Chávez wrote: > On 2016/07/15 17:31:13, cylee1 wrote: > > On 2016/07/15 00:55:05, Yusuke Sato wrote: > > > enum class > > > > > > ? > > > > Yes I can make it so, at the expense of overloading operator<< because I cout > << > > process_type somewhere. > > Or I'll encounter the error > > > http://stackoverflow.com/questions/11421432/how-can-i-output-the-value-of-an-... > > which I need to use underlying_type to resolve otherwise. > > I think it's fine to overload operator<<. You are already doing it for Candidate > and it will improve readability of logs. ok agree. https://codereview.chromium.org/2095413002/diff/60001/chrome/browser/memory/t... chrome/browser/memory/tab_manager_delegate_chromeos.h:252: ProcessType GetProcessType() { On 2016/07/19 01:31:36, Yusuke Sato (ooo Jul 21-31) wrote: > On 2016/07/15 18:10:06, Luis Héctor Chávez wrote: > > On 2016/07/15 17:31:13, cylee1 wrote: > > > On 2016/07/15 00:55:05, Yusuke Sato wrote: > > > > * This can be const. > > > > * Or better, can you make this 'static ProcessType GetProcessType(const > > > > TabStats* tab, const arc::ArcProcess* app);'? Having process_type() and > > > > GetProcessType() with the same signature is very confusing to me. > > > > > > Can I just use a const ? Passing all its member so another static function > > > doesn't look scalable to me. > > > (Imagine if process type depends on other members in the future) > > > > Can you at the very least call this GetProcessTypeInternal()? I'll defer the > > final decision on this to yusukes@. > > Yeah, please rename it then. Done. https://codereview.chromium.org/2095413002/diff/60001/chrome/browser/memory/t... chrome/browser/memory/tab_manager_delegate_chromeos.h:252: ProcessType GetProcessType() { On 2016/07/19 01:31:36, Yusuke Sato (ooo Jul 21-31) wrote: > On 2016/07/15 18:10:06, Luis Héctor Chávez wrote: > > On 2016/07/15 17:31:13, cylee1 wrote: > > > On 2016/07/15 00:55:05, Yusuke Sato wrote: > > > > * This can be const. > > > > * Or better, can you make this 'static ProcessType GetProcessType(const > > > > TabStats* tab, const arc::ArcProcess* app);'? Having process_type() and > > > > GetProcessType() with the same signature is very confusing to me. > > > > > > Can I just use a const ? Passing all its member so another static function > > > doesn't look scalable to me. > > > (Imagine if process type depends on other members in the future) > > > > Can you at the very least call this GetProcessTypeInternal()? I'll defer the > > final decision on this to yusukes@. > > Yeah, please rename it then. Done. https://codereview.chromium.org/2095413002/diff/60001/chrome/browser/memory/t... chrome/browser/memory/tab_manager_delegate_chromeos.h:271: }; On 2016/07/15 18:10:06, Luis Héctor Chávez wrote: > On 2016/07/15 17:31:13, cylee1 wrote: > > On 2016/07/15 00:55:05, Yusuke Sato wrote: > > > DISALLOW_COPY_AND_ASSIGN is missing. > > > > This is used in a STL vector so it needs to allow copy and assign > > non-copyable classes can be used in STL vectors. The only caveat is that you are > using std::sort(), which needs an explicit move-assignment constructor. If you > add > > Candidate(Candidate&&) = default; > Candidate& operator=(Candidate&& other) { > // This should go in the .cc. > tab_ = std::move(other.tab_); > app_ = std::move(other.app_); > process_type_ = std::move(other.process_type_); > return *this; > } > > you should be fine. Due to the |process_type_| assignment, it indeed cannot be > const. OK you're right. But do I need move() here given those members are primitive types?? tab_ = std::move(other.tab_); could be just tab_ = other.tab_ ; ? After thinking it again, I don't get the point of making the class move-only. I mean conceptually it makes sense to be copiable. It's unlike type like uniq_ptr which needs to manage ownership/ Also it's not more efficiency because the suggested move assignment basically just does what the default copy assignment does. How about just make it copiable ? https://codereview.chromium.org/2095413002/diff/80001/chrome/browser/memory/t... File chrome/browser/memory/tab_manager_delegate_chromeos.cc (right): https://codereview.chromium.org/2095413002/diff/80001/chrome/browser/memory/t... chrome/browser/memory/tab_manager_delegate_chromeos.cc:89: out = "FOCUSED_APP/FOCUSED_TAB"; On 2016/07/15 18:10:07, Luis Héctor Chávez wrote: > nit: return os << "FOCUSED_APP/FOCUSED_TAB"; > > that avoids unnecessary copies. Done. https://codereview.chromium.org/2095413002/diff/80001/chrome/browser/memory/t... chrome/browser/memory/tab_manager_delegate_chromeos.cc:665: // The list is sorted by importance descendingly, so we go through the list On 2016/07/15 18:10:06, Luis Héctor Chávez wrote: > nit: descending importance Done.
ping Luis, Yusuke is OOO, could you approve the CL if you think it's done? Thanks
lgtm with nits https://codereview.chromium.org/2095413002/diff/120001/chrome/browser/memory/... File chrome/browser/memory/tab_manager_delegate_chromeos.cc (right): https://codereview.chromium.org/2095413002/diff/120001/chrome/browser/memory/... chrome/browser/memory/tab_manager_delegate_chromeos.cc:88: os << "FOCUSED_APP/FOCUSED_TAB"; style nit: having "return os << ..." saves you one line per case (the break; statement) https://codereview.chromium.org/2095413002/diff/120001/chrome/browser/memory/... chrome/browser/memory/tab_manager_delegate_chromeos.cc:626: candidates.push_back(Candidate(&tab)); nit: can all the calls to candidates.push_back(Candidate(...)) become candidates.emplace_back(...) ?
https://codereview.chromium.org/2095413002/diff/60001/chrome/browser/memory/t... File chrome/browser/memory/tab_manager_delegate_chromeos.h (right): https://codereview.chromium.org/2095413002/diff/60001/chrome/browser/memory/t... chrome/browser/memory/tab_manager_delegate_chromeos.h:271: }; On 2016/07/20 17:57:53, cylee1 wrote: > On 2016/07/15 18:10:06, Luis Héctor Chávez wrote: > > On 2016/07/15 17:31:13, cylee1 wrote: > > > On 2016/07/15 00:55:05, Yusuke Sato wrote: > > > > DISALLOW_COPY_AND_ASSIGN is missing. > > > > > > This is used in a STL vector so it needs to allow copy and assign > > > > non-copyable classes can be used in STL vectors. The only caveat is that you > are > > using std::sort(), which needs an explicit move-assignment constructor. If you > > add > > > > Candidate(Candidate&&) = default; > > Candidate& operator=(Candidate&& other) { > > // This should go in the .cc. > > tab_ = std::move(other.tab_); > > app_ = std::move(other.app_); > > process_type_ = std::move(other.process_type_); > > return *this; > > } > > > > you should be fine. Due to the |process_type_| assignment, it indeed cannot be > > const. > > OK you're right. > But do I need move() here given those members are primitive types?? > tab_ = std::move(other.tab_); > could be just > tab_ = other.tab_ ; > ? > > After thinking it again, I don't get the point of making the class move-only. I > mean conceptually it makes sense to be copiable. It's unlike type like uniq_ptr > which needs to manage ownership/ Also it's not more efficiency because the > suggested move assignment basically just does what the default copy assignment > does. How about just make it copiable ? > > I decided to drop the DISALLOW_COPY_AND_ASSIGN line and make the class copiable. Let me know if you object to this change. I can fix it in another CL. https://codereview.chromium.org/2095413002/diff/120001/chrome/browser/memory/... File chrome/browser/memory/tab_manager_delegate_chromeos.cc (right): https://codereview.chromium.org/2095413002/diff/120001/chrome/browser/memory/... chrome/browser/memory/tab_manager_delegate_chromeos.cc:88: os << "FOCUSED_APP/FOCUSED_TAB"; On 2016/07/22 03:01:43, Luis Héctor Chávez wrote: > style nit: having "return os << ..." saves you one line per case (the break; > statement) Done. https://codereview.chromium.org/2095413002/diff/120001/chrome/browser/memory/... chrome/browser/memory/tab_manager_delegate_chromeos.cc:626: candidates.push_back(Candidate(&tab)); On 2016/07/22 03:01:43, Luis Héctor Chávez wrote: > nit: can all the calls to candidates.push_back(Candidate(...)) become > candidates.emplace_back(...) ? Done.
https://codereview.chromium.org/2095413002/diff/60001/chrome/browser/memory/t... File chrome/browser/memory/tab_manager_delegate_chromeos.h (right): https://codereview.chromium.org/2095413002/diff/60001/chrome/browser/memory/t... chrome/browser/memory/tab_manager_delegate_chromeos.h:271: }; On 2016/07/22 22:18:43, cylee1 wrote: > On 2016/07/20 17:57:53, cylee1 wrote: > > On 2016/07/15 18:10:06, Luis Héctor Chávez wrote: > > > On 2016/07/15 17:31:13, cylee1 wrote: > > > > On 2016/07/15 00:55:05, Yusuke Sato wrote: > > > > > DISALLOW_COPY_AND_ASSIGN is missing. > > > > > > > > This is used in a STL vector so it needs to allow copy and assign > > > > > > non-copyable classes can be used in STL vectors. The only caveat is that you > > are > > > using std::sort(), which needs an explicit move-assignment constructor. If > you > > > add > > > > > > Candidate(Candidate&&) = default; > > > Candidate& operator=(Candidate&& other) { > > > // This should go in the .cc. > > > tab_ = std::move(other.tab_); > > > app_ = std::move(other.app_); > > > process_type_ = std::move(other.process_type_); > > > return *this; > > > } > > > > > > you should be fine. Due to the |process_type_| assignment, it indeed cannot > be > > > const. > > > > OK you're right. > > But do I need move() here given those members are primitive types?? > > tab_ = std::move(other.tab_); > > could be just > > tab_ = other.tab_ ; > > ? > > > > After thinking it again, I don't get the point of making the class move-only. > I > > mean conceptually it makes sense to be copiable. It's unlike type like > uniq_ptr > > which needs to manage ownership/ Also it's not more efficiency because the > > suggested move assignment basically just does what the default copy assignment > > does. How about just make it copiable ? > > > > > > I decided to drop the DISALLOW_COPY_AND_ASSIGN line and make the class copiable. > Let me know if you object to this change. I can fix it in another CL. It should be slightly more efficient to make it non-copyable since you can do more performant swaps in std::sort. What was your motivation to drop it? The style guide suggests that most classes should _not_ be copiable: https://chromium-cpp.appspot.com/
The CQ bit was checked by cylee@chromium.org 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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
https://codereview.chromium.org/2095413002/diff/60001/chrome/browser/memory/t... File chrome/browser/memory/tab_manager_delegate_chromeos.h (right): https://codereview.chromium.org/2095413002/diff/60001/chrome/browser/memory/t... chrome/browser/memory/tab_manager_delegate_chromeos.h:271: }; On 2016/07/22 22:30:31, Luis Héctor Chávez wrote: > On 2016/07/22 22:18:43, cylee1 wrote: > > On 2016/07/20 17:57:53, cylee1 wrote: > > > On 2016/07/15 18:10:06, Luis Héctor Chávez wrote: > > > > On 2016/07/15 17:31:13, cylee1 wrote: > > > > > On 2016/07/15 00:55:05, Yusuke Sato wrote: > > > > > > DISALLOW_COPY_AND_ASSIGN is missing. > > > > > > > > > > This is used in a STL vector so it needs to allow copy and assign > > > > > > > > non-copyable classes can be used in STL vectors. The only caveat is that > you > > > are > > > > using std::sort(), which needs an explicit move-assignment constructor. If > > you > > > > add > > > > > > > > Candidate(Candidate&&) = default; > > > > Candidate& operator=(Candidate&& other) { > > > > // This should go in the .cc. > > > > tab_ = std::move(other.tab_); > > > > app_ = std::move(other.app_); > > > > process_type_ = std::move(other.process_type_); > > > > return *this; > > > > } > > > > > > > > you should be fine. Due to the |process_type_| assignment, it indeed > cannot > > be > > > > const. > > > > > > OK you're right. > > > But do I need move() here given those members are primitive types?? > > > tab_ = std::move(other.tab_); > > > could be just > > > tab_ = other.tab_ ; > > > ? > > > > > > After thinking it again, I don't get the point of making the class > move-only. > > I > > > mean conceptually it makes sense to be copiable. It's unlike type like > > uniq_ptr > > > which needs to manage ownership/ Also it's not more efficiency because the > > > suggested move assignment basically just does what the default copy > assignment > > > does. How about just make it copiable ? > > > > > > > > > > I decided to drop the DISALLOW_COPY_AND_ASSIGN line and make the class > copiable. > > Let me know if you object to this change. I can fix it in another CL. > > It should be slightly more efficient to make it non-copyable since you can do > more performant swaps in std::sort. What was your motivation to drop it? The > style guide suggests that most classes should _not_ be copiable: > https://chromium-cpp.appspot.com/ Per IM discussion, reverted back to move assignment for now. But to be honest I still doubt about the in-place swap part . How can be a swap be done in-place by move semantic? Isn't you still need something like tmp = std::move(a); a = std::move(b); b = std::move(tmp); ? I can understand implementing move by swap though (and it's a common practice)
The CQ bit was checked by cylee@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rickyz@chromium.org, georgesak@chromium.org, lhchavez@chromium.org Link to the patchset: https://codereview.chromium.org/2095413002/#ps160001 (title: "revert to move assignment")
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
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
The CQ bit was checked by cylee@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lhchavez@chromium.org, rickyz@chromium.org, georgesak@chromium.org Link to the patchset: https://codereview.chromium.org/2095413002/#ps180001 (title: "sync to ToT")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== TabManagerDelegate: Better prioritize ARC processes. Add new fields "is_focused" and "last_active_time" in struct RunningAppProcessInfo of process.mojom so Chrome knows how to prioritize Android processes better. BUG=625851 TEST=minnie ========== to ========== TabManagerDelegate: Better prioritize ARC processes. Add new fields "is_focused" and "last_active_time" in struct RunningAppProcessInfo of process.mojom so Chrome knows how to prioritize Android processes better. BUG=625851 TEST=minnie ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== TabManagerDelegate: Better prioritize ARC processes. Add new fields "is_focused" and "last_active_time" in struct RunningAppProcessInfo of process.mojom so Chrome knows how to prioritize Android processes better. BUG=625851 TEST=minnie ========== to ========== TabManagerDelegate: Better prioritize ARC processes. Add new fields "is_focused" and "last_active_time" in struct RunningAppProcessInfo of process.mojom so Chrome knows how to prioritize Android processes better. BUG=625851 TEST=minnie Committed: https://crrev.com/c2e58df8d48e216450dd83af783a3e5d97a6e329 Cr-Commit-Position: refs/heads/master@{#407444} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/c2e58df8d48e216450dd83af783a3e5d97a6e329 Cr-Commit-Position: refs/heads/master@{#407444} |
