Rename enums/functions that collide in chromium style in core/layout/.

Rename enums/functions that collide in chromium style in core/layout/.

R=esprehn@chromium.org, pdr
TBR=haraken
BUG=582312
Committed: https://crrev.com/0b5c86b7b67ab235998e7dcddc00df65d833f87d
Cr-Commit-Position: refs/heads/master@{#378076}

[danakj, 2016-02-26 21:02:42]

https://codereview.chromium.org/1741073002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/line/InlineBox.h (right):

https://codereview.chromium.org/1741073002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/line/InlineBox.h:174: LineLayoutItem
getLineLayoutItem() const { return LineLayoutItem(&m_layoutObject); }
Did you know that this method is shadowed by subclasses, but isn't virtual?
Kinda shady!

I wasn't sure get is the right prefix here since it's constructing a
LineLayoutItem, but it's wrapping a member so it kinda is an accessor, just a
diff view of it.

[danakj, 2016-02-26 21:02:49]

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

[commit-bot: I haz the power, 2016-02-26 21:08:11]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1741073002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1741073002/1

[pdr., 2016-02-26 21:08:19]

pdr@chromium.org changed reviewers:
+ eae@chromium.org

[pdr., 2016-02-26 21:08:20]

https://codereview.chromium.org/1741073002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/line/InlineBox.h (right):

https://codereview.chromium.org/1741073002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/line/InlineBox.h:174: LineLayoutItem
getLineLayoutItem() const { return LineLayoutItem(&m_layoutObject); }
On 2016/02/26 at 21:02:42, danakj wrote:
> Did you know that this method is shadowed by subclasses, but isn't virtual?
Kinda shady!
> 
> I wasn't sure get is the right prefix here since it's constructing a
LineLayoutItem, but it's wrapping a member so it kinda is an accessor, just a
diff view of it.

I'd like to defer to Elliott (or eae) on this one. There's a large line layout
refactor going on and I'm not sure what they would like to do.

[eae, 2016-02-26 23:28:42]

https://codereview.chromium.org/1741073002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/line/InlineBox.h (right):

https://codereview.chromium.org/1741073002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/line/InlineBox.h:174: LineLayoutItem
getLineLayoutItem() const { return LineLayoutItem(&m_layoutObject); }
On 2016/02/26 21:02:42, danakj wrote:
> Did you know that this method is shadowed by subclasses, but isn't virtual?
> Kinda shady!

That's pretty shady, I seem to recall it being for performance reasons to
minimize the cost of going through the API. We should probably add an ASSERT to
ensure the right version is called though.

> I wasn't sure get is the right prefix here since it's constructing a
> LineLayoutItem, but it's wrapping a member so it kinda is an accessor, just a
> diff view of it.

get feels wrong but lineLayoutItemForLayoutObject is a bit of mouthful and
create feels even more wrong given that it's a wrapper.

What would it collide with that triggers the rename?

[danakj, 2016-02-26 23:30:04]

https://codereview.chromium.org/1741073002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/line/InlineBox.h (right):

https://codereview.chromium.org/1741073002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/line/InlineBox.h:174: LineLayoutItem
getLineLayoutItem() const { return LineLayoutItem(&m_layoutObject); }
On 2016/02/26 23:28:41, eae wrote:
> On 2016/02/26 21:02:42, danakj wrote:
> > Did you know that this method is shadowed by subclasses, but isn't virtual?
> > Kinda shady!
> 
> That's pretty shady, I seem to recall it being for performance reasons to
> minimize the cost of going through the API. We should probably add an ASSERT
to
> ensure the right version is called though.
> 
> > I wasn't sure get is the right prefix here since it's constructing a
> > LineLayoutItem, but it's wrapping a member so it kinda is an accessor, just
a
> > diff view of it.
> 
> get feels wrong but lineLayoutItemForLayoutObject is a bit of mouthful and
> create feels even more wrong given that it's a wrapper.
> 
> What would it collide with that triggers the rename?

The return type.

LineLayoutItem LineLayoutItem();  // ambiguous name

[esprehn, 2016-02-26 23:32:58]

On 2016/02/26 at 23:30:04, danakj wrote:
> ...
> 
> The return type.
> 
> LineLayoutItem LineLayoutItem();  // ambiguous name

Hopefully this method is temporary, eventually the line layout code shouldn't
use LayoutObject at all, it'll just use LineLayoutItem etc.

So I'd suggest GetLineLayoutItem()

[eae, 2016-02-26 23:33:52]

On 2016/02/26 23:32:58, esprehn wrote:
> On 2016/02/26 at 23:30:04, danakj wrote:
> > ...
> > 
> > The return type.
> > 
> > LineLayoutItem LineLayoutItem();  // ambiguous name
> 
> Hopefully this method is temporary, eventually the line layout code shouldn't
> use LayoutObject at all, it'll just use LineLayoutItem etc.
> 
> So I'd suggest GetLineLayoutItem()

Oh, ok. In that case GetLineLayoutItem() is the least bad option.

Thanks

[danakj, 2016-02-26 23:34:41]

Ok thanks, that's what's here then, anyone want to LG? :)

On Fri, Feb 26, 2016 at 3:33 PM, <eae@chromium.org> wrote:

> On 2016/02/26 23:32:58, esprehn wrote:
> > On 2016/02/26 at 23:30:04, danakj wrote:
> > > ...
> > >
> > > The return type.
> > >
> > > LineLayoutItem LineLayoutItem(); // ambiguous name
> >
> > Hopefully this method is temporary, eventually the line layout code
> shouldn't
> > use LayoutObject at all, it'll just use LineLayoutItem etc.
> >
> > So I'd suggest GetLineLayoutItem()
>
> Oh, ok. In that case GetLineLayoutItem() is the least bad option.
>
> Thanks
>
>
> https://codereview.chromium.org/1741073002/
>

-- 
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.

[esprehn, 2016-02-26 23:39:54]

lgtm

[danakj, 2016-02-26 23:41:05]

Ok thanks, that's what's here then, anyone want to LG? :)

On Fri, Feb 26, 2016 at 3:33 PM, <eae@chromium.org> wrote:

> On 2016/02/26 23:32:58, esprehn wrote:
> > On 2016/02/26 at 23:30:04, danakj wrote:
> > > ...
> > >
> > > The return type.
> > >
> > > LineLayoutItem LineLayoutItem(); // ambiguous name
> >
> > Hopefully this method is temporary, eventually the line layout code
> shouldn't
> > use LayoutObject at all, it'll just use LineLayoutItem etc.
> >
> > So I'd suggest GetLineLayoutItem()
>
> Oh, ok. In that case GetLineLayoutItem() is the least bad option.
>
> Thanks
>
>
> https://codereview.chromium.org/1741073002/
>

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

[danakj, 2016-02-26 23:42:24]

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

[danakj, 2016-02-26 23:42:25]

TBR=haraken for modules

[danakj, 2016-02-26 23:42:42]

Description was changed from

==========
Rename enums/functions that collide in chromium style in core/layout/.

R=esprehn@chromium.org, pdr
BUG=582312
==========

to

==========
Rename enums/functions that collide in chromium style in core/layout/.

R=esprehn@chromium.org, pdr
TBR=haraken
BUG=582312
==========

[danakj, 2016-02-26 23:42:55]

The CQ bit was unchecked by danakj@chromium.org

[danakj, 2016-02-26 23:43:13]

The CQ bit was checked by danakj@chromium.org

[eae, 2016-02-26 23:46:49]

LGTM

[commit-bot: I haz the power, 2016-02-26 23:48:13]

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

[commit-bot: I haz the power, 2016-02-26 23:48:15]

This CL has an open dependency (Issue 1740483004 Patch 80001). Please resolve
the dependency and try again.

[danakj, 2016-02-26 23:50:15]

The CQ bit was checked by danakj@chromium.org

[danakj, 2016-02-26 23:50:16]

The patchset sent to the CQ was uploaded after l-g-t-m from
esprehn@chromium.org, eae@chromium.org
Link to the patchset: https://codereview.chromium.org/1741073002/#ps20001
(title: "get-names-7: rebase")

[commit-bot: I haz the power, 2016-02-26 23:55:42]

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

[commit-bot: I haz the power, 2016-02-27 02:08:13]

Description was changed from

==========
Rename enums/functions that collide in chromium style in core/layout/.

R=esprehn@chromium.org, pdr
TBR=haraken
BUG=582312
==========

to

==========
Rename enums/functions that collide in chromium style in core/layout/.

R=esprehn@chromium.org, pdr
TBR=haraken
BUG=582312
==========

[commit-bot: I haz the power, 2016-02-27 02:08:16]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-02-27 02:09:58]

Description was changed from

==========
Rename enums/functions that collide in chromium style in core/layout/.

R=esprehn@chromium.org, pdr
TBR=haraken
BUG=582312
==========

to

==========
Rename enums/functions that collide in chromium style in core/layout/.

R=esprehn@chromium.org, pdr
TBR=haraken
BUG=582312

Committed: https://crrev.com/0b5c86b7b67ab235998e7dcddc00df65d833f87d
Cr-Commit-Position: refs/heads/master@{#378076}
==========

[commit-bot: I haz the power, 2016-02-27 02:10:00]

Patchset 2 (id:??) landed as
https://crrev.com/0b5c86b7b67ab235998e7dcddc00df65d833f87d
Cr-Commit-Position: refs/heads/master@{#378076}

[haraken, 2016-02-27 03:10:17]

LGTM