Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(140)

Issue 698113005: The ASSERT in appendNode() should not fire when OBJECTS are cloned. (Closed)

Created:
6 years, 1 month ago by zherczeg
Modified:
6 years ago
CC:
blink-reviews
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Project:
blink
Visibility:
Public.

Description

The ASSERT in appendNode() should not fire when OBJECTS are cloned. The ASSERT may be triggered in CompositeEditCommand::appendNode when the fallback content of an OBJECT element is cloned because the canHaveChildrenForEditing is not reliable for OBJECTs until their render object is created. This issue is fixed by ignoring the return value of canHaveChildrenForEditing when an OBJECT is created. BUG=399628 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=187132

Patch Set 1 #

Patch Set 2 : Full patch #

Total comments: 6

Patch Set 3 : Next attempt #

Total comments: 1

Patch Set 4 : Make test async #

Total comments: 1

Patch Set 5 : no setTimeout #

Unified diffs Side-by-side diffs Delta from patch set Stats (+25 lines, -1 line) Patch
A LayoutTests/editing/execCommand/crash-object-cloning.html View 1 2 3 4 1 chunk +18 lines, -0 lines 0 comments Download
A LayoutTests/editing/execCommand/crash-object-cloning-expected.txt View 1 3 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/editing/CompositeEditCommand.cpp View 1 2 1 chunk +6 lines, -1 line 0 comments Download

Messages

Total messages: 32 (5 generated)
zherczeg
6 years, 1 month ago (2014-11-18 08:48:52 UTC) #2
Yuta Kitamura
-tkent, +yosin
6 years, 1 month ago (2014-11-18 10:38:03 UTC) #4
yosin_UTC9
https://codereview.chromium.org/698113005/diff/20001/LayoutTests/editing/execCommand/crash-object-cloning.html File LayoutTests/editing/execCommand/crash-object-cloning.html (right): https://codereview.chromium.org/698113005/diff/20001/LayoutTests/editing/execCommand/crash-object-cloning.html#newcode4 LayoutTests/editing/execCommand/crash-object-cloning.html:4: { nit: Move |{| to end of |if| line. ...
6 years, 1 month ago (2014-11-20 01:26:42 UTC) #5
yosin_UTC9
Please describe about this patch rather than root cause, e.g. Change CompositeEditiCommand::appendNode blah blah ...
6 years, 1 month ago (2014-11-20 01:31:00 UTC) #6
zherczeg
Thank you for the review. I tried to address all of your concerns and uploaded ...
6 years, 1 month ago (2014-11-20 13:43:19 UTC) #7
zherczeg
On 2014/11/20 13:43:19, zherczeg wrote: > Thank you for the review. I tried to address ...
6 years, 1 month ago (2014-11-24 07:43:50 UTC) #8
yosin_UTC9
code changes and test script are looks fine. Please update description to explain changes in ...
6 years ago (2014-11-25 01:07:32 UTC) #9
zherczeg
On 2014/11/25 01:07:32, Yosi_UTC9 wrote: > code changes and test script are looks fine. > ...
6 years ago (2014-11-25 08:26:25 UTC) #10
yosin_UTC9
On 2014/11/25 08:26:25, zherczeg wrote: > On 2014/11/25 01:07:32, Yosi_UTC9 wrote: > > code changes ...
6 years ago (2014-11-26 01:00:52 UTC) #11
zherczeg
> You can change patch description and subject by "Edit Issue" link in left side ...
6 years ago (2014-11-26 07:42:14 UTC) #12
yosin_UTC9
https://codereview.chromium.org/698113005/diff/40001/LayoutTests/editing/execCommand/crash-object-cloning.html File LayoutTests/editing/execCommand/crash-object-cloning.html (right): https://codereview.chromium.org/698113005/diff/40001/LayoutTests/editing/execCommand/crash-object-cloning.html#newcode5 LayoutTests/editing/execCommand/crash-object-cloning.html:5: testRunner.dumpAsText(); Could you call |testRunner.waitUntilDone()| and call |testRunner.notifyDone()| in ...
6 years ago (2014-11-27 01:13:19 UTC) #13
zherczeg
> I think test expectation should have indented "abcd" from OBJECT's fallback > contents. Good ...
6 years ago (2014-11-27 12:15:55 UTC) #14
zherczeg
> Anything else? Please review this patch.
6 years ago (2014-12-02 07:31:40 UTC) #15
yosin_UTC9
https://codereview.chromium.org/698113005/diff/60001/LayoutTests/editing/execCommand/crash-object-cloning.html File LayoutTests/editing/execCommand/crash-object-cloning.html (right): https://codereview.chromium.org/698113005/diff/60001/LayoutTests/editing/execCommand/crash-object-cloning.html#newcode14 LayoutTests/editing/execCommand/crash-object-cloning.html:14: setTimeout(function() { testRunner.notifyDone(); }, 0); Why do we need ...
6 years ago (2014-12-03 01:10:33 UTC) #16
zherczeg
> setTimeout(function() { testRunner.notifyDone(); }, 0); > Why do we need to call |notifyDone()| in ...
6 years ago (2014-12-03 07:57:57 UTC) #17
zherczeg
Ping Yosi_UTC9
6 years ago (2014-12-05 11:12:20 UTC) #18
yosin_UTC9
On 2014/12/03 07:57:57, zherczeg wrote: > > setTimeout(function() { testRunner.notifyDone(); }, 0); > > Why ...
6 years ago (2014-12-08 02:27:38 UTC) #19
zherczeg
> What do you mean "does not work"? Timeout? > It is hard to believe ...
6 years ago (2014-12-08 11:36:47 UTC) #20
yosin_UTC9
On 2014/12/08 11:36:47, zherczeg wrote: > > What do you mean "does not work"? Timeout? ...
6 years ago (2014-12-09 03:42:07 UTC) #21
zherczeg
> How about adding output after executing "Indent" command? > Like: > document.execCommand("Indent"); > document.body.innerHTML ...
6 years ago (2014-12-09 12:01:44 UTC) #22
yosin_UTC9
lgtm Thanks!
6 years ago (2014-12-10 00:45:53 UTC) #23
Yuta Kitamura
lgtm
6 years ago (2014-12-10 09:57:32 UTC) #24
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/698113005/80001
6 years ago (2014-12-15 06:44:01 UTC) #26
commit-bot: I haz the power
Try jobs failed on following builders: win_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/41712)
6 years ago (2014-12-15 08:37:37 UTC) #28
zherczeg
> win_blink_rel on tryserver.blink > (http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/41712) New failures: media/encrypted-media/encrypted-media-lifetime-mediakeysession-release.html I don't think it is related. ...
6 years ago (2014-12-15 10:47:41 UTC) #29
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/698113005/80001
6 years ago (2014-12-15 10:48:22 UTC) #31
commit-bot: I haz the power
6 years ago (2014-12-15 11:26:09 UTC) #32
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=187132

Powered by Google App Engine
This is Rietveld 408576698