Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
299793002:
Forcibly notify text input type changed when RenderWidgetHostViewAura gets focused. (Closed)
|
Created:
6 years, 7 months ago by Shu Chen Modified:
6 years, 7 months ago CC:
chromium-reviews, yusukes+watch_chromium.org, yukishiino+watch_chromium.org, jam, penghuang+watch_chromium.org, sievers+watch_chromium.org, jbauman+watch_chromium.org, nona+watch_chromium.org, darin-cc_chromium.org, kalyank, piman+watch_chromium.org, danakj+watch_chromium.org, James Su, miu+watch_chromium.org, kevers, sadrul, Charlie Reis, jamesr Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionSends TextInputTypeChanged from renderer to browser when UpdateTextInputState. UpdateTextInputState used to only work with Android, but r245932 made it work with both Android and Aura.
BUG=345080
TEST=Verified in sandbox.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=272784
Patch Set 1 #Patch Set 2 : correct the solution of the fix. #Patch Set 3 : #
Total comments: 2
Patch Set 4 : #Messages
Total messages: 16 (0 generated)
Sadrul, can you please review this cl? Thanks, Shu
Consulted suzhe@, this cl takes a wrong way to fix the bug. Will update the patchset soon.
jochen@, can you please help to review this cl? creis@ and jamesr@ are both out of office. Thanks, Shu
is it possible to write a test for this?
jochen@, thanks for your review. I'm going to do a small refactoring soon to merge TextInputTypeChanged to TextInputStateChanged message. After that refactoring, the tests for this cl won't make sense any more. I would prefer to include necessary tests in that refactoring CL instead of this one. This cl tends to be a quick fix and the tests for it would likely be wasted in the near future (in M37 time frame).
On 2014/05/22 09:02:10, Shu Chen wrote: > jochen@, thanks for your review. I'm going to do a small refactoring soon to > merge TextInputTypeChanged to TextInputStateChanged message. > After that refactoring, the tests for this cl won't make sense any more. > I would prefer to include necessary tests in that refactoring CL instead of this > one. This cl tends to be a quick fix and the tests for it would likely be wasted > in the near future (in M37 time frame). I'm not sure I follow. A test would help to ensure that when you refactor this in the future, you don't break the functionality, no?
On 2014/05/22 12:22:24, jochen wrote: > On 2014/05/22 09:02:10, Shu Chen wrote: > > jochen@, thanks for your review. I'm going to do a small refactoring soon to > > merge TextInputTypeChanged to TextInputStateChanged message. > > After that refactoring, the tests for this cl won't make sense any more. > > I would prefer to include necessary tests in that refactoring CL instead of > this > > one. This cl tends to be a quick fix and the tests for it would likely be > wasted > > in the near future (in M37 time frame). > > I'm not sure I follow. > > A test would help to ensure that when you refactor this in the future, you don't > break the functionality, no? I assume the test you suggested for this change would be like "test the TextInputTypeChanged message is sent when UpdateTextInputState". If that's true, the test will be obsolete because there won't be TextInputTypeChanged message after the refactoring. And if you were suggesting an end-to-end test to simulate the bug's repro steps, I think it would be very hard to automate/verify in tests.
On 2014/05/22 14:33:37, Shu Chen wrote: > On 2014/05/22 12:22:24, jochen wrote: > > On 2014/05/22 09:02:10, Shu Chen wrote: > > > jochen@, thanks for your review. I'm going to do a small refactoring soon to > > > merge TextInputTypeChanged to TextInputStateChanged message. > > > After that refactoring, the tests for this cl won't make sense any more. > > > I would prefer to include necessary tests in that refactoring CL instead of > > this > > > one. This cl tends to be a quick fix and the tests for it would likely be > > wasted > > > in the near future (in M37 time frame). > > > > I'm not sure I follow. > > > > A test would help to ensure that when you refactor this in the future, you > don't > > break the functionality, no? > > I assume the test you suggested for this change would be like "test the > TextInputTypeChanged message is sent when UpdateTextInputState". > If that's true, the test will be obsolete because there won't be > TextInputTypeChanged message after the refactoring. > And if you were suggesting an end-to-end test to simulate the bug's repro steps, > I think it would be very hard to automate/verify in tests. as is, it's very difficult to judge for me whether the CL actually fixes the bug. e.g. in 1627, you change the behavior for datetime inputs, but in line 1657 you don't. Having entire code paths that are not covered by tests is very bad for the entire project - the feature will constantly break because ppl update the code during refactorings without any indication of whether the intended functionality was broken or not.
On 2014/05/22 14:46:41, jochen wrote: > On 2014/05/22 14:33:37, Shu Chen wrote: > > On 2014/05/22 12:22:24, jochen wrote: > > > On 2014/05/22 09:02:10, Shu Chen wrote: > > > > jochen@, thanks for your review. I'm going to do a small refactoring soon > to > > > > merge TextInputTypeChanged to TextInputStateChanged message. > > > > After that refactoring, the tests for this cl won't make sense any more. > > > > I would prefer to include necessary tests in that refactoring CL instead > of > > > this > > > > one. This cl tends to be a quick fix and the tests for it would likely be > > > wasted > > > > in the near future (in M37 time frame). > > > > > > I'm not sure I follow. > > > > > > A test would help to ensure that when you refactor this in the future, you > > don't > > > break the functionality, no? > > > > I assume the test you suggested for this change would be like "test the > > TextInputTypeChanged message is sent when UpdateTextInputState". > > If that's true, the test will be obsolete because there won't be > > TextInputTypeChanged message after the refactoring. > > And if you were suggesting an end-to-end test to simulate the bug's repro > steps, > > I think it would be very hard to automate/verify in tests. > > as is, it's very difficult to judge for me whether the CL actually fixes the > bug. > > e.g. in 1627, you change the behavior for datetime inputs, but in line 1657 you > don't. > > Having entire code paths that are not covered by tests is very bad for the > entire project - the feature will constantly break because ppl update the code > during refactorings without any indication of whether the intended functionality > was broken or not. The change in 1627 was not the actual fix to the bug. I've reverted that part. The bug was introduced by cl https://codereview.chromium.org/29943002. It's obvious that the cl missed the consideration for consistency between TextInputStateChanged and TextInputTypeChanged messages. With the knowledge of how IMF works, I think the risk of causing regressions by this cl is limited. (yukishiino@ could also help to estimate the risks) I can't agree more that it's bad we don't have the tests cover the IME logics in renderer. However, if you think it's ok to hold on this P1 bug fix until we have a meaningful test coverage, I would be ok to drop this cl and star working on the refactoring (with test consideration) aggressively. Thanks, Shu
On 2014/05/24 10:40:01, Shu Chen wrote: > On 2014/05/22 14:46:41, jochen wrote: > > On 2014/05/22 14:33:37, Shu Chen wrote: > > > On 2014/05/22 12:22:24, jochen wrote: > > > > On 2014/05/22 09:02:10, Shu Chen wrote: > > > > > jochen@, thanks for your review. I'm going to do a small refactoring > soon > > to > > > > > merge TextInputTypeChanged to TextInputStateChanged message. > > > > > After that refactoring, the tests for this cl won't make sense any more. > > > > > I would prefer to include necessary tests in that refactoring CL instead > > of > > > > this > > > > > one. This cl tends to be a quick fix and the tests for it would likely > be > > > > wasted > > > > > in the near future (in M37 time frame). > > > > > > > > I'm not sure I follow. > > > > > > > > A test would help to ensure that when you refactor this in the future, you > > > don't > > > > break the functionality, no? > > > > > > I assume the test you suggested for this change would be like "test the > > > TextInputTypeChanged message is sent when UpdateTextInputState". > > > If that's true, the test will be obsolete because there won't be > > > TextInputTypeChanged message after the refactoring. > > > And if you were suggesting an end-to-end test to simulate the bug's repro > > steps, > > > I think it would be very hard to automate/verify in tests. > > > > as is, it's very difficult to judge for me whether the CL actually fixes the > > bug. > > > > e.g. in 1627, you change the behavior for datetime inputs, but in line 1657 > you > > don't. > > > > Having entire code paths that are not covered by tests is very bad for the > > entire project - the feature will constantly break because ppl update the code > > during refactorings without any indication of whether the intended > functionality > > was broken or not. > > The change in 1627 was not the actual fix to the bug. I've reverted that part. > > The bug was introduced by cl https://codereview.chromium.org/29943002. > It's obvious that the cl missed the consideration for consistency between > TextInputStateChanged and TextInputTypeChanged messages. > With the knowledge of how IMF works, I think the risk of causing regressions by > this cl is limited. (yukishiino@ could also help to estimate the risks) > > I can't agree more that it's bad we don't have the tests cover the IME logics in > renderer. > However, if you think it's ok to hold on this P1 bug fix until we have a > meaningful test coverage, I would be ok to drop this cl and star working on the > refactoring (with test consideration) aggressively. > > Thanks, > Shu ok, lgtm then I trust you that you will add appropriate test coverage for this during/after the refactoring.
https://codereview.chromium.org/299793002/diff/40001/content/renderer/render_... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/299793002/diff/40001/content/renderer/render_... content/renderer/render_widget.cc:1626: return; // Not considered as a text input field in WebKit/Chromium. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Implem... Put two spaces before comments.
Thanks for your review! crbug.com/377169 is opened to track the refactoring. https://codereview.chromium.org/299793002/diff/40001/content/renderer/render_... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/299793002/diff/40001/content/renderer/render_... content/renderer/render_widget.cc:1626: return; // Not considered as a text input field in WebKit/Chromium. On 2014/05/24 12:11:33, Yuki wrote: > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Implem... > > Put two spaces before comments. Done.
The CQ bit was checked by shuchen@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/shuchen@chromium.org/299793002/60001
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: android_aosp on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...) android_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/18...)
Message was sent while issue was closed.
Change committed as 272784 |
