Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2910813002:
Editor::executeCommand() should have DCEHCK about frame is active (Closed)
DescriptionEditor::executeCommand() should have DCEHCK about frame is active
BUG=695275
Review-Url: https://codereview.chromium.org/2910813002
Cr-Commit-Position: refs/heads/master@{#475856}
Committed: https://chromium.googlesource.com/chromium/src/+/bdc74113995ad8e4c1b7cb0150431f90d30618bc
Patch Set 1 #
Total comments: 4
Patch Set 2 : Corrected isActive()->IsActive() #
Total comments: 4
Patch Set 3 : Removed line DCHECK(GetFrame().GetDocument()); #Messages
Total messages: 30 (20 generated)
Description was changed from ========== Editor::executeCommand() should have DCEHCK about frame is active BUG=695275 ========== to ========== Editor::executeCommand() should have DCEHCK about frame is active BUG=695275 Please review the code changes. ==========
tripta.g@samsung.com changed reviewers: + a.suchit@chromium.org, a.suchit@samsung.com
Description was changed from ========== Editor::executeCommand() should have DCEHCK about frame is active BUG=695275 Please review the code changes. ========== to ========== Editor::executeCommand() should have DCEHCK about frame is active BUG=695275 ==========
Please review the code changes.
The CQ bit was checked by a.suchit@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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_headless_rel on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
https://codereview.chromium.org/2910813002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp (right): https://codereview.chromium.org/2910813002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp:2859: DCHECK(GetFrame().GetDocument()->isActive()); It should be IsActive(). https://codereview.chromium.org/2910813002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp:2897: DCHECK(GetFrame().GetDocument()->isActive()); It should be IsActive().
This patch may not be needed because all layout calls are going to remove. This the meta bug for it : http://crbug.com/370457 Let it decide by chromium owner.
The CQ bit was checked by a.suchit@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 tripta.g@samsung.com
The CQ bit was checked by tripta.g@samsung.com
The CQ bit was unchecked by tripta.g@samsung.com
The CQ bit was checked by tripta.g@samsung.com
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
No L-G-T-M from a valid reviewer yet. CQ run can only be started once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a full super star committer. Committers are members of the group "project-chromium-committers". Note that this has nothing to do with OWNERS files.
tripta.g@samsung.com changed reviewers: + yosin@chromium.org
Please review code changes.
lgtm w/ nits Thanks for working this! https://codereview.chromium.org/2910813002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp (right): https://codereview.chromium.org/2910813002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp:2858: DCHECK(GetFrame().GetDocument()); nit: We don't need to have |DCHECK(GetFrame().GetDocument())| since below line use it. https://codereview.chromium.org/2910813002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp:2896: DCHECK(GetFrame().GetDocument()); nit: We don't need to have |DCHECK(GetFrame().GetDocument())| since below line use it.
Have removed line DCHECK(GetFrame().GetDocument()); Thank you https://codereview.chromium.org/2910813002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp (right): https://codereview.chromium.org/2910813002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp:2859: DCHECK(GetFrame().GetDocument()->isActive()); On 2017/05/29 05:39:18, a.suchit2 wrote: > It should be IsActive(). Done https://codereview.chromium.org/2910813002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp:2897: DCHECK(GetFrame().GetDocument()->isActive()); On 2017/05/29 05:39:18, a.suchit2 wrote: > It should be IsActive(). Done https://codereview.chromium.org/2910813002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp (right): https://codereview.chromium.org/2910813002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp:2858: DCHECK(GetFrame().GetDocument()); On 2017/05/31 02:12:46, yosin_UTC9 wrote: > nit: We don't need to have |DCHECK(GetFrame().GetDocument())| since > below line use it. Done. https://codereview.chromium.org/2910813002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp:2896: DCHECK(GetFrame().GetDocument()); On 2017/05/31 02:12:46, yosin_UTC9 wrote: > nit: We don't need to have |DCHECK(GetFrame().GetDocument())| since > below line use it. Done.
The CQ bit was checked by yosin@chromium.org
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/2910813002/#ps40001 (title: "Removed line DCHECK(GetFrame().GetDocument());")
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": 40001, "attempt_start_ts": 1496215089931180,
"parent_rev": "e4c379cd8ceb33e1168fb071a2f27c0d74562e11", "commit_rev":
"bdc74113995ad8e4c1b7cb0150431f90d30618bc"}
Message was sent while issue was closed.
Description was changed from ========== Editor::executeCommand() should have DCEHCK about frame is active BUG=695275 ========== to ========== Editor::executeCommand() should have DCEHCK about frame is active BUG=695275 Review-Url: https://codereview.chromium.org/2910813002 Cr-Commit-Position: refs/heads/master@{#475856} Committed: https://chromium.googlesource.com/chromium/src/+/bdc74113995ad8e4c1b7cb015043... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/bdc74113995ad8e4c1b7cb015043...
Message was sent while issue was closed.
Patchset #4 (id:60001) has been deleted |
