Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2466833002:
selection.setBaseAndExtent with null should not crash. (Closed)
|
Created:
4 years, 1 month ago by yoichio Modified:
4 years ago Reviewers:
yosin_UTC9 CC:
blink-reviews, chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionselection.setBaseAndExtent with null should not crash.
setBaseAndExtent is non-nullable in spec:
http://w3c.github.io/selection-api/#widl-Selection-setBaseAndExtent-void-Node-
anchorNode-unsigned-long-anchorOffset-Node-focusNode-unsigned-long-focusOffset
However, there is the issue for that:
https://github.com/w3c/selection-api/issues/72
Edge clears selection with null node and Blink virtually does same.
'Virtually' means we go though false DCHECK statements in the release build.
This CL refines code path both on debug and release.
Besides exact behavior, selection.setBaseAndExtent(null,x,null,y) should not crash
and this CL checks it in DOMSelection.
This CL makes selection.setBaseAndExtent(null,x,null,y) clear Selection
and keeps selection.setBaseAndExtent(base,x,null,y) as selection.collpase(base,x)
except x can over base's length.
BUG=660848
Committed: https://crrev.com/ebec09cdbe2965a07c54715cf4ba3b50603efe39
Cr-Commit-Position: refs/heads/master@{#435915}
Patch Set 1 #Patch Set 2 : update #
Total comments: 3
Patch Set 3 : update #
Total comments: 1
Patch Set 4 : update #
Messages
Total messages: 49 (38 generated)
The CQ bit was checked by yoichio@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== selection.setBaseAndExtent with null should clear BUG=660848 ========== to ========== selection.setBaseAndExtent with null should not crash. setBaseAndExtent is non-nullable in spec: http://w3c.github.io/selection-api/#widl-Selection-setBaseAndExtent-void-Node- anchorNode-unsigned-long-anchorOffset-Node-focusNode-unsigned-long-focusOffset However, there is the issue for that: https://github.com/w3c/selection-api/issues/72 Edge clears selection with null node and Blink virtually does same. 'Virtually' means we go though false DCHECK statements in the release build. This CL refines code path both on debug and release. BUG=660848 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by yoichio@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by yoichio@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== selection.setBaseAndExtent with null should not crash. setBaseAndExtent is non-nullable in spec: http://w3c.github.io/selection-api/#widl-Selection-setBaseAndExtent-void-Node- anchorNode-unsigned-long-anchorOffset-Node-focusNode-unsigned-long-focusOffset However, there is the issue for that: https://github.com/w3c/selection-api/issues/72 Edge clears selection with null node and Blink virtually does same. 'Virtually' means we go though false DCHECK statements in the release build. This CL refines code path both on debug and release. BUG=660848 ========== to ========== selection.setBaseAndExtent with null should not crash. setBaseAndExtent is non-nullable in spec: http://w3c.github.io/selection-api/#widl-Selection-setBaseAndExtent-void-Node- anchorNode-unsigned-long-anchorOffset-Node-focusNode-unsigned-long-focusOffset However, there is the issue for that: https://github.com/w3c/selection-api/issues/72 Edge clears selection with null node and Blink virtually does same. 'Virtually' means we go though false DCHECK statements in the release build. This CL refines code path both on debug and release. Besides exact behavior, selection.setBaseAndExtent(null,x,null,y) should not crash and this CL checks it in DOMSelection. BUG=660848 ==========
yoichio@chromium.org changed reviewers: + yosin@chromium.org
Since this patch changes API behavior, Before this patch: setBaseAndExtent(a,1, null, 0) == collapse(a,1) After this patch: setBaseAndExtent(a,1, null, 0) == removeAllRanges() So, we should have "Intent to Ship" to get approval from API owner. https://codereview.chromium.org/2466833002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/editing/selection/set_base_and_extent/set_null_to_clear.html (right): https://codereview.chromium.org/2466833002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/selection/set_base_and_extent/set_null_to_clear.html:25: const div = selection.document.getElementById('div'); For simplicity, it is better to use querySelector('div') to avoid having "id="div"" https://codereview.chromium.org/2466833002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/selection/set_base_and_extent/set_null_to_clear.html:26: assert_true(div != null); nit: assert_not_equals(div, null) https://codereview.chromium.org/2466833002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/selection/set_base_and_extent/set_null_to_clear.html:35: assert_true(div != null); nit: assert_not_equals(div, null)
The CQ bit was checked by yoichio@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by yoichio@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by yoichio@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by yoichio@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #3 (id:40001) has been deleted
Description was changed from ========== selection.setBaseAndExtent with null should not crash. setBaseAndExtent is non-nullable in spec: http://w3c.github.io/selection-api/#widl-Selection-setBaseAndExtent-void-Node- anchorNode-unsigned-long-anchorOffset-Node-focusNode-unsigned-long-focusOffset However, there is the issue for that: https://github.com/w3c/selection-api/issues/72 Edge clears selection with null node and Blink virtually does same. 'Virtually' means we go though false DCHECK statements in the release build. This CL refines code path both on debug and release. Besides exact behavior, selection.setBaseAndExtent(null,x,null,y) should not crash and this CL checks it in DOMSelection. BUG=660848 ========== to ========== selection.setBaseAndExtent with null should not crash. setBaseAndExtent is non-nullable in spec: http://w3c.github.io/selection-api/#widl-Selection-setBaseAndExtent-void-Node- anchorNode-unsigned-long-anchorOffset-Node-focusNode-unsigned-long-focusOffset However, there is the issue for that: https://github.com/w3c/selection-api/issues/72 Edge clears selection with null node and Blink virtually does same. 'Virtually' means we go though false DCHECK statements in the release build. This CL refines code path both on debug and release. Besides exact behavior, selection.setBaseAndExtent(null,x,null,y) should not crash and this CL checks it in DOMSelection. This CL makes selection.setBaseAndExtent(null,x,null,y) clear Selection and keeps selection.setBaseAndExtent(base,x,null,y) as selection.collpase(base,x) except x can over base's length. BUG=660848 ==========
On 2016/11/29 09:58:32, Yosi_UTC9 wrote: > Since this patch changes API behavior, > I changed the CL to keep behavior: setBaseAndExtent(a,x, null, 0) == collapse(a,x) except x can over base's length.
https://codereview.chromium.org/2466833002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/DOMSelection.cpp (right): https://codereview.chromium.org/2466833002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/DOMSelection.cpp:279: if (!baseNode) { It seems that dealing nullptr explicitly is more readable. if (!baseNode) { UseCounter::count(frame(), UseCounter::SelectionSetBaseAndExtentNull); frame->selection().clear(); return; } if (!extendNode) { UseCounter::count(frame(), UseCounter::SelectionSetBaseAndExtentNull); collapse(baseNode, baseOffset, exceptionState); return; }
On 2016/12/02 04:38:02, Yosi_UTC9 wrote:
>
> if (!extendNode) {
> UseCounter::count(frame(), UseCounter::SelectionSetBaseAndExtentNull);
> collapse(baseNode, baseOffset, exceptionState);
> return;
> }
No. That wont work because SetBaseAndExtent(base, offset) can be called
with offset over base.length as described.
On 2016/12/02 at 04:53:33, yoichio wrote:
> On 2016/12/02 04:38:02, Yosi_UTC9 wrote:
> >
> > if (!extendNode) {
> > UseCounter::count(frame(), UseCounter::SelectionSetBaseAndExtentNull);
> > collapse(baseNode, baseOffset, exceptionState);
> > return;
> > }
>
> No. That wont work because SetBaseAndExtent(base, offset) can be called
> with offset over base.length as described.
OK, what about below?
if (!baseNode) {
UseCounter::count(frame(), UseCounter::SelectionSetBaseAndExtentNull);
frame->selection().clear();
return;
}
if (!extendNode) {
UseCounter::count(frame(), UseCounter::SelectionSetBaseAndExtentNull);
extendOffset = 0;
}
We dont' want to check |baseNode| twice.
The CQ bit was checked by yoichio@chromium.org to run a CQ dry run
On 2016/12/02 04:59:40, Yosi_UTC9 wrote:
> On 2016/12/02 at 04:53:33, yoichio wrote:
> > On 2016/12/02 04:38:02, Yosi_UTC9 wrote:
> > >
> > > if (!extendNode) {
> > > UseCounter::count(frame(), UseCounter::SelectionSetBaseAndExtentNull);
> > > collapse(baseNode, baseOffset, exceptionState);
> > > return;
> > > }
> >
> > No. That wont work because SetBaseAndExtent(base, offset) can be called
> > with offset over base.length as described.
>
> OK, what about below?
>
>
> if (!baseNode) {
> UseCounter::count(frame(), UseCounter::SelectionSetBaseAndExtentNull);
> frame->selection().clear();
> return;
> }
>
> if (!extendNode) {
> UseCounter::count(frame(), UseCounter::SelectionSetBaseAndExtentNull);
> extendOffset = 0;
> }
>
> We dont' want to check |baseNode| twice.
Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by yosin@chromium.org
The CQ bit was checked by yosin@chromium.org
lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1480664098852880,
"parent_rev": "4a9d6d2b0fb8d8cc9ed3119236f8e9dec7ff4c45", "commit_rev":
"bea49eec3617dd50ca3732206952fe480a1d12ba"}
Message was sent while issue was closed.
Description was changed from ========== selection.setBaseAndExtent with null should not crash. setBaseAndExtent is non-nullable in spec: http://w3c.github.io/selection-api/#widl-Selection-setBaseAndExtent-void-Node- anchorNode-unsigned-long-anchorOffset-Node-focusNode-unsigned-long-focusOffset However, there is the issue for that: https://github.com/w3c/selection-api/issues/72 Edge clears selection with null node and Blink virtually does same. 'Virtually' means we go though false DCHECK statements in the release build. This CL refines code path both on debug and release. Besides exact behavior, selection.setBaseAndExtent(null,x,null,y) should not crash and this CL checks it in DOMSelection. This CL makes selection.setBaseAndExtent(null,x,null,y) clear Selection and keeps selection.setBaseAndExtent(base,x,null,y) as selection.collpase(base,x) except x can over base's length. BUG=660848 ========== to ========== selection.setBaseAndExtent with null should not crash. setBaseAndExtent is non-nullable in spec: http://w3c.github.io/selection-api/#widl-Selection-setBaseAndExtent-void-Node- anchorNode-unsigned-long-anchorOffset-Node-focusNode-unsigned-long-focusOffset However, there is the issue for that: https://github.com/w3c/selection-api/issues/72 Edge clears selection with null node and Blink virtually does same. 'Virtually' means we go though false DCHECK statements in the release build. This CL refines code path both on debug and release. Besides exact behavior, selection.setBaseAndExtent(null,x,null,y) should not crash and this CL checks it in DOMSelection. This CL makes selection.setBaseAndExtent(null,x,null,y) clear Selection and keeps selection.setBaseAndExtent(base,x,null,y) as selection.collpase(base,x) except x can over base's length. BUG=660848 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== selection.setBaseAndExtent with null should not crash. setBaseAndExtent is non-nullable in spec: http://w3c.github.io/selection-api/#widl-Selection-setBaseAndExtent-void-Node- anchorNode-unsigned-long-anchorOffset-Node-focusNode-unsigned-long-focusOffset However, there is the issue for that: https://github.com/w3c/selection-api/issues/72 Edge clears selection with null node and Blink virtually does same. 'Virtually' means we go though false DCHECK statements in the release build. This CL refines code path both on debug and release. Besides exact behavior, selection.setBaseAndExtent(null,x,null,y) should not crash and this CL checks it in DOMSelection. This CL makes selection.setBaseAndExtent(null,x,null,y) clear Selection and keeps selection.setBaseAndExtent(base,x,null,y) as selection.collpase(base,x) except x can over base's length. BUG=660848 ========== to ========== selection.setBaseAndExtent with null should not crash. setBaseAndExtent is non-nullable in spec: http://w3c.github.io/selection-api/#widl-Selection-setBaseAndExtent-void-Node- anchorNode-unsigned-long-anchorOffset-Node-focusNode-unsigned-long-focusOffset However, there is the issue for that: https://github.com/w3c/selection-api/issues/72 Edge clears selection with null node and Blink virtually does same. 'Virtually' means we go though false DCHECK statements in the release build. This CL refines code path both on debug and release. Besides exact behavior, selection.setBaseAndExtent(null,x,null,y) should not crash and this CL checks it in DOMSelection. This CL makes selection.setBaseAndExtent(null,x,null,y) clear Selection and keeps selection.setBaseAndExtent(base,x,null,y) as selection.collpase(base,x) except x can over base's length. BUG=660848 Committed: https://crrev.com/ebec09cdbe2965a07c54715cf4ba3b50603efe39 Cr-Commit-Position: refs/heads/master@{#435915} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/ebec09cdbe2965a07c54715cf4ba3b50603efe39 Cr-Commit-Position: refs/heads/master@{#435915} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
