Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2529803002:
[Editing] Let InputMethodController::hasComposition check Range::isConnected(). (Closed)
Description[Editing] Let InputMethodController::hasComposition check Range::connected().
InputMethodConroller(abbr as IMC) has members of |bool m_hasComposition| and
|Range m_compositionRange| and a function hasComposition(), which returns
m_hasComposition;
Usually they sync, or m_hasComposition == (m_compositionRange.connected());
However, it brakes when m_compositionRange crosses over node which will be removed.
In the situation, m_hasComposition is true but m_compositionRange.connected()
returns false.
If we call IMC::finishCompositionText under the condition, calling composingText,
which creates text from range, causes crash because the range is not connected.
This CL change hasCompostion to check also m_compositionRange.
This CL also insert DCHECK(hasComposition()) before where m_compositionRange is used.
Attached test checks the crash but also confirms IMC::textInputType
while changing focus.
BUG=666659, 664317
Committed: https://crrev.com/7a4e741d1b8d72ac687163baa1feb0c651797495
Cr-Commit-Position: refs/heads/master@{#434467}
Patch Set 1 : update #
Total comments: 13
Patch Set 2 : Move to InputMethodConrollerTest #
Messages
Total messages: 26 (16 generated)
The CQ bit was checked by yoichio@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: blimp_linux_dbg on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
Patchset #1 (id:1) has been deleted
Description was changed from ========== imc ephemeral crash BUG=666659 ========== to ========== [Editing] Let InputMethodController::hasComposition check Range::connected(). InputMethodConroller(abbr as IMC) has members of |bool m_hasComposition| and |Range m_compositionRange| and a function hasComposition(), which returns m_hasComposition; Usually they sync, or m_hasComposition == (m_compositionRange.connected()); However, it brakes when m_compositionRange crosses over node which will be removed. In the situation, m_hasComposition is true but m_compositionRange.connected() returns false. If we call IMC::finishCompositionText under the condition, calling composingText, which creates text from range, causes crash because the range is not connected. This CL change hasCompostion to check also m_compositionRange. This CL also insert hasComposition() before where m_compositionRange is used. Attached test checks the crash but also should confirm WebView::textInputType after focusing another input element. This works will be done later. BUG=666659 ==========
yoichio@chromium.org changed reviewers: + changwan@chromium.org, yosin@chromium.org
https://codereview.chromium.org/2529803002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/2529803002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/InputMethodController.cpp:319: return false; This is probably not needed. https://codereview.chromium.org/2529803002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/tests/WebViewTest.cpp (right): https://codereview.chromium.org/2529803002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/WebViewTest.cpp:4369: // webView->setInitialFocus(false); could you remove this? https://codereview.chromium.org/2529803002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/WebViewTest.cpp:4394: // TODO: EXPECT_EQ(WebTextInputTypeTelephone, webView->textInputType()); Hmm... Does it still not fix crbug.com/664317? I thought that this can be fixed if we try the other approach that yosin@ suggested... Is there any reason you didn't try the preferred approach?
https://codereview.chromium.org/2529803002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/tests/WebViewTest.cpp (right): https://codereview.chromium.org/2529803002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/WebViewTest.cpp:4367: "data:text/html, <input id='a' /><br><input type='te' id='b' />"); typo: te -> tel
https://codereview.chromium.org/2529803002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/tests/WebViewTest.cpp (right): https://codereview.chromium.org/2529803002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/WebViewTest.cpp:4365: TEST_P(WebViewTest, CompositionIsRemovedOnDeletingElement) { Hmm... Could you also rename this to something like DeleteElementWhileComposingText and add crbug.com references? https://codereview.chromium.org/2529803002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/WebViewTest.cpp:4394: // TODO: EXPECT_EQ(WebTextInputTypeTelephone, webView->textInputType()); On 2016/11/24 09:15:00, Changwan Ryu wrote: > Hmm... Does it still not fix crbug.com/664317? > > I thought that this can be fixed if we try the other approach that yosin@ > suggested... Is there any reason you didn't try the preferred approach? If the test passes after typo is fixed, then this will probably fix crbug.com/664317 as well.
https://codereview.chromium.org/2529803002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/2529803002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/InputMethodController.cpp:319: return false; On 2016/11/24 at 09:15:00, Changwan Ryu wrote: > This is probably not needed. Let's use |DCHECK(hasComposition()| https://codereview.chromium.org/2529803002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/tests/WebViewTest.cpp (right): https://codereview.chromium.org/2529803002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/WebViewTest.cpp:4365: TEST_P(WebViewTest, CompositionIsRemovedOnDeletingElement) { Can we do this test in |InputMethodControllerTest|? What motivation do we test in |WebViewTest|?
The CQ bit was checked by yoichio@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: This issue passed the CQ dry run.
Description was changed from ========== [Editing] Let InputMethodController::hasComposition check Range::connected(). InputMethodConroller(abbr as IMC) has members of |bool m_hasComposition| and |Range m_compositionRange| and a function hasComposition(), which returns m_hasComposition; Usually they sync, or m_hasComposition == (m_compositionRange.connected()); However, it brakes when m_compositionRange crosses over node which will be removed. In the situation, m_hasComposition is true but m_compositionRange.connected() returns false. If we call IMC::finishCompositionText under the condition, calling composingText, which creates text from range, causes crash because the range is not connected. This CL change hasCompostion to check also m_compositionRange. This CL also insert hasComposition() before where m_compositionRange is used. Attached test checks the crash but also should confirm WebView::textInputType after focusing another input element. This works will be done later. BUG=666659 ========== to ========== [Editing] Let InputMethodController::hasComposition check Range::connected(). InputMethodConroller(abbr as IMC) has members of |bool m_hasComposition| and |Range m_compositionRange| and a function hasComposition(), which returns m_hasComposition; Usually they sync, or m_hasComposition == (m_compositionRange.connected()); However, it brakes when m_compositionRange crosses over node which will be removed. In the situation, m_hasComposition is true but m_compositionRange.connected() returns false. If we call IMC::finishCompositionText under the condition, calling composingText, which creates text from range, causes crash because the range is not connected. This CL change hasCompostion to check also m_compositionRange. This CL also insert DCHECK(hasComposition()) before where m_compositionRange is used. Attached test checks the crash but also confirms IMC::textInputType while changing focus. BUG=666659,664317 ==========
O.K. Fixed textInputType too. https://codereview.chromium.org/2529803002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/2529803002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/InputMethodController.cpp:319: return false; On 2016/11/25 01:28:22, Yosi_UTC9 wrote: > On 2016/11/24 at 09:15:00, Changwan Ryu wrote: > > This is probably not needed. > > Let's use |DCHECK(hasComposition()| Done. https://codereview.chromium.org/2529803002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/tests/WebViewTest.cpp (right): https://codereview.chromium.org/2529803002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/WebViewTest.cpp:4365: TEST_P(WebViewTest, CompositionIsRemovedOnDeletingElement) { On 2016/11/25 01:28:23, Yosi_UTC9 wrote: > Can we do this test in |InputMethodControllerTest|? > What motivation do we test in |WebViewTest|? Done. https://codereview.chromium.org/2529803002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/WebViewTest.cpp:4367: "data:text/html, <input id='a' /><br><input type='te' id='b' />"); On 2016/11/24 09:23:51, Changwan Ryu wrote: > typo: te -> tel Done. https://codereview.chromium.org/2529803002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/WebViewTest.cpp:4369: // webView->setInitialFocus(false); On 2016/11/24 09:15:00, Changwan Ryu wrote: > could you remove this? Done. https://codereview.chromium.org/2529803002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/WebViewTest.cpp:4394: // TODO: EXPECT_EQ(WebTextInputTypeTelephone, webView->textInputType()); On 2016/11/24 09:34:38, Changwan Ryu wrote: > On 2016/11/24 09:15:00, Changwan Ryu wrote: > > Hmm... Does it still not fix crbug.com/664317? > > > > I thought that this can be fixed if we try the other approach that yosin@ > > suggested... Is there any reason you didn't try the preferred approach? > > If the test passes after typo is fixed, then this will probably fix > crbug.com/664317 as well. Done.
The CQ bit was checked by yosin@chromium.org
lgtm
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": 1480065987979330,
"parent_rev": "2ce182d83b623279f01657b9de4f617603e228a8", "commit_rev":
"84588c7340644b57b2eb05036b44aad9e1357580"}
Message was sent while issue was closed.
Description was changed from ========== [Editing] Let InputMethodController::hasComposition check Range::connected(). InputMethodConroller(abbr as IMC) has members of |bool m_hasComposition| and |Range m_compositionRange| and a function hasComposition(), which returns m_hasComposition; Usually they sync, or m_hasComposition == (m_compositionRange.connected()); However, it brakes when m_compositionRange crosses over node which will be removed. In the situation, m_hasComposition is true but m_compositionRange.connected() returns false. If we call IMC::finishCompositionText under the condition, calling composingText, which creates text from range, causes crash because the range is not connected. This CL change hasCompostion to check also m_compositionRange. This CL also insert DCHECK(hasComposition()) before where m_compositionRange is used. Attached test checks the crash but also confirms IMC::textInputType while changing focus. BUG=666659,664317 ========== to ========== [Editing] Let InputMethodController::hasComposition check Range::connected(). InputMethodConroller(abbr as IMC) has members of |bool m_hasComposition| and |Range m_compositionRange| and a function hasComposition(), which returns m_hasComposition; Usually they sync, or m_hasComposition == (m_compositionRange.connected()); However, it brakes when m_compositionRange crosses over node which will be removed. In the situation, m_hasComposition is true but m_compositionRange.connected() returns false. If we call IMC::finishCompositionText under the condition, calling composingText, which creates text from range, causes crash because the range is not connected. This CL change hasCompostion to check also m_compositionRange. This CL also insert DCHECK(hasComposition()) before where m_compositionRange is used. Attached test checks the crash but also confirms IMC::textInputType while changing focus. BUG=666659,664317 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [Editing] Let InputMethodController::hasComposition check Range::connected(). InputMethodConroller(abbr as IMC) has members of |bool m_hasComposition| and |Range m_compositionRange| and a function hasComposition(), which returns m_hasComposition; Usually they sync, or m_hasComposition == (m_compositionRange.connected()); However, it brakes when m_compositionRange crosses over node which will be removed. In the situation, m_hasComposition is true but m_compositionRange.connected() returns false. If we call IMC::finishCompositionText under the condition, calling composingText, which creates text from range, causes crash because the range is not connected. This CL change hasCompostion to check also m_compositionRange. This CL also insert DCHECK(hasComposition()) before where m_compositionRange is used. Attached test checks the crash but also confirms IMC::textInputType while changing focus. BUG=666659,664317 ========== to ========== [Editing] Let InputMethodController::hasComposition check Range::connected(). InputMethodConroller(abbr as IMC) has members of |bool m_hasComposition| and |Range m_compositionRange| and a function hasComposition(), which returns m_hasComposition; Usually they sync, or m_hasComposition == (m_compositionRange.connected()); However, it brakes when m_compositionRange crosses over node which will be removed. In the situation, m_hasComposition is true but m_compositionRange.connected() returns false. If we call IMC::finishCompositionText under the condition, calling composingText, which creates text from range, causes crash because the range is not connected. This CL change hasCompostion to check also m_compositionRange. This CL also insert DCHECK(hasComposition()) before where m_compositionRange is used. Attached test checks the crash but also confirms IMC::textInputType while changing focus. BUG=666659,664317 Committed: https://crrev.com/7a4e741d1b8d72ac687163baa1feb0c651797495 Cr-Commit-Position: refs/heads/master@{#434467} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/7a4e741d1b8d72ac687163baa1feb0c651797495 Cr-Commit-Position: refs/heads/master@{#434467} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
