|
|
Created:
3 years, 9 months ago by yosin_UTC9 Modified:
3 years, 9 months ago CC:
blink-reviews, chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake FormatBlockCommand::elementForFormatBlockCommand() to take EphemeralRange
This patch makes |FormatBlockCommand::elementForFormatBlockCommand()| to take
|EphemeralRange| instead of |Range| object to avoid registering temporary
|Range| object to range update list to reduce memory usage and relocation cost
for improving code health.
BUG=691199
TEST=n/a; no behavior change
Review-Url: https://codereview.chromium.org/2729823002
Cr-Commit-Position: refs/heads/master@{#455681}
Committed: https://chromium.googlesource.com/chromium/src/+/b66326f26629d4e161ad020ea34052c3f52598af
Patch Set 1 : 2017-03-02T16:28:37 #
Total comments: 6
Patch Set 2 : 2017-03-09T13:09:11 #
Messages
Total messages: 23 (14 generated)
Description was changed from ========== 2017-03-02T16:28:37 2017-03-02T16:26:01 BUG= ========== to ========== 2017-03-02T16:28:37 2017-03-02T16:26:01 BUG=691199 ==========
The CQ bit was checked by yosin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== 2017-03-02T16:28:37 2017-03-02T16:26:01 BUG=691199 ========== to ========== Make FormatBlockCommand::elementForFormatBlockCommand() to take EphemeralRange This patch makes |FormatBlockCommand::elementForFormatBlockCommand()| to take |EphemeralRange| instead of |Range| object to avoid registering temporary |Range| object to range update list to reduce memory usage and relocation cost for improving code health. BUG=691199 TEST=n/a; no behavior change ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
yosin@chromium.org changed reviewers: + tkent@chromium.org, xiaochengh@chromium.org, yoichio@chromium.org
PTAL
https://codereview.chromium.org/2729823002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/commands/FormatBlockCommand.cpp (right): https://codereview.chromium.org/2729823002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/commands/FormatBlockCommand.cpp:143: const EphemeralRange& range) { How about passing |selection| directly from caller and using selection.start/end().computeContainerNode() ?
https://codereview.chromium.org/2729823002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/commands/FormatBlockCommand.cpp (right): https://codereview.chromium.org/2729823002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/commands/FormatBlockCommand.cpp:143: const EphemeralRange& range) { On 2017/03/02 at 09:21:36, yoichio wrote: > How about passing |selection| directly from caller and using > selection.start/end().computeContainerNode() ? No, we don't want to increase usage of |VisibleSelection|.
https://codereview.chromium.org/2729823002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp (left): https://codereview.chromium.org/2729823002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp:2079: FormatBlockCommand::elementForFormatBlockCommand(firstRangeOf(selection)); This patch has conflict with https://codereview.chromium.org/2724333002. Could you make this patch a dependant of that one? https://codereview.chromium.org/2729823002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/commands/FormatBlockCommand.cpp (right): https://codereview.chromium.org/2729823002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/commands/FormatBlockCommand.cpp:144: Node* commonAncestor = Range::commonAncestorContainer( How about waiting until https://codereview.chromium.org/2725603002 lands, and then we can call |range.commonAncestorContainer()| directly?
"BLOCKED BY 2729823002" 2729823002 is this patch itself. Could you update?
The CQ bit was checked by yosin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
PTAL Bots failures aren't related to this patch. https://codereview.chromium.org/2729823002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp (left): https://codereview.chromium.org/2729823002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp:2079: FormatBlockCommand::elementForFormatBlockCommand(firstRangeOf(selection)); On 2017/03/02 at 20:21:05, Xiaocheng wrote: > This patch has conflict with https://codereview.chromium.org/2724333002. > > Could you make this patch a dependant of that one? The patch is committed. There is no conflict anymore. https://codereview.chromium.org/2729823002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/commands/FormatBlockCommand.cpp (right): https://codereview.chromium.org/2729823002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/commands/FormatBlockCommand.cpp:144: Node* commonAncestor = Range::commonAncestorContainer( On 2017/03/02 at 20:21:05, Xiaocheng wrote: > How about waiting until https://codereview.chromium.org/2725603002 lands, and then we can call |range.commonAncestorContainer()| directly? Done.
lgtm
The CQ bit was checked by yosin@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 20001, "attempt_start_ts": 1489038485006530, "parent_rev": "35a17d3593ecde3b7cea1ad36ada111d659b974a", "commit_rev": "b66326f26629d4e161ad020ea34052c3f52598af"}
Message was sent while issue was closed.
Description was changed from ========== Make FormatBlockCommand::elementForFormatBlockCommand() to take EphemeralRange This patch makes |FormatBlockCommand::elementForFormatBlockCommand()| to take |EphemeralRange| instead of |Range| object to avoid registering temporary |Range| object to range update list to reduce memory usage and relocation cost for improving code health. BUG=691199 TEST=n/a; no behavior change ========== to ========== Make FormatBlockCommand::elementForFormatBlockCommand() to take EphemeralRange This patch makes |FormatBlockCommand::elementForFormatBlockCommand()| to take |EphemeralRange| instead of |Range| object to avoid registering temporary |Range| object to range update list to reduce memory usage and relocation cost for improving code health. BUG=691199 TEST=n/a; no behavior change Review-Url: https://codereview.chromium.org/2729823002 Cr-Commit-Position: refs/heads/master@{#455681} Committed: https://chromium.googlesource.com/chromium/src/+/b66326f26629d4e161ad020ea340... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/b66326f26629d4e161ad020ea340... |