|
|
Created:
5 years, 6 months ago by Habib Virji Modified:
5 years, 4 months ago Reviewers:
yoichio, philipj_slow CC:
blink-reviews, vivekg, arv+blink, Inactive, vivekg_samsung Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionSelection attributes changes from long to unsigned long
Selection.idl values changed from long to unsigned long. Also changes done
to non-standard API as they all use same single function.
Also includes changes where check was done if value is less than 0 has been
removed. Since value is unsigned value cannot be less than 0.
BUG=496400
R=philipj, yoichio
Patch Set 1 #Patch Set 2 : Fixes offset error message #
Total comments: 5
Patch Set 3 : Using static_cast checks for checking if the value are -ve. #
Total comments: 2
Patch Set 4 : Updated missed return statement #
Total comments: 1
Created: 5 years, 4 months ago
Messages
Total messages: 21 (8 generated)
The CQ bit was checked by habib.virji@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1195833002/1
The CQ bit was unchecked by habib.virji@samsung.com
The CQ bit was checked by habib.virji@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1195833002/20001
The CQ bit was unchecked by habib.virji@samsung.com
https://codereview.chromium.org/1195833002/diff/20001/Source/core/editing/DOM... File Source/core/editing/DOMSelection.cpp (right): https://codereview.chromium.org/1195833002/diff/20001/Source/core/editing/DOM... Source/core/editing/DOMSelection.cpp:267: exceptionState.throwDOMException(IndexSizeError, String::number(baseOffset) + " is larger than the base node's length."); Raising exception when offset is over content length is not speced. How about other browsers?
https://codereview.chromium.org/1195833002/diff/20001/Source/core/editing/DOM... File Source/core/editing/DOMSelection.cpp (right): https://codereview.chromium.org/1195833002/diff/20001/Source/core/editing/DOM... Source/core/editing/DOMSelection.cpp:267: exceptionState.throwDOMException(IndexSizeError, String::number(baseOffset) + " is larger than the base node's length."); On 2015/07/13 02:29:45, yoichio wrote: > Raising exception when offset is over content length is not speced. > How about other browsers? The spec has different variable names, but it actually does throw an exception here: http://w3c.github.io/selection-api/#widl-Selection-setBaseAndExtent-void-Node... Note that the spec is also confused about signed/unsigned: https://github.com/w3c/selection-api/issues/33 This is tricky, because previously passing -1 for the second argument would throw an exception, but to preserve that behavior we'd have to do something weird like if (static_cast<int>(baseOffset) < 0) throw(). Where is the current handling for when offset is too large? https://codereview.chromium.org/1195833002/diff/20001/Source/core/editing/Sel... File Source/core/editing/Selection.idl (right): https://codereview.chromium.org/1195833002/diff/20001/Source/core/editing/Sel... Source/core/editing/Selection.idl:47: // TODO(yoichio): The node argumendt should not be nullable. crbug.com/391673 Introduced typo here.
The CQ bit was checked by habib.virji@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1195833002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1195833002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks philipj and yoichio for your comments. Have replied inline to your queries can you please have a look. https://codereview.chromium.org/1195833002/diff/20001/Source/core/editing/DOM... File Source/core/editing/DOMSelection.cpp (right): https://codereview.chromium.org/1195833002/diff/20001/Source/core/editing/DOM... Source/core/editing/DOMSelection.cpp:267: exceptionState.throwDOMException(IndexSizeError, String::number(baseOffset) + " is larger than the base node's length."); Actually spec says to throw an error for both the scenarios... "If anchorOffset is negative or longer than anchorNode's length or if focusOffset is negative or longer than focusNode's length, throw an IndexSizeError exception and abort these steps." Firefox uses int and not unsigned and thus have same error as the current chromium and thus exceeding length error is not shown. https://codereview.chromium.org/1195833002/diff/20001/Source/core/editing/DOM... Source/core/editing/DOMSelection.cpp:267: exceptionState.throwDOMException(IndexSizeError, String::number(baseOffset) + " is larger than the base node's length."); @philipj: As far as I can see no error. If it exceeds UINT_MAX it gives an error with negative value. To comply with previous code I have added as per your suggestion.
https://codereview.chromium.org/1195833002/diff/40001/Source/core/editing/DOM... File Source/core/editing/DOMSelection.cpp (right): https://codereview.chromium.org/1195833002/diff/40001/Source/core/editing/DOM... Source/core/editing/DOMSelection.cpp:347: exceptionState.throwDOMException(IndexSizeError, String::number(offset) + " is larger than the given node's length."); Why don't you return?
@yoichoi: Updated removed return statement... https://codereview.chromium.org/1195833002/diff/40001/Source/core/editing/DOM... File Source/core/editing/DOMSelection.cpp (right): https://codereview.chromium.org/1195833002/diff/40001/Source/core/editing/DOM... Source/core/editing/DOMSelection.cpp:347: exceptionState.throwDOMException(IndexSizeError, String::number(offset) + " is larger than the given node's length."); Apologise it was removed by mistake. New patch uploaded correcting the change.
The CQ bit was checked by habib.virji@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1195833002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1195833002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1195833002/diff/60001/Source/core/editing/DOM... File Source/core/editing/DOMSelection.cpp (right): https://codereview.chromium.org/1195833002/diff/60001/Source/core/editing/DOM... Source/core/editing/DOMSelection.cpp:207: if (static_cast<int>(offset) < 0) { This is really quite strange, and is strange in the spec as well: https://github.com/w3c/selection-api/issues/33 Every |if (x < 0)| that has now been replaced by |if (static_cast<int>(x) < 0)| ought to simply be removed I think. If the value is later cast to int such that the case here tested matters, the change has to go deeper. If that's too complicated, then we probably shouldn't make the change at all.
There are ample of test that would need updating for x < 0 scenario. So that's the reason I opted for the static_int. In terms of deep change, it is not needed but there are several test which sends -1 and the same issues as Image arises as value is converted to UINT_MAX - 1. Solution for Image should be applicable here too. |