Dispose temporary and not-cached Range in DOMSelection

Dispose temporary and not-cached Range in DOMSelection

We can dispose temporary Range in Selection of shadow root because 
 it is neither cached nor exposed to js but we create it each time
 when we call getRangeAt().

BUG=693975

[yoichio, 2017-03-30 05:48:49]

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

[commit-bot: I haz the power, 2017-03-30 05:49:06]

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

[commit-bot: I haz the power, 2017-03-30 07:11:02]

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

[commit-bot: I haz the power, 2017-03-30 07:11:03]

Dry run: This issue passed the CQ dry run.

[yoichio, 2017-03-30 08:27:50]

Description was changed from

==========
Dispose temporary and not-cached Range in DOMSelection

BUG=693975
==========

to

==========
Dispose temporary and not-cached Range in DOMSelection

We can dispose temporary Range in Selection of shadow root because 
 it is neither cached nor exposed to js but we create it each time
 when we call getRangeAt().

BUG=693975
==========

[yoichio, 2017-03-30 08:27:50]

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

[tkent, 2017-03-30 08:53:09]

> // However, Selection of shadow root returns copied Range everytime so far

This is just a limitation of our current implementation, and we can fix it.

Anyway, can we resolve this issue by updating primaryRangeOrNull() like:

Range* DOMSelection::primaryRangeOrNull() const {
  if (rangeCount() <= 0)
    return nullptr;
  Range* range = getRangeAt(0, ASSERT_NO_EXCEPTION);
  if (!isSelectionOfDocument())
    range->dispose();
  return range;
}

?

[yosin_UTC9, 2017-03-30 08:58:17]

https://codereview.chromium.org/2783103002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/DOMSelection.cpp (right):

https://codereview.chromium.org/2783103002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/DOMSelection.cpp:128: class ScopedRange {
Please introduce a function acts like primaryRangeOrNull() but returns
EphemeralRange, e.g.

Optinal<EphemeralRange> compurePrimaryEphemeralRange().

We don't want to introduce |ScopedRange| anywhere. We discussed introducing it
on Jun 2014 when
[1] was filed.


[1] http://crbug.com/388681 Avoid registering temporary Range object for
updating on the Document

[yoichio, 2017-04-03 02:33:31]

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

[commit-bot: I haz the power, 2017-04-03 02:33:37]

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

[commit-bot: I haz the power, 2017-04-03 05:25:25]

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

[commit-bot: I haz the power, 2017-04-03 05:25:26]

Dry run: This issue passed the CQ dry run.

[yoichio, 2017-04-03 08:51:54]

> Range* DOMSelection::primaryRangeOrNull() const {
>   if (rangeCount() <= 0)
>     return nullptr;
>   Range* range = getRangeAt(0, ASSERT_NO_EXCEPTION);
>   if (!isSelectionOfDocument())
>     range->dispose();
>   return range;
> }
> 
Disposing inside primaryRangeOrNull looks better. Done.
PTAL

[yoichio, 2017-04-04 01:55:49]

PTAL

[yosin_UTC9, 2017-04-04 04:50:07]

https://codereview.chromium.org/2783103002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/DOMSelection.cpp (right):

https://codereview.chromium.org/2783103002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/DOMSelection.cpp:544: if
(!isSelectionOfDocument())
I'm not a fan of this approach. We still create temporary Range.
Disposing it doesn't make execution speed, but we still have temporary object
eating GC heap up.

[yoichio, 2017-04-04 05:32:42]

https://codereview.chromium.org/2783103002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/DOMSelection.cpp (right):

https://codereview.chromium.org/2783103002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/DOMSelection.cpp:544: if
(!isSelectionOfDocument())
On 2017/04/04 04:50:07, yosin_UTC9 wrote:
> I'm not a fan of this approach. We still create temporary Range.
> Disposing it doesn't make execution speed, but we still have temporary object
> eating GC heap up.
Creating temporary Ranges and Disposing them is different matter.
At least disposing Range from document makes tree modification 
 faster though temporary Range is in heap.

I will implement somehow there is cached Range for each TreeScope 
 later.

[yosin_UTC9, 2017-04-04 07:21:31]

https://codereview.chromium.org/2783103002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/DOMSelection.cpp (right):

https://codereview.chromium.org/2783103002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/DOMSelection.cpp:544: if
(!isSelectionOfDocument())
On 2017/04/04 at 05:32:42, yoichio wrote:
> On 2017/04/04 04:50:07, yosin_UTC9 wrote:
> > I'm not a fan of this approach. We still create temporary Range.
> > Disposing it doesn't make execution speed, but we still have temporary
object
> > eating GC heap up.
> Creating temporary Ranges and Disposing them is different matter.
> At least disposing Range from document makes tree modification 
>  faster though temporary Range is in heap.
> 
> I will implement somehow there is cached Range for each TreeScope 
>  later.

Calling Range::dispose() is error prone in this case and hacky.
Let's make clear code rather hacky code.

[tkent, 2017-04-04 08:41:24]

https://codereview.chromium.org/2783103002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/DOMSelection.cpp (right):

https://codereview.chromium.org/2783103002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/DOMSelection.cpp:544: if
(!isSelectionOfDocument())
> Calling Range::dispose() is error prone in this case and hacky.
> Let's make clear code rather hacky code.

I agree that it's a hack :)
If we have no reason to be hurry, we should avoid this approach.