Improve the about/discards UI, allowing each tab to be individually closed.

Improve the about/discards UI, allowing each tab to be individually closed.

Changed the format of the tab lines to make the special aspects of the tab (pinned, audio, discarded) easier on the eye.

BUG=457527
Committed: https://crrev.com/4550b46a4d391429945886691f6a0dd9f3d90983
Cr-Commit-Position: refs/heads/master@{#340075}

[proberge, 2015-07-21 17:52:36]

proberge@chromium.org changed reviewers:
+ georgesak@chromium.org

[Georges Khalil, 2015-07-21 18:13:37]

Looking good! Only minor modifications. And git cl format.

https://codereview.chromium.org/1249693002/diff/20001/chrome/browser/memory/o...
File chrome/browser/memory/oom_priority_manager.h (right):

https://codereview.chromium.org/1249693002/diff/20001/chrome/browser/memory/o...
chrome/browser/memory/oom_priority_manager.h:59: // Returns the list of the
stats for all renderers.
nit: Add "Must be called on UI thread." (not implicit anymore, because of name
change).

https://codereview.chromium.org/1249693002/diff/20001/chrome/browser/memory/o...
chrome/browser/memory/oom_priority_manager.h:67: // Discards a tab with the
given unique ID.
nit: Fix wrap.

https://codereview.chromium.org/1249693002/diff/20001/chrome/browser/memory/o...
chrome/browser/memory/oom_priority_manager.h:69: // Returns true if it
successfully found a tab and discarded it.
Better comments, good! :)

https://codereview.chromium.org/1249693002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/about_ui.cc (right):

https://codereview.chromium.org/1249693002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/about_ui.cc:505: std::string AboutDiscardsRun(int64
web_content_id) {
Let's give this a different name. How about AboutDiscardsRunSingleTab?

https://codereview.chromium.org/1249693002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/about_ui.cc:534: str +=
base::ASCIIToUTF16(base::StringPrintf(" %d ", it->oom_score));
Let's wrap this in parenthesis for better clarity.

https://codereview.chromium.org/1249693002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/about_ui.cc:552: const char kRunCommand[] = "run";
Let's dedupe this and put it higher up and give it a more significant name.
kAboutDiscardsRunCommand?

[proberge, 2015-07-21 18:29:07]

https://codereview.chromium.org/1249693002/diff/20001/chrome/browser/memory/o...
File chrome/browser/memory/oom_priority_manager.h (right):

https://codereview.chromium.org/1249693002/diff/20001/chrome/browser/memory/o...
chrome/browser/memory/oom_priority_manager.h:59: // Returns the list of the
stats for all renderers.
On 2015/07/21 18:13:37, Georges Khalil wrote:
> nit: Add "Must be called on UI thread." (not implicit anymore, because of name
> change).

Done.

https://codereview.chromium.org/1249693002/diff/20001/chrome/browser/memory/o...
chrome/browser/memory/oom_priority_manager.h:67: // Discards a tab with the
given unique ID.
On 2015/07/21 18:13:37, Georges Khalil wrote:
> nit: Fix wrap.

Done.

https://codereview.chromium.org/1249693002/diff/20001/chrome/browser/memory/o...
chrome/browser/memory/oom_priority_manager.h:69: // Returns true if it
successfully found a tab and discarded it.
On 2015/07/21 18:13:37, Georges Khalil wrote:
> Better comments, good! :)

Acknowledged.

https://codereview.chromium.org/1249693002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/about_ui.cc (right):

https://codereview.chromium.org/1249693002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/about_ui.cc:505: std::string AboutDiscardsRun(int64
web_content_id) {
On 2015/07/21 18:13:37, Georges Khalil wrote:
> Let's give this a different name. How about AboutDiscardsRunSingleTab?

I don't think that's a good name for it either. Do we typically avoid function
overloading?

https://codereview.chromium.org/1249693002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/about_ui.cc:534: str +=
base::ASCIIToUTF16(base::StringPrintf(" %d ", it->oom_score));
On 2015/07/21 18:13:37, Georges Khalil wrote:
> Let's wrap this in parenthesis for better clarity.

I don't understand what to wrap - this is an #if, not an if() {}.

https://codereview.chromium.org/1249693002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/about_ui.cc:552: const char kRunCommand[] = "run";
On 2015/07/21 18:13:37, Georges Khalil wrote:
> Let's dedupe this and put it higher up and give it a more significant name.
> kAboutDiscardsRunCommand?

Done.

[Georges Khalil, 2015-07-21 18:38:29]

https://codereview.chromium.org/1249693002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/about_ui.cc (right):

https://codereview.chromium.org/1249693002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/about_ui.cc:505: std::string AboutDiscardsRun(int64
web_content_id) {
On 2015/07/21 18:29:07, proberge wrote:
> On 2015/07/21 18:13:37, Georges Khalil wrote:
> > Let's give this a different name. How about AboutDiscardsRunSingleTab?
> 
> I don't think that's a good name for it either. Do we typically avoid
function
> overloading?

Yes, if possible. In that case, there's no reason to overload the name.

I'm thinking let's eliminate those two functions, they're only called from a
single place each. Inline them instead.

https://codereview.chromium.org/1249693002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/about_ui.cc:534: str +=
base::ASCIIToUTF16(base::StringPrintf(" %d ", it->oom_score));
On 2015/07/21 18:29:07, proberge wrote:
> On 2015/07/21 18:13:37, Georges Khalil wrote:
> > Let's wrap this in parenthesis for better clarity.
> 
> I don't understand what to wrap - this is an #if, not an if() {}.

Should have been more specific. Let's wrap the score in parenthesis. So that
when it's displayed, it looks less part of the title.

https://codereview.chromium.org/1249693002/diff/40001/chrome/browser/memory/o...
File chrome/browser/memory/oom_priority_manager.h (right):

https://codereview.chromium.org/1249693002/diff/40001/chrome/browser/memory/o...
chrome/browser/memory/oom_priority_manager.h:59: // Returns the list of the
stats for all renderers. Must be called on an UI
nit: on the UI thread (there's only one).

[proberge, 2015-07-21 18:55:09]

https://codereview.chromium.org/1249693002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/about_ui.cc (right):

https://codereview.chromium.org/1249693002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/about_ui.cc:505: std::string AboutDiscardsRun(int64
web_content_id) {
On 2015/07/21 18:38:29, Georges Khalil wrote:
> On 2015/07/21 18:29:07, proberge wrote:
> > On 2015/07/21 18:13:37, Georges Khalil wrote:
> > > Let's give this a different name. How about AboutDiscardsRunSingleTab?
> > 
> > I don't think that's a good name for it either. Do we typically avoid
function
> > overloading?
> 
> Yes, if possible. In that case, there's no reason to overload the name.
> 
> I'm thinking let's eliminate those two functions, they're only called from a
> single place each. Inline them instead.

Done.

https://codereview.chromium.org/1249693002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/about_ui.cc:534: str +=
base::ASCIIToUTF16(base::StringPrintf(" %d ", it->oom_score));
On 2015/07/21 18:38:29, Georges Khalil wrote:
> On 2015/07/21 18:29:07, proberge wrote:
> > On 2015/07/21 18:13:37, Georges Khalil wrote:
> > > Let's wrap this in parenthesis for better clarity.
> > 
> > I don't understand what to wrap - this is an #if, not an if() {}.
> 
> Should have been more specific. Let's wrap the score in parenthesis. So that
> when it's displayed, it looks less part of the title.

Done.

https://codereview.chromium.org/1249693002/diff/40001/chrome/browser/memory/o...
File chrome/browser/memory/oom_priority_manager.h (right):

https://codereview.chromium.org/1249693002/diff/40001/chrome/browser/memory/o...
chrome/browser/memory/oom_priority_manager.h:59: // Returns the list of the
stats for all renderers. Must be called on an UI
On 2015/07/21 18:38:29, Georges Khalil wrote:
> nit: on the UI thread (there's only one).

Done.

[proberge, 2015-07-21 19:00:16]

proberge@chromium.org changed reviewers:
+ sky@chromium.org

[sky, 2015-07-21 22:57:10]

https://codereview.chromium.org/1249693002/diff/80001/chrome/browser/memory/o...
File chrome/browser/memory/oom_priority_manager.cc (right):

https://codereview.chromium.org/1249693002/diff/80001/chrome/browser/memory/o...
chrome/browser/memory/oom_priority_manager.cc:180: LOG(WARNING) << "Discarding
tab " << idx << " id "
I see you're just moving code, but I'm surprised we do this as a WARNING.

https://codereview.chromium.org/1249693002/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/about_ui.cc (right):

https://codereview.chromium.org/1249693002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/about_ui.cc:506: std::vector<std::string> GetTabLines()
{
How about a more specific name?

https://codereview.chromium.org/1249693002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/about_ui.cc:512: for (; it != stats.end(); ++it) {
nit: move 511 into for loop.

https://codereview.chromium.org/1249693002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/about_ui.cc:522: str += net::EscapeForHTML(it->title);
Seems like you could avoid lots of the conversions if you made str a utf8 string
and changed this line to convert to utf8.

[proberge, 2015-07-22 14:21:06]

https://codereview.chromium.org/1249693002/diff/80001/chrome/browser/memory/o...
File chrome/browser/memory/oom_priority_manager.cc (right):

https://codereview.chromium.org/1249693002/diff/80001/chrome/browser/memory/o...
chrome/browser/memory/oom_priority_manager.cc:180: LOG(WARNING) << "Discarding
tab " << idx << " id "
On 2015/07/21 22:57:10, sky wrote:
> I see you're just moving code, but I'm surprised we do this as a WARNING.

I tried demoting it to LOG(INFO) but am getting a presubmit error:
These files spam the console log with LOG(INFO):
  chrome/browser/memory/oom_priority_manager.cc

https://codereview.chromium.org/1249693002/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/about_ui.cc (right):

https://codereview.chromium.org/1249693002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/about_ui.cc:506: std::vector<std::string> GetTabLines()
{
On 2015/07/21 22:57:10, sky wrote:
> How about a more specific name?

Done.

https://codereview.chromium.org/1249693002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/about_ui.cc:512: for (; it != stats.end(); ++it) {
On 2015/07/21 22:57:10, sky wrote:
> nit: move 511 into for loop.

Done.

https://codereview.chromium.org/1249693002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/about_ui.cc:522: str += net::EscapeForHTML(it->title);
On 2015/07/21 22:57:10, sky wrote:
> Seems like you could avoid lots of the conversions if you made str a utf8
string
> and changed this line to convert to utf8.

Done.

[sky, 2015-07-22 15:29:41]

https://codereview.chromium.org/1249693002/diff/80001/chrome/browser/memory/o...
File chrome/browser/memory/oom_priority_manager.cc (right):

https://codereview.chromium.org/1249693002/diff/80001/chrome/browser/memory/o...
chrome/browser/memory/oom_priority_manager.cc:180: LOG(WARNING) << "Discarding
tab " << idx << " id "
On 2015/07/22 14:21:06, proberge wrote:
> On 2015/07/21 22:57:10, sky wrote:
> > I see you're just moving code, but I'm surprised we do this as a WARNING.
> 
> I tried demoting it to LOG(INFO) but am getting a presubmit error:
> These files spam the console log with LOG(INFO):
>   chrome/browser/memory/oom_priority_manager.cc

You should use VLOG

[proberge, 2015-07-22 18:05:29]

https://codereview.chromium.org/1249693002/diff/80001/chrome/browser/memory/o...
File chrome/browser/memory/oom_priority_manager.cc (right):

https://codereview.chromium.org/1249693002/diff/80001/chrome/browser/memory/o...
chrome/browser/memory/oom_priority_manager.cc:180: LOG(WARNING) << "Discarding
tab " << idx << " id "
On 2015/07/22 15:29:41, sky wrote:
> On 2015/07/22 14:21:06, proberge wrote:
> > On 2015/07/21 22:57:10, sky wrote:
> > > I see you're just moving code, but I'm surprised we do this as a WARNING.
> > 
> > I tried demoting it to LOG(INFO) but am getting a presubmit error:
> > These files spam the console log with LOG(INFO):
> >   chrome/browser/memory/oom_priority_manager.cc
> 
> You should use VLOG

Thanks! Done.

[sky, 2015-07-22 22:21:45]

LGTM

[Georges Khalil, 2015-07-23 11:31:31]

The CQ bit was checked by georgesak@chromium.org

[commit-bot: I haz the power, 2015-07-23 11:32:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1249693002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1249693002/120001

[commit-bot: I haz the power, 2015-07-23 12:20:59]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-07-23 12:21:58]

Patchset 7 (id:??) landed as
https://crrev.com/4550b46a4d391429945886691f6a0dd9f3d90983
Cr-Commit-Position: refs/heads/master@{#340075}