Let insertNewLineInQuotedContent not do anything out of a quote element

Let insertNewLineInQuotedContent not do anything out of a quote element

The issue happens when insertNewLineInQuotedContent deletes selected range 
 to insert a break but after that caret disappears because no visible 
element. 

Anyway we should do nothing if insertNewLineInQuotedContent is called
 when selection is out side of quote element.

BUG=692063
TEST=run-webkit-tests 
 LayoutTests/editing/execCommand/insertNewLineInQuotedContent-outside-quote.html

Review-Url: https://codereview.chromium.org/2750533007
Cr-Commit-Position: refs/heads/master@{#458345}
Committed: https://chromium.googlesource.com/chromium/src/+/3d4203963eb508258f3b407630f0cc8be7fd0697

[yoichio, 2017-03-16 08:19:13]

The CQ bit was checked by yoichio@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-16 08:19:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yoichio, 2017-03-16 08:21:17]

Description was changed from

==========
insertbq

BUG=692063
==========

to

==========
Let insertNewLineInQuotedContent not nothing out of a quote element

BUG=692063
TEST=run-webkit-tests 
 LayoutTests/editing/execCommand/insertNewLineInQuotedContent-outside-quote.html
==========

[yoichio, 2017-03-16 08:24:37]

Description was changed from

==========
Let insertNewLineInQuotedContent not nothing out of a quote element

BUG=692063
TEST=run-webkit-tests 
 LayoutTests/editing/execCommand/insertNewLineInQuotedContent-outside-quote.html
==========

to

==========
Let insertNewLineInQuotedContent not nothing out of a quote element

The issue happens when insertNewLineInQuotedContent deletes selected range 
 to insert a break but after that caret disappears because no visible 
element. 

Anyway we should do nothing if insertNewLineInQuotedContent is called
 when selection is out side of quote element.


BUG=692063
TEST=run-webkit-tests 
 LayoutTests/editing/execCommand/insertNewLineInQuotedContent-outside-quote.html
==========

[commit-bot: I haz the power, 2017-03-16 09:25:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-16 09:25:53]

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_...)

[yoichio, 2017-03-17 02:49:56]

The CQ bit was checked by yoichio@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-17 02:50:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yoichio, 2017-03-17 04:38:59]

Description was changed from

==========
Let insertNewLineInQuotedContent not nothing out of a quote element

The issue happens when insertNewLineInQuotedContent deletes selected range 
 to insert a break but after that caret disappears because no visible 
element. 

Anyway we should do nothing if insertNewLineInQuotedContent is called
 when selection is out side of quote element.


BUG=692063
TEST=run-webkit-tests 
 LayoutTests/editing/execCommand/insertNewLineInQuotedContent-outside-quote.html
==========

to

==========
Let insertNewLineInQuotedContent not do anything out of a quote element

The issue happens when insertNewLineInQuotedContent deletes selected range 
 to insert a break but after that caret disappears because no visible 
element. 

Anyway we should do nothing if insertNewLineInQuotedContent is called
 when selection is out side of quote element.

BUG=692063
TEST=run-webkit-tests 
 LayoutTests/editing/execCommand/insertNewLineInQuotedContent-outside-quote.html
==========

[yoichio, 2017-03-17 04:39:15]

yoichio@chromium.org changed reviewers:
+ yosin@chromium.org

[commit-bot: I haz the power, 2017-03-17 04:45:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-17 04:45:18]

Dry run: This issue passed the CQ dry run.

[yosin_UTC9, 2017-03-17 09:27:02]

https://codereview.chromium.org/2750533007/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/editing/execCommand/insertNewLineInQuotedContent-outside-quote.html
(right):

https://codereview.chromium.org/2750533007/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/editing/execCommand/insertNewLineInQuotedContent-outside-quote.html:11:
document.designMode = "on";
nit: s/"on"/'on'/

https://codereview.chromium.org/2750533007/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/events/scoped/editing-commands.html
(right):

https://codereview.chromium.org/2750533007/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/events/scoped/editing-commands.html:86: var
initial = 'hello, <input type="text"><blockquote type="cite" align="right"><u><a
href="about:blank">world</a></u></blockquote>';
Why do you add |type="cite"|?

https://codereview.chromium.org/2750533007/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/commands/BreakBlockquoteCommand.cpp
(right):

https://codereview.chromium.org/2750533007/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/commands/BreakBlockquoteCommand.cpp:77:
static HTMLQuoteElement* topBlockquoteOf(const VisibleSelection& selection) {
Please use |Position| as parameter to reduce usage of |VisibleSelection|.

https://codereview.chromium.org/2750533007/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/commands/BreakBlockquoteCommand.cpp:78:
// pos is a position equivalent to the caret.  We use downstream() so that pos
nit: s/pos/|pos|/
nit: s/downstream()/|downstream()|/

https://codereview.chromium.org/2750533007/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/commands/BreakBlockquoteCommand.cpp:81:
const Position pos = mostForwardCaretPosition(selection.start());
nit: s/pos/position/
Blink doesn't want to use abbrev.

https://codereview.chromium.org/2750533007/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/commands/BreakBlockquoteCommand.cpp:83:
// Find the top-most blockquote from the start.
not need to have a comment. The statement explains itself.

https://codereview.chromium.org/2750533007/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/commands/BreakBlockquoteCommand.cpp:120:
HTMLQuoteElement* const topBlockquote = topBlockquoteOf(endingSelection());
Can we use result of |topBlockquoteOf()| at L92 here?

[yoichio, 2017-03-21 03:47:57]

The CQ bit was checked by yoichio@chromium.org to run a CQ dry run

[yoichio, 2017-03-21 03:48:07]

PTAL

https://codereview.chromium.org/2750533007/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/editing/execCommand/insertNewLineInQuotedContent-outside-quote.html
(right):

https://codereview.chromium.org/2750533007/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/editing/execCommand/insertNewLineInQuotedContent-outside-quote.html:11:
document.designMode = "on";
On 2017/03/17 09:27:02, yosin_UTC9 wrote:
> nit: s/"on"/'on'/

Done.

https://codereview.chromium.org/2750533007/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/events/scoped/editing-commands.html
(right):

https://codereview.chromium.org/2750533007/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/events/scoped/editing-commands.html:86: var
initial = 'hello, <input type="text"><blockquote type="cite" align="right"><u><a
href="about:blank">world</a></u></blockquote>';
On 2017/03/17 09:27:02, yosin_UTC9 wrote:
> Why do you add |type="cite"|?

Because InsertNewlineInQuotedContent inserts a break inside 
"isMailHTMLBlockquoteElement" element as L85 which was originally
 intended.
isMailHTMLBlockquoteElement is defined as
element.hasTagName(blockquoteTag) &&
  element.getAttribute("type") == "cite" in EditingUtilities.

Question is why this layouttest had worked before.
The answer is InsertNewlineInQuotedContent command just deleted
 selected range and aborted on L108 and this test just confirms
 DOM changing anyway. This layout test is ridiculous and should 
 be refactored.

https://codereview.chromium.org/2750533007/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/commands/BreakBlockquoteCommand.cpp
(right):

https://codereview.chromium.org/2750533007/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/commands/BreakBlockquoteCommand.cpp:77:
static HTMLQuoteElement* topBlockquoteOf(const VisibleSelection& selection) {
On 2017/03/17 09:27:02, yosin_UTC9 wrote:
> Please use |Position| as parameter to reduce usage of |VisibleSelection|.

Done.

https://codereview.chromium.org/2750533007/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/commands/BreakBlockquoteCommand.cpp:78:
// pos is a position equivalent to the caret.  We use downstream() so that pos
On 2017/03/17 09:27:02, yosin_UTC9 wrote:
> nit: s/pos/|pos|/
> nit: s/downstream()/|downstream()|/

Done.

https://codereview.chromium.org/2750533007/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/commands/BreakBlockquoteCommand.cpp:81:
const Position pos = mostForwardCaretPosition(selection.start());
On 2017/03/17 09:27:02, yosin_UTC9 wrote:
> nit: s/pos/position/
> Blink doesn't want to use abbrev.

Done.

https://codereview.chromium.org/2750533007/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/commands/BreakBlockquoteCommand.cpp:83:
// Find the top-most blockquote from the start.
On 2017/03/17 09:27:02, yosin_UTC9 wrote:
> not need to have a comment. The statement explains itself.

Done.

https://codereview.chromium.org/2750533007/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/commands/BreakBlockquoteCommand.cpp:120:
HTMLQuoteElement* const topBlockquote = topBlockquoteOf(endingSelection());
On 2017/03/17 09:27:02, yosin_UTC9 wrote:
> Can we use result of |topBlockquoteOf()| at L92 here?

No, deletingSelection on L97 changes the result.

[commit-bot: I haz the power, 2017-03-21 03:48:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-21 05:15:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-21 05:15:50]

Dry run: This issue passed the CQ dry run.

[yosin_UTC9, 2017-03-21 05:30:45]

lgtm w/ nit

https://codereview.chromium.org/2750533007/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/commands/BreakBlockquoteCommand.cpp
(right):

https://codereview.chromium.org/2750533007/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/commands/BreakBlockquoteCommand.cpp:82: 
nit: We don't need to have an extra blank line here.

[yoichio, 2017-03-21 05:36:58]

The CQ bit was checked by yoichio@chromium.org

[yoichio, 2017-03-21 05:36:58]

The patchset sent to the CQ was uploaded after l-g-t-m from yosin@chromium.org
Link to the patchset: https://codereview.chromium.org/2750533007/#ps60001
(title: "update")

[commit-bot: I haz the power, 2017-03-21 05:37:17]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-21 07:38:35]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1490074618537500,
"parent_rev": "170e8dd6e7055e5c583be95da018cafc3c79d7fb", "commit_rev":
"3d4203963eb508258f3b407630f0cc8be7fd0697"}

[commit-bot: I haz the power, 2017-03-21 07:39:19]

Description was changed from

==========
Let insertNewLineInQuotedContent not do anything out of a quote element

The issue happens when insertNewLineInQuotedContent deletes selected range 
 to insert a break but after that caret disappears because no visible 
element. 

Anyway we should do nothing if insertNewLineInQuotedContent is called
 when selection is out side of quote element.

BUG=692063
TEST=run-webkit-tests 
 LayoutTests/editing/execCommand/insertNewLineInQuotedContent-outside-quote.html
==========

to

==========
Let insertNewLineInQuotedContent not do anything out of a quote element

The issue happens when insertNewLineInQuotedContent deletes selected range 
 to insert a break but after that caret disappears because no visible 
element. 

Anyway we should do nothing if insertNewLineInQuotedContent is called
 when selection is out side of quote element.

BUG=692063
TEST=run-webkit-tests 
 LayoutTests/editing/execCommand/insertNewLineInQuotedContent-outside-quote.html

Review-Url: https://codereview.chromium.org/2750533007
Cr-Commit-Position: refs/heads/master@{#458345}
Committed:
https://chromium.googlesource.com/chromium/src/+/3d4203963eb508258f3b407630f0...
==========

[commit-bot: I haz the power, 2017-03-21 07:39:20]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/3d4203963eb508258f3b407630f0...