Create system tray item for accessing chrome://slow

Create system tray item for accessing chrome://slow

BUG=269843
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=216915

[Zachary Kuznia, 2013-08-08 02:19:43]

Please take a look.

[rkc, 2013-08-08 21:56:24]

lgtm once the comments are addressed.

https://codereview.chromium.org/22654003/diff/5001/ash/system/tray/system_tra...
File ash/system/tray/system_tray.cc (right):

https://codereview.chromium.org/22654003/diff/5001/ash/system/tray/system_tra...
ash/system/tray/system_tray.cc:171: AddTrayItem(tray_tracing_);
This can be replaced with,
AddTrayItem(new internal::TrayTracing(this));

https://codereview.chromium.org/22654003/diff/5001/ash/system/tray/system_tray.h
File ash/system/tray/system_tray.h (right):

https://codereview.chromium.org/22654003/diff/5001/ash/system/tray/system_tra...
ash/system/tray/system_tray.h:234: internal::TrayTracing* tray_tracing_;  // not
owned
This doesn't seem to be used.

[Zachary Kuznia, 2013-08-08 22:44:23]

https://codereview.chromium.org/22654003/diff/5001/ash/system/tray/system_tra...
File ash/system/tray/system_tray.cc (right):

https://codereview.chromium.org/22654003/diff/5001/ash/system/tray/system_tra...
ash/system/tray/system_tray.cc:171: AddTrayItem(tray_tracing_);
On 2013/08/08 21:56:25, Rahul Chaturvedi wrote:
> This can be replaced with,
> AddTrayItem(new internal::TrayTracing(this));

Done.

https://codereview.chromium.org/22654003/diff/5001/ash/system/tray/system_tray.h
File ash/system/tray/system_tray.h (right):

https://codereview.chromium.org/22654003/diff/5001/ash/system/tray/system_tra...
ash/system/tray/system_tray.h:234: internal::TrayTracing* tray_tracing_;  // not
owned
On 2013/08/08 21:56:25, Rahul Chaturvedi wrote:
> This doesn't seem to be used.

Done.

[Zachary Kuznia, 2013-08-08 22:46:19]

sadrul:  Could you do an OWNERS review of ash/system/tray/*?
jhawkins:  COuld you do an OWNERS review of chrome/browser/*?

[sadrul, 2013-08-09 04:56:44]

+jamescook@

Do we really want to show this in the tray?

https://codereview.chromium.org/22654003/diff/13001/ash/system/tray/system_tr...
File ash/system/tray/system_tray.h (right):

https://codereview.chromium.org/22654003/diff/13001/ash/system/tray/system_tr...
ash/system/tray/system_tray.h:49: class TrayTracing;
Not necessary here.

https://codereview.chromium.org/22654003/diff/13001/ash/system/tray_tracing.cc
File ash/system/tray_tracing.cc (right):

https://codereview.chromium.org/22654003/diff/13001/ash/system/tray_tracing.c...
ash/system/tray_tracing.cc:88: return true;
Should this get the setting from somewhere?

[Zachary Kuznia, 2013-08-09 15:39:19]

On 2013/08/09 04:56:44, sadrul wrote:
> +jamescook@
> 
> Do we really want to show this in the tray?

This is only shown when the setting is enabled, similar to the accessibility
item.

[James Cook, 2013-08-09 17:43:37]

On 2013/08/09 15:39:19, Zachary Kuznia wrote:
> On 2013/08/09 04:56:44, sadrul wrote:
> > +jamescook@
> > 
> > Do we really want to show this in the tray?
> 
> This is only shown when the setting is enabled, similar to the accessibility
> item.

Is there a design doc or mocks I can look at for this feature?  I've heard a
little about it, but I'm not clear on exactly what is getting implemented here.

[James Cook, 2013-08-09 20:06:02]

https://codereview.chromium.org/22654003/diff/13001/ash/system/tray/system_tr...
File ash/system/tray/system_tray.cc (right):

https://codereview.chromium.org/22654003/diff/13001/ash/system/tray/system_tr...
ash/system/tray/system_tray.cc:170: AddTrayItem(new
internal::TrayTracing(this));
I think you want this inside #if defined(OS_CHROMEOS) since the chrome://slow
page sources only exist on Chrome OS.  I presume Windows-Ash doesn't have
chrome://slow ?

https://codereview.chromium.org/22654003/diff/13001/ash/system/tray/system_tr...
File ash/system/tray/system_tray_delegate.h (right):

https://codereview.chromium.org/22654003/diff/13001/ash/system/tray/system_tr...
ash/system/tray/system_tray_delegate.h:172: virtual void ShowSlow() = 0;
This seems like the name should be more descriptive, like
ShowPerformanceTracing() or ShowChromeSlow() or ShowAboutSlow() or some such.

https://codereview.chromium.org/22654003/diff/13001/ash/system/tray_tracing.cc
File ash/system/tray_tracing.cc (right):

https://codereview.chromium.org/22654003/diff/13001/ash/system/tray_tracing.c...
ash/system/tray_tracing.cc:1: // Copyright (c) 2013 The Chromium Authors. All
rights reserved.
nit: "(c)" is no longer required in the copyright header

https://codereview.chromium.org/22654003/diff/13001/ash/system/tray_tracing.c...
ash/system/tray_tracing.cc:29: explicit DefaultTracingView() {
not explicit

https://codereview.chromium.org/22654003/diff/13001/ash/system/tray_tracing.c...
ash/system/tray_tracing.cc:49: virtual ~DefaultTracingView() {
optional nit: {} on same line?

https://codereview.chromium.org/22654003/diff/13001/ash/system/tray_tracing.h
File ash/system/tray_tracing.h (right):

https://codereview.chromium.org/22654003/diff/13001/ash/system/tray_tracing.h...
ash/system/tray_tracing.h:27: class TrayTracing : public TrayImageItem,
needs class comment

https://codereview.chromium.org/22654003/diff/13001/chrome/app/chromeos_strin...
File chrome/app/chromeos_strings.grdp (right):

https://codereview.chromium.org/22654003/diff/13001/chrome/app/chromeos_strin...
chrome/app/chromeos_strings.grdp:1643: <ph
name="BEGIN_BOLD">&lt;strong&gt;</ph>Note:<ph
name="END_BOLD">&lt;/strong&gt;</ph> only enable if you know what you are doing
or if you have been asked to do so, as collection of data will affect
performance.
Unless the UI reviewers felt strongly about the specific text of these strings,
I would consider saying something like "may reduce performance" or "may slow
down your browser".  I keep hoping that it will "affect performance" by
improving it! :-)

Also, I would probably capitalize "only"

https://codereview.chromium.org/22654003/diff/13001/chrome/browser/ui/webui/c...
File chrome/browser/ui/webui/chromeos/slow_ui.cc (right):

https://codereview.chromium.org/22654003/diff/13001/chrome/browser/ui/webui/c...
chrome/browser/ui/webui/chromeos/slow_ui.cc:58: return false;
Are you sure this is the right thing to do?  I thought we wanted
content-security for chrome: pages.  The header comment for
ShouldAddContentSecurityPolicy says:

  // If you are overriding this, then you have a bug.
  // It is not acceptable to disable content-security-policy on chrome:// pages
  // to permit functionality excluded by CSP, such as inline script.
  // Instead, you must go back and change your WebUI page so that it is
  // compliant with the policy. This typically involves ensuring that all script
  // is delivered through the data manager backend. Talk to tsepez for more
  // info.

https://codereview.chromium.org/22654003/diff/13001/chrome/browser/ui/webui/c...
chrome/browser/ui/webui/chromeos/slow_ui.cc:67: // The handler for Javascript
messages related to the "sim-unlock" view.
I'm thinking this isn't "sim-unlock". :-)

https://codereview.chromium.org/22654003/diff/13001/chrome/browser/ui/webui/c...
File chrome/browser/ui/webui/chromeos/slow_ui.h (right):

https://codereview.chromium.org/22654003/diff/13001/chrome/browser/ui/webui/c...
chrome/browser/ui/webui/chromeos/slow_ui.h:1: // Copyright (c) 2013 The Chromium
Authors. All rights reserved.
nit: no (c) required anymore

[Zachary Kuznia, 2013-08-09 20:58:04]

https://codereview.chromium.org/22654003/diff/13001/ash/system/tray/system_tr...
File ash/system/tray/system_tray.cc (right):

https://codereview.chromium.org/22654003/diff/13001/ash/system/tray/system_tr...
ash/system/tray/system_tray.cc:170: AddTrayItem(new
internal::TrayTracing(this));
On 2013/08/09 20:06:02, James Cook wrote:
> I think you want this inside #if defined(OS_CHROMEOS) since the chrome://slow
> page sources only exist on Chrome OS.  I presume Windows-Ash doesn't have
> chrome://slow ?

Done.

https://codereview.chromium.org/22654003/diff/13001/ash/system/tray/system_tr...
File ash/system/tray/system_tray.h (right):

https://codereview.chromium.org/22654003/diff/13001/ash/system/tray/system_tr...
ash/system/tray/system_tray.h:49: class TrayTracing;
On 2013/08/09 04:56:44, sadrul wrote:
> Not necessary here.

Done.

https://codereview.chromium.org/22654003/diff/13001/ash/system/tray/system_tr...
File ash/system/tray/system_tray_delegate.h (right):

https://codereview.chromium.org/22654003/diff/13001/ash/system/tray/system_tr...
ash/system/tray/system_tray_delegate.h:172: virtual void ShowSlow() = 0;
On 2013/08/09 20:06:02, James Cook wrote:
> This seems like the name should be more descriptive, like
> ShowPerformanceTracing() or ShowChromeSlow() or ShowAboutSlow() or some such.

Done.

https://codereview.chromium.org/22654003/diff/13001/ash/system/tray_tracing.cc
File ash/system/tray_tracing.cc (right):

https://codereview.chromium.org/22654003/diff/13001/ash/system/tray_tracing.c...
ash/system/tray_tracing.cc:1: // Copyright (c) 2013 The Chromium Authors. All
rights reserved.
On 2013/08/09 20:06:02, James Cook wrote:
> nit: "(c)" is no longer required in the copyright header

Done.

https://codereview.chromium.org/22654003/diff/13001/ash/system/tray_tracing.c...
ash/system/tray_tracing.cc:29: explicit DefaultTracingView() {
On 2013/08/09 20:06:02, James Cook wrote:
> not explicit

Done.

https://codereview.chromium.org/22654003/diff/13001/ash/system/tray_tracing.c...
ash/system/tray_tracing.cc:49: virtual ~DefaultTracingView() {
On 2013/08/09 20:06:02, James Cook wrote:
> optional nit: {} on same line?

Done.

https://codereview.chromium.org/22654003/diff/13001/ash/system/tray_tracing.c...
ash/system/tray_tracing.cc:88: return true;
On 2013/08/09 04:56:44, sadrul wrote:
> Should this get the setting from somewhere?

I'm relying on the initial state being set when the delegate is initialized. 
Would you prefer I add a query function as well?

https://codereview.chromium.org/22654003/diff/13001/ash/system/tray_tracing.h
File ash/system/tray_tracing.h (right):

https://codereview.chromium.org/22654003/diff/13001/ash/system/tray_tracing.h...
ash/system/tray_tracing.h:27: class TrayTracing : public TrayImageItem,
On 2013/08/09 20:06:02, James Cook wrote:
> needs class comment

Done.

https://codereview.chromium.org/22654003/diff/13001/chrome/app/chromeos_strin...
File chrome/app/chromeos_strings.grdp (right):

https://codereview.chromium.org/22654003/diff/13001/chrome/app/chromeos_strin...
chrome/app/chromeos_strings.grdp:1643: <ph
name="BEGIN_BOLD">&lt;strong&gt;</ph>Note:<ph
name="END_BOLD">&lt;/strong&gt;</ph> only enable if you know what you are doing
or if you have been asked to do so, as collection of data will affect
performance.
On 2013/08/09 20:06:02, James Cook wrote:
> Unless the UI reviewers felt strongly about the specific text of these
strings,
> I would consider saying something like "may reduce performance" or "may slow
> down your browser".  I keep hoping that it will "affect performance" by
> improving it! :-)
> 
> Also, I would probably capitalize "only"

Done.

https://codereview.chromium.org/22654003/diff/13001/chrome/browser/ui/webui/c...
File chrome/browser/ui/webui/chromeos/slow_ui.cc (right):

https://codereview.chromium.org/22654003/diff/13001/chrome/browser/ui/webui/c...
chrome/browser/ui/webui/chromeos/slow_ui.cc:58: return false;
On 2013/08/09 20:06:02, James Cook wrote:
> Are you sure this is the right thing to do?  I thought we wanted
> content-security for chrome: pages.  The header comment for
> ShouldAddContentSecurityPolicy says:
> 
>   // If you are overriding this, then you have a bug.
>   // It is not acceptable to disable content-security-policy on chrome://
pages
>   // to permit functionality excluded by CSP, such as inline script.
>   // Instead, you must go back and change your WebUI page so that it is
>   // compliant with the policy. This typically involves ensuring that all
script
>   // is delivered through the data manager backend. Talk to tsepez for more
>   // info.

Done.

https://codereview.chromium.org/22654003/diff/13001/chrome/browser/ui/webui/c...
chrome/browser/ui/webui/chromeos/slow_ui.cc:67: // The handler for Javascript
messages related to the "sim-unlock" view.
On 2013/08/09 20:06:02, James Cook wrote:
> I'm thinking this isn't "sim-unlock". :-)

Done.

https://codereview.chromium.org/22654003/diff/13001/chrome/browser/ui/webui/c...
File chrome/browser/ui/webui/chromeos/slow_ui.h (right):

https://codereview.chromium.org/22654003/diff/13001/chrome/browser/ui/webui/c...
chrome/browser/ui/webui/chromeos/slow_ui.h:1: // Copyright (c) 2013 The Chromium
Authors. All rights reserved.
On 2013/08/09 20:06:02, James Cook wrote:
> nit: no (c) required anymore

Done.

[James Cook, 2013-08-09 21:16:51]

LGTM with one suggestion

https://codereview.chromium.org/22654003/diff/13001/ash/system/tray_tracing.cc
File ash/system/tray_tracing.cc (right):

https://codereview.chromium.org/22654003/diff/13001/ash/system/tray_tracing.c...
ash/system/tray_tracing.cc:88: return true;
On 2013/08/09 20:58:04, Zachary Kuznia wrote:
> On 2013/08/09 04:56:44, sadrul wrote:
> > Should this get the setting from somewhere?
> 
> I'm relying on the initial state being set when the delegate is initialized. 
> Would you prefer I add a query function as well?

Maybe it should default to false?

[sadrul, 2013-08-09 21:23:47]

https://codereview.chromium.org/22654003/diff/30001/ash/system/tray/system_tr...
File ash/system/tray/system_tray.cc (right):

https://codereview.chromium.org/22654003/diff/30001/ash/system/tray/system_tr...
ash/system/tray/system_tray.cc:171: AddTrayItem(new
internal::TrayTracing(this));
tray_tracing should live inside ash/system/chromeos/

[Zachary Kuznia, 2013-08-09 22:22:32]

https://codereview.chromium.org/22654003/diff/13001/ash/system/tray_tracing.cc
File ash/system/tray_tracing.cc (right):

https://codereview.chromium.org/22654003/diff/13001/ash/system/tray_tracing.c...
ash/system/tray_tracing.cc:88: return true;
On 2013/08/09 21:16:51, James Cook wrote:
> On 2013/08/09 20:58:04, Zachary Kuznia wrote:
> > On 2013/08/09 04:56:44, sadrul wrote:
> > > Should this get the setting from somewhere?
> > 
> > I'm relying on the initial state being set when the delegate is initialized.

> > Would you prefer I add a query function as well?
> 
> Maybe it should default to false?

Done.

https://codereview.chromium.org/22654003/diff/30001/ash/system/tray/system_tr...
File ash/system/tray/system_tray.cc (right):

https://codereview.chromium.org/22654003/diff/30001/ash/system/tray/system_tr...
ash/system/tray/system_tray.cc:171: AddTrayItem(new
internal::TrayTracing(this));
On 2013/08/09 21:23:47, sadrul wrote:
> tray_tracing should live inside ash/system/chromeos/

Done.

[Zachary Kuznia, 2013-08-09 22:24:49]

Hello thestig,

Could you do an OWNERS review of:
chrome/browser/ui/chrome_pages.{cc,h} ?

Thanks,
-Zach

[Lei Zhang, 2013-08-09 22:42:12]

chrome/browser/ui lgtm

https://codereview.chromium.org/22654003/diff/54001/chrome/browser/ui/chrome_...
File chrome/browser/ui/chrome_pages.cc (right):

https://codereview.chromium.org/22654003/diff/54001/chrome/browser/ui/chrome_...
chrome/browser/ui/chrome_pages.cc:146: void ShowSlow(Browser* browser) {
nit: put this after ShowPolicy() so it's in the same order as defs in the
header.

[Zachary Kuznia, 2013-08-09 23:17:22]

https://codereview.chromium.org/22654003/diff/54001/chrome/browser/ui/chrome_...
File chrome/browser/ui/chrome_pages.cc (right):

https://codereview.chromium.org/22654003/diff/54001/chrome/browser/ui/chrome_...
chrome/browser/ui/chrome_pages.cc:146: void ShowSlow(Browser* browser) {
On 2013/08/09 22:42:12, Lei Zhang wrote:
> nit: put this after ShowPolicy() so it's in the same order as defs in the
> header.

Done.

[sadrul, 2013-08-09 23:26:26]

LGTM

[commit-bot: I haz the power, 2013-08-10 15:51:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zork@chromium.org/22654003/60001

[commit-bot: I haz the power, 2013-08-10 16:26:30]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_aura.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-08-10 19:54:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zork@chromium.org/22654003/78001

[commit-bot: I haz the power, 2013-08-10 20:29:53]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_aura.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-08-11 03:34:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zork@chromium.org/22654003/96001

[commit-bot: I haz the power, 2013-08-12 04:50:10]

Change committed as 216915