Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
451403002:
Range.deleteContents shouldn't throw HierarchyRequestError on doctype (Closed)
DescriptionRange.deleteContents shouldn't throw HierarchyRequestError on doctype
SPEC: http://dom.spec.whatwg.org/#dom-range-deletecontents
Unlike extractContents, deleteContents doesn't throw error.
So this CL modified implementation as per specified.
Behavior in other browsers.
*)FF: PASS
*)IE: PASS
In addition 4 failed cases will be passed with this CL in
http://w3c-test.org/dom/ranges/Range-deleteContents.html
TEST=LayoutTests/fast/dom/Range/deleteContents-doctype.html
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=180143
Patch Set 1 #
Total comments: 4
Patch Set 2 : Take review comment into consideration #
Total comments: 11
Patch Set 3 : Take review comment into consideration #Patch Set 4 : Fix typo #
Total comments: 2
Patch Set 5 : Take review comment into consideration. #
Total comments: 4
Patch Set 6 : Take review comment into consideration #
Total comments: 8
Patch Set 7 : Take review comment into consideration. #
Messages
Total messages: 25 (0 generated)
PTAL fyi, console.log is used on test case since document was blown off.
C++ change is good. https://codereview.chromium.org/451403002/diff/1/LayoutTests/fast/dom/Range/d... File LayoutTests/fast/dom/Range/deleteContents-doctype.html (right): https://codereview.chromium.org/451403002/diff/1/LayoutTests/fast/dom/Range/d... LayoutTests/fast/dom/Range/deleteContents-doctype.html:18: console.log('Ran as expected!'); We can use |shouldThrow| in "js-test.js".
https://codereview.chromium.org/451403002/diff/1/LayoutTests/fast/dom/Range/d... File LayoutTests/fast/dom/Range/deleteContents-doctype.html (right): https://codereview.chromium.org/451403002/diff/1/LayoutTests/fast/dom/Range/d... LayoutTests/fast/dom/Range/deleteContents-doctype.html:18: console.log('Ran as expected!'); On 2014/08/11 00:34:24, Yosi_UTC9 wrote: > We can use |shouldThrow| in "js-test.js". Yes, I usually use it. But, in this case, shouldNotThrow("range.deleteContents()"); will cause result like below; CONSOLE ERROR: line 32: Uncaught TypeError: Cannot read property 'firstChild' of null CONSOLE ERROR: line 32: Uncaught TypeError: Cannot read property 'firstChild' of null It is because function getOrCreate in js-test.js can't refer to firstChild due to blown document off. Is it ok to remain console error instead of console.log?
https://codereview.chromium.org/451403002/diff/1/LayoutTests/fast/dom/Range/d... File LayoutTests/fast/dom/Range/deleteContents-doctype.html (right): https://codereview.chromium.org/451403002/diff/1/LayoutTests/fast/dom/Range/d... LayoutTests/fast/dom/Range/deleteContents-doctype.html:18: console.log('Ran as expected!'); On 2014/08/11 00:42:20, kangil_ wrote: > On 2014/08/11 00:34:24, Yosi_UTC9 wrote: > > We can use |shouldThrow| in "js-test.js". > > Yes, I usually use it. But, in this case, > shouldNotThrow("range.deleteContents()"); will cause result like below; > > CONSOLE ERROR: line 32: Uncaught TypeError: Cannot read property 'firstChild' of > null > CONSOLE ERROR: line 32: Uncaught TypeError: Cannot read property 'firstChild' of > null > > It is because function getOrCreate in js-test.js can't refer to firstChild due > to blown document off. > > Is it ok to remain console error instead of console.log? I see. How about using document.write, document.documentElement.textContent, etc. I think it is better to display result in page rather than console.
Thanks for the review comment. PTAL! https://codereview.chromium.org/451403002/diff/1/LayoutTests/fast/dom/Range/d... File LayoutTests/fast/dom/Range/deleteContents-doctype.html (right): https://codereview.chromium.org/451403002/diff/1/LayoutTests/fast/dom/Range/d... LayoutTests/fast/dom/Range/deleteContents-doctype.html:18: console.log('Ran as expected!'); On 2014/08/11 02:16:42, Yosi_UTC9 wrote: > On 2014/08/11 00:42:20, kangil_ wrote: > > On 2014/08/11 00:34:24, Yosi_UTC9 wrote: > > > We can use |shouldThrow| in "js-test.js". > > > > Yes, I usually use it. But, in this case, > > shouldNotThrow("range.deleteContents()"); will cause result like below; > > > > CONSOLE ERROR: line 32: Uncaught TypeError: Cannot read property 'firstChild' > of > > null > > CONSOLE ERROR: line 32: Uncaught TypeError: Cannot read property 'firstChild' > of > > null > > > > It is because function getOrCreate in js-test.js can't refer to firstChild due > > to blown document off. > > > > Is it ok to remain console error instead of console.log? > > I see. How about using document.write, document.documentElement.textContent, > etc. > > I think it is better to display result in page rather than console. Done.
Gentle ping to yosin. :)
https://codereview.chromium.org/451403002/diff/20001/LayoutTests/fast/dom/Ran... File LayoutTests/fast/dom/Range/deleteContents-doctype.html (right): https://codereview.chromium.org/451403002/diff/20001/LayoutTests/fast/dom/Ran... LayoutTests/fast/dom/Range/deleteContents-doctype.html:1: <html> For this test, we should have <!DOCTYPE html>. I tried below html, it works but when I added <!DOCTYPE html> at the top, it throws an exception: <div>foobar</div> <script> var range = document.createRange(); range.setStart(document, 0); range.setEnd(document, 1); range.deleteContents(); </script> https://codereview.chromium.org/451403002/diff/20001/LayoutTests/fast/dom/Ran... LayoutTests/fast/dom/Range/deleteContents-doctype.html:1: <html> nit: We don't need to have html, head, and body markup. They are automatically inserted by parser. https://codereview.chromium.org/451403002/diff/20001/LayoutTests/fast/dom/Ran... LayoutTests/fast/dom/Range/deleteContents-doctype.html:5: <div id='test'> This test doesn't use these id's and this test doesn't matter what contents in body. Just text node is fine. https://codereview.chromium.org/451403002/diff/20001/LayoutTests/fast/dom/Ran... LayoutTests/fast/dom/Range/deleteContents-doctype.html:14: var html = document.createElement('html'); document.write('PASS') is shortest. https://codereview.chromium.org/451403002/diff/20001/LayoutTests/fast/dom/Ran... LayoutTests/fast/dom/Range/deleteContents-doctype.html:25: try { We don't need to have try block. Since, range.deleteContents() doesn't throw.
https://codereview.chromium.org/451403002/diff/20001/LayoutTests/fast/dom/Ran... File LayoutTests/fast/dom/Range/deleteContents-doctype.html (right): https://codereview.chromium.org/451403002/diff/20001/LayoutTests/fast/dom/Ran... LayoutTests/fast/dom/Range/deleteContents-doctype.html:1: <html> On 2014/08/12 01:00:43, Yosi_UTC9 wrote: > For this test, we should have <!DOCTYPE html>. > I tried below html, it works but when I added <!DOCTYPE html> at the top, it > throws an exception: > > <div>foobar</div> > <script> > var range = document.createRange(); > range.setStart(document, 0); > range.setEnd(document, 1); > range.deleteContents(); > </script> > I will check and get back to you with result.
Thanks for review comments! PTAL. https://codereview.chromium.org/451403002/diff/20001/LayoutTests/fast/dom/Ran... File LayoutTests/fast/dom/Range/deleteContents-doctype.html (right): https://codereview.chromium.org/451403002/diff/20001/LayoutTests/fast/dom/Ran... LayoutTests/fast/dom/Range/deleteContents-doctype.html:1: <html> On 2014/08/12 01:00:44, Yosi_UTC9 wrote: > nit: We don't need to have html, head, and body markup. They are automatically > inserted by parser. Done. https://codereview.chromium.org/451403002/diff/20001/LayoutTests/fast/dom/Ran... LayoutTests/fast/dom/Range/deleteContents-doctype.html:1: <html> On 2014/08/12 01:00:43, Yosi_UTC9 wrote: > For this test, we should have <!DOCTYPE html>. > I tried below html, it works but when I added <!DOCTYPE html> at the top, it > throws an exception: > > <div>foobar</div> > <script> > var range = document.createRange(); > range.setStart(document, 0); > range.setEnd(document, 1); > range.deleteContents(); > </script> > Done. https://codereview.chromium.org/451403002/diff/20001/LayoutTests/fast/dom/Ran... LayoutTests/fast/dom/Range/deleteContents-doctype.html:5: <div id='test'> On 2014/08/12 01:00:43, Yosi_UTC9 wrote: > This test doesn't use these id's and this test doesn't matter what contents in > body. Just text node is fine. Done. https://codereview.chromium.org/451403002/diff/20001/LayoutTests/fast/dom/Ran... LayoutTests/fast/dom/Range/deleteContents-doctype.html:14: var html = document.createElement('html'); On 2014/08/12 01:00:43, Yosi_UTC9 wrote: > document.write('PASS') is shortest. Done. https://codereview.chromium.org/451403002/diff/20001/LayoutTests/fast/dom/Ran... LayoutTests/fast/dom/Range/deleteContents-doctype.html:25: try { On 2014/08/12 01:00:43, Yosi_UTC9 wrote: > We don't need to have try block. Since, range.deleteContents() doesn't throw. Done.
LGTM w/ nits. https://codereview.chromium.org/451403002/diff/60001/LayoutTests/fast/dom/Ran... File LayoutTests/fast/dom/Range/deleteContents-doctype.html (right): https://codereview.chromium.org/451403002/diff/60001/LayoutTests/fast/dom/Ran... LayoutTests/fast/dom/Range/deleteContents-doctype.html:7: function generatePassMessage() { nit: We don't need to have separate function for this. Please add comment why do we use document.write(). It may not be obvious.
Thanks! https://codereview.chromium.org/451403002/diff/60001/LayoutTests/fast/dom/Ran... File LayoutTests/fast/dom/Range/deleteContents-doctype.html (right): https://codereview.chromium.org/451403002/diff/60001/LayoutTests/fast/dom/Ran... LayoutTests/fast/dom/Range/deleteContents-doctype.html:7: function generatePassMessage() { On 2014/08/12 03:22:52, Yosi_UTC9 wrote: > nit: We don't need to have separate function for this. > Please add comment why do we use document.write(). It may not be obvious. Done.
The CQ bit was checked by kangil.han@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kangil.han@samsung.com/451403002/80001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: blink_presubmit on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/1...) linux_blink_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/2...) linux_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/2...) mac_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/19383) win_blink_compile_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_compile_dbg/bu...) win_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/22178)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: blink_presubmit on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/1...)
@yutak: Can you give owner lgtm? Thanks!
https://codereview.chromium.org/451403002/diff/80001/LayoutTests/fast/dom/Ran... File LayoutTests/fast/dom/Range/deleteContents-doctype.html (right): https://codereview.chromium.org/451403002/diff/80001/LayoutTests/fast/dom/Ran... LayoutTests/fast/dom/Range/deleteContents-doctype.html:12: // If Range.deleteContents() throws error, then we will not see below PASS in result. We have "shouldNotThrow()" in js-test.js to test this specific situation. I recommend using some existing JS testing framework (either js-test or testharness) instead of inventing your own. https://codereview.chromium.org/451403002/diff/80001/Source/core/dom/Range.cpp File Source/core/dom/Range.cpp (right): https://codereview.chromium.org/451403002/diff/80001/Source/core/dom/Range.cp... Source/core/dom/Range.cpp:465: ASSERT(boundaryPointsValid()); Now that the function name "checkDeleteExtract" does not really make sense, because it is named after its function that checks the precondition for deleteContents() and extractContents(). Can you meld its content into extractContents(), or rename the function to something more sensible like "checkExtractPrecondition"?
Thanks for review, PTAL! https://codereview.chromium.org/451403002/diff/80001/LayoutTests/fast/dom/Ran... File LayoutTests/fast/dom/Range/deleteContents-doctype.html (right): https://codereview.chromium.org/451403002/diff/80001/LayoutTests/fast/dom/Ran... LayoutTests/fast/dom/Range/deleteContents-doctype.html:12: // If Range.deleteContents() throws error, then we will not see below PASS in result. On 2014/08/12 09:07:43, Yuta Kitamura wrote: > We have "shouldNotThrow()" in js-test.js to test > this specific situation. > > I recommend using some existing JS testing framework > (either js-test or testharness) instead of inventing > your own. Done. https://codereview.chromium.org/451403002/diff/80001/Source/core/dom/Range.cpp File Source/core/dom/Range.cpp (right): https://codereview.chromium.org/451403002/diff/80001/Source/core/dom/Range.cp... Source/core/dom/Range.cpp:465: ASSERT(boundaryPointsValid()); On 2014/08/12 09:07:43, Yuta Kitamura wrote: > Now that the function name "checkDeleteExtract" does not > really make sense, because it is named after its function > that checks the precondition for deleteContents() and > extractContents(). > > Can you meld its content into extractContents(), or rename > the function to something more sensible like > "checkExtractPrecondition"? Done.
LGTM after revision https://codereview.chromium.org/451403002/diff/100001/LayoutTests/fast/dom/Ra... File LayoutTests/fast/dom/Range/deleteContents-doctype.html (right): https://codereview.chromium.org/451403002/diff/100001/LayoutTests/fast/dom/Ra... LayoutTests/fast/dom/Range/deleteContents-doctype.html:1: <!doctype html> super-nit/optional: it is recommended to spell DOCTYPE in capital letters: <!DOCTYPE html> https://codereview.chromium.org/451403002/diff/100001/LayoutTests/fast/dom/Ra... LayoutTests/fast/dom/Range/deleteContents-doctype.html:4: <p>This test verifies if Range.deleteContents can handle doctype.</p> With js-test, the test description should be emitted with description() function. https://codereview.chromium.org/451403002/diff/100001/LayoutTests/fast/dom/Ra... LayoutTests/fast/dom/Range/deleteContents-doctype.html:8: testRunner.dumpAsText(); js-test calls dumpAsText() on your behalf, so you don't have to do this yourself. https://codereview.chromium.org/451403002/diff/100001/LayoutTests/fast/dom/Ra... LayoutTests/fast/dom/Range/deleteContents-doctype.html:17: document.getElementById('container').outerHTML = ''; Please don't remove the test description; just leave it and let -expected.txt have it.
Thanks for reviewing this CL! https://codereview.chromium.org/451403002/diff/100001/LayoutTests/fast/dom/Ra... File LayoutTests/fast/dom/Range/deleteContents-doctype.html (right): https://codereview.chromium.org/451403002/diff/100001/LayoutTests/fast/dom/Ra... LayoutTests/fast/dom/Range/deleteContents-doctype.html:1: <!doctype html> On 2014/08/13 03:02:40, Yuta Kitamura wrote: > super-nit/optional: it is recommended to spell DOCTYPE in > capital letters: <!DOCTYPE html> Done. https://codereview.chromium.org/451403002/diff/100001/LayoutTests/fast/dom/Ra... LayoutTests/fast/dom/Range/deleteContents-doctype.html:4: <p>This test verifies if Range.deleteContents can handle doctype.</p> On 2014/08/13 03:02:40, Yuta Kitamura wrote: > With js-test, the test description should be emitted with > description() function. Done. https://codereview.chromium.org/451403002/diff/100001/LayoutTests/fast/dom/Ra... LayoutTests/fast/dom/Range/deleteContents-doctype.html:8: testRunner.dumpAsText(); On 2014/08/13 03:02:40, Yuta Kitamura wrote: > js-test calls dumpAsText() on your behalf, so you don't have > to do this yourself. Done. https://codereview.chromium.org/451403002/diff/100001/LayoutTests/fast/dom/Ra... LayoutTests/fast/dom/Range/deleteContents-doctype.html:17: document.getElementById('container').outerHTML = ''; On 2014/08/13 03:02:40, Yuta Kitamura wrote: > Please don't remove the test description; just leave it and > let -expected.txt have it. Done.
The CQ bit was checked by kangil.han@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kangil.han@samsung.com/451403002/120001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/22458)
Message was sent while issue was closed.
Change committed as 180143 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
