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

Issue 16001010: Make replacing selection in heading/paragraph with heading/paragraph to work (Closed)

Created:
7 years, 6 months ago by yosin_UTC9
Modified:
7 years, 6 months ago
Reviewers:
rniwa-cr, ojan, eseidel
CC:
blink-reviews, eae+blinkwatch
Visibility:
Public.

Description

Before this patch, replacing selection in heading/paragraph with heading/paragraph gets assertion at InsertNodeBeforeCommand constructor, or crash when assertion disabled. It tries to move replaced heading/paragraph out of ancestor, ReplaceSelectionCommand::moveNodeOutOfAncestor(). This patch changes ReplaceSelectionCommand::moveNodeOutOfAncestor() to avoid the assertion. BUG=246991 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=153031

Patch Set 1 : 2013-06-06T17:16:29 #

Total comments: 2

Patch Set 2 : 2013-06-11T17:06 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+67 lines, -2 lines) Patch
A LayoutTests/editing/inserting/replace-in-heading-001.html View 1 chunk +31 lines, -0 lines 0 comments Download
A + LayoutTests/editing/inserting/replace-in-heading-001-expected.txt View 1 chunk +1 line, -1 line 0 comments Download
A LayoutTests/editing/inserting/replace-in-paragraph-001.html View 1 chunk +31 lines, -0 lines 0 comments Download
A + LayoutTests/editing/inserting/replace-in-paragraph-001-expected.txt View 1 chunk +1 line, -1 line 0 comments Download
M Source/core/editing/ReplaceSelectionCommand.cpp View 1 1 chunk +3 lines, -0 lines 0 comments Download

Messages

Total messages: 8 (0 generated)
yosin_UTC9
Hi guys, Could you review this patch for crash bug? Thanks in advance. -yosi
7 years, 6 months ago (2013-06-06 08:27:23 UTC) #1
yosin_UTC9
ping...
7 years, 6 months ago (2013-06-11 03:33:09 UTC) #2
rniwa-cr
https://codereview.chromium.org/16001010/diff/5001/Source/core/editing/ReplaceSelectionCommand.cpp File Source/core/editing/ReplaceSelectionCommand.cpp (right): https://codereview.chromium.org/16001010/diff/5001/Source/core/editing/ReplaceSelectionCommand.cpp#newcode641 Source/core/editing/ReplaceSelectionCommand.cpp:641: if (ancestor->parentNode()->attached() && !ancestor->parentNode()->rendererIsEditable()) Why are we checking attached()? ...
7 years, 6 months ago (2013-06-11 06:32:00 UTC) #3
yosin_UTC9
Could you review this patch? Thanks in advance. https://codereview.chromium.org/16001010/diff/5001/Source/core/editing/ReplaceSelectionCommand.cpp File Source/core/editing/ReplaceSelectionCommand.cpp (right): https://codereview.chromium.org/16001010/diff/5001/Source/core/editing/ReplaceSelectionCommand.cpp#newcode641 Source/core/editing/ReplaceSelectionCommand.cpp:641: if ...
7 years, 6 months ago (2013-06-11 08:07:41 UTC) #4
rniwa-cr
The new patch looks correct.
7 years, 6 months ago (2013-06-13 05:22:50 UTC) #5
eseidel
lgtm
7 years, 6 months ago (2013-06-25 21:12:49 UTC) #6
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yosin@chromium.org/16001010/11001
7 years, 6 months ago (2013-06-25 21:14:40 UTC) #7
commit-bot: I haz the power
7 years, 6 months ago (2013-06-25 22:42:00 UTC) #8
Message was sent while issue was closed.
Change committed as 153031

Powered by Google App Engine
This is Rietveld 408576698