Prune ComputeVisibleSelectionInDOMTreeDeprecated from InputMethodController [chromium/src : main]
Siye Liu (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
- Anupam Snigdha
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Anupam Snigdha (Gerrit)
Anupam Snigdha added 2 comments![]( "Open in Gerrit")
GetDocument().UpdateStyleAndLayout(DocumentUpdateReason::kEditing);Since we are running layout before creation selection offsets in plain text range, should these (and others below) be `kSelection` instead of `kEditing`?
.ComputeVisibleSelectionInDOMTree()Since we have now removed the implicit layout and style updates, can we change the DCHECK TO CHECK in `SelectionEditor::UpdateCachedVisibleSelectionIfNeeded`?
If not, can we add `DCHECK_GE(GetDocument().Lifecycle().GetState(), DocumentLifecycle::kAfterPerformLayout);` before calling `ComputeVisibleSelectionInDOMTree` in this file?
Related details
- Siye Liu
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Siye Liu (Gerrit)
Siye Liu added 3 comments![]( "Open in Gerrit")
GetDocument().UpdateStyleAndLayout(DocumentUpdateReason::kEditing);Since we are running layout before creation selection offsets in plain text range, should these (and others below) be `kSelection` instead of `kEditing`?
There should be no behavior change in the tests. But I agree `kSelection` would be more appropriate. Updated in next iteration.
Since we have now removed the implicit layout and style updates, can we change the DCHECK TO CHECK in `SelectionEditor::UpdateCachedVisibleSelectionIfNeeded`?
If not, can we add `DCHECK_GE(GetDocument().Lifecycle().GetState(), DocumentLifecycle::kAfterPerformLayout);` before calling `ComputeVisibleSelectionInDOMTree` in this file?
We already have a DCHECK in `SelectionEditor::UpdateCachedVisibleSelectionIfNeeded' if the layout is not clean when creating visible selection. I think we should be good here.
Related details
- Anupam Snigdha
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Anupam Snigdha (Gerrit)
Anupam Snigdha added 1 comment![]( "Open in Gerrit")
GetDocument().UpdateStyleAndLayout(DocumentUpdateReason::kSelection);It looks like in the test we have to invoke layout update before we call `GetSelectionOffsets`, can we add the layout update inside `GetSelectionOffsets` instead? I wonder if these tests crash because of the DCHECK if you omit these layout updates? This is also why I think we should change the DCHECK to CHECK so we can catch all regressions in production where DCHECK is disabled. Thoughts?
Related details
- Siye Liu
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Siye Liu (Gerrit)
Siye Liu added 1 comment![]( "Open in Gerrit")
GetDocument().UpdateStyleAndLayout(DocumentUpdateReason::kSelection);It looks like in the test we have to invoke layout update before we call `GetSelectionOffsets`, can we add the layout update inside `GetSelectionOffsets` instead? I wonder if these tests crash because of the DCHECK if you omit these layout updates? This is also why I think we should change the DCHECK to CHECK so we can catch all regressions in production where DCHECK is disabled. Thoughts?
This is expected because the whole point of this change is to hoist layout update to the caller when invoking `ComputeVisibleSelectionInDOMTree` to reduce unnecessary layout updates.
I verified that all the callers of `GetSelectionOffsets` already have clean layout so there is no behavior difference after this patch. This also matches the point to reduce unnecessary layout updates.
Related details
- Anupam Snigdha
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Anupam Snigdha (Gerrit)
Anupam Snigdha added 2 comments![]( "Open in Gerrit")
DCHECK(!GetDocument().NeedsLayoutTreeUpdate());Since we're changing DCHECK to CHECK in `SelectionEditor::UpdateCachedVisibleSelectionIfNeeded`, can we remove this DCHECK here?
GetDocument().UpdateStyleAndLayout(DocumentUpdateReason::kEditing);There are couple of callers of `TextInputInfo` - `WidgetBase::UpdateTextInputStateInternal` & `DidHandleGestureEvent`. `UpdateTextInputStateInternal` should have a clean layout right? Did you have to add the layout update because of `DidHandleGestureEvent`?
Related details
- Siye Liu
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Siye Liu (Gerrit)
Siye Liu added 2 comments![]( "Open in Gerrit")
Since we're changing DCHECK to CHECK in `SelectionEditor::UpdateCachedVisibleSelectionIfNeeded`, can we remove this DCHECK here?
Acknowledged
GetDocument().UpdateStyleAndLayout(DocumentUpdateReason::kEditing);There are couple of callers of `TextInputInfo` - `WidgetBase::UpdateTextInputStateInternal` & `DidHandleGestureEvent`. `UpdateTextInputStateInternal` should have a clean layout right? Did you have to add the layout update because of `DidHandleGestureEvent`?
It's not guaranteed that `UpdateTextInputStateInternal` has clean layout. So it's safer to update layout here. Besides, the comment also mentioned that we should update layout here, although the usage needs to be audited.
Related details
- Anupam Snigdha
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Anupam Snigdha (Gerrit)
Anupam Snigdha voted and added 1 comment![]( "Open in Gerrit")
Votes added by Anupam Snigdha
| Code-Review | +1 |
1 comment
Related details
- Siye Liu
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Koji Ishii (Gerrit)
Koji Ishii voted and added 1 comment![]( "Open in Gerrit")
Votes added by Koji Ishii
| Code-Review | +1 |
1 comment
lgtm
Related details
- Siye Liu
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Siye Liu (Gerrit)
Siye Liu voted Commit-Queue+2![]( "Open in Gerrit")
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Prune ComputeVisibleSelectionInDOMTreeDeprecated from InputMethodController
This patch prunes several sites of the deprecated function by:
- hoisting layout update out from it:
- InputMethodController::CancelComposition.
- InputMethodController::TextInputInfo.
- layout already clean:
- InputMethodController::ClearImeTextSpansByType.
- InputMethodController::FinishComposingText.
- InputMethodController::ReplaceComposition.
- InputMethodController::ReplaceCompositionAndMoveCaret.
- InputMethodController::InsertTextAndMoveCaret.
- InputMethodController::SetComposition.
- InputMethodController::SetCompositionFromExistingText.
- InputMethodController::AddImeTextSpansToExistingText.
- InputMethodController::GetSelectionOffsets.
- InputMethodController::EphemeralRangeForOffsets.
- InputMethodController::CreateRangeForSelection.
- InputMethodController::ExtendSelectionAndDelete.
- InputMethodController::DeleteSurroundingText.
- InputMethodController::GetImeTextSpans.
- M third_party/blink/renderer/core/editing/ime/cached_text_input_info_test.cc
- M third_party/blink/renderer/core/editing/ime/input_method_controller.cc
- M third_party/blink/renderer/core/editing/selection_editor.cc
Code-Review: +1 by Koji Ishii, +1 by Anupam Snigdha
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Grieve (Gerrit)
Andrew Grieve added 1 comment![]( "Open in Gerrit")
FYI - This might be related to a new crash bug crbug.com/336992377
With stack:
```
0x8a3e0db4 (libmonochrome.so - immediate_crash.h: 176) base::ImmediateCrash()
0x8a3e0db4 (libmonochrome.so - check.h: 211) logging::CheckFailure()
0x8a3e0db4 (libmonochrome.so - selection_editor.cc: 418) blink::SelectionEditor::UpdateCachedVisibleSelectionIfNeeded() const
0x88c5d56f (libmonochrome.so - selection_editor.cc: 79) blink::SelectionEditor::ComputeVisibleSelectionInDOMTree() const
0x88c5d4f3 (libmonochrome.so - frame_selection.cc: 135) blink::FrameSelection::ComputeVisibleSelectionInDOMTree() const
0x8a3d27e1 (libmonochrome.so - input_method_controller.cc: 1253) blink::InputMethodController::EphemeralRangeForOffsets(blink::PlainTextRange const&) const
0x8a3d332d (libmonochrome.so - input_method_controller.cc: 1269) blink::InputMethodController::SetSelectionOffsets(blink::PlainTextRange const&, blink::InputMethodController::TypingContinuation)
0x8a3d357f (libmonochrome.so - input_method_controller.cc: 1263) blink::InputMethodController::SetSelectionOffsets(blink::PlainTextRange const&)
0x8a3d357f (libmonochrome.so - input_method_controller.cc: 1504) blink::InputMethodController::DeleteSurroundingText(int, int)
0x8a422b85 (libmonochrome.so - web_local_frame_impl.cc: 1777) blink::WebLocalFrameImpl::DeleteSurroundingText(int, int)
0x8a64cf1b (libmonochrome.so - frame_widget_input_handler_impl.cc: 131) blink::FrameWidgetInputHandlerImpl::DeleteSurroundingText(int, int)::$_0::operator()(base::WeakPtr<blink::mojom::blink::FrameWidgetInputHandler>, int, int) const
0x8a64cf1b (libmonochrome.so - bind_internal.h: 656) void base::internal::DecayedFunctorTraits<blink::FrameWidgetInputHandlerImpl::DeleteSurroundingText(int, int)::$_0, base::WeakPtr<blink::mojom::blink::FrameWidgetInputHandler>&&, int&&, int&&>::Invoke<blink::FrameWidgetInputHandlerImpl::DeleteSurroundingText(int, int)::$_0, base::WeakPtr<blink::mojom::blink::FrameWidgetInputHandler>, int, int>(blink::FrameWidgetInputHandlerImpl::DeleteSurroundingText(int, int)::$_0&&, base::WeakPtr<blink::mojom::blink::FrameWidgetInputHandler>&&, int&&, int&&)
0x8a64cf1b (libmonochrome.so - bind_internal.h: 930) void base::internal::InvokeHelper<false, base::internal::FunctorTraits<blink::FrameWidgetInputHandlerImpl::DeleteSurroundingText(int, int)::$_0&&, base::WeakPtr<blink::mojom::blink::FrameWidgetInputHandler>&&, int&&, int&&>, void, 0u, 1u, 2u>::MakeItSo<blink::FrameWidgetInputHandlerImpl::DeleteSurroundingText(int, int)::$_0, std::__Cr::tuple<base::WeakPtr<blink::mojom::blink::FrameWidgetInputHandler>, int, int>>(blink::FrameWidgetInputHandlerImpl::DeleteSurroundingText(int, int)::$_0&&, std::__Cr::tuple<base::WeakPtr<blink::mojom::blink::FrameWidgetInputHandler>, int, int>&&)
0x8a64cf1b (libmonochrome.so - bind_internal.h: 1067) void base::internal::Invoker<base::internal::FunctorTraits<blink::FrameWidgetInputHandlerImpl::DeleteSurroundingText(int, int)::$_0&&, base::WeakPtr<blink::mojom::blink::FrameWidgetInputHandler>&&, int&&, int&&>, base::internal::BindState<false, false, false, blink::FrameWidgetInputHandlerImpl::DeleteSurroundingText(int, int)::$_0, base::WeakPtr<blink::mojom::blink::FrameWidgetInputHandler>, int, int>, void ()>::RunImpl<blink::FrameWidgetInputHandlerImpl::DeleteSurroundingText(int, int)::$_0, std::__Cr::tuple<base::WeakPtr<blink::mojom::blink::FrameWidgetInputHandler>, int, int>, 0u, 1u, 2u>(blink::FrameWidgetInputHandlerImpl::DeleteSurroundingText(int, int)::$_0&&, std::__Cr::tuple<base::WeakPtr<blink::mojom::blink::FrameWidgetInputHandler>, int, int>&&, std::__Cr::integer_sequence<unsigned int, 0u, 1u, 2u>)
0x8a64cf1b (libmonochrome.so - bind_internal.h: 980) base::internal::Invoker<base::internal::FunctorTraits<blink::FrameWidgetInputHandlerImpl::DeleteSurroundingText(int, int)::$_0&&, base::WeakPtr<blink::mojom::blink::FrameWidgetInputHandler>&&, int&&, int&&>, base::internal::BindState<false, false, false, blink::FrameWidgetInputHandlerImpl::DeleteSurroundingText(int, int)::$_0, base::WeakPtr<blink::mojom::blink::FrameWidgetInputHandler>, int, int>, void ()>::RunOnce(base::internal::BindStateBase*)
0x8850dbd5 (libmonochrome.so - callback.h: 156) base::OnceCallback<void ()>::Run() &&
0x88928a45 (libmonochrome.so - main_thread_event_queue.cc: 510) blink::MainThreadEventQueue::DispatchEvents()
```
Related details
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |