Cleanup return values in DOM Range.

Cleanup return values in DOM Range.

This CL turns const by value return values into
const by ref.

Replacing const value by a const ref when a member
variable is safe, because the member variable will
live at least as long as the temporary object
returned now.
It can also be more efficient, because returning
a ref can spare copying.

BUG=568959

[JakeYoon, 2015-12-20 14:56:33]

Description was changed from

==========
Cleanup return values in DOM Range.

This CL turns const by value return values into
const by ref.

Replacing const value by a const ref when a member
variable is safe, because the member variable will
live at least as long as the temporary object
returned now.
It can also be more efficient, because returning
a ref can spare copying.

BUG=568959
==========

to

==========
Cleanup return values in DOM Range.

This CL turns const by value return values into
const by ref.

Replacing const value by a const ref when a member
variable is safe, because the member variable will
live at least as long as the temporary object
returned now.
It can also be more efficient, because returning
a ref can spare copying.

BUG=568959
==========

[JakeYoon, 2015-12-20 14:56:33]

yjaeseok@gmail.com changed reviewers:
+ haraken@chromium.org

[JakeYoon, 2015-12-20 14:57:41]

Hello Haraken,

Can you please take a look at this small cleanup?

Cheers,
Jaeseok Yoon.

[haraken, 2015-12-20 17:23:19]

https://codereview.chromium.org/1535403002/diff/1/AUTHORS
File AUTHORS (right):

https://codereview.chromium.org/1535403002/diff/1/AUTHORS#newcode247
AUTHORS:247: Jaeseok Yoon <yjaeseok@gmail.com>

Please make this change separately before landing this CL.

Also have you signed up a CLA?

[JakeYoon, 2015-12-20 21:02:57]

Hello, 

Separate AUTHORS!
And I already have signed up a CLA.
Thanks Haraken. :)

[haraken, 2015-12-21 01:03:27]

On 2015/12/20 21:02:57, JakeYoon wrote:
> Hello, 
> 
> Separate AUTHORS!
> And I already have signed up a CLA.
> Thanks Haraken. :)

Where is the AUTHORS CL? You need to land it first.

[JakeYoon, 2015-12-21 14:44:55]

On 2015/12/21 01:03:27, haraken wrote:
> On 2015/12/20 21:02:57, JakeYoon wrote:
> > Hello, 
> > 
> > Separate AUTHORS!
> > And I already have signed up a CLA.
> > Thanks Haraken. :)
> 
> Where is the AUTHORS CL? You need to land it first.

AUTHORS CL is https://codereview.chromium.org/1544533002
Thanks Haraken :)

[JakeYoon, 2015-12-22 05:32:03]

On 2015/12/21 14:44:55, JakeYoon wrote:
> On 2015/12/21 01:03:27, haraken wrote:
> > On 2015/12/20 21:02:57, JakeYoon wrote:
> > > Hello, 
> > > 
> > > Separate AUTHORS!
> > > And I already have signed up a CLA.
> > > Thanks Haraken. :)
> > 
> > Where is the AUTHORS CL? You need to land it first.
> 
> AUTHORS CL is https://codereview.chromium.org/1544533002
> Thanks Haraken :)

haraken@ Please review this issue.
And could you review https://codereview.chromium.org/1444173002 also?

Thanks :)

[haraken, 2015-12-22 06:45:37]

haraken@chromium.org changed reviewers:
+ yoichio@chromium.org, yosin@chromium.org

[haraken, 2015-12-22 06:45:38]

This change needs to be reviewed by yosin@ or yoichio@.

https://codereview.chromium.org/1535403002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Range.h (right):

https://codereview.chromium.org/1535403002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Range.h:117: const Position& endPosition()
const { return m_end.toPosition(); }

Why doesn't toPosition return const Position&?

[JakeYoon, 2015-12-30 14:25:08]

Hi yosin and yoichio!!

Why don't you review my first commit?!
Maybe it is your good opportunity.!!
On Dec 22, 2015 3:45 PM, <haraken@chromium.org> wrote:

> This change needs to be reviewed by yosin@ or yoichio@.
>
>
>
>
>
https://codereview.chromium.org/1535403002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/dom/Range.h (right):
>
>
>
https://codereview.chromium.org/1535403002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/dom/Range.h:117: const Position&
> endPosition() const { return m_end.toPosition(); }
>
> Why doesn't toPosition return const Position&?
>
> https://codereview.chromium.org/1535403002/
>

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

[JakeYoon, 2015-12-30 14:25:09]

Hi yosin and yoichio!!

Why don't you review my first commit?!
Maybe it is your good opportunity.!!
On Dec 22, 2015 3:45 PM, <haraken@chromium.org> wrote:

> This change needs to be reviewed by yosin@ or yoichio@.
>
>
>
>
>
https://codereview.chromium.org/1535403002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/dom/Range.h (right):
>
>
>
https://codereview.chromium.org/1535403002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/dom/Range.h:117: const Position&
> endPosition() const { return m_end.toPosition(); }
>
> Why doesn't toPosition return const Position&?
>
> https://codereview.chromium.org/1535403002/
>

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

[tfarina, 2015-12-30 14:39:38]

On Sun, Dec 20, 2015 at 11:03 PM, <haraken@chromium.org> wrote:

> On 2015/12/20 21:02:57, JakeYoon wrote:
>
>> Hello,
>>
>
> Separate AUTHORS!
>> And I already have signed up a CLA.
>> Thanks Haraken. :)
>>
>
> Where is the AUTHORS CL? You need to land it first.
>
> Why? I think this is rather very confusing. Why land a separate CL adding
his name to AUTHORS file before even getting the "real" CL in? This does
not make any sense.

-- 
Thiago Farina

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

[tfarina, 2015-12-30 14:39:38]

On Sun, Dec 20, 2015 at 11:03 PM, <haraken@chromium.org> wrote:

> On 2015/12/20 21:02:57, JakeYoon wrote:
>
>> Hello,
>>
>
> Separate AUTHORS!
>> And I already have signed up a CLA.
>> Thanks Haraken. :)
>>
>
> Where is the AUTHORS CL? You need to land it first.
>
> Why? I think this is rather very confusing. Why land a separate CL adding
his name to AUTHORS file before even getting the "real" CL in? This does
not make any sense.

-- 
Thiago Farina

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

[haraken, 2015-12-30 15:58:17]

On 2015/12/30 14:39:38, tfarina wrote:
> On Sun, Dec 20, 2015 at 11:03 PM, <mailto:haraken@chromium.org> wrote:
> 
> > On 2015/12/20 21:02:57, JakeYoon wrote:
> >
> >> Hello,
> >>
> >
> > Separate AUTHORS!
> >> And I already have signed up a CLA.
> >> Thanks Haraken. :)
> >>
> >
> > Where is the AUTHORS CL? You need to land it first.
> >
> > Why? I think this is rather very confusing. Why land a separate CL adding
> his name to AUTHORS file before even getting the "real" CL in? This does
> not make any sense.

I was thinking that we need to confirm the CLA and land the AUTHORS change
before landing any actual CL.

Is our convention to land the AUTHORS change in the first CL?

[tfarina, 2015-12-30 18:04:35]

On Wednesday, December 30, 2015, <haraken@chromium.org> wrote:

> On 2015/12/30 14:39:38, tfarina wrote:
>
>> On Sun, Dec 20, 2015 at 11:03 PM, <mailto:haraken@chromium.org> wrote:
>>
>
> > On 2015/12/20 21:02:57, JakeYoon wrote:
>> >
>> >> Hello,
>> >>
>> >
>> > Separate AUTHORS!
>> >> And I already have signed up a CLA.
>> >> Thanks Haraken. :)
>> >>
>> >
>> > Where is the AUTHORS CL? You need to land it first.
>> >
>> > Why? I think this is rather very confusing. Why land a separate CL
>> adding
>> his name to AUTHORS file before even getting the "real" CL in? This does
>> not make any sense.
>>
>
> I was thinking that we need to confirm the CLA and land the AUTHORS change
> before landing any actual CL.

How come this could be any good? Suppose his real patch does not get
approved by any means, then he (by him I just mean anyone contributing)
will be listed as one of the authors of the chromium project without having
any change checked in the source tree..

To me any change to the AUTHORS file should be done *after* real work has
been committed to the project, not before.


> Is our convention to land the AUTHORS change in the first CL?
>
> I don't see why not, that or after.

I think the signing of CLA is a separate process.


>
> https://codereview.chromium.org/1535403002/
>


-- 
Thiago Farina

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

[tfarina, 2015-12-30 18:04:36]

On Wednesday, December 30, 2015, <haraken@chromium.org> wrote:

> On 2015/12/30 14:39:38, tfarina wrote:
>
>> On Sun, Dec 20, 2015 at 11:03 PM, <mailto:haraken@chromium.org> wrote:
>>
>
> > On 2015/12/20 21:02:57, JakeYoon wrote:
>> >
>> >> Hello,
>> >>
>> >
>> > Separate AUTHORS!
>> >> And I already have signed up a CLA.
>> >> Thanks Haraken. :)
>> >>
>> >
>> > Where is the AUTHORS CL? You need to land it first.
>> >
>> > Why? I think this is rather very confusing. Why land a separate CL
>> adding
>> his name to AUTHORS file before even getting the "real" CL in? This does
>> not make any sense.
>>
>
> I was thinking that we need to confirm the CLA and land the AUTHORS change
> before landing any actual CL.

How come this could be any good? Suppose his real patch does not get
approved by any means, then he (by him I just mean anyone contributing)
will be listed as one of the authors of the chromium project without having
any change checked in the source tree..

To me any change to the AUTHORS file should be done *after* real work has
been committed to the project, not before.


> Is our convention to land the AUTHORS change in the first CL?
>
> I don't see why not, that or after.

I think the signing of CLA is a separate process.


>
> https://codereview.chromium.org/1535403002/
>


-- 
Thiago Farina

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

[yosin_UTC9, 2016-01-05 04:32:31]

Sorry for late response. I was on vacation during last December.

https://codereview.chromium.org/1535403002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Range.h (right):

https://codereview.chromium.org/1535403002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Range.h:116: const Position& startPosition()
const { return m_start.toPosition(); }
We can't return const ref here at least VS2013 and VS2015. I got following error
message:

d:\src\w\cr\src\third_party\webkit\source\core\dom\range.h(116) : warning C4172:
returning address of local variable or temporary
d:\src\w\cr\src\third_party\webkit\source\core\dom\range.h(117) : warning C4172:
returning address of local variable or temporary

Can clang/gcc compile this change w/o warnings/errors?

[JakeYoon, 2016-01-05 13:12:46]

https://codereview.chromium.org/1535403002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Range.h (right):

https://codereview.chromium.org/1535403002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Range.h:116: const Position& startPosition()
const { return m_start.toPosition(); }
On 2016/01/05 04:32:31, Yosi_UTC9 wrote:
> We can't return const ref here at least VS2013 and VS2015. I got following
error
> message:
> 
> d:\src\w\cr\src\third_party\webkit\source\core\dom\range.h(116) : warning
C4172:
> returning address of local variable or temporary
> d:\src\w\cr\src\third_party\webkit\source\core\dom\range.h(117) : warning
C4172:
> returning address of local variable or temporary
> 
> Can clang/gcc compile this change w/o warnings/errors?
> 

In fact, This patch is for replacing const value 
by a const ref when a member.

But startPosition function doesn't return
a member variable, So it looks like incorrect.
How do you think Yoshi@ ??

[yosin_UTC9, 2016-01-06 01:25:12]

On 2016/01/05 at 13:12:46, yjaeseok wrote:
>
https://codereview.chromium.org/1535403002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/dom/Range.h (right):
> 
>
https://codereview.chromium.org/1535403002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/dom/Range.h:116: const Position&
startPosition() const { return m_start.toPosition(); }
> On 2016/01/05 04:32:31, Yosi_UTC9 wrote:
> > We can't return const ref here at least VS2013 and VS2015. I got following
error
> > message:
> > 
> > d:\src\w\cr\src\third_party\webkit\source\core\dom\range.h(116) : warning
C4172:
> > returning address of local variable or temporary
> > d:\src\w\cr\src\third_party\webkit\source\core\dom\range.h(117) : warning
C4172:
> > returning address of local variable or temporary
> > 
> > Can clang/gcc compile this change w/o warnings/errors?
> > 
> 
> In fact, This patch is for replacing const value 
> by a const ref when a member.
> 
> But startPosition function doesn't return
> a member variable, So it looks like incorrect.
> How do you think Yoshi@ ??

start and end positions are calculated from RangeBoundaryPoint which holds
before
node, container node and "cached" offset for ease of relocation and avoiding
offset computation. We don't want to change it to Position object.

Returning |const Position| is for note that Position object immutable. There are
no member functions for mutating member variables in Position.