Bug 82501 – [chromium] Add tracing events around CCLayerTreeHostImpl visibility.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Michal Mocny]

[chromium] Add tracing events around CCLayerTreeHostImpl visibility.

[Michal Mocny]

Created attachment 134363 [details]
Patch

[Michal Mocny]

Comment on attachment 134363 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=134363&action=review

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:109
> +    TRACE_EVENT_START1("webkit", "CCLayerTreeHostImpl visible", this, "CCLayerTreeHostImpl", this);

This is a repeated line -- perhaps the two TRACE events should be moved into an anonymous namespace helper function that gets called instead.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:479
> +    if (m_visible) {

The braces here are against style, but are NOT optional given how the following macro is written.  Gives compile errors without them.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:480
> +      TRACE_EVENT_START1("webkit", "CCLayerTreeHostImpl visible", this, "CCLayerTreeHostImpl", this);

I added an argument to identify the tree by address.  This is because of the way tracing information is laid out, it is hard to tell when which tree is visible.

[WebKit Review Bot]

Attachment 134363 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:481:  One line control clauses should not use braces.  [whitespace/braces] [4]
Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:483:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 2 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Michal Mocny]

Created attachment 134365 [details]
Patch

[Michal Mocny]

Comment on attachment 134365 [details]
Patch

Using helper functions solves both the code reuse issue and style violation.

[Nat Duca]

View in context: https://bugs.webkit.org/attachment.cgi?id=134363&action=review

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:109
>> +    TRACE_EVENT_START1("webkit", "CCLayerTreeHostImpl visible", this, "CCLayerTreeHostImpl", this);
> 
> This is a repeated line -- perhaps the two TRACE events should be moved into an anonymous namespace helper function that gets called instead.

Yeah, no clean way to do this...

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:479
>> +    if (m_visible) {
> 
> The braces here are against style, but are NOT optional given how the following macro is written.  Gives compile errors without them.

I just pinged jbates, /me hopes he can fix this. Lets workaround this by making a didVisibilityChange helper function for now...

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:480
>> +      TRACE_EVENT_START1("webkit", "CCLayerTreeHostImpl visible", this, "CCLayerTreeHostImpl", this);
> 
> I added an argument to identify the tree by address.  This is because of the way tracing information is laid out, it is hard to tell when which tree is visible.

Thats fine. Lets also say "CCLayerTreeHostImpl::Visible"

[Michal Mocny]

Comment on attachment 134365 [details]
Patch

minor patch incoming.

[Michal Mocny]

Created attachment 134371 [details]
Patch

[Michal Mocny]

Created attachment 134375 [details]
Patch

[Michal Mocny]

Comment on attachment 134375 [details]
Patch

Changed didVisibilityChange function to meet webkit style of replacing if {} else {} with if { ... return; } ...

[WebKit Review Bot]

Comment on attachment 134375 [details]
Patch

Clearing flags on attachment: 134375

Committed r112439: <http://trac.webkit.org/changeset/112439>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.