New issue
Advanced search Search tips

Issue 731540 link

Starred by 2 users

Issue metadata

Status: WontFix
Owner: ----
Closed: Jun 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Task

Blocking:
issue 707656



Sign in to add a comment

Get rid of EditCommand::IsRenderedCharacter()

Project Member Reported by yosin@chromium.org, Jun 9 2017

Issue description

We should replace Editor::IsRenderedCharacter() by InRenderedCharacter() in "VisibleUnits.cpp".
 

Comment 1 by yosin@chromium.org, Jun 9 2017

Blocking: 707656
Since Editor::IsRenderedCharacter() is the only caller of LayoutText::IsRenderedCharacter(), we can get rid of LayoutText::IsRenderedCharacter() once we remove Editor::IsRenderedCharacter().

Comment 2 by yosin@chromium.org, Jun 9 2017

Summary: Get rid of EditCommand::IsRenderedCharacter() (was: Get rid of Editor::IsRenderedCharacter())
Hi Yosin, I will take this one.
Did you meant InRenderedText()  in "VisibleUnits.cpp" ? Thanks.

Comment 4 by yosin@chromium.org, Jun 21 2017

Status: WontFix (was: Available)
It seems InRenderedText() is not compatible with Editor::IsRenderedCharacter()
regarding the patch[1].

So, we can't get rid of Get rid of EditCommand::IsRenderedCharacter().


[1] http://crrev.com/2940013004: Get rid of EditCommand::IsRenderedCharacter()
Project Member

Comment 5 by bugdroid1@chromium.org, Oct 4 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2a8d71964a5947c149343db49b20605db49abcae

commit 2a8d71964a5947c149343db49b20605db49abcae
Author: Xiaocheng Hu <xiaochengh@chromium.org>
Date: Wed Oct 04 03:54:09 2017

Make LayoutText::IsRenderedCharacter absorbed by EditCommand::IsRenderedCharacter

LayoutText::IsRenderedCharacter is only used by editing commands. Hence,
this patch makes it absorbed by its only call site in EditCommand, as
a preparation step to stop editing from depending on the legacy line
layout structure.

This patch also removes some TODOs related to this function. These TODOs
originates from an ancient CL [1], but do not make a lot of sense.

[1] crrev.com/289fa98c43250e4e9fc44aad39670122e4046bf6

Bug: 771398,  731540 
Change-Id: I38b85f5a4c8a8adfba0568632337f1e42cfeded5
Reviewed-on: https://chromium-review.googlesource.com/699011
Commit-Queue: Xiaocheng Hu <xiaochengh@chromium.org>
Reviewed-by: Emil A Eklund <eae@chromium.org>
Reviewed-by: Yoshifumi Inoue <yosin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#506297}
[modify] https://crrev.com/2a8d71964a5947c149343db49b20605db49abcae/third_party/WebKit/Source/core/editing/commands/DeleteSelectionCommand.cpp
[modify] https://crrev.com/2a8d71964a5947c149343db49b20605db49abcae/third_party/WebKit/Source/core/editing/commands/EditCommand.cpp
[modify] https://crrev.com/2a8d71964a5947c149343db49b20605db49abcae/third_party/WebKit/Source/core/editing/commands/EditCommand.h
[modify] https://crrev.com/2a8d71964a5947c149343db49b20605db49abcae/third_party/WebKit/Source/core/editing/commands/InsertLineBreakCommand.cpp
[modify] https://crrev.com/2a8d71964a5947c149343db49b20605db49abcae/third_party/WebKit/Source/core/editing/commands/InsertParagraphSeparatorCommand.cpp
[modify] https://crrev.com/2a8d71964a5947c149343db49b20605db49abcae/third_party/WebKit/Source/core/layout/LayoutText.cpp
[modify] https://crrev.com/2a8d71964a5947c149343db49b20605db49abcae/third_party/WebKit/Source/core/layout/LayoutText.h

Sign in to add a comment