Make setBaseAndExtent's arguments non-optional

Make setBaseAndExtent's arguments non-optional

These arguments are already non-optional in Edge. Firefox doesn't
support setBaseAndExtent. In WebKit the arguments are still optional.

For the first three arguments, the risk is bounded by the
SelectionSetBaseAndExtentNull use counter, which rbyers@ reports as ~0%
on the stable channel. If the fourth argument is omitted the use counter
wouldn't catch that, however. (The use counter can also be triggered by
explicitly passing null, which is likely the most common case.)

BUG=460722
Committed: https://crrev.com/314d573dc2b6818487d403462c72476b8ba046ea
Cr-Commit-Position: refs/heads/master@{#388460}

[philipj_slow, 2016-03-10 16:00:12]

Description was changed from

==========
Make setBaseAndExtent's arguments non-optional

BUG=
==========

to

==========
Make setBaseAndExtent's arguments non-optional

BUG=460722
==========

[philipj_slow, 2016-03-10 16:01:28]

Description was changed from

==========
Make setBaseAndExtent's arguments non-optional

BUG=460722
==========

to

==========
Make setBaseAndExtent's arguments non-optional

These arguments are already non-optional in Edge. Firefox doesn't
support setBaseAndExtent. In WebKit the arguments are still optional.

BUG=460722
==========

[philipj_slow, 2016-03-11 10:43:46]

Description was changed from

==========
Make setBaseAndExtent's arguments non-optional

These arguments are already non-optional in Edge. Firefox doesn't
support setBaseAndExtent. In WebKit the arguments are still optional.

BUG=460722
==========

to

==========
Make setBaseAndExtent's arguments non-optional

These arguments are already non-optional in Edge. Firefox doesn't
support setBaseAndExtent. In WebKit the arguments are still optional.

For the first three arguments, the risk is bounded by the
SelectionSetBaseAndExtentNull use counter, which rbyers@ reports as ~0%
on the stable channel. If the fourth argument is omitted the use counter
wouldn't catch that, however. (The use counter can also be triggered by
explicitly passing null, which is likely the most common case.)

BUG=460722
==========

[philipj_slow, 2016-03-11 10:44:22]

philipj@opera.com changed reviewers:
+ rbyers@chromium.org, yoichio@chromium.org

[philipj_slow, 2016-03-11 10:44:22]

yoichio@, PTAL.

rbyers@, there is some compat risk here, but it seems worth trying to me, please
sanity check :)

[Rick Byers, 2016-03-11 15:12:46]

On 2016/03/11 10:44:22, philipj_UTC7 wrote:
> yoichio@, PTAL.
> 
> rbyers@, there is some compat risk here, but it seems worth trying to me,
please
> sanity check :)

Between the UMA and Edge status, I'd say the compat risk seems low enough to me
here that I'd consider this a bugfix, so no need for an intent.  Plus the
failure mode is very self explanatory so no need for a chromestatus entry, blog
post, etc.

Of course please re-evaluate if you find examples of sites broken in the wild. 
I'm actually kind of surprised how low the UMA counter is given how easy it
could be (as is clear from our tests) to forget to supply some arguments.

[Rick Byers, 2016-03-11 15:14:02]

Might as well give my LGTM here, but please wait for yoichio@ to comment.

[philipj_slow, 2016-03-11 18:55:58]

On 2016/03/11 15:12:46, Rick Byers wrote:
> On 2016/03/11 10:44:22, philipj_UTC7 wrote:
> > yoichio@, PTAL.
> > 
> > rbyers@, there is some compat risk here, but it seems worth trying to me,
> please
> > sanity check :)
> 
> Between the UMA and Edge status, I'd say the compat risk seems low enough to
me
> here that I'd consider this a bugfix, so no need for an intent.  Plus the
> failure mode is very self explanatory so no need for a chromestatus entry,
blog
> post, etc.
> 
> Of course please re-evaluate if you find examples of sites broken in the wild.

> I'm actually kind of surprised how low the UMA counter is given how easy it
> could be (as is clear from our tests) to forget to supply some arguments.

I'm also quite surprised.
https://www.chromestatus.com/metrics/feature/timeline/popularity/406 is around
0.02%, so the proportion of uses that have <4 arguments must be <0.5% for
https://www.chromestatus.com/metrics/feature/timeline/popularity/1084 to round
to zero.

Sure would be nice to have chromestatus.com back to increase confidence that
we're comparing the right values :)

[philipj_slow, 2016-04-18 22:23:19]

Ping yoichio@

[yoichio, 2016-04-19 07:21:16]

https://codereview.chromium.org/1785663002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/editing/execCommand/apply-inline-style-to-element-with-no-renderer-crash.html
(left):

https://codereview.chromium.org/1785663002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/editing/execCommand/apply-inline-style-to-element-with-no-renderer-crash.html:28:
window.getSelection().setBaseAndExtent(input, 4);
setBaseAndExtent(node, offset) should be collapse(node, offset).

[philipj_slow, 2016-04-19 12:34:32]

I've switched from setBaseAndExtent to collapse in
fast/css-generated-content/crash-selection-editing-removes-pseudo.html, the only
test where switching didn't break the test.

https://codereview.chromium.org/1785663002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/editing/execCommand/apply-inline-style-to-element-with-no-renderer-crash.html
(left):

https://codereview.chromium.org/1785663002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/editing/execCommand/apply-inline-style-to-element-with-no-renderer-crash.html:28:
window.getSelection().setBaseAndExtent(input, 4);
On 2016/04/19 07:21:16, yoichio wrote:
> setBaseAndExtent(node, offset) should be collapse(node, offset).

If this particular instance is replaced with collapse(input, 4), throws "Failed
to execute 'collapse' on 'Selection': There is no child at offset 4."

Per spec both collapse and setBaseAndExtent end up setting a range and so there
should be some equivalence, but as actually implemented it doesn't look like
they do the same thing, as setBaseAndExtent takes a rather different path.

I could check what the selection was after this call originally and try to make
it the same using collapse, but is it anchorNode+focusNode or
baseNode+extentNode or all of them that I should try to keep the same?

[yoichio, 2016-04-20 01:32:00]

On 2016/04/19 12:34:32, philipj_slow wrote:
> I've switched from setBaseAndExtent to collapse in
> fast/css-generated-content/crash-selection-editing-removes-pseudo.html, the
only
> test where switching didn't break the test.
> 
>
https://codereview.chromium.org/1785663002/diff/40001/third_party/WebKit/Layo...
> File
>
third_party/WebKit/LayoutTests/editing/execCommand/apply-inline-style-to-element-with-no-renderer-crash.html
> (left):
> 
>
https://codereview.chromium.org/1785663002/diff/40001/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/editing/execCommand/apply-inline-style-to-element-with-no-renderer-crash.html:28:
> window.getSelection().setBaseAndExtent(input, 4);
> On 2016/04/19 07:21:16, yoichio wrote:
> > setBaseAndExtent(node, offset) should be collapse(node, offset).
> 
> If this particular instance is replaced with collapse(input, 4), throws
"Failed
> to execute 'collapse' on 'Selection': There is no child at offset 4."
> 
> Per spec both collapse and setBaseAndExtent end up setting a range and so
there
> should be some equivalence, but as actually implemented it doesn't look like
> they do the same thing, as setBaseAndExtent takes a rather different path.
> 
> I could check what the selection was after this call originally and try to
make
> it the same using collapse, but is it anchorNode+focusNode or
> baseNode+extentNode or all of them that I should try to keep the same?
I understand the problem and it is a kind of spec issue.
Now we can't abbreviate any arguments of setBaseAndExtent so
 compatibility between setBaseAndExtent(node, offset) and collapse(node, offset)
 wont't happen.

lgtm to this change.

[philipj_slow, 2016-04-20 07:56:18]

The CQ bit was checked by philipj@opera.com

[philipj_slow, 2016-04-20 07:56:19]

The patchset sent to the CQ was uploaded after l-g-t-m from rbyers@chromium.org
Link to the patchset: https://codereview.chromium.org/1785663002/#ps60001
(title: "use collapse() where possible (one test)")

[commit-bot: I haz the power, 2016-04-20 07:56:26]

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

[commit-bot: I haz the power, 2016-04-20 09:27:52]

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

[commit-bot: I haz the power, 2016-04-20 09:27:53]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[philipj_slow, 2016-04-20 09:29:40]

The CQ bit was checked by philipj@opera.com

[commit-bot: I haz the power, 2016-04-20 09:29:52]

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

[commit-bot: I haz the power, 2016-04-20 10:16:15]

Description was changed from

==========
Make setBaseAndExtent's arguments non-optional

These arguments are already non-optional in Edge. Firefox doesn't
support setBaseAndExtent. In WebKit the arguments are still optional.

For the first three arguments, the risk is bounded by the
SelectionSetBaseAndExtentNull use counter, which rbyers@ reports as ~0%
on the stable channel. If the fourth argument is omitted the use counter
wouldn't catch that, however. (The use counter can also be triggered by
explicitly passing null, which is likely the most common case.)

BUG=460722
==========

to

==========
Make setBaseAndExtent's arguments non-optional

These arguments are already non-optional in Edge. Firefox doesn't
support setBaseAndExtent. In WebKit the arguments are still optional.

For the first three arguments, the risk is bounded by the
SelectionSetBaseAndExtentNull use counter, which rbyers@ reports as ~0%
on the stable channel. If the fourth argument is omitted the use counter
wouldn't catch that, however. (The use counter can also be triggered by
explicitly passing null, which is likely the most common case.)

BUG=460722
==========

[commit-bot: I haz the power, 2016-04-20 10:16:16]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-04-22 19:22:09]

Description was changed from

==========
Make setBaseAndExtent's arguments non-optional

These arguments are already non-optional in Edge. Firefox doesn't
support setBaseAndExtent. In WebKit the arguments are still optional.

For the first three arguments, the risk is bounded by the
SelectionSetBaseAndExtentNull use counter, which rbyers@ reports as ~0%
on the stable channel. If the fourth argument is omitted the use counter
wouldn't catch that, however. (The use counter can also be triggered by
explicitly passing null, which is likely the most common case.)

BUG=460722
==========

to

==========
Make setBaseAndExtent's arguments non-optional

These arguments are already non-optional in Edge. Firefox doesn't
support setBaseAndExtent. In WebKit the arguments are still optional.

For the first three arguments, the risk is bounded by the
SelectionSetBaseAndExtentNull use counter, which rbyers@ reports as ~0%
on the stable channel. If the fourth argument is omitted the use counter
wouldn't catch that, however. (The use counter can also be triggered by
explicitly passing null, which is likely the most common case.)

BUG=460722

Committed: https://crrev.com/314d573dc2b6818487d403462c72476b8ba046ea
Cr-Commit-Position: refs/heads/master@{#388460}
==========

[commit-bot: I haz the power, 2016-04-22 19:22:11]

Patchset 4 (id:??) landed as
https://crrev.com/314d573dc2b6818487d403462c72476b8ba046ea
Cr-Commit-Position: refs/heads/master@{#388460}