Refactor ARC OOM handler code

chrome/browser/chromeos/arc/process/arc_process.cc

Index: chrome/browser/chromeos/arc/process/arc_process.cc
diff --git a/chrome/browser/chromeos/arc/process/arc_process.cc b/chrome/browser/chromeos/arc/process/arc_process.cc
index abb054a67c82ae8c9a0208f138eea0935f1e33cc..cb8bc9c02b37fbde297fbc76cde56d0364eac161 100644
--- a/chrome/browser/chromeos/arc/process/arc_process.cc
+++ b/chrome/browser/chromeos/arc/process/arc_process.cc
@@ -8,6 +8,15 @@
 
 namespace arc {
 
+namespace {
+
+// A special process on Android side which serves as a dummy "focused" app
+// when the focused window is a Chrome side window (i.e., all Android
+// processes are running in the background). We don't want to kill it anyway.
+constexpr char kArcHomeProcess[] = "org.chromium.arc.home";

[cylee1, 2017/05/10 16:31:54]

A no-so-relevant comment : I'm not sure if what described in the comment still
hold now. Something may had changed and I'm not sure if org.chromium.arc.home is
still used that way. Could you do a quick check to see if the process is still
used that way? (marked as TOP when a Chrome side window is focused, and not TOP
is an Android app window is focused).

[Yusuke Sato, 2017/05/10 18:29:43]

The process still works like that.

+
+}  // namespace
+
 ArcProcess::ArcProcess(base::ProcessId nspid,
                        base::ProcessId pid,
                        const std::string& process_name,
@@ -34,4 +43,17 @@ bool ArcProcess::operator<(const ArcProcess& rhs) const {
 ArcProcess::ArcProcess(ArcProcess&& other) = default;
 ArcProcess& ArcProcess::operator=(ArcProcess&& other) = default;
 
+bool ArcProcess::IsUserVisible() const {

[cylee1, 2017/05/10 16:31:54]

The name is not so right because processes like persistent app would be included
here. And it's not always visible ? Also kArcHomeProcess (if still needed) is
not "user visible" in the strict sense.

[Yusuke Sato, 2017/05/10 18:29:43]

Done. Changed to IsImportant.

+  return process_state() <= mojom::ProcessState::IMPORTANT_FOREGROUND ||
+         process_name() == kArcHomeProcess;
+}
+
+bool ArcProcess::IsKernelKillable() const {
+  // Protect PERSISTENT, PERSISTENT_UI, and our HOME processes since they should

[cylee1, 2017/05/10 16:31:55]

Thinking it twice, I feel there's no need to separate IsUserVisible() (or what
its name) and IsKernelKillable().
I remember that in some discussion thread, as Luigi suggested, we should try to
make chrome and kernel do the same thing. So maybe we just leave one function as
IsKillable() or IsImportant(), which uses

return process_state() <= mojom::ProcessState::IMPORTANT_FOREGROUND ||
process_name() == kArcHomeProcess;

as criteria ? 

[Yusuke Sato, 2017/05/10 18:29:43]

With this CL, because of the explicit SetOomScore(-1000) call, merging these two
functions is possible. It may be a good idea.

However, because merging the two _drastically_ changes the kernel OOM behavior,
I'd avoid doing so in this pure refactoring CL. We can investigate the current
OOM behavior more, and change this file later.

Also, please note that Chrome and kernel do a different thing for tabs. Chrome
never kills FOCUSED_TAB but the kernel still can kill it. Are you saying this
should be changed too?

Anyway, added a TODO to the header side.

[cylee1, 2017/05/11 08:59:53]

Yes. I remember that's what Luigi and me discussed then (whether to kill focused
tab). The bottom line is that there's no reason to make one killable by us but
not by kernel or vice versa. If TabDiscarder shouldn't kill it, it means we
can't handle the UI nicely. It's impossible for kernel to do it better. So let's
sync them in the next CL.  Thanks !
We may loop other folks to discuss this. I'll find the discussion later... 

[Yusuke Sato, 2017/05/11 18:50:16]

Acknowledged.

+  // never be killed even by the kernel. Returning false for them allows their
+  // OOM adjustment scores to remain negative.
+  return process_state() > arc::mojom::ProcessState::PERSISTENT_UI &&
+         process_name() != kArcHomeProcess;
+}
+
 }  // namespace arc