Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1414393003:
Rename LayoutTextFragment::m_end to m_length (Closed)
|
Created:
5 years, 2 months ago by Srirama Modified:
5 years, 1 month ago CC:
blink-reviews, blink-reviews-layout_chromium.org, chromium-reviews, eae+blinkwatch, jchaffraix+rendering, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, szager+layoutwatch_chromium.org, zoltan1 Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRename LayoutTextFragment::m_end to m_fragmentLength
BUG=545789
Committed: https://crrev.com/0ce9458242f0ee218df05102c71f3d443671369e
Cr-Commit-Position: refs/heads/master@{#356050}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Address comments #
Total comments: 4
Patch Set 3 : rename m_length to m_fragmentLength #Patch Set 4 : make fragmentLength() private #
Total comments: 1
Messages
Total messages: 20 (6 generated)
Description was changed from ========== fix BUG= ========== to ========== Rename LayoutTextFragment::m_end to m_length BUG=545789 ==========
srirama.m@samsung.com changed reviewers: + yosin@chromium.org
PTAL. There is a conflict with the function name "length" in LayoutText.h, so i made it fragLength.
yosin@chromium.org changed reviewers: + dsinclair@chromium.org
Thanks for working this! I've been confusing so far. +dsinclair@ in layout team. https://codereview.chromium.org/1414393003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutTextFragment.h (right): https://codereview.chromium.org/1414393003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutTextFragment.h:47: unsigned fragLength() const { return m_length; } Please use |fragmentLength()|, or another whcih Layout team suggested, rather than abbreviate name.
https://codereview.chromium.org/1414393003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutTextFragment.h (right): https://codereview.chromium.org/1414393003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutTextFragment.h:47: unsigned fragLength() const { return m_length; } On 2015/10/22 07:23:16, Yosi_UTC9 wrote: > Please use |fragmentLength()|, or another whcih Layout team suggested, rather > than abbreviate name. fragmentLength is fine.
Thanks for the review. PTAL. https://codereview.chromium.org/1414393003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutTextFragment.h (right): https://codereview.chromium.org/1414393003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutTextFragment.h:47: unsigned fragLength() const { return m_length; } On 2015/10/22 13:04:50, dsinclair wrote: > On 2015/10/22 07:23:16, Yosi_UTC9 wrote: > > Please use |fragmentLength()|, or another whcih Layout team suggested, rather > > than abbreviate name. > > > fragmentLength is fine. Done.
lgtm Since, I'm not OWNER of core/layout/, please wait OWNER's lgtm.
https://codereview.chromium.org/1414393003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutTextFragment.h (right): https://codereview.chromium.org/1414393003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTextFragment.h:47: unsigned fragmentLength() const { return m_length; } This should probably be m_fragmentLength to be consistent. https://codereview.chromium.org/1414393003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTextFragment.h:47: unsigned fragmentLength() const { return m_length; } Is this never called outside this class? Can we make it private?
https://codereview.chromium.org/1414393003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutTextFragment.h (right): https://codereview.chromium.org/1414393003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTextFragment.h:47: unsigned fragmentLength() const { return m_length; } On 2015/10/26 13:19:34, dsinclair wrote: > This should probably be m_fragmentLength to be consistent. Done. https://codereview.chromium.org/1414393003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTextFragment.h:47: unsigned fragmentLength() const { return m_length; } On 2015/10/26 13:19:34, dsinclair wrote: > Is this never called outside this class? Can we make it private? This is not called outside, though start() is used outside this class. So should we keep it here itself together with start() for better readability?
Description was changed from ========== Rename LayoutTextFragment::m_end to m_length BUG=545789 ========== to ========== Rename LayoutTextFragment::m_end to m_fragmentLength BUG=545789 ==========
On 2015/10/26 13:40:30, Srirama wrote:
> third_party/WebKit/Source/core/layout/LayoutTextFragment.h:47: unsigned
> fragmentLength() const { return m_length; }
> On 2015/10/26 13:19:34, dsinclair wrote:
> > Is this never called outside this class? Can we make it private?
>
> This is not called outside, though start() is used outside this class. So
should
> we keep it here itself together with start() for better readability?
Let's make it private to show it's used only by this class.
On 2015/10/26 13:41:16, dsinclair wrote:
> On 2015/10/26 13:40:30, Srirama wrote:
> > third_party/WebKit/Source/core/layout/LayoutTextFragment.h:47: unsigned
> > fragmentLength() const { return m_length; }
> > On 2015/10/26 13:19:34, dsinclair wrote:
> > > Is this never called outside this class? Can we make it private?
> >
> > This is not called outside, though start() is used outside this class. So
> should
> > we keep it here itself together with start() for better readability?
>
>
> Let's make it private to show it's used only by this class.
Done.
The CQ bit was checked by dsinclair@chromium.org
lgtm
The patchset sent to the CQ was uploaded after l-g-t-m from yosin@chromium.org Link to the patchset: https://codereview.chromium.org/1414393003/#ps60001 (title: "make fragmentLength() private")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1414393003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1414393003/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/0ce9458242f0ee218df05102c71f3d443671369e Cr-Commit-Position: refs/heads/master@{#356050}
Message was sent while issue was closed.
https://codereview.chromium.org/1414393003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutTextFragment.h (right): https://codereview.chromium.org/1414393003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTextFragment.h:83: unsigned fragmentLength() const { return m_fragmentLength; } Oops, I'm a first user of using |fragmentLength()|. http://crrev.com/1404423005 is an example. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
