New issue
Advanced search Search tips

Issue 639320 link

Starred by 1 user
Status: Fixed
Owner:
ekaramad@chromium.org
Closed: Feb 2017
Cc:
dcheng@chromium.org
wfh@chromium.org
creis@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 3
Type: Bug



Sign in to add a comment

Layering violation in WebFrameWidgetImpl::applyReplacementRange

Project Member Reported by dglazkov@chromium.org, Aug 19 2016 
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp?rcl=0&l=902

void WebFrameWidgetImpl::applyReplacementRange(int start, int length)
{
    if (LocalFrame* frame = focusedLocalFrameInWidget()) {
        // TODO(dglazkov): Going from LocalFrame to WebLocalFrameImpl seems
        // silly. What is going on here?
        WebLocalFrameImpl::fromFrame(frame)->selectRange(WebRange(start, length));
    }
}

The code seems to be going the opposite direction of the way layers are structured. Let's fix that.

Comment 1 by dglazkov@chromium.org, Aug 19 2016

Cc: dcheng@chromium.org

Comment 2 by dglazkov@chromium.org, Jan 27 2017

Components: Blink>Internals
Owner: ----
Status: Untriaged (was: Assigned)

Comment 3 by dcheng@chromium.org, Jan 27 2017

Components: Internals>Sandbox>SiteIsolation
Owner: ekaramad@chromium.org
Status: Assigned (was: Untriaged)
Project Member

Comment 4 by bugdroid1@chromium.org, Feb 9 2017 

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

commit ce32ef9fc7230bbba732162c60cdc90f18fbbbc0
Author: ekaramad <ekaramad@chromium.org>
Date: Thu Feb 09 17:33:56 2017

Remove WebWidget::applyReplacementRange

The method WebWidget::applyReplacementRange was added to accomodate the
support for IME in OOPIFs. Before, the IME logic in content/renderer used to
live in RenderViewImpl which has access to the frame tree on the renderer side.
To accomodate this transition, some methods were added to the blink
public API.

Specifically, applyReplacement range selects a range to be later replaceed by
the next call to set or commit composition. This simple task could be
part of the implementation of those methods to the newly added
WebInputMethodController instead. Moreover, the method does not do
anything special to be used outside the scope of handling the IME
events so it should not be part of the public API.

Specifically, this CL will:
  1- Remove WebWidget::applyReplacementRange which will also remove it
  from WebFrameWidgetImpl, WebViewFrameWidget, WebViewImpl.

  2- Adds a new argument, replacementRange to
  WebInputMethodController::setComposition and
  WebInputMethodController::commitText.

The benefits of this change are:
  1- Removing a layering violation inside applyReplacementRange.
  2- Further deduplicate the IME code inside WebFrameWidgetImpl and
  WebViewImpl.
  2- Help with the separation of WebWidget and WebViewImpl.
  3- Reducing some lines of code.

BUG= 639320 , 629721 

Review-Url: https://codereview.chromium.org/2674253004
Cr-Commit-Position: refs/heads/master@{#449339}

[modify] https://crrev.com/ce32ef9fc7230bbba732162c60cdc90f18fbbbc0/components/test_runner/text_input_controller.cc
[modify] https://crrev.com/ce32ef9fc7230bbba732162c60cdc90f18fbbbc0/content/renderer/render_widget.cc
[modify] https://crrev.com/ce32ef9fc7230bbba732162c60cdc90f18fbbbc0/content/renderer/render_widget_browsertest.cc
[modify] https://crrev.com/ce32ef9fc7230bbba732162c60cdc90f18fbbbc0/third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp
[modify] https://crrev.com/ce32ef9fc7230bbba732162c60cdc90f18fbbbc0/third_party/WebKit/Source/web/WebFrameWidgetImpl.h
[modify] https://crrev.com/ce32ef9fc7230bbba732162c60cdc90f18fbbbc0/third_party/WebKit/Source/web/WebInputMethodControllerImpl.cpp
[modify] https://crrev.com/ce32ef9fc7230bbba732162c60cdc90f18fbbbc0/third_party/WebKit/Source/web/WebInputMethodControllerImpl.h
[modify] https://crrev.com/ce32ef9fc7230bbba732162c60cdc90f18fbbbc0/third_party/WebKit/Source/web/WebViewFrameWidget.cpp
[modify] https://crrev.com/ce32ef9fc7230bbba732162c60cdc90f18fbbbc0/third_party/WebKit/Source/web/WebViewFrameWidget.h
[modify] https://crrev.com/ce32ef9fc7230bbba732162c60cdc90f18fbbbc0/third_party/WebKit/Source/web/WebViewImpl.cpp
[modify] https://crrev.com/ce32ef9fc7230bbba732162c60cdc90f18fbbbc0/third_party/WebKit/Source/web/WebViewImpl.h
[modify] https://crrev.com/ce32ef9fc7230bbba732162c60cdc90f18fbbbc0/third_party/WebKit/Source/web/tests/WebViewTest.cpp
[modify] https://crrev.com/ce32ef9fc7230bbba732162c60cdc90f18fbbbc0/third_party/WebKit/public/web/WebInputMethodController.h
[modify] https://crrev.com/ce32ef9fc7230bbba732162c60cdc90f18fbbbc0/third_party/WebKit/public/web/WebWidget.h

Comment 5 by ekaramad@chromium.org, Feb 10 2017 

The method is removed as of comment 4. So Marking as fixed.

Comment 6 by ekaramad@chromium.org, Feb 10 2017

Status: Fixed (was: Assigned)
►

Sign in to add a comment