ui: Add debug flag to show composited layer borders in ChromeOS UI.

ui: Add debug flag to show composited layer borders in ChromeOS UI.

This adds an entry to chrome://flags that allows layer borders to be
enabled for Chrome OS UI.

It also makes the debug border setting a bitset so borders for individual
layer types can be enabled. This makes the debug flag more useful when
debugging a specific layer type (e.g. render surface layers).

BUG=678236
TEST=chrome --ui-show-composited-layer-borders=renderpass
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2795703002
Cr-Commit-Position: refs/heads/master@{#464640}
Committed: https://chromium.googlesource.com/chromium/src/+/5be07ac89aea29234eb908ec661881fb977bdd9b

[reveman, 2017-04-03 10:52:41]

Description was changed from

==========
ui: Add debug flag to show render surface borders in ChromeOS UI.

This is useful when debugging render surface usage by ChromeOS UI. It
provides the same functionality as --ui-show-layer-borders but is
limited to render surface layer borders, which makes it easier to use
and is also exposed on about:flags for easy access.

BUG=678236
TEST=chrome --ui-show-render-surface-borders
==========

to

==========
ui: Add debug flag to show render surface borders in ChromeOS UI.

This is useful when debugging render surface usage by ChromeOS UI. It
provides the same functionality as --ui-show-layer-borders but is
limited to render surface layer borders, which makes it easier to use
and is also exposed on about:flags for easy access.

BUG=678236
TEST=chrome --ui-show-render-surface-borders
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[reveman, 2017-04-05 13:38:11]

reveman@chromium.org changed reviewers:
+ danakj@chromium.org

[reveman, 2017-04-05 13:38:12]

Please take a look. Having an about flag for this that is limited to render
surface borders for Chrome OS UI is important for solving crbug.com/708518 and
preventing similar issues in the future.

[danakj, 2017-04-05 14:53:38]

Did you think about ways to just improve ui-show-layer-borders? It'd be nice to
have less flags, and the layerization in general is important too.

[danakj, 2017-04-05 14:55:28]

If you think filtering is mandatory, then maybe
--ui-show-layer-borders=renderpass? Then we could add other things like
renderpass,surface,layer.. idk what, and you could choose what to show, which
scales better than multiple flags.

[danakj, 2017-04-05 14:56:23]

Also, fwiw, I suggest using renderpass instead of render surface as terminology
always. RenderSurfaceImpl is the only instance of the latter name and its
possible it just goes away eventually.

[reveman, 2017-04-05 22:40:54]

Description was changed from

==========
ui: Add debug flag to show render surface borders in ChromeOS UI.

This is useful when debugging render surface usage by ChromeOS UI. It
provides the same functionality as --ui-show-layer-borders but is
limited to render surface layer borders, which makes it easier to use
and is also exposed on about:flags for easy access.

BUG=678236
TEST=chrome --ui-show-render-surface-borders
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
ui: Add debug flag to show composited layer borders in ChromeOS UI.

This adds an entry to chrome://flags that allows layer borders to be
enabled for Chrome OS UI.

It also makes the debug border setting a bitset so borders for individual
layer types can be enabled. This makes the debug flag more useful when
debugging a specific layer type (e.g. render surface layers).

BUG=678236
TEST=chrome --ui-show-composited-layer-borders=renderpass
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[reveman, 2017-04-05 22:55:11]

ptal

[danakj, 2017-04-13 15:25:51]

LG just want existing use of the flag to not change behaviour

https://codereview.chromium.org/2795703002/diff/20001/chrome/browser/about_fl...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/2795703002/diff/20001/chrome/browser/about_fl...
chrome/browser/about_flags.cc:443: cc::switches::kUIShowCompositedLayerBorders,
"renderpass"},
should we introduce constants for these parameters? (maybe not for the all ""
case tho)

https://codereview.chromium.org/2795703002/diff/20001/chrome/browser/about_fl...
chrome/browser/about_flags.cc:449: cc::switches::kUIShowCompositedLayerBorders,
"renderpass,surface,layer"}};
maybe just make this "" and empty => all?

https://codereview.chromium.org/2795703002/diff/20001/ui/compositor/composito...
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/2795703002/diff/20001/ui/compositor/composito...
ui/compositor/compositor.cc:112: if
(command_line->HasSwitch(cc::switches::kUIShowCompositedLayerBorders)) {
I'd like --ui-show-composited-layer-borders (with no =stuff) to do the same
thing as before, ie empty string means all. This makes the about:flags stuff a
bit simpler and harder to mess up if we add new ones, as we don't have to list
everything in the all case.

[reveman, 2017-04-13 17:00:45]

reveman@chromium.org changed reviewers:
+ derat@chromium.org, jbauman@chromium.org

[reveman, 2017-04-13 17:00:46]

ptal

+derat for ash/debug.cc
+jbauman for content/renderer/gpu/render_widget_compositor.cc

https://codereview.chromium.org/2795703002/diff/20001/chrome/browser/about_fl...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/2795703002/diff/20001/chrome/browser/about_fl...
chrome/browser/about_flags.cc:443: cc::switches::kUIShowCompositedLayerBorders,
"renderpass"},
On 2017/04/13 at 15:25:51, danakj (out sick) wrote:
> should we introduce constants for these parameters? (maybe not for the all ""
case tho)

Done.

https://codereview.chromium.org/2795703002/diff/20001/chrome/browser/about_fl...
chrome/browser/about_flags.cc:449: cc::switches::kUIShowCompositedLayerBorders,
"renderpass,surface,layer"}};
On 2017/04/13 at 15:25:51, danakj (out sick) wrote:
> maybe just make this "" and empty => all?

Done.

https://codereview.chromium.org/2795703002/diff/20001/ui/compositor/composito...
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/2795703002/diff/20001/ui/compositor/composito...
ui/compositor/compositor.cc:112: if
(command_line->HasSwitch(cc::switches::kUIShowCompositedLayerBorders)) {
On 2017/04/13 at 15:25:51, danakj (out sick) wrote:
> I'd like --ui-show-composited-layer-borders (with no =stuff) to do the same
thing as before, ie empty string means all. This makes the about:flags stuff a
bit simpler and harder to mess up if we add new ones, as we don't have to list
everything in the all case.

Done.

[reveman, 2017-04-13 17:00:51]

The CQ bit was checked by reveman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-13 17:02:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Daniel Erat, 2017-04-13 17:23:54]

//ash lgtm

[commit-bot: I haz the power, 2017-04-13 18:17:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-13 18:17:45]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[reveman, 2017-04-13 19:29:09]

reveman@chromium.org changed reviewers:
+ asvitkine@chromium.org

[reveman, 2017-04-13 19:29:09]

+asvitkine for tools/metrics/histograms/histograms.xml

[reveman, 2017-04-13 19:38:03]

The CQ bit was checked by reveman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-13 19:39:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Alexei Svitkine (slow), 2017-04-13 20:52:37]

lgtm

[commit-bot: I haz the power, 2017-04-13 21:07:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-13 21:07:17]

Dry run: This issue passed the CQ dry run.

[jbauman, 2017-04-13 21:09:36]

lgtm

[danakj, 2017-04-13 22:35:42]

LGTM

https://codereview.chromium.org/2795703002/diff/60001/cc/base/switches.h
File cc/base/switches.h (right):

https://codereview.chromium.org/2795703002/diff/60001/cc/base/switches.h#newc...
cc/base/switches.h:52: CC_BASE_EXPORT extern const char
kCompositedRenderpassBorders[];
nit: RenderPass is how its written elsewhere

https://codereview.chromium.org/2795703002/diff/60001/chrome/browser/flag_des...
File chrome/browser/flag_descriptions.h (right):

https://codereview.chromium.org/2795703002/diff/60001/chrome/browser/flag_des...
chrome/browser/flag_descriptions.h:836: // Description for the flag to show UI
composited renderpass layer borders.
i think renderpass layer isnt right, there's no layer involved, i'd just say
RenderPass borders.

https://codereview.chromium.org/2795703002/diff/60001/chrome/browser/flag_des...
chrome/browser/flag_descriptions.h:842: // Description for the flag to show UI
composited layer borders.
This is the same text as on kUiShowCompositedLayerBordersDescription, it would
be good to differentiate what this is vs that.

https://codereview.chromium.org/2795703002/diff/60001/ui/compositor/composito...
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/2795703002/diff/60001/ui/compositor/composito...
ui/compositor/compositor.cc:115: std::vector<std::string> entries =
base::SplitString(
base::SplitStringPiece is lighter, doesn't require allocing new strings for the
results, which won't outlive this scope anyway.

[reveman, 2017-04-13 23:32:46]

The CQ bit was checked by reveman@chromium.org

[reveman, 2017-04-13 23:32:47]

https://codereview.chromium.org/2795703002/diff/60001/cc/base/switches.h
File cc/base/switches.h (right):

https://codereview.chromium.org/2795703002/diff/60001/cc/base/switches.h#newc...
cc/base/switches.h:52: CC_BASE_EXPORT extern const char
kCompositedRenderpassBorders[];
On 2017/04/13 at 22:35:42, danakj (out sick) wrote:
> nit: RenderPass is how its written elsewhere

Done.

https://codereview.chromium.org/2795703002/diff/60001/chrome/browser/flag_des...
File chrome/browser/flag_descriptions.h (right):

https://codereview.chromium.org/2795703002/diff/60001/chrome/browser/flag_des...
chrome/browser/flag_descriptions.h:836: // Description for the flag to show UI
composited renderpass layer borders.
On 2017/04/13 at 22:35:42, danakj (out sick) wrote:
> i think renderpass layer isnt right, there's no layer involved, i'd just say
RenderPass borders.

Done.

https://codereview.chromium.org/2795703002/diff/60001/chrome/browser/flag_des...
chrome/browser/flag_descriptions.h:842: // Description for the flag to show UI
composited layer borders.
On 2017/04/13 at 22:35:42, danakj (out sick) wrote:
> This is the same text as on kUiShowCompositedLayerBordersDescription, it would
be good to differentiate what this is vs that.

Changed all these to:
"Description of the flag option..." so it's clear they describe an option and
not a flag.

https://codereview.chromium.org/2795703002/diff/60001/ui/compositor/composito...
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/2795703002/diff/60001/ui/compositor/composito...
ui/compositor/compositor.cc:115: std::vector<std::string> entries =
base::SplitString(
On 2017/04/13 at 22:35:42, danakj (out sick) wrote:
> base::SplitStringPiece is lighter, doesn't require allocing new strings for
the results, which won't outlive this scope anyway.

Done.

[reveman, 2017-04-13 23:32:47]

The patchset sent to the CQ was uploaded after l-g-t-m from derat@chromium.org,
danakj@chromium.org, asvitkine@chromium.org, jbauman@chromium.org
Link to the patchset: https://codereview.chromium.org/2795703002/#ps80001
(title: "nits")

[commit-bot: I haz the power, 2017-04-13 23:33:19]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-14 01:06:12]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1492126366128760,
"parent_rev": "fcfe426cf8f209b41779a43fca011a8c33c9176d", "commit_rev":
"5be07ac89aea29234eb908ec661881fb977bdd9b"}

[commit-bot: I haz the power, 2017-04-14 01:06:57]

Description was changed from

==========
ui: Add debug flag to show composited layer borders in ChromeOS UI.

This adds an entry to chrome://flags that allows layer borders to be
enabled for Chrome OS UI.

It also makes the debug border setting a bitset so borders for individual
layer types can be enabled. This makes the debug flag more useful when
debugging a specific layer type (e.g. render surface layers).

BUG=678236
TEST=chrome --ui-show-composited-layer-borders=renderpass
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
ui: Add debug flag to show composited layer borders in ChromeOS UI.

This adds an entry to chrome://flags that allows layer borders to be
enabled for Chrome OS UI.

It also makes the debug border setting a bitset so borders for individual
layer types can be enabled. This makes the debug flag more useful when
debugging a specific layer type (e.g. render surface layers).

BUG=678236
TEST=chrome --ui-show-composited-layer-borders=renderpass
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2795703002
Cr-Commit-Position: refs/heads/master@{#464640}
Committed:
https://chromium.googlesource.com/chromium/src/+/5be07ac89aea29234eb908ec6618...
==========

[commit-bot: I haz the power, 2017-04-14 01:06:59]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/5be07ac89aea29234eb908ec6618...

[ericrk, 2017-04-14 21:39:26]

On 2017/04/14 01:06:59, commit-bot: I haz the power wrote:
> Committed patchset #5 (id:80001) as
>
https://chromium.googlesource.com/chromium/src/+/5be07ac89aea29234eb908ec6618...

From a quick bisect, it appears that this change made it so the "layer borders"
button in Inspect > Rendering no longer works.

[reveman, 2017-04-15 00:16:20]

On 2017/04/14 at 21:39:26, ericrk wrote:
> On 2017/04/14 01:06:59, commit-bot: I haz the power wrote:
> > Committed patchset #5 (id:80001) as
> >
https://chromium.googlesource.com/chromium/src/+/5be07ac89aea29234eb908ec6618...
> 
> From a quick bisect, it appears that this change made it so the "layer
borders" button in Inspect > Rendering no longer works.

Thanks for the bisect. I'll make sure this is fixed asap. Please revert the
change if needed.