Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
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?
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
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.
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
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?
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
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.
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
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`?
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
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.
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Code-Review | +1 |
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Code-Review | +1 |
lgtm
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
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.
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
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()
```
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |