Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
843603004:
Moves smart deploy UI into the IME tray. (Closed)
DescriptionMoves smart deploy UI into the IME tray.
BUG=447642, 443766
TEST=TrayIMETest.PerformActionOnDetailedView, TrayIMETest.HidesOnA11yEnabled, TrayIMETest.HiddenWithNoIMEs
Committed: https://crrev.com/e94b540b1f2da1bfca727d30f51242d18214f3d9
Cr-Commit-Position: refs/heads/master@{#311394}
Patch Set 1 : Made tray_ime chromeos only. #
Total comments: 17
Patch Set 2 : Address reviewer comments. #Patch Set 3 : Added unittests + fixed IME refresh bug. #Patch Set 4 : Revert accidental edit of a file. #
Total comments: 13
Patch Set 5 : Address reviewer feedback. #Patch Set 6 : Re-add visibility test. #Patch Set 7 : Split into chromeos #Patch Set 8 : Rebase to master. #Patch Set 9 : Unsplit. Remove IME tray in Linux. #
Messages
Total messages: 35 (9 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
rsadam@chromium.org changed reviewers: + skuhne@chromium.org
Hi Skuhne, PTAL! tray_ime is only ever instantiated in chromeos and the new keyboard related code i'm adding has some chromeos references so I changed the entire file to be chromeos only. Please let me know if this approach is okay, and if so I'll start working on the unit tests to add to this patch.
It would also be great if you could add some unit tests to verify that the behavior is what you expect. https://codereview.chromium.org/843603004/diff/40001/ash/system/ime/tray_ime_... File ash/system/ime/tray_ime_chromeos.cc (right): https://codereview.chromium.org/843603004/diff/40001/ash/system/ime/tray_ime_... ash/system/ime/tray_ime_chromeos.cc:51: AddLabel(label, gfx::ALIGN_LEFT, According to style guide each parameter needs to go into a separate line if there is a break. https://codereview.chromium.org/843603004/diff/40001/ash/system/ime/tray_ime_... ash/system/ime/tray_ime_chromeos.cc:140: void AppendIMEList(const IMEInfoList& list) { I am not sure that this is correct. Isn't it conceivable that you get multiple Update commands which add the same elements? If so you might end up with multiple items of the same type. Also - looking at the change it appears that you do only incremental changes. Couldn't an IME entry disappear? So if this is indeed incremental (and you want to get rid of these clear's), you should add some logic in line 142 to not add the entry if it already exists. https://codereview.chromium.org/843603004/diff/40001/ash/system/ime/tray_ime_... ash/system/ime/tray_ime_chromeos.cc:150: for (size_t i = 0; i < property_list.size(); i++) { Same here. Shouldn't you check if the property doesn't already exist before you add it again? At least a DCHECK of some kind might be good. https://codereview.chromium.org/843603004/diff/40001/ash/system/ime/tray_ime_... ash/system/ime/tray_ime_chromeos.cc:178: gfx::ALIGN_LEFT, gfx::Font::NORMAL); As before: According to style guide: either all parameter fit into a single line - or - each parameter goes into a separate line as it was before. https://codereview.chromium.org/843603004/diff/40001/ash/system/ime/tray_ime_... ash/system/ime/tray_ime_chromeos.cc:246: this); I know it does not make a difference - but could you please reverse the order of the two Observer's to inversely match the adding? https://codereview.chromium.org/843603004/diff/40001/ash/system/ime/tray_ime_... ash/system/ime/tray_ime_chromeos.cc:296: base::string16 TrayIME::GetDefaultViewLabel(bool show_ime_label) { The order of the functions in the c++ file should match the order of the functions as defined in the header file. https://codereview.chromium.org/843603004/diff/40001/ash/system/ime/tray_ime_... File ash/system/ime/tray_ime_chromeos.h (right): https://codereview.chromium.org/843603004/diff/40001/ash/system/ime/tray_ime_... ash/system/ime/tray_ime_chromeos.h:44: void Update(); You might want to add some better comments here towards what these functions do.
On 2015/01/09 22:50:38, Mr4D wrote: > It would also be great if you could add some unit tests to verify that the > behavior is what you expect. I wasn't sure my approach of making this file chromeos only was correct, which is why I hadn't written the tests yet. Will add them presently.
https://codereview.chromium.org/843603004/diff/40001/ash/system/ime/tray_ime_... File ash/system/ime/tray_ime_chromeos.cc (right): https://codereview.chromium.org/843603004/diff/40001/ash/system/ime/tray_ime_... ash/system/ime/tray_ime_chromeos.cc:51: AddLabel(label, gfx::ALIGN_LEFT, On 2015/01/09 22:50:38, Mr4D wrote: > According to style guide each parameter needs to go into a separate line if > there is a break. Weird, git cl format changes it to the wrong thing. I'll file a bug for that. https://codereview.chromium.org/843603004/diff/40001/ash/system/ime/tray_ime_... ash/system/ime/tray_ime_chromeos.cc:140: void AppendIMEList(const IMEInfoList& list) { On 2015/01/09 22:50:38, Mr4D wrote: > I am not sure that this is correct. Isn't it conceivable that you get multiple > Update commands which add the same elements? If so you might end up with > multiple items of the same type. > > Also - looking at the change it appears that you do only incremental changes. > Couldn't an IME entry disappear? > > So if this is indeed incremental (and you want to get rid of these clear's), you > should add some logic in line 142 to not add the entry if it already exists. I'm not sure I follow. AppendIMEList is only ever called once per update, and so this list wouldn't have any duplicates. Would you mind clarifying what you mean by incremental updates? https://codereview.chromium.org/843603004/diff/40001/ash/system/ime/tray_ime_... ash/system/ime/tray_ime_chromeos.cc:150: for (size_t i = 0; i < property_list.size(); i++) { On 2015/01/09 22:50:38, Mr4D wrote: > Same here. Shouldn't you check if the property doesn't already exist before you > add it again? At least a DCHECK of some kind might be good. Same as above, I think the confusion might be in the original naming of the function. AppendIMEProperties doesn't append it to the existing property list, but rather to the scrollable content. I added comments to make this clearer. https://codereview.chromium.org/843603004/diff/40001/ash/system/ime/tray_ime_... ash/system/ime/tray_ime_chromeos.cc:178: gfx::ALIGN_LEFT, gfx::Font::NORMAL); On 2015/01/09 22:50:38, Mr4D wrote: > As before: According to style guide: either all parameter fit into a single line > - or - each parameter goes into a separate line as it was before. Seems git cl format borked here as well. Manually reverted, will follow up. https://codereview.chromium.org/843603004/diff/40001/ash/system/ime/tray_ime_... ash/system/ime/tray_ime_chromeos.cc:246: this); On 2015/01/09 22:50:38, Mr4D wrote: > I know it does not make a difference - but could you please reverse the order of > the two Observer's to inversely match the adding? Done. https://codereview.chromium.org/843603004/diff/40001/ash/system/ime/tray_ime_... ash/system/ime/tray_ime_chromeos.cc:296: base::string16 TrayIME::GetDefaultViewLabel(bool show_ime_label) { On 2015/01/09 22:50:38, Mr4D wrote: > The order of the functions in the c++ file should match the order of the > functions as defined in the header file. Done. https://codereview.chromium.org/843603004/diff/40001/ash/system/ime/tray_ime_... File ash/system/ime/tray_ime_chromeos.h (right): https://codereview.chromium.org/843603004/diff/40001/ash/system/ime/tray_ime_... ash/system/ime/tray_ime_chromeos.h:44: void Update(); On 2015/01/09 22:50:38, Mr4D wrote: > You might want to add some better comments here towards what these functions do. Done.
On 2015/01/09 23:44:53, rsadam wrote: > On 2015/01/09 22:50:38, Mr4D wrote: > > It would also be great if you could add some unit tests to verify that the > > behavior is what you expect. > > I wasn't sure my approach of making this file chromeos only was correct, which > is why I hadn't written the tests yet. Will add them presently. I wasn't either, but according to system_tray.cc the item only got added for ChromeOS - so it appears that for Windows we must handle the VK differently. Do you add the tests to this CL then?
As stated before - yes, I was also irritated about OS_CHROMEOS, but looking at system_tray.cc shows that this is a ChromeOS only handling (at least for now). I am waiting then for the unit tests(?). https://codereview.chromium.org/843603004/diff/40001/ash/system/ime/tray_ime_... File ash/system/ime/tray_ime_chromeos.cc (right): https://codereview.chromium.org/843603004/diff/40001/ash/system/ime/tray_ime_... ash/system/ime/tray_ime_chromeos.cc:51: AddLabel(label, gfx::ALIGN_LEFT, Hmm. interesting - let me know how that goes! (I personally do not mind shortening the listings by combining parameters into a line). https://codereview.chromium.org/843603004/diff/40001/ash/system/ime/tray_ime_... ash/system/ime/tray_ime_chromeos.cc:140: void AppendIMEList(const IMEInfoList& list) { Ah. I haven't seen that you have cleared the maps already in Update above. However.. if someone would add a call to this function later on, he will get into problems. As such I would add a DCHECK(ime_map_.empty()); at the beginning of this function to make sure that this assumption is maintained. https://codereview.chromium.org/843603004/diff/40001/ash/system/ime/tray_ime_... ash/system/ime/tray_ime_chromeos.cc:150: for (size_t i = 0; i < property_list.size(); i++) { Same reasoning. Yes, in the moment this is correct, but there is no guarantee that someone might not call it later from somewhere else. So please add a dcheck to make sure that this does not happen.
Found a bug when writing the unittests with how defaultview is created, fixed in this version.
Accidentally edited the original tray_keyboard_test file, reverted.
Only a few more nits and we are there! https://codereview.chromium.org/843603004/diff/100001/ash/system/ime/tray_ime... File ash/system/ime/tray_ime_chromeos.cc (right): https://codereview.chromium.org/843603004/diff/100001/ash/system/ime/tray_ime... ash/system/ime/tray_ime_chromeos.cc:141: friend class TrayIMETest; I think this can go... https://codereview.chromium.org/843603004/diff/100001/ash/system/ime/tray_ime... ash/system/ime/tray_ime_chromeos.cc:189: gfx::ALIGN_LEFT, gfx::Font::NORMAL); And another of this missing parameter line breaks ... https://codereview.chromium.org/843603004/diff/100001/ash/system/ime/tray_ime... ash/system/ime/tray_ime_chromeos.cc:204: Shell::GetInstance() Sorry for the nit, but you should be able to move line 205 at the end of 204 and then break into one more line (resulting one line less). https://codereview.chromium.org/843603004/diff/100001/ash/system/ime/tray_ime... File ash/system/ime/tray_ime_chromeos.h (right): https://codereview.chromium.org/843603004/diff/100001/ash/system/ime/tray_ime... ash/system/ime/tray_ime_chromeos.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. Can you remove the "(c) " ? (we apparently do not use that anymore) https://codereview.chromium.org/843603004/diff/100001/ash/system/ime/tray_ime... ash/system/ime/tray_ime_chromeos.h:10: #include "ash/system/tray/system_tray_delegate.h" Just checking - Why do we need this header include (system_tray_delegate.h)? https://codereview.chromium.org/843603004/diff/100001/ash/system/ime/tray_ime... File ash/system/ime/tray_ime_chromeos_unittest.cc (right): https://codereview.chromium.org/843603004/diff/100001/ash/system/ime/tray_ime... ash/system/ime/tray_ime_chromeos_unittest.cc:159: EXPECT_FALSE(keyboard::IsKeyboardEnabled()); If the opposite is now true (!visible) - chouldn't you explicitly check for that?
https://codereview.chromium.org/843603004/diff/100001/ash/system/ime/tray_ime... File ash/system/ime/tray_ime_chromeos.cc (right): https://codereview.chromium.org/843603004/diff/100001/ash/system/ime/tray_ime... ash/system/ime/tray_ime_chromeos.cc:141: friend class TrayIMETest; On 2015/01/12 22:35:37, Mr4D wrote: > I think this can go... Done. https://codereview.chromium.org/843603004/diff/100001/ash/system/ime/tray_ime... ash/system/ime/tray_ime_chromeos.cc:189: gfx::ALIGN_LEFT, gfx::Font::NORMAL); On 2015/01/12 22:35:37, Mr4D wrote: > And another of this missing parameter line breaks ... Oops. Removed. https://codereview.chromium.org/843603004/diff/100001/ash/system/ime/tray_ime... ash/system/ime/tray_ime_chromeos.cc:204: Shell::GetInstance() On 2015/01/12 22:35:37, Mr4D wrote: > Sorry for the nit, but you should be able to move line 205 at the end of 204 and > then break into one more line (resulting one line less). I completely missed that, thanks for pointing it out :) Fixed! https://codereview.chromium.org/843603004/diff/100001/ash/system/ime/tray_ime... File ash/system/ime/tray_ime_chromeos.h (right): https://codereview.chromium.org/843603004/diff/100001/ash/system/ime/tray_ime... ash/system/ime/tray_ime_chromeos.h:10: #include "ash/system/tray/system_tray_delegate.h" On 2015/01/12 22:35:37, Mr4D wrote: > Just checking - Why do we need this header include (system_tray_delegate.h)? For definitions of IMEPropertyInfoList and IMEInfoList. https://codereview.chromium.org/843603004/diff/100001/ash/system/ime/tray_ime... File ash/system/ime/tray_ime_chromeos_unittest.cc (right): https://codereview.chromium.org/843603004/diff/100001/ash/system/ime/tray_ime... ash/system/ime/tray_ime_chromeos_unittest.cc:159: EXPECT_FALSE(keyboard::IsKeyboardEnabled()); On 2015/01/12 22:35:37, Mr4D wrote: > If the opposite is now true (!visible) - chouldn't you explicitly check for > that? I'm not sure I follow - the keyboard is disabled, but the option to re-enable it should now be there.
The one test - is the default view visible or not (or - why did you remove the test)? https://codereview.chromium.org/843603004/diff/100001/ash/system/ime/tray_ime... File ash/system/ime/tray_ime_chromeos.h (right): https://codereview.chromium.org/843603004/diff/100001/ash/system/ime/tray_ime... ash/system/ime/tray_ime_chromeos.h:10: #include "ash/system/tray/system_tray_delegate.h" On 2015/01/13 00:05:34, rsadam wrote: > On 2015/01/12 22:35:37, Mr4D wrote: > > Just checking - Why do we need this header include (system_tray_delegate.h)? > > For definitions of IMEPropertyInfoList and IMEInfoList. Acknowledged. https://codereview.chromium.org/843603004/diff/100001/ash/system/ime/tray_ime... File ash/system/ime/tray_ime_chromeos_unittest.cc (right): https://codereview.chromium.org/843603004/diff/100001/ash/system/ime/tray_ime... ash/system/ime/tray_ime_chromeos_unittest.cc:159: EXPECT_FALSE(keyboard::IsKeyboardEnabled()); Oh - I have meant the test which you have removed (EXPECT_TRUE(default_view()->visible());) - sorry to not be clearer.
In the original test the toggle was in the default view which is why I added the check that it remains visible during the toggle. In the current test, this has been moved to the detailed view, and so the visibility of the default view didn't seem important. I could have changed it to check if the detailed_view was visible, but we never change it's visibility. On further though, perhaps it's worth checking that toggling the keyboard does not affect the visibility of the original default view since if that regressed the user would be stuck with the keyboard always on/off which would be add. Re-added.
Thanks! lgtm
The CQ bit was checked by rsadam@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/843603004/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_gn_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2015/01/13 19:17:07, I haz the power (commit-bot) wrote: > Try jobs failed on following builders: > linux_chromium_gn_rel on tryserver.chromium.linux > (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) Seems I missed a place where TrayIME is added - it's also added if OS_LINUX. Would it make sense to split the code into tray_ime and tray_ime_chromeos?
Patchset #8 (id:180001) has been deleted
PTANL! Split the class into the CROS and non CROS parts.
Rebased to master - in hindsight I should have landed this first before deleting the old UI, since the unittest file was based on the previous which confused git.
Patchset #8 (id:200001) has been deleted
As discussed offline, removed the IMETray for Linux. Seems all the bots are green (other than ios, but that's unrelated to this CL)
As discussed offline - LGTM!
The CQ bit was checked by rsadam@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/843603004/240001
Message was sent while issue was closed.
Committed patchset #9 (id:240001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/e94b540b1f2da1bfca727d30f51242d18214f3d9 Cr-Commit-Position: refs/heads/master@{#311394}
Message was sent while issue was closed.
plundblad@chromium.org changed reviewers: + plundblad@chromium.org
Message was sent while issue was closed.
Hi, I just discovered that it broke a feature related to the Braille IME that is used for inputting braille using either a braille display or cording on a standrard 'PC' keyboard. See crbug.com/458529. The issue is that this CL requires an IME to expose more than one menu item for any item to be shown and not just one item. I think the checks for property_list.size() > 1 should have remaing as !property_list.emtpy() in the Update method in tray_ime_chromeos.cc. Was there a reason for that particular change or was it just a mistake? (I understand why the check was change for the ime list, I think, this is about the IME properties). Since I have the hardware and this is trivial, I'll work on a fix, just wanting to know if I am missing something here.
Message was sent while issue was closed.
I see. Yes, I have reviewed the CL and I was wondering about that change, but was under the impression that if there is only one choice, there needs not to be an option. I guess that was not correct then - but you should wait for rsadam to comment. -Stefan On Fri, Feb 13, 2015 at 7:07 AM, <plundblad@chromium.org> wrote: > Hi, > > I just discovered that it broke a feature related to the Braille IME that > is > used for inputting braille using either a braille display or cording on a > standrard 'PC' keyboard. See crbug.com/458529. > The issue is that this CL requires an IME to expose more than one menu > item for > any item to be shown and not just one item. I think the checks for > property_list.size() > 1 should have remaing as !property_list.emtpy() in > the > Update method in tray_ime_chromeos.cc. Was there a reason for that > particular > change or was it just a mistake? (I understand why the check was change > for the > ime list, I think, this is about the IME properties). > > Since I have the hardware and this is trivial, I'll work on a fix, just > wanting > to know if I am missing something here. > > https://codereview.chromium.org/843603004/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
Hi, I didn't realize that property_list items were checkboxes - in which case having one item makes sense, unlike in the list of IME where it doesn't. I don't think that changing this back would break anything - though please update the check 4 lines below as well, since we'll want the scroll-separator if only one property is present. On Fri, Feb 13, 2015 at 11:03 AM, Stefan Kuhne <skuhne@chromium.org> wrote: > I see. Yes, I have reviewed the CL and I was wondering about that change, > but was under the impression that if there is only one choice, there needs > not to be an option. > > I guess that was not correct then - but you should wait for rsadam to > comment. > > -Stefan > > On Fri, Feb 13, 2015 at 7:07 AM, <plundblad@chromium.org> wrote: > >> Hi, >> >> I just discovered that it broke a feature related to the Braille IME that >> is >> used for inputting braille using either a braille display or cording on a >> standrard 'PC' keyboard. See crbug.com/458529. >> The issue is that this CL requires an IME to expose more than one menu >> item for >> any item to be shown and not just one item. I think the checks for >> property_list.size() > 1 should have remaing as !property_list.emtpy() in >> the >> Update method in tray_ime_chromeos.cc. Was there a reason for that >> particular >> change or was it just a mistake? (I understand why the check was change >> for the >> ime list, I think, this is about the IME properties). >> >> Since I have the hardware and this is trivial, I'll work on a fix, just >> wanting >> to know if I am missing something here. >> >> https://codereview.chromium.org/843603004/ >> > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
