Use private helpers in LayerImpl to shorten property tree references.

Use private helpers in LayerImpl to shorten property tree references.

Improves code readability, sharing, and future maintenance. I don't
expect a performance impact but if we do see something material we
could inline.
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

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

[wkorman, 2017-01-06 03:29:52]

Description was changed from

==========
Use private helpers in LayerImpl to shorten property tree references.

Improves code readability, sharing, and future maintenance. I don't
expect a performance impact but if we do see something material we
could inline.
==========

to

==========
Use private helpers in LayerImpl to shorten property tree references.

Improves code readability, sharing, and future maintenance. I don't
expect a performance impact but if we do see something material we
could inline.
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[wkorman, 2017-01-06 03:30:35]

wkorman@chromium.org changed reviewers:
+ ajuma@chromium.org, chrishtr@chromium.org

[wkorman, 2017-01-06 03:31:19]

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

[commit-bot: I haz the power, 2017-01-06 03:31:51]

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

[wkorman, 2017-01-06 03:41:02]

https://codereview.chromium.org/2611253003/diff/1/cc/layers/layer_impl.h
File cc/layers/layer_impl.h (right):

https://codereview.chromium.org/2611253003/diff/1/cc/layers/layer_impl.h#newc...
cc/layers/layer_impl.h:549: PropertyTrees* GetPropertyTrees() const;
I named these as GetXxx() since not-infrequently one would otherwise have an
issue with masking for example:

PropertyTrees* property_trees = property_trees();
// subsequently multiple refs to property_trees the variable

will require revision to use this->property_trees() on the RHS.

Also it's my understanding the lowercase-underscore names are only intended for
trivial getters and in this case we're doing some non-trivial dereferencing.

LayerImpl::GetElementTypeForAnimation() is a recent example that follows this
naming style.

Counterpoints are LayerImpl::ScrollOffsetForAnimation() and
LayerImpl::CurrentScrollOffset() which are camel case but don't have Get prefix.

YMMV. News at 11? Vote now!

[commit-bot: I haz the power, 2017-01-06 04:23:49]

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

[commit-bot: I haz the power, 2017-01-06 04:23:50]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[ajuma, 2017-01-06 15:11:14]

lgtm

[wkorman, 2017-01-06 17:37:32]

https://codereview.chromium.org/2611253003/diff/1/cc/layers/layer_impl.h
File cc/layers/layer_impl.h (right):

https://codereview.chromium.org/2611253003/diff/1/cc/layers/layer_impl.h#newc...
cc/layers/layer_impl.h:549: PropertyTrees* GetPropertyTrees() const;
On 2017/01/06 at 03:41:02, wkorman wrote:
> I named these as GetXxx() since not-infrequently one would otherwise have an
issue with masking for example:

On reflection I'd like to rename these to remove the Get prefix but keep the
camel-case. That seems more common. Does this sound ok?

[danakj, 2017-01-06 17:50:05]

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

[danakj, 2017-01-06 17:50:06]

https://codereview.chromium.org/2611253003/diff/1/cc/layers/layer_impl.h
File cc/layers/layer_impl.h (right):

https://codereview.chromium.org/2611253003/diff/1/cc/layers/layer_impl.h#newc...
cc/layers/layer_impl.h:549: PropertyTrees* GetPropertyTrees() const;
On 2017/01/06 17:37:32, wkorman wrote:
> On 2017/01/06 at 03:41:02, wkorman wrote:
> > I named these as GetXxx() since not-infrequently one would otherwise have an
> issue with masking for example:
> 
> On reflection I'd like to rename these to remove the Get prefix but keep the
> camel-case. That seems more common. Does this sound ok?

That will collide with the type name - and functions should be verbs and Get
prefix is the standard way to write that for accessors that are not hacker_case
(unless you're proposing that then ignore me).

[wkorman, 2017-01-06 17:56:05]

On 2017/01/06 at 17:50:06, danakj wrote:
> > On reflection I'd like to rename these to remove the Get prefix but keep the
> > camel-case. That seems more common. Does this sound ok?
> 
> That will collide with the type name - and functions should be verbs and Get
prefix is the standard way to write that for accessors that are not hacker_case
(unless you're proposing that then ignore me).

Good point, did not think of that. Yeah, I'm trying to fit with current style
thinking and the existing code seems inconsistent. The most recent examples I
saw that made me think to remove Get -- LayerImpl::ScrollOffsetForAnimation()
and LayerImpl::CurrentScrollOffset().

I initially had hacker_case but that masks/shadows ~5 cases in existing code,
see #6. I'll land it as is.

[wkorman, 2017-01-06 18:02:11]

The CQ bit was checked by wkorman@chromium.org

[commit-bot: I haz the power, 2017-01-06 18:02:41]

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

[danakj, 2017-01-06 18:30:40]

On Fri, Jan 6, 2017 at 12:56 PM, <wkorman@chromium.org> wrote:

> On 2017/01/06 at 17:50:06, danakj wrote:
> > > On reflection I'd like to rename these to remove the Get prefix but
> keep the
> > > camel-case. That seems more common. Does this sound ok?
> >
> > That will collide with the type name - and functions should be verbs and
> Get
> prefix is the standard way to write that for accessors that are not
> hacker_case
> (unless you're proposing that then ignore me).
>
> Good point, did not think of that. Yeah, I'm trying to fit with current
> style
> thinking and the existing code seems inconsistent. The most recent
> examples I
> saw that made me think to remove Get -- LayerImpl::
> ScrollOffsetForAnimation()
> and LayerImpl::CurrentScrollOffset().
>

Compositor is esp inconsistent about this cuz it comes from WebKit where
Get was not allowed, but chromium in general uses Get.


>
> I initially had hacker_case but that masks/shadows ~5 cases in existing
> code,
> see #6. I'll land it as is.
>
> https://codereview.chromium.org/2611253003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2017-01-06 18:41:29]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1483725731525250, "parent_rev":
"39aabd2e6f55099ec252ee0fa12f51549e4b011c", "commit_rev":
"5a126a5c512ebfc6fcf3361b56b601266ca06f50"}

[commit-bot: I haz the power, 2017-01-06 18:42:01]

Description was changed from

==========
Use private helpers in LayerImpl to shorten property tree references.

Improves code readability, sharing, and future maintenance. I don't
expect a performance impact but if we do see something material we
could inline.
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Use private helpers in LayerImpl to shorten property tree references.

Improves code readability, sharing, and future maintenance. I don't
expect a performance impact but if we do see something material we
could inline.
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

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

[commit-bot: I haz the power, 2017-01-06 18:42:02]

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