Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1996563002:
Add ImeMenuTray element. (Closed)
DescriptionAdds ImeMenuTray element on status area widget for IME menu. It has the following behaviors:
- Lays on the status tray, which has a consistent spacing between other shelf items.
- Unmoveable by users, but automatically moving when status tray show/hide other items, like VK/web notification tray element.
- Show/Hide the icon according to activation of new menu feature.
- Show the short name of the current selected IME.
- Change background color when clicked.
BUG=570761, 624186
TEST=Verified on local build.
Committed: https://crrev.com/c0b0a4f3a8397cb257d975d947bf035e2b0d9a9e
Cr-Commit-Position: refs/heads/master@{#405728}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Check the valid of |last_item_index|. #
Total comments: 4
Patch Set 3 : Addressed sky@'s comments. #Patch Set 4 : Add ImeMenuTray. #Patch Set 5 : Revert error change. #Patch Set 6 : #Patch Set 7 : Add tests for ImeMenuTray. #Patch Set 8 : Fix patch confict. #Patch Set 9 #Patch Set 10 #Patch Set 11 : Fix build failure. #Patch Set 12 #Patch Set 13 #Patch Set 14 #
Total comments: 15
Patch Set 15 : Add kShowIconOnTrayKey property for window. #
Total comments: 2
Patch Set 16 : Remove kStatusTrayShelfID. #
Total comments: 17
Patch Set 17 : Addressed Sadrul's comments. #Patch Set 18 : Rename property kShelfItemOnTrayForImeMenu. #Patch Set 19 : Activates window when clicked. #
Total comments: 12
Patch Set 20 : Addressed comments. #Patch Set 21 : sync code. #
Total comments: 2
Patch Set 22 : Change SetWindowState() with SetImeWindow(). #
Total comments: 12
Patch Set 23 : Move to ash/common/ and use WmWindow. #
Total comments: 32
Patch Set 24 : Add test file. #Patch Set 25 : Remove window related changes. #
Total comments: 21
Patch Set 26 : Addressed comments. #
Total comments: 11
Patch Set 27 : Addressed nits. #Patch Set 28 : Fix patch failure. #Messages
Total messages: 98 (30 generated)
Description was changed from ========== Make IME menu closer to the status tray. There has consistent spacing between panel windows with notification center/ status tray. This cl makes the spacing exists between the panels and the IME menu, and makes the IME menu closer with the status tray, to make the IME menu more system-like. BUG=570761 TEST=Verified on local build. ========== to ========== Make IME menu closer to the status tray. There has consistent spacing between panel windows with notification center/ status tray. This cl makes the spacing exists between the panels and the IME menu, and makes the IME menu closer with the status tray, to make the IME menu more system-like. BUG=570761 TEST=Verified on local build. ==========
azurewei@chromium.org changed reviewers: + shuchen@chromium.org, sky@chromium.org
Hi Shu and sky@, please review this cl. Thanks!
Can you add screenshots to the bug as to what you are after. Did you consider not showing an item for IME in the shelf and instead having it in the tray? That seems more of what you are after. https://codereview.chromium.org/1996563002/diff/1/ash/shelf/shelf_view.cc File ash/shelf/shelf_view.cc (right): https://codereview.chromium.org/1996563002/diff/1/ash/shelf/shelf_view.cc#new... ash/shelf/shelf_view.cc:837: int last_item_index = view_model_->view_size() - 1; Might view_size be 0 at this point?
I added the comment in the bug with screenshots: Before this cl: https://drive.google.com/open?id=0Bw82WB1eNjSBZFlTanZGMzFfcE0 After this cl: https://drive.google.com/open?id=0Bw82WB1eNjSBby1OaXAyck5CeEU This bug is reviewed by UI and based on their opinion, the menu should be rightmost, unable to move and having consistent spacing between with other panels. The position of the menu (left or right of the notification center) is acceptable. On the other hand, the menu is created with panel API, and we've done the most work based on the panel attributes. For example, the menu icon will change when switching input methods, or opening/closing the menu. I'm not sure the work effort of creating a panel item in the tray. It seems requires more codes changing of the panel API. This cl seems a easier way to change the shown behavior of the menu. https://codereview.chromium.org/1996563002/diff/1/ash/shelf/shelf_view.cc File ash/shelf/shelf_view.cc (right): https://codereview.chromium.org/1996563002/diff/1/ash/shelf/shelf_view.cc#new... ash/shelf/shelf_view.cc:837: int last_item_index = view_model_->view_size() - 1; On 2016/05/19 16:32:48, sky wrote: > Might view_size be 0 at this point? Added the check of |last_item_index|.
sky@chromium.org changed reviewers: + sadrul@chromium.org
+sadrul for his thoughts. Seems to me this icon is better served from the tray rather than shelf Also, I don't your changes has subtle ramifications. It breaks DetermineLastVisibleIndex for one. https://codereview.chromium.org/1996563002/diff/20001/ash/shelf/shelf_view.cc File ash/shelf/shelf_view.cc (right): https://codereview.chromium.org/1996563002/diff/20001/ash/shelf/shelf_view.cc... ash/shelf/shelf_view.cc:836: // IME menu icon. This comment isn't helpful. It should describe what this code is doing, why we need to special case the ime menu. https://codereview.chromium.org/1996563002/diff/20001/ash/shelf/shelf_view.cc... ash/shelf/shelf_view.cc:838: bool is_last_item_menu = is_last_item_ime_menu
On 2016/05/20 16:23:39, sky wrote: > +sadrul for his thoughts. Seems to me this icon is better served from the tray > rather than shelf Agree. If we want this IME item to match the other items in the tray, then it should live with the items in tray.
On 2016/05/20 17:32:40, sadrul wrote: > On 2016/05/20 16:23:39, sky wrote: > > +sadrul for his thoughts. Seems to me this icon is better served from the tray > > rather than shelf > > Agree. If we want this IME item to match the other items in the tray, then it > should live with the items in tray. The IME item is a separated item, there won't be IME related items in the tray menu. This new IME menu feature is like pulling the traditional IME menu out of the tray menu and putting onto the shelf bar.
On Fri, May 20, 2016 at 8:34 PM, <shuchen@chromium.org> wrote: > On 2016/05/20 17:32:40, sadrul wrote: >> On 2016/05/20 16:23:39, sky wrote: >> > +sadrul for his thoughts. Seems to me this icon is better served from >> > the > tray >> > rather than shelf >> >> Agree. If we want this IME item to match the other items in the tray, then >> it >> should live with the items in tray. > > The IME item is a separated item, there won't be IME related items in the > tray > menu. > This new IME menu feature is like pulling the traditional IME menu out of > the > tray menu and putting onto the shelf bar. The mocks want to position the item as though it's in the tray. That to me says it should be part of the tray. Why do you think it is like pulling the menu out of the tray? -Scott -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
To make it clearer, I made a screenshot: https://drive.google.com/a/chromium.org/file/d/0Bw82WB1eNjSBWkktM1UxMU85VHc/v... I think Shu means, the IME menu will pull the traditional IME item out of the 'system tray' (green box in the picture). And you mean the red box when talking about 'tray'? Currently, there's a TrayIME item (yellow box) in the system tray (green box). When enabling the opt-in IME menu, the TrayIME item will be invisible (or pulled out). And we want to add a new button on the left of the system tray. As the IME menu is created with panel API, it will be put in the shelf view (blue box) by default. This cl makes the spacing from: {ShelfView |panel_icon|...|IME_menu|}{spacing}{VritualKeyboardTray}{WebNotificaitonTray}{SystemTray} to: {ShelfView |panel_icon|...{spacing}|IME_menu|}{VritualKeyboardTray}{WebNotificaitonTray}{SystemTray} to make the menu unlike part of the shelf view. By having the menu in the tray, do you mean I should add a new class like ImeMenuTray and make it work with panel API? https://codereview.chromium.org/1996563002/diff/20001/ash/shelf/shelf_view.cc File ash/shelf/shelf_view.cc (right): https://codereview.chromium.org/1996563002/diff/20001/ash/shelf/shelf_view.cc... ash/shelf/shelf_view.cc:836: // IME menu icon. On 2016/05/20 16:23:39, sky wrote: > This comment isn't helpful. It should describe what this code is doing, why we > need to special case the ime menu. Updated. https://codereview.chromium.org/1996563002/diff/20001/ash/shelf/shelf_view.cc... ash/shelf/shelf_view.cc:838: bool is_last_item_menu = On 2016/05/20 16:23:39, sky wrote: > is_last_item_ime_menu Done.
On Tue, May 24, 2016 at 12:21 AM, <azurewei@chromium.org> wrote: > To make it clearer, I made a screenshot: > https://drive.google.com/a/chromium.org/file/d/0Bw82WB1eNjSBWkktM1UxMU85VHc/v... > > I think Shu means, the IME menu will pull the traditional IME item out of > the > 'system tray' (green box in the picture). And you mean the red box when > talking > about 'tray'? > > Currently, there's a TrayIME item (yellow box) in the system tray (green > box). > When enabling the opt-in IME menu, the TrayIME item will be invisible (or > pulled > out). And we want to add a new button on the left of the system tray. > > As the IME menu is created with panel API, it will be put in the shelf view > (blue box) by default. This cl makes the spacing from: > {ShelfView > |panel_icon|...|IME_menu|}{spacing}{VritualKeyboardTray}{WebNotificaitonTray}{SystemTray} > to: > {ShelfView > |panel_icon|...{spacing}|IME_menu|}{VritualKeyboardTray}{WebNotificaitonTray}{SystemTray} > to make the menu unlike part of the shelf view. > > By having the menu in the tray, do you mean I should add a new class like > ImeMenuTray and make it work with panel API? Yes, that's what I was thinking. Couple of questions though. Does the IME_menu behave like other items on the shelf? Can you drag it? Does it get highlights beneath it? Thanks, -Scott > > > https://codereview.chromium.org/1996563002/diff/20001/ash/shelf/shelf_view.cc > File ash/shelf/shelf_view.cc (right): > > https://codereview.chromium.org/1996563002/diff/20001/ash/shelf/shelf_view.cc... > ash/shelf/shelf_view.cc:836: // IME menu icon. > On 2016/05/20 16:23:39, sky wrote: >> This comment isn't helpful. It should describe what this code is > doing, why we >> need to special case the ime menu. > > Updated. > > https://codereview.chromium.org/1996563002/diff/20001/ash/shelf/shelf_view.cc... > ash/shelf/shelf_view.cc:838: bool is_last_item_menu = > On 2016/05/20 16:23:39, sky wrote: >> is_last_item_ime_menu > > Done. > > https://codereview.chromium.org/1996563002/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/05/24 16:26:34, sky wrote: > On Tue, May 24, 2016 at 12:21 AM, <mailto:azurewei@chromium.org> wrote: > > To make it clearer, I made a screenshot: > > > https://drive.google.com/a/chromium.org/file/d/0Bw82WB1eNjSBWkktM1UxMU85VHc/v... > > > > I think Shu means, the IME menu will pull the traditional IME item out of > > the > > 'system tray' (green box in the picture). And you mean the red box when > > talking > > about 'tray'? > > > > Currently, there's a TrayIME item (yellow box) in the system tray (green > > box). > > When enabling the opt-in IME menu, the TrayIME item will be invisible (or > > pulled > > out). And we want to add a new button on the left of the system tray. > > > > As the IME menu is created with panel API, it will be put in the shelf view > > (blue box) by default. This cl makes the spacing from: > > {ShelfView > > > |panel_icon|...|IME_menu|}{spacing}{VritualKeyboardTray}{WebNotificaitonTray}{SystemTray} > > to: > > {ShelfView > > > |panel_icon|...{spacing}|IME_menu|}{VritualKeyboardTray}{WebNotificaitonTray}{SystemTray} > > to make the menu unlike part of the shelf view. > > > > By having the menu in the tray, do you mean I should add a new class like > > ImeMenuTray and make it work with panel API? > > Yes, that's what I was thinking. Couple of questions though. Does the > IME_menu behave like other items on the shelf? Can you drag it? Does > it get highlights beneath it? > > Thanks, > > -Scott The menu is unlike other shelf items in these ways: 1) It can't be dragged. (Done in this cl: https://codereview.chromium.org/1978443002/) 2) It can't be closed with right-click. (Done in this cl: https://codereview.chromium.org/1611463002/) Besides these, the menu has other features implemented with JS codes: 3) Be highlighted (blue) when clicking/opening the menu. 4) Changes the text when switching the current input method. 5) ChromeVox says 'Input menu button, current keyboard: xxxx' when focusing.
All of these say to me the item should be part of the tray, or alternatively handled by something else. They don't really feel part of the shelf. On Tue, May 24, 2016 at 6:41 PM, <azurewei@chromium.org> wrote: > On 2016/05/24 16:26:34, sky wrote: > >> On Tue, May 24, 2016 at 12:21 AM, <mailto:azurewei@chromium.org> wrote: >> > To make it clearer, I made a screenshot: >> > >> > https://drive.google.com/a/chromium.org/file/d/0Bw82WB1eNjSBWkktM1UxMU85VHc/v... >> > >> > I think Shu means, the IME menu will pull the traditional IME item out >> > of >> > the >> > 'system tray' (green box in the picture). And you mean the red box when >> > talking >> > about 'tray'? >> > >> > Currently, there's a TrayIME item (yellow box) in the system tray (green >> > box). >> > When enabling the opt-in IME menu, the TrayIME item will be invisible >> > (or >> > pulled >> > out). And we want to add a new button on the left of the system tray. >> > >> > As the IME menu is created with panel API, it will be put in the shelf >> > view >> > (blue box) by default. This cl makes the spacing from: >> > {ShelfView >> > >> > |panel_icon|...|IME_menu|}{spacing}{VritualKeyboardTray}{WebNotificaitonTray}{SystemTray} >> > to: >> > {ShelfView >> > >> > |panel_icon|...{spacing}|IME_menu|}{VritualKeyboardTray}{WebNotificaitonTray}{SystemTray} >> > to make the menu unlike part of the shelf view. >> > >> > By having the menu in the tray, do you mean I should add a new class >> > like >> > ImeMenuTray and make it work with panel API? >> >> Yes, that's what I was thinking. Couple of questions though. Does the >> IME_menu behave like other items on the shelf? Can you drag it? Does >> it get highlights beneath it? >> >> Thanks, >> >> -Scott > > The menu is unlike other shelf items in these ways: > 1) It can't be dragged. (Done in this cl: > https://codereview.chromium.org/1978443002/) > 2) It can't be closed with right-click. (Done in this cl: > https://codereview.chromium.org/1611463002/) > > Besides these, the menu has other features implemented with JS codes: > 3) Be highlighted (blue) when clicking/opening the menu. > 4) Changes the text when switching the current input method. > 5) ChromeVox says 'Input menu button, current keyboard: xxxx' when focusing. > > > > > https://codereview.chromium.org/1996563002/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/05/25 15:49:10, sky wrote: > All of these say to me the item should be part of the tray, or > alternatively handled by something else. They don't really feel part > of the shelf. > > On Tue, May 24, 2016 at 6:41 PM, <mailto:azurewei@chromium.org> wrote: > > On 2016/05/24 16:26:34, sky wrote: > > > >> On Tue, May 24, 2016 at 12:21 AM, <mailto:azurewei@chromium.org> wrote: > >> > To make it clearer, I made a screenshot: > >> > > >> > > > https://drive.google.com/a/chromium.org/file/d/0Bw82WB1eNjSBWkktM1UxMU85VHc/v... > >> > > >> > I think Shu means, the IME menu will pull the traditional IME item out > >> > of > >> > the > >> > 'system tray' (green box in the picture). And you mean the red box when > >> > talking > >> > about 'tray'? > >> > > >> > Currently, there's a TrayIME item (yellow box) in the system tray (green > >> > box). > >> > When enabling the opt-in IME menu, the TrayIME item will be invisible > >> > (or > >> > pulled > >> > out). And we want to add a new button on the left of the system tray. > >> > > >> > As the IME menu is created with panel API, it will be put in the shelf > >> > view > >> > (blue box) by default. This cl makes the spacing from: > >> > {ShelfView > >> > > >> > > > |panel_icon|...|IME_menu|}{spacing}{VritualKeyboardTray}{WebNotificaitonTray}{SystemTray} > >> > to: > >> > {ShelfView > >> > > >> > > > |panel_icon|...{spacing}|IME_menu|}{VritualKeyboardTray}{WebNotificaitonTray}{SystemTray} > >> > to make the menu unlike part of the shelf view. > >> > > >> > By having the menu in the tray, do you mean I should add a new class > >> > like > >> > ImeMenuTray and make it work with panel API? > >> > >> Yes, that's what I was thinking. Couple of questions though. Does the > >> IME_menu behave like other items on the shelf? Can you drag it? Does > >> it get highlights beneath it? > >> > >> Thanks, > >> > >> -Scott > > > > The menu is unlike other shelf items in these ways: > > 1) It can't be dragged. (Done in this cl: > > https://codereview.chromium.org/1978443002/) > > 2) It can't be closed with right-click. (Done in this cl: > > https://codereview.chromium.org/1611463002/) > > > > Besides these, the menu has other features implemented with JS codes: > > 3) Be highlighted (blue) when clicking/opening the menu. > > 4) Changes the text when switching the current input method. > > 5) ChromeVox says 'Input menu button, current keyboard: xxxx' when focusing. > > > > > > > > > > https://codereview.chromium.org/1996563002/ > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. > Thanks for the advise. It makes sense to make the IME menu as a tray item rather than shelf item. But I don't see other advantage of doing this except for changing the IME menu's position, especially when I've already done all the other work (making it unmovable, unable to close) based on it's part of shelf. Let me confirm with UI reviewers if I have to make this change. And if you both persist, I will make it as a new ImeMenuTray item in the tray. Really appreciate your review.
On 2016/05/26 02:48:03, Azure Wei wrote: > On 2016/05/25 15:49:10, sky wrote: > > All of these say to me the item should be part of the tray, or > > alternatively handled by something else. They don't really feel part > > of the shelf. > > > > On Tue, May 24, 2016 at 6:41 PM, <mailto:azurewei@chromium.org> wrote: > > > On 2016/05/24 16:26:34, sky wrote: > > > > > >> On Tue, May 24, 2016 at 12:21 AM, <mailto:azurewei@chromium.org> wrote: > > >> > To make it clearer, I made a screenshot: > > >> > > > >> > > > > > > https://drive.google.com/a/chromium.org/file/d/0Bw82WB1eNjSBWkktM1UxMU85VHc/v... > > >> > > > >> > I think Shu means, the IME menu will pull the traditional IME item out > > >> > of > > >> > the > > >> > 'system tray' (green box in the picture). And you mean the red box when > > >> > talking > > >> > about 'tray'? > > >> > > > >> > Currently, there's a TrayIME item (yellow box) in the system tray (green > > >> > box). > > >> > When enabling the opt-in IME menu, the TrayIME item will be invisible > > >> > (or > > >> > pulled > > >> > out). And we want to add a new button on the left of the system tray. > > >> > > > >> > As the IME menu is created with panel API, it will be put in the shelf > > >> > view > > >> > (blue box) by default. This cl makes the spacing from: > > >> > {ShelfView > > >> > > > >> > > > > > > |panel_icon|...|IME_menu|}{spacing}{VritualKeyboardTray}{WebNotificaitonTray}{SystemTray} > > >> > to: > > >> > {ShelfView > > >> > > > >> > > > > > > |panel_icon|...{spacing}|IME_menu|}{VritualKeyboardTray}{WebNotificaitonTray}{SystemTray} > > >> > to make the menu unlike part of the shelf view. > > >> > > > >> > By having the menu in the tray, do you mean I should add a new class > > >> > like > > >> > ImeMenuTray and make it work with panel API? > > >> > > >> Yes, that's what I was thinking. Couple of questions though. Does the > > >> IME_menu behave like other items on the shelf? Can you drag it? Does > > >> it get highlights beneath it? > > >> > > >> Thanks, > > >> > > >> -Scott > > > > > > The menu is unlike other shelf items in these ways: > > > 1) It can't be dragged. (Done in this cl: > > > https://codereview.chromium.org/1978443002/) > > > 2) It can't be closed with right-click. (Done in this cl: > > > https://codereview.chromium.org/1611463002/) > > > > > > Besides these, the menu has other features implemented with JS codes: > > > 3) Be highlighted (blue) when clicking/opening the menu. > > > 4) Changes the text when switching the current input method. > > > 5) ChromeVox says 'Input menu button, current keyboard: xxxx' when focusing. > > > > > > > > > > > > > > > https://codereview.chromium.org/1996563002/ > > > > -- > > You received this message because you are subscribed to the Google Groups > > "Chromium-reviews" group. > > To unsubscribe from this group and stop receiving emails from it, send an > email > > to mailto:chromium-reviews+unsubscribe@chromium.org. > > > > Thanks for the advise. It makes sense to make the IME menu as a tray item rather > than shelf item. But I don't see other advantage of doing this except for > changing the IME menu's position, especially when I've already done all the > other work (making it unmovable, unable to close) based on it's part of shelf. > Let me confirm with UI reviewers if I have to make this change. And if you both > persist, I will make it as a new ImeMenuTray item in the tray. Really appreciate > your review. Working on it to make the item be part of the tray rather than shelf.
Description was changed from ========== Make IME menu closer to the status tray. There has consistent spacing between panel windows with notification center/ status tray. This cl makes the spacing exists between the panels and the IME menu, and makes the IME menu closer with the status tray, to make the IME menu more system-like. BUG=570761 TEST=Verified on local build. ========== to ========== Make IME menu closer to the status tray. There has consistent spacing between panel windows with notification center/ status tray. This cl puts the IME menu icon on the status tray rather than in the shelf. BUG=570761 TEST=Verified on local build. ==========
On 2016/06/07 03:07:51, Azure Wei wrote: > On 2016/05/26 02:48:03, Azure Wei wrote: > > On 2016/05/25 15:49:10, sky wrote: > > > All of these say to me the item should be part of the tray, or > > > alternatively handled by something else. They don't really feel part > > > of the shelf. > > > > > > On Tue, May 24, 2016 at 6:41 PM, <mailto:azurewei@chromium.org> wrote: > > > > On 2016/05/24 16:26:34, sky wrote: > > > > > > > >> On Tue, May 24, 2016 at 12:21 AM, <mailto:azurewei@chromium.org> wrote: > > > >> > To make it clearer, I made a screenshot: > > > >> > > > > >> > > > > > > > > > > https://drive.google.com/a/chromium.org/file/d/0Bw82WB1eNjSBWkktM1UxMU85VHc/v... > > > >> > > > > >> > I think Shu means, the IME menu will pull the traditional IME item out > > > >> > of > > > >> > the > > > >> > 'system tray' (green box in the picture). And you mean the red box when > > > >> > talking > > > >> > about 'tray'? > > > >> > > > > >> > Currently, there's a TrayIME item (yellow box) in the system tray > (green > > > >> > box). > > > >> > When enabling the opt-in IME menu, the TrayIME item will be invisible > > > >> > (or > > > >> > pulled > > > >> > out). And we want to add a new button on the left of the system tray. > > > >> > > > > >> > As the IME menu is created with panel API, it will be put in the shelf > > > >> > view > > > >> > (blue box) by default. This cl makes the spacing from: > > > >> > {ShelfView > > > >> > > > > >> > > > > > > > > > > |panel_icon|...|IME_menu|}{spacing}{VritualKeyboardTray}{WebNotificaitonTray}{SystemTray} > > > >> > to: > > > >> > {ShelfView > > > >> > > > > >> > > > > > > > > > > |panel_icon|...{spacing}|IME_menu|}{VritualKeyboardTray}{WebNotificaitonTray}{SystemTray} > > > >> > to make the menu unlike part of the shelf view. > > > >> > > > > >> > By having the menu in the tray, do you mean I should add a new class > > > >> > like > > > >> > ImeMenuTray and make it work with panel API? > > > >> > > > >> Yes, that's what I was thinking. Couple of questions though. Does the > > > >> IME_menu behave like other items on the shelf? Can you drag it? Does > > > >> it get highlights beneath it? > > > >> > > > >> Thanks, > > > >> > > > >> -Scott > > > > > > > > The menu is unlike other shelf items in these ways: > > > > 1) It can't be dragged. (Done in this cl: > > > > https://codereview.chromium.org/1978443002/) > > > > 2) It can't be closed with right-click. (Done in this cl: > > > > https://codereview.chromium.org/1611463002/) > > > > > > > > Besides these, the menu has other features implemented with JS codes: > > > > 3) Be highlighted (blue) when clicking/opening the menu. > > > > 4) Changes the text when switching the current input method. > > > > 5) ChromeVox says 'Input menu button, current keyboard: xxxx' when > focusing. > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1996563002/ > > > > > > -- > > > You received this message because you are subscribed to the Google Groups > > > "Chromium-reviews" group. > > > To unsubscribe from this group and stop receiving emails from it, send an > > email > > > to mailto:chromium-reviews+unsubscribe@chromium.org. > > > > > > > Thanks for the advise. It makes sense to make the IME menu as a tray item > rather > > than shelf item. But I don't see other advantage of doing this except for > > changing the IME menu's position, especially when I've already done all the > > other work (making it unmovable, unable to close) based on it's part of shelf. > > Let me confirm with UI reviewers if I have to make this change. And if you > both > > persist, I will make it as a new ImeMenuTray item in the tray. Really > appreciate > > your review. > > Working on it to make the item be part of the tray rather than shelf. Hi all, this cl has been updated. Please review it and I'll add more tests later. Thanks!
On 2016/06/07 14:59:03, Azure Wei wrote: > On 2016/06/07 03:07:51, Azure Wei wrote: > > On 2016/05/26 02:48:03, Azure Wei wrote: > > > On 2016/05/25 15:49:10, sky wrote: > > > > All of these say to me the item should be part of the tray, or > > > > alternatively handled by something else. They don't really feel part > > > > of the shelf. > > > > > > > > On Tue, May 24, 2016 at 6:41 PM, <mailto:azurewei@chromium.org> wrote: > > > > > On 2016/05/24 16:26:34, sky wrote: > > > > > > > > > >> On Tue, May 24, 2016 at 12:21 AM, <mailto:azurewei@chromium.org> wrote: > > > > >> > To make it clearer, I made a screenshot: > > > > >> > > > > > >> > > > > > > > > > > > > > > > https://drive.google.com/a/chromium.org/file/d/0Bw82WB1eNjSBWkktM1UxMU85VHc/v... > > > > >> > > > > > >> > I think Shu means, the IME menu will pull the traditional IME item > out > > > > >> > of > > > > >> > the > > > > >> > 'system tray' (green box in the picture). And you mean the red box > when > > > > >> > talking > > > > >> > about 'tray'? > > > > >> > > > > > >> > Currently, there's a TrayIME item (yellow box) in the system tray > > (green > > > > >> > box). > > > > >> > When enabling the opt-in IME menu, the TrayIME item will be invisible > > > > >> > (or > > > > >> > pulled > > > > >> > out). And we want to add a new button on the left of the system tray. > > > > >> > > > > > >> > As the IME menu is created with panel API, it will be put in the > shelf > > > > >> > view > > > > >> > (blue box) by default. This cl makes the spacing from: > > > > >> > {ShelfView > > > > >> > > > > > >> > > > > > > > > > > > > > > > |panel_icon|...|IME_menu|}{spacing}{VritualKeyboardTray}{WebNotificaitonTray}{SystemTray} > > > > >> > to: > > > > >> > {ShelfView > > > > >> > > > > > >> > > > > > > > > > > > > > > > |panel_icon|...{spacing}|IME_menu|}{VritualKeyboardTray}{WebNotificaitonTray}{SystemTray} > > > > >> > to make the menu unlike part of the shelf view. > > > > >> > > > > > >> > By having the menu in the tray, do you mean I should add a new class > > > > >> > like > > > > >> > ImeMenuTray and make it work with panel API? > > > > >> > > > > >> Yes, that's what I was thinking. Couple of questions though. Does the > > > > >> IME_menu behave like other items on the shelf? Can you drag it? Does > > > > >> it get highlights beneath it? > > > > >> > > > > >> Thanks, > > > > >> > > > > >> -Scott > > > > > > > > > > The menu is unlike other shelf items in these ways: > > > > > 1) It can't be dragged. (Done in this cl: > > > > > https://codereview.chromium.org/1978443002/) > > > > > 2) It can't be closed with right-click. (Done in this cl: > > > > > https://codereview.chromium.org/1611463002/) > > > > > > > > > > Besides these, the menu has other features implemented with JS codes: > > > > > 3) Be highlighted (blue) when clicking/opening the menu. > > > > > 4) Changes the text when switching the current input method. > > > > > 5) ChromeVox says 'Input menu button, current keyboard: xxxx' when > > focusing. > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1996563002/ > > > > > > > > -- > > > > You received this message because you are subscribed to the Google Groups > > > > "Chromium-reviews" group. > > > > To unsubscribe from this group and stop receiving emails from it, send an > > > email > > > > to mailto:chromium-reviews+unsubscribe@chromium.org. > > > > > > > > > > Thanks for the advise. It makes sense to make the IME menu as a tray item > > rather > > > than shelf item. But I don't see other advantage of doing this except for > > > changing the IME menu's position, especially when I've already done all the > > > other work (making it unmovable, unable to close) based on it's part of > shelf. > > > Let me confirm with UI reviewers if I have to make this change. And if you > > both > > > persist, I will make it as a new ImeMenuTray item in the tray. Really > > appreciate > > > your review. > > > > Working on it to make the item be part of the tray rather than shelf. > > Hi all, this cl has been updated. Please review it and I'll add more tests > later. Thanks! Unit tests were added for ImeMenuTray. Please review this cl when you have time. Thanks!
The CQ bit was checked by azurewei@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1996563002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by azurewei@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1996563002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) cast_shell_android on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by azurewei@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1996563002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by azurewei@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1996563002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by azurewei@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1996563002/200001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by azurewei@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1996563002/220001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Sadrul should be the main reviewer at this point.
The CQ bit was checked by azurewei@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1996563002/240001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by azurewei@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1996563002/260001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/06/16 15:14:58, sky wrote: > Sadrul should be the main reviewer at this point. soft ping~ Sadrul, can you review this cl? Thanks!
https://codereview.chromium.org/1996563002/diff/260001/ash/shelf/shelf.cc File ash/shelf/shelf.cc (right): https://codereview.chromium.org/1996563002/diff/260001/ash/shelf/shelf.cc#new... ash/shelf/shelf.cc:140: if (id == -1) { How can you tell that -1 is the ime tray item? https://codereview.chromium.org/1996563002/diff/260001/ash/system/chromeos/im... File ash/system/chromeos/ime_menu/ime_menu_tray.cc (right): https://codereview.chromium.org/1996563002/diff/260001/ash/system/chromeos/im... ash/system/chromeos/ime_menu/ime_menu_tray.cc:29: gfx::Size GetPreferredSize() const override { // views:Label: https://codereview.chromium.org/1996563002/diff/260001/ash/system/chromeos/im... ash/system/chromeos/ime_menu/ime_menu_tray.cc:32: }; DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/1996563002/diff/260001/ash/system/chromeos/im... ash/system/chromeos/ime_menu/ime_menu_tray.cc:125: window_state_ = window_state; This should be a DCHECK_EQ(window_state_, window_state) instead, right? (or just removed altogether) https://codereview.chromium.org/1996563002/diff/260001/ash/system/chromeos/im... ash/system/chromeos/ime_menu/ime_menu_tray.cc:147: base::string16 ImeMenuTray::GetLabelTextForTest() { function order in .cc should match the order in .h https://codereview.chromium.org/1996563002/diff/260001/ash/system/chromeos/im... File ash/system/chromeos/ime_menu/ime_menu_tray.h (right): https://codereview.chromium.org/1996563002/diff/260001/ash/system/chromeos/im... ash/system/chromeos/ime_menu/ime_menu_tray.h:53: void SetWindowState(wm::WindowState* window_state); non-overrides before overrides. Also, document. https://codereview.chromium.org/1996563002/diff/260001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc (right): https://codereview.chromium.org/1996563002/diff/260001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:133: ash::SetShelfIDForWindow(-1, window); I don't think setting special values for the ShelfID is a good way of doing this.
https://codereview.chromium.org/1996563002/diff/260001/ash/shelf/shelf.cc File ash/shelf/shelf.cc (right): https://codereview.chromium.org/1996563002/diff/260001/ash/shelf/shelf.cc#new... ash/shelf/shelf.cc:140: if (id == -1) { On 2016/06/20 16:18:12, sadrul wrote: > How can you tell that -1 is the ime tray item? Set it in extension_app_window_launcher_controller.cc https://codereview.chromium.org/1996563002/diff/260001/ash/system/chromeos/im... File ash/system/chromeos/ime_menu/ime_menu_tray.cc (right): https://codereview.chromium.org/1996563002/diff/260001/ash/system/chromeos/im... ash/system/chromeos/ime_menu/ime_menu_tray.cc:29: gfx::Size GetPreferredSize() const override { On 2016/06/20 16:18:12, sadrul wrote: > // views:Label: Done. https://codereview.chromium.org/1996563002/diff/260001/ash/system/chromeos/im... ash/system/chromeos/ime_menu/ime_menu_tray.cc:32: }; On 2016/06/20 16:18:12, sadrul wrote: > DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/1996563002/diff/260001/ash/system/chromeos/im... ash/system/chromeos/ime_menu/ime_menu_tray.cc:125: window_state_ = window_state; On 2016/06/20 16:18:12, sadrul wrote: > This should be a DCHECK_EQ(window_state_, window_state) instead, right? (or just > removed altogether) Done. https://codereview.chromium.org/1996563002/diff/260001/ash/system/chromeos/im... ash/system/chromeos/ime_menu/ime_menu_tray.cc:147: base::string16 ImeMenuTray::GetLabelTextForTest() { On 2016/06/20 16:18:12, sadrul wrote: > function order in .cc should match the order in .h Done. https://codereview.chromium.org/1996563002/diff/260001/ash/system/chromeos/im... File ash/system/chromeos/ime_menu/ime_menu_tray.h (right): https://codereview.chromium.org/1996563002/diff/260001/ash/system/chromeos/im... ash/system/chromeos/ime_menu/ime_menu_tray.h:53: void SetWindowState(wm::WindowState* window_state); On 2016/06/20 16:18:12, sadrul wrote: > non-overrides before overrides. > > Also, document. Done. https://codereview.chromium.org/1996563002/diff/260001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc (right): https://codereview.chromium.org/1996563002/diff/260001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:133: ash::SetShelfIDForWindow(-1, window); On 2016/06/20 16:18:12, sadrul wrote: > I don't think setting special values for the ShelfID is a good way of doing > this. I added a property kShowIconOnTrayKey for window to indicates its icon should be put on the tray. While, I still needs to set a shelf id that's different from the initial 0 here. Otherwise, the window can't be shown. It's maybe unsafe to set with -1. But I don't find a clue why the window is invisible with kInvalidShelfID. Do you have advice about this?
https://codereview.chromium.org/1996563002/diff/280001/ash/shelf/shelf.cc File ash/shelf/shelf.cc (right): https://codereview.chromium.org/1996563002/diff/280001/ash/shelf/shelf.cc#new... ash/shelf/shelf.cc:137: if (id != -1) { s/-1/kStatusTrayShelfID
https://codereview.chromium.org/1996563002/diff/260001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc (right): https://codereview.chromium.org/1996563002/diff/260001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:133: ash::SetShelfIDForWindow(-1, window); On 2016/06/22 01:47:10, Azure Wei wrote: > On 2016/06/20 16:18:12, sadrul wrote: > > I don't think setting special values for the ShelfID is a good way of doing > > this. > > I added a property kShowIconOnTrayKey for window to indicates its icon should be > put on the tray. While, I still needs to set a shelf id that's different from > the initial 0 here. Otherwise, the window can't be shown. It's maybe unsafe to > set with -1. But I don't find a clue why the window is invisible with > kInvalidShelfID. Do you have advice about this? The window has not been shown was caused by wrong condition in shelf::GetScreenBoundsOfItemIconForWindow. Turns out we can show the window icon without setting the shelf id. https://codereview.chromium.org/1996563002/diff/280001/ash/shelf/shelf.cc File ash/shelf/shelf.cc (right): https://codereview.chromium.org/1996563002/diff/280001/ash/shelf/shelf.cc#new... ash/shelf/shelf.cc:137: if (id != -1) { On 2016/06/27 01:11:10, Shu Chen wrote: > s/-1/kStatusTrayShelfID Removed the invalid shelf id kStatusTrayShelfID. The window icon could be shown on the tray only based on the value of window property kShowIconOnTrayKey.
lgtm
https://codereview.chromium.org/1996563002/diff/300001/ash/shelf/shelf.cc File ash/shelf/shelf.cc (right): https://codereview.chromium.org/1996563002/diff/300001/ash/shelf/shelf.cc#new... ash/shelf/shelf.cc:125: int offset_y = 0; Use a gfx::Vector2d offset here instead. https://codereview.chromium.org/1996563002/diff/300001/ash/shelf/shelf.cc#new... ash/shelf/shelf.cc:129: bounds = shelf_widget_->status_area_widget()->ime_menu_tray()->bounds(); It's not obvious at all that 'show icon on tray' means ime-menu item in the tray. Can this be more obvious? https://codereview.chromium.org/1996563002/diff/300001/ash/shelf/shelf.cc#new... ash/shelf/shelf.cc:133: offset_y = status_area_bounds.y(); offset = status_area_bounds.OffsetFromOrigin(); https://codereview.chromium.org/1996563002/diff/300001/ash/shelf/shelf.cc#new... ash/shelf/shelf.cc:144: return gfx::Rect(offset_x + bounds.x(), offset_y + bounds.y(), bounds.width(), return bounds + offset; https://codereview.chromium.org/1996563002/diff/300001/ash/system/chromeos/im... File ash/system/chromeos/ime_menu/ime_menu_tray.cc (right): https://codereview.chromium.org/1996563002/diff/300001/ash/system/chromeos/im... ash/system/chromeos/ime_menu/ime_menu_tray.cc:35: DISALLOW_COPY_AND_ASSIGN(ImeMenuLabel); Put this in an anon namespace. https://codereview.chromium.org/1996563002/diff/300001/ash/system/status_area... File ash/system/status_area_widget.h (right): https://codereview.chromium.org/1996563002/diff/300001/ash/system/status_area... ash/system/status_area_widget.h:25: class ImeMenuTray; sort https://codereview.chromium.org/1996563002/diff/300001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc (right): https://codereview.chromium.org/1996563002/diff/300001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:196: DCHECK(iter1 != window_to_app_shelf_id_map_.end()); Do you not need to update this too? Instead of having IMEMenuTray being a WindowObserver, would it make sense to clean up its states from here instead (e.g. by calling SetWindowState(nullptr) from here)? https://codereview.chromium.org/1996563002/diff/300001/ui/aura/client/aura_co... File ui/aura/client/aura_constants.h (right): https://codereview.chromium.org/1996563002/diff/300001/ui/aura/client/aura_co... ui/aura/client/aura_constants.h:51: This key should go somewhere in ash/shelf/, not in here.
https://codereview.chromium.org/1996563002/diff/300001/ash/shelf/shelf.cc File ash/shelf/shelf.cc (right): https://codereview.chromium.org/1996563002/diff/300001/ash/shelf/shelf.cc#new... ash/shelf/shelf.cc:125: int offset_y = 0; On 2016/06/27 15:08:05, sadrul wrote: > Use a gfx::Vector2d offset here instead. Done. https://codereview.chromium.org/1996563002/diff/300001/ash/shelf/shelf.cc#new... ash/shelf/shelf.cc:129: bounds = shelf_widget_->status_area_widget()->ime_menu_tray()->bounds(); On 2016/06/27 15:08:04, sadrul wrote: > It's not obvious at all that 'show icon on tray' means ime-menu item in the > tray. Can this be more obvious? Renamed as kShelfItemOnTrayForImeMenu. Is that better? https://codereview.chromium.org/1996563002/diff/300001/ash/shelf/shelf.cc#new... ash/shelf/shelf.cc:133: offset_y = status_area_bounds.y(); On 2016/06/27 15:08:04, sadrul wrote: > offset = status_area_bounds.OffsetFromOrigin(); Done. https://codereview.chromium.org/1996563002/diff/300001/ash/shelf/shelf.cc#new... ash/shelf/shelf.cc:144: return gfx::Rect(offset_x + bounds.x(), offset_y + bounds.y(), bounds.width(), On 2016/06/27 15:08:04, sadrul wrote: > return bounds + offset; Done. https://codereview.chromium.org/1996563002/diff/300001/ash/system/chromeos/im... File ash/system/chromeos/ime_menu/ime_menu_tray.cc (right): https://codereview.chromium.org/1996563002/diff/300001/ash/system/chromeos/im... ash/system/chromeos/ime_menu/ime_menu_tray.cc:35: DISALLOW_COPY_AND_ASSIGN(ImeMenuLabel); On 2016/06/27 15:08:05, sadrul wrote: > Put this in an anon namespace. Moved to ime_menu_tray.h. https://codereview.chromium.org/1996563002/diff/300001/ash/system/status_area... File ash/system/status_area_widget.h (right): https://codereview.chromium.org/1996563002/diff/300001/ash/system/status_area... ash/system/status_area_widget.h:25: class ImeMenuTray; On 2016/06/27 15:08:05, sadrul wrote: > sort Done. https://codereview.chromium.org/1996563002/diff/300001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc (right): https://codereview.chromium.org/1996563002/diff/300001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:196: DCHECK(iter1 != window_to_app_shelf_id_map_.end()); On 2016/06/27 15:08:05, sadrul wrote: > Do you not need to update this too? > > Instead of having IMEMenuTray being a WindowObserver, would it make sense to > clean up its states from here instead (e.g. by calling SetWindowState(nullptr) > from here)? Thanks for the advise. Sets the window state nullptr here, rather than listening on the window destroying. https://codereview.chromium.org/1996563002/diff/300001/ui/aura/client/aura_co... File ui/aura/client/aura_constants.h (right): https://codereview.chromium.org/1996563002/diff/300001/ui/aura/client/aura_co... ui/aura/client/aura_constants.h:51: On 2016/06/27 15:08:05, sadrul wrote: > This key should go somewhere in ash/shelf/, not in here. Moved to ash/shelf/shelf_util.*
Pinging...
Description was changed from ========== Make IME menu closer to the status tray. There has consistent spacing between panel windows with notification center/ status tray. This cl puts the IME menu icon on the status tray rather than in the shelf. BUG=570761 TEST=Verified on local build. ========== to ========== Make IME menu closer to the status tray. There has consistent spacing between panel windows with notification center/ status tray. This cl puts the IME menu icon on the status tray rather than in the shelf. BUG=570761, 624186 TEST=Verified on local build. ==========
Mostly nits. lgtm with those addressed. https://codereview.chromium.org/1996563002/diff/300001/ash/system/chromeos/im... File ash/system/chromeos/ime_menu/ime_menu_tray.cc (right): https://codereview.chromium.org/1996563002/diff/300001/ash/system/chromeos/im... ash/system/chromeos/ime_menu/ime_menu_tray.cc:35: DISALLOW_COPY_AND_ASSIGN(ImeMenuLabel); On 2016/06/27 18:33:49, Azure Wei wrote: > On 2016/06/27 15:08:05, sadrul wrote: > > Put this in an anon namespace. > > Moved to ime_menu_tray.h. Why? It's not used from anywhere else. Put this back in the .cc file, in an anonymous namespace. https://codereview.chromium.org/1996563002/diff/360001/ash/system/chromeos/im... File ash/system/chromeos/ime_menu/ime_menu_tray.cc (right): https://codereview.chromium.org/1996563002/diff/360001/ash/system/chromeos/im... ash/system/chromeos/ime_menu/ime_menu_tray.cc:115: // Updates the tray label base on the current input method. *based https://codereview.chromium.org/1996563002/diff/360001/ash/system/chromeos/im... File ash/system/chromeos/ime_menu/ime_menu_tray.h (right): https://codereview.chromium.org/1996563002/diff/360001/ash/system/chromeos/im... ash/system/chromeos/ime_menu/ime_menu_tray.h:29: DISALLOW_COPY_AND_ASSIGN(ImeMenuLabel); This should go in the .cc file https://codereview.chromium.org/1996563002/diff/360001/ash/system/chromeos/im... ash/system/chromeos/ime_menu/ime_menu_tray.h:67: base::string16 GetLabelTextForTesting(); const & (although since ImeMenuTrayTest is already a friend, do you need this additional function? Can't it just get the text directly from |label_|?) https://codereview.chromium.org/1996563002/diff/360001/ash/system/chromeos/im... ash/system/chromeos/ime_menu/ime_menu_tray.h:69: ImeMenuLabel* label_; |label_| can be a views::Label (since ImeMenuLabel does not introduce new API.) https://codereview.chromium.org/1996563002/diff/360001/ash/system/status_area... File ash/system/status_area_widget.cc (right): https://codereview.chromium.org/1996563002/diff/360001/ash/system/status_area... ash/system/status_area_widget.cc:95: ime_menu_tray_ = nullptr; the destruction order should normally be the reverse of the creation/init order. https://codereview.chromium.org/1996563002/diff/360001/ash/test/test_system_t... File ash/test/test_system_tray_delegate.h (right): https://codereview.chromium.org/1996563002/diff/360001/ash/test/test_system_t... ash/test/test_system_tray_delegate.h:60: void SetCurrentIME(IMEInfo info); non-override before overrides. Also, const&
Thanks for the review. https://codereview.chromium.org/1996563002/diff/360001/ash/system/chromeos/im... File ash/system/chromeos/ime_menu/ime_menu_tray.cc (right): https://codereview.chromium.org/1996563002/diff/360001/ash/system/chromeos/im... ash/system/chromeos/ime_menu/ime_menu_tray.cc:115: // Updates the tray label base on the current input method. On 2016/06/29 15:14:10, sadrul wrote: > *based Done. https://codereview.chromium.org/1996563002/diff/360001/ash/system/chromeos/im... File ash/system/chromeos/ime_menu/ime_menu_tray.h (right): https://codereview.chromium.org/1996563002/diff/360001/ash/system/chromeos/im... ash/system/chromeos/ime_menu/ime_menu_tray.h:29: DISALLOW_COPY_AND_ASSIGN(ImeMenuLabel); On 2016/06/29 15:14:10, sadrul wrote: > This should go in the .cc file Done. https://codereview.chromium.org/1996563002/diff/360001/ash/system/chromeos/im... ash/system/chromeos/ime_menu/ime_menu_tray.h:67: base::string16 GetLabelTextForTesting(); On 2016/06/29 15:14:10, sadrul wrote: > const & (although since ImeMenuTrayTest is already a friend, do you need this > additional function? Can't it just get the text directly from |label_|?) Removed this. https://codereview.chromium.org/1996563002/diff/360001/ash/system/chromeos/im... ash/system/chromeos/ime_menu/ime_menu_tray.h:69: ImeMenuLabel* label_; On 2016/06/29 15:14:10, sadrul wrote: > |label_| can be a views::Label (since ImeMenuLabel does not introduce new API.) Done. https://codereview.chromium.org/1996563002/diff/360001/ash/system/status_area... File ash/system/status_area_widget.cc (right): https://codereview.chromium.org/1996563002/diff/360001/ash/system/status_area... ash/system/status_area_widget.cc:95: ime_menu_tray_ = nullptr; On 2016/06/29 15:14:10, sadrul wrote: > the destruction order should normally be the reverse of the creation/init order. Done. https://codereview.chromium.org/1996563002/diff/360001/ash/test/test_system_t... File ash/test/test_system_tray_delegate.h (right): https://codereview.chromium.org/1996563002/diff/360001/ash/test/test_system_t... ash/test/test_system_tray_delegate.h:60: void SetCurrentIME(IMEInfo info); On 2016/06/29 15:14:10, sadrul wrote: > non-override before overrides. > > Also, const& Done.
sky@, can you help review this cl? It still needs your approval for the following files: - ash/shelf/* - chrome/browser/ui/ash/* Thanks!
sky@chromium.org changed reviewers: + jamescook@chromium.org
Sorry to keep passing the buck. James has been doing refactoring of shelf code and so has a better handle on where things are at right now. I'm adding him for ash/shelf. I'll look at chrome now.
https://codereview.chromium.org/1996563002/diff/400001/ash/system/chromeos/im... File ash/system/chromeos/ime_menu/ime_menu_tray.h (right): https://codereview.chromium.org/1996563002/diff/400001/ash/system/chromeos/im... ash/system/chromeos/ime_menu/ime_menu_tray.h:30: void SetWindowState(wm::WindowState* window_state); WindowState is owned by the corresponding window. How do you ensure you reset window_state_ if the window is destroyed? I recommend changing this to SetImeWindow() and attaching an observer to it. Once you have the window you can get the windowstate if needed. I believe if you did this you wouldn't need to change ExtensionAppWindowLauncherController::UnregisterApp either.
https://codereview.chromium.org/1996563002/diff/400001/ash/system/chromeos/im... File ash/system/chromeos/ime_menu/ime_menu_tray.h (right): https://codereview.chromium.org/1996563002/diff/400001/ash/system/chromeos/im... ash/system/chromeos/ime_menu/ime_menu_tray.h:30: void SetWindowState(wm::WindowState* window_state); On 2016/06/30 16:21:39, sky wrote: > WindowState is owned by the corresponding window. How do you ensure you reset > window_state_ if the window is destroyed? I recommend changing this to > SetImeWindow() and attaching an observer to it. Once you have the window you can > get the windowstate if needed. I believe if you did this you wouldn't need to > change ExtensionAppWindowLauncherController::UnregisterApp either. Changed the method SetWindowState(wm::WindowState* window_state) with SetImeWindow(aura::Window). And make ImeMenuTray listen on the window behaviour directly.
On 2016/06/30 16:21:39, sky wrote: > https://codereview.chromium.org/1996563002/diff/400001/ash/system/chromeos/im... > File ash/system/chromeos/ime_menu/ime_menu_tray.h (right): > > https://codereview.chromium.org/1996563002/diff/400001/ash/system/chromeos/im... > ash/system/chromeos/ime_menu/ime_menu_tray.h:30: void > SetWindowState(wm::WindowState* window_state); > WindowState is owned by the corresponding window. How do you ensure you reset > window_state_ if the window is destroyed? I recommend changing this to > SetImeWindow() and attaching an observer to it. Once you have the window you can > get the windowstate if needed. I believe if you did this you wouldn't need to > change ExtensionAppWindowLauncherController::UnregisterApp either. I know I haven't been involved in this feature before, and you didn't ask me to review your code, but I don't think that using panels for this feature is the right technical approach. I will comment in the launch bug.
On 2016/06/30 17:45:42, James Cook wrote: > On 2016/06/30 16:21:39, sky wrote: > > > https://codereview.chromium.org/1996563002/diff/400001/ash/system/chromeos/im... > > File ash/system/chromeos/ime_menu/ime_menu_tray.h (right): > > > > > https://codereview.chromium.org/1996563002/diff/400001/ash/system/chromeos/im... > > ash/system/chromeos/ime_menu/ime_menu_tray.h:30: void > > SetWindowState(wm::WindowState* window_state); > > WindowState is owned by the corresponding window. How do you ensure you reset > > window_state_ if the window is destroyed? I recommend changing this to > > SetImeWindow() and attaching an observer to it. Once you have the window you > can > > get the windowstate if needed. I believe if you did this you wouldn't need to > > change ExtensionAppWindowLauncherController::UnregisterApp either. > > I know I haven't been involved in this feature before, and you didn't ask me to > review your code, but I don't think that using panels for this feature is the > right technical approach. I will comment in the launch bug. (This code is a step in the right direction -- I think this should be built as a system tray button like the virtual keyboard button.)
On 2016/06/30 17:48:20, James Cook wrote: > On 2016/06/30 17:45:42, James Cook wrote: > > On 2016/06/30 16:21:39, sky wrote: > > > > > > https://codereview.chromium.org/1996563002/diff/400001/ash/system/chromeos/im... > > > File ash/system/chromeos/ime_menu/ime_menu_tray.h (right): > > > > > > > > > https://codereview.chromium.org/1996563002/diff/400001/ash/system/chromeos/im... > > > ash/system/chromeos/ime_menu/ime_menu_tray.h:30: void > > > SetWindowState(wm::WindowState* window_state); > > > WindowState is owned by the corresponding window. How do you ensure you > reset > > > window_state_ if the window is destroyed? I recommend changing this to > > > SetImeWindow() and attaching an observer to it. Once you have the window you > > can > > > get the windowstate if needed. I believe if you did this you wouldn't need > to > > > change ExtensionAppWindowLauncherController::UnregisterApp either. > > > > I know I haven't been involved in this feature before, and you didn't ask me > to > > review your code, but I don't think that using panels for this feature is the > > right technical approach. I will comment in the launch bug. > > (This code is a step in the right direction -- I think this should be built as a > system tray button like the virtual keyboard button.) Yes, this is what's the current cl doing: make the window icon a tray button rather than a shelf item. As for the using of panel window, it mostly because the AppWindow APIs. The IME menu is created with IME extensions, and we are trying to avoid introducing new APIs for this.
On 2016/06/30 17:57:47, Azure Wei wrote: > On 2016/06/30 17:48:20, James Cook wrote: > > On 2016/06/30 17:45:42, James Cook wrote: > > > On 2016/06/30 16:21:39, sky wrote: > > > > > > > > > > https://codereview.chromium.org/1996563002/diff/400001/ash/system/chromeos/im... > > > > File ash/system/chromeos/ime_menu/ime_menu_tray.h (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1996563002/diff/400001/ash/system/chromeos/im... > > > > ash/system/chromeos/ime_menu/ime_menu_tray.h:30: void > > > > SetWindowState(wm::WindowState* window_state); > > > > WindowState is owned by the corresponding window. How do you ensure you > > reset > > > > window_state_ if the window is destroyed? I recommend changing this to > > > > SetImeWindow() and attaching an observer to it. Once you have the window > you > > > can > > > > get the windowstate if needed. I believe if you did this you wouldn't need > > to > > > > change ExtensionAppWindowLauncherController::UnregisterApp either. > > > > > > I know I haven't been involved in this feature before, and you didn't ask me > > to > > > review your code, but I don't think that using panels for this feature is > the > > > right technical approach. I will comment in the launch bug. > > > > (This code is a step in the right direction -- I think this should be built as > a > > system tray button like the virtual keyboard button.) > > Yes, this is what's the current cl doing: make the window icon a tray button > rather than a shelf item. > As for the using of panel window, it mostly because the AppWindow APIs. The IME > menu is created with IME extensions, and we are trying to avoid introducing new > APIs for this. If you decide to continue with this CL, the status tray item code should be implemented in //ash/common/system/chromeos/ and use types like WmWindow instead of aura::Window. You might need to add methods to WmWindow to support this. However, I would recommend that you first investigate if there is a way to do this without panels.
A few related comments. https://codereview.chromium.org/1996563002/diff/420001/ash/ash_chromeos_strin... File ash/ash_chromeos_strings.grdp (right): https://codereview.chromium.org/1996563002/diff/420001/ash/ash_chromeos_strin... ash/ash_chromeos_strings.grdp:502: IME menu button nit: Should this be "IME menu" or "Show IME menu"? This string seems inconsistent with our other "accessible name" strings. Other ones have an action ("Toggle window overview" or "Show on-screen keyboard"). https://codereview.chromium.org/1996563002/diff/420001/ash/system/chromeos/im... File ash/system/chromeos/ime_menu/ime_menu_tray.cc (right): https://codereview.chromium.org/1996563002/diff/420001/ash/system/chromeos/im... ash/system/chromeos/ime_menu/ime_menu_tray.cc:7: #include "ash/aura/wm_window_aura.h" not needed https://codereview.chromium.org/1996563002/diff/420001/ash/system/chromeos/im... ash/system/chromeos/ime_menu/ime_menu_tray.cc:12: #include "ash/shell.h" Not needed https://codereview.chromium.org/1996563002/diff/420001/ash/system/chromeos/im... ash/system/chromeos/ime_menu/ime_menu_tray.cc:18: #include "ui/events/event.h" needed? https://codereview.chromium.org/1996563002/diff/420001/ash/system/chromeos/im... ash/system/chromeos/ime_menu/ime_menu_tray.cc:41: ImeMenuTray::ImeMenuTray(StatusAreaWidget* status_area_widget) This should take WmShelf directly, then you don't need to include status_area_widget.h https://codereview.chromium.org/1996563002/diff/420001/ash/system/chromeos/im... ash/system/chromeos/ime_menu/ime_menu_tray.cc:49: // The Shell may not exist in some unit tests. Is this true? I think WmShell always exists when you use AshTestBase and don't directly create an instance of this object. You probably don't need the HasInstance checks.
On 2016/06/30 18:18:53, James Cook wrote: > On 2016/06/30 17:57:47, Azure Wei wrote: > > On 2016/06/30 17:48:20, James Cook wrote: > > > On 2016/06/30 17:45:42, James Cook wrote: > > > > On 2016/06/30 16:21:39, sky wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/1996563002/diff/400001/ash/system/chromeos/im... > > > > > File ash/system/chromeos/ime_menu/ime_menu_tray.h (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1996563002/diff/400001/ash/system/chromeos/im... > > > > > ash/system/chromeos/ime_menu/ime_menu_tray.h:30: void > > > > > SetWindowState(wm::WindowState* window_state); > > > > > WindowState is owned by the corresponding window. How do you ensure you > > > reset > > > > > window_state_ if the window is destroyed? I recommend changing this to > > > > > SetImeWindow() and attaching an observer to it. Once you have the window > > you > > > > can > > > > > get the windowstate if needed. I believe if you did this you wouldn't > need > > > to > > > > > change ExtensionAppWindowLauncherController::UnregisterApp either. > > > > > > > > I know I haven't been involved in this feature before, and you didn't ask > me > > > to > > > > review your code, but I don't think that using panels for this feature is > > the > > > > right technical approach. I will comment in the launch bug. > > > > > > (This code is a step in the right direction -- I think this should be built > as > > a > > > system tray button like the virtual keyboard button.) > > > > Yes, this is what's the current cl doing: make the window icon a tray button > > rather than a shelf item. > > As for the using of panel window, it mostly because the AppWindow APIs. The > IME > > menu is created with IME extensions, and we are trying to avoid introducing > new > > APIs for this. > > If you decide to continue with this CL, the status tray item code should be > implemented in //ash/common/system/chromeos/ and use types like WmWindow instead > of aura::Window. You might need to add methods to WmWindow to support this. > > However, I would recommend that you first investigate if there is a way to do > this without panels. Thanks for the review. I've removed the ime_menu_tray* files to //ash/common/. And Replaced aura::Window with WmWindow. I still want to go with this cl first, as the ImeMenuTray is needed whether we use panel or other window.
https://codereview.chromium.org/1996563002/diff/420001/ash/ash_chromeos_strin... File ash/ash_chromeos_strings.grdp (right): https://codereview.chromium.org/1996563002/diff/420001/ash/ash_chromeos_strin... ash/ash_chromeos_strings.grdp:502: IME menu button On 2016/06/30 18:20:57, James Cook wrote: > nit: Should this be "IME menu" or "Show IME menu"? > > This string seems inconsistent with our other "accessible name" strings. Other > ones have an action ("Toggle window overview" or "Show on-screen keyboard"). This is raised by A11y reviewer. https://codereview.chromium.org/1996563002/diff/420001/ash/system/chromeos/im... File ash/system/chromeos/ime_menu/ime_menu_tray.cc (right): https://codereview.chromium.org/1996563002/diff/420001/ash/system/chromeos/im... ash/system/chromeos/ime_menu/ime_menu_tray.cc:7: #include "ash/aura/wm_window_aura.h" On 2016/06/30 18:20:57, James Cook wrote: > not needed Removed. https://codereview.chromium.org/1996563002/diff/420001/ash/system/chromeos/im... ash/system/chromeos/ime_menu/ime_menu_tray.cc:12: #include "ash/shell.h" On 2016/06/30 18:20:58, James Cook wrote: > Not needed Removed. https://codereview.chromium.org/1996563002/diff/420001/ash/system/chromeos/im... ash/system/chromeos/ime_menu/ime_menu_tray.cc:18: #include "ui/events/event.h" On 2016/06/30 18:20:58, James Cook wrote: > needed? Removed. https://codereview.chromium.org/1996563002/diff/420001/ash/system/chromeos/im... ash/system/chromeos/ime_menu/ime_menu_tray.cc:41: ImeMenuTray::ImeMenuTray(StatusAreaWidget* status_area_widget) On 2016/06/30 18:20:58, James Cook wrote: > This should take WmShelf directly, then you don't need to include > status_area_widget.h Done. https://codereview.chromium.org/1996563002/diff/420001/ash/system/chromeos/im... ash/system/chromeos/ime_menu/ime_menu_tray.cc:49: // The Shell may not exist in some unit tests. On 2016/06/30 18:20:57, James Cook wrote: > Is this true? I think WmShell always exists when you use AshTestBase and don't > directly create an instance of this object. You probably don't need the > HasInstance checks. This is not necessary. Removed the HasInstance() check.
BTW, I like this feature overall. Putting the IME menu outside the system menu feels like a nice refinement. https://codereview.chromium.org/1996563002/diff/440001/ash/ash.gyp File ash/ash.gyp (right): https://codereview.chromium.org/1996563002/diff/440001/ash/ash.gyp#newcode905 ash/ash.gyp:905: 'common/system/chromeos/ime_menu/ime_menu_tray_unittest.cc', Did you forget to "git add" this file? I don't see it anymore. (And thanks for writing a test suite! It's very helpful.) https://codereview.chromium.org/1996563002/diff/440001/ash/common/system/chro... File ash/common/system/chromeos/ime_menu/ime_menu_tray.cc (right): https://codereview.chromium.org/1996563002/diff/440001/ash/common/system/chro... ash/common/system/chromeos/ime_menu/ime_menu_tray.cc:23: ImeMenuLabel() : views::Label(base::string16()) {} nit: Just use views::Label(). This could be ImeMenuLabel() {}. https://codereview.chromium.org/1996563002/diff/440001/ash/common/system/chro... ash/common/system/chromeos/ime_menu/ime_menu_tray.cc:27: return gfx::Size(kNotificationIconWidth, kNotificationIconWidth); I would introduce a kTrayImeIconWidth in tray_constants.cc instead of reusing the one for the web notification tray. I would also call it "size" since you use it for both width and height. https://codereview.chromium.org/1996563002/diff/440001/ash/common/system/chro... ash/common/system/chromeos/ime_menu/ime_menu_tray.cc:37: : TrayBackgroundView(wm_shelf), label_(nullptr), window_(nullptr) { inline label_(new ImeMenuLabel) https://codereview.chromium.org/1996563002/diff/440001/ash/common/system/chro... ash/common/system/chromeos/ime_menu/ime_menu_tray.cc:64: SetDrawBackgroundAsActive(window_->IsActive()); Do you still want to do this if |window| is nullptr? If |window| is nullptr should you set |window_| to null? What does it mean to call SetImeWindow(nullptr)? I would document that in the header file. https://codereview.chromium.org/1996563002/diff/440001/ash/common/system/chro... ash/common/system/chromeos/ime_menu/ime_menu_tray.cc:108: // Sets the background color based on the visiablity of the window. nit: either visiability -> visibility, or just delete the comment https://codereview.chromium.org/1996563002/diff/440001/ash/common/system/chro... ash/common/system/chromeos/ime_menu/ime_menu_tray.cc:114: if (window_ && window_ == window) { just check window_ == window https://codereview.chromium.org/1996563002/diff/440001/ash/common/system/chro... ash/common/system/chromeos/ime_menu/ime_menu_tray.cc:125: delegate->GetCurrentIME(¤t_ime_); inline WmShell::Get()->system_tray_delegate()->GetCurrentIme() https://codereview.chromium.org/1996563002/diff/440001/ash/common/system/chro... File ash/common/system/chromeos/ime_menu/ime_menu_tray.h (right): https://codereview.chromium.org/1996563002/diff/440001/ash/common/system/chro... ash/common/system/chromeos/ime_menu/ime_menu_tray.h:10: #include "ash/common/system/tray/system_tray_delegate.h" not needed https://codereview.chromium.org/1996563002/diff/440001/ash/common/system/chro... ash/common/system/chromeos/ime_menu/ime_menu_tray.h:15: namespace wm { namespace wm not needed, WmWindow is in namespace ash https://codereview.chromium.org/1996563002/diff/440001/ash/common/system/chro... ash/common/system/chromeos/ime_menu/ime_menu_tray.h:63: ash::IMEInfo current_ime_; ash:: not needed https://codereview.chromium.org/1996563002/diff/440001/ash/shelf/shelf.cc File ash/shelf/shelf.cc (right): https://codereview.chromium.org/1996563002/diff/440001/ash/shelf/shelf.cc#new... ash/shelf/shelf.cc:131: } nit: This might be easier to read with an early return for this case. You could keep the original code for the non-ime-menu case. I think kShelfItemOnTrayForImeMenu is only used on chromeos, right? https://codereview.chromium.org/1996563002/diff/440001/ash/test/test_system_t... File ash/test/test_system_tray_delegate.cc (right): https://codereview.chromium.org/1996563002/diff/440001/ash/test/test_system_t... ash/test/test_system_tray_delegate.cc:116: info->id = current_ime_.id; Can you use *info = current_ime_? If not, can you add an assignment operator to IMEInfo? I think it could just be "= default" https://codereview.chromium.org/1996563002/diff/440001/ash/test/test_system_t... File ash/test/test_system_tray_delegate.h (right): https://codereview.chromium.org/1996563002/diff/440001/ash/test/test_system_t... ash/test/test_system_tray_delegate.h:68: IMEInfo current_ime_; #include the header for this https://codereview.chromium.org/1996563002/diff/440001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc (right): https://codereview.chromium.org/1996563002/diff/440001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:124: // lanucher. Instead, is should show ImeMenuTray in the status tray. nit: lanucher -> launcher. Or did you mean shelf? Also "is should show" -> it should show https://codereview.chromium.org/1996563002/diff/440001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:130: if (ime_menu_tray) { I think this pointer is never null when you have a status_area_widget. You can remove the if().
https://codereview.chromium.org/1996563002/diff/440001/ash/ash.gyp File ash/ash.gyp (right): https://codereview.chromium.org/1996563002/diff/440001/ash/ash.gyp#newcode905 ash/ash.gyp:905: 'common/system/chromeos/ime_menu/ime_menu_tray_unittest.cc', On 2016/06/30 21:15:46, James Cook wrote: > Did you forget to "git add" this file? I don't see it anymore. > > (And thanks for writing a test suite! It's very helpful.) Sorry for the silly mistake. ime_menu_tray_unittest.cc breaks some include rules, as it need status_area_widget.h and aura/window.h, so I put it under ash/system/. Is that OK? https://codereview.chromium.org/1996563002/diff/440001/ash/common/system/chro... File ash/common/system/chromeos/ime_menu/ime_menu_tray.cc (right): https://codereview.chromium.org/1996563002/diff/440001/ash/common/system/chro... ash/common/system/chromeos/ime_menu/ime_menu_tray.cc:23: ImeMenuLabel() : views::Label(base::string16()) {} On 2016/06/30 21:15:47, James Cook wrote: > nit: Just use views::Label(). This could be ImeMenuLabel() {}. Done. https://codereview.chromium.org/1996563002/diff/440001/ash/common/system/chro... ash/common/system/chromeos/ime_menu/ime_menu_tray.cc:27: return gfx::Size(kNotificationIconWidth, kNotificationIconWidth); On 2016/06/30 21:15:46, James Cook wrote: > I would introduce a kTrayImeIconWidth in tray_constants.cc instead of reusing > the one for the web notification tray. > > I would also call it "size" since you use it for both width and height. Done. https://codereview.chromium.org/1996563002/diff/440001/ash/common/system/chro... ash/common/system/chromeos/ime_menu/ime_menu_tray.cc:37: : TrayBackgroundView(wm_shelf), label_(nullptr), window_(nullptr) { On 2016/06/30 21:15:46, James Cook wrote: > inline label_(new ImeMenuLabel) Done. https://codereview.chromium.org/1996563002/diff/440001/ash/common/system/chro... ash/common/system/chromeos/ime_menu/ime_menu_tray.cc:64: SetDrawBackgroundAsActive(window_->IsActive()); On 2016/06/30 21:15:47, James Cook wrote: > Do you still want to do this if |window| is nullptr? > > If |window| is nullptr should you set |window_| to null? > > What does it mean to call SetImeWindow(nullptr)? I would document that in the > header file. Put SetDrawBackgroundAsActive(window_->IsActive()); under the if(window) condition. https://codereview.chromium.org/1996563002/diff/440001/ash/common/system/chro... ash/common/system/chromeos/ime_menu/ime_menu_tray.cc:108: // Sets the background color based on the visiablity of the window. On 2016/06/30 21:15:46, James Cook wrote: > nit: either visiability -> visibility, or just delete the comment Done. https://codereview.chromium.org/1996563002/diff/440001/ash/common/system/chro... ash/common/system/chromeos/ime_menu/ime_menu_tray.cc:114: if (window_ && window_ == window) { On 2016/06/30 21:15:47, James Cook wrote: > just check window_ == window Done. https://codereview.chromium.org/1996563002/diff/440001/ash/common/system/chro... ash/common/system/chromeos/ime_menu/ime_menu_tray.cc:125: delegate->GetCurrentIME(¤t_ime_); On 2016/06/30 21:15:46, James Cook wrote: > inline WmShell::Get()->system_tray_delegate()->GetCurrentIme() Done. https://codereview.chromium.org/1996563002/diff/440001/ash/common/system/chro... File ash/common/system/chromeos/ime_menu/ime_menu_tray.h (right): https://codereview.chromium.org/1996563002/diff/440001/ash/common/system/chro... ash/common/system/chromeos/ime_menu/ime_menu_tray.h:10: #include "ash/common/system/tray/system_tray_delegate.h" On 2016/06/30 21:15:47, James Cook wrote: > not needed This is for IMEInfo. https://codereview.chromium.org/1996563002/diff/440001/ash/common/system/chro... ash/common/system/chromeos/ime_menu/ime_menu_tray.h:15: namespace wm { On 2016/06/30 21:15:47, James Cook wrote: > namespace wm not needed, WmWindow is in namespace ash Done. https://codereview.chromium.org/1996563002/diff/440001/ash/common/system/chro... ash/common/system/chromeos/ime_menu/ime_menu_tray.h:63: ash::IMEInfo current_ime_; On 2016/06/30 21:15:47, James Cook wrote: > ash:: not needed Done. https://codereview.chromium.org/1996563002/diff/440001/ash/shelf/shelf.cc File ash/shelf/shelf.cc (right): https://codereview.chromium.org/1996563002/diff/440001/ash/shelf/shelf.cc#new... ash/shelf/shelf.cc:131: } On 2016/06/30 21:15:47, James Cook wrote: > nit: This might be easier to read with an early return for this case. You could > keep the original code for the non-ime-menu case. I think > kShelfItemOnTrayForImeMenu is only used on chromeos, right? Yes, kShelfItemOnTrayForImeMenu is only for ChromeOs. Adds early return here. https://codereview.chromium.org/1996563002/diff/440001/ash/test/test_system_t... File ash/test/test_system_tray_delegate.cc (right): https://codereview.chromium.org/1996563002/diff/440001/ash/test/test_system_t... ash/test/test_system_tray_delegate.cc:116: info->id = current_ime_.id; On 2016/06/30 21:15:47, James Cook wrote: > Can you use *info = current_ime_? If not, can you add an assignment operator to > IMEInfo? I think it could just be "= default" Done. https://codereview.chromium.org/1996563002/diff/440001/ash/test/test_system_t... File ash/test/test_system_tray_delegate.h (right): https://codereview.chromium.org/1996563002/diff/440001/ash/test/test_system_t... ash/test/test_system_tray_delegate.h:68: IMEInfo current_ime_; On 2016/06/30 21:15:47, James Cook wrote: > #include the header for this Done. https://codereview.chromium.org/1996563002/diff/440001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc (right): https://codereview.chromium.org/1996563002/diff/440001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:124: // lanucher. Instead, is should show ImeMenuTray in the status tray. On 2016/06/30 21:15:47, James Cook wrote: > nit: lanucher -> launcher. Or did you mean shelf? > > Also "is should show" -> it should show Done. https://codereview.chromium.org/1996563002/diff/440001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:130: if (ime_menu_tray) { On 2016/06/30 21:15:47, James Cook wrote: > I think this pointer is never null when you have a status_area_widget. You can > remove the if(). Done.
On 2016/07/01 03:48:05, Azure Wei wrote: > https://codereview.chromium.org/1996563002/diff/440001/ash/ash.gyp > File ash/ash.gyp (right): > > https://codereview.chromium.org/1996563002/diff/440001/ash/ash.gyp#newcode905 > ash/ash.gyp:905: 'common/system/chromeos/ime_menu/ime_menu_tray_unittest.cc', > On 2016/06/30 21:15:46, James Cook wrote: > > Did you forget to "git add" this file? I don't see it anymore. > > > > (And thanks for writing a test suite! It's very helpful.) > > Sorry for the silly mistake. > ime_menu_tray_unittest.cc breaks some include rules, as it need > status_area_widget.h and aura/window.h, so I put it under ash/system/. Is that > OK? > > https://codereview.chromium.org/1996563002/diff/440001/ash/common/system/chro... > File ash/common/system/chromeos/ime_menu/ime_menu_tray.cc (right): > > https://codereview.chromium.org/1996563002/diff/440001/ash/common/system/chro... > ash/common/system/chromeos/ime_menu/ime_menu_tray.cc:23: ImeMenuLabel() : > views::Label(base::string16()) {} > On 2016/06/30 21:15:47, James Cook wrote: > > nit: Just use views::Label(). This could be ImeMenuLabel() {}. > > Done. > > https://codereview.chromium.org/1996563002/diff/440001/ash/common/system/chro... > ash/common/system/chromeos/ime_menu/ime_menu_tray.cc:27: return > gfx::Size(kNotificationIconWidth, kNotificationIconWidth); > On 2016/06/30 21:15:46, James Cook wrote: > > I would introduce a kTrayImeIconWidth in tray_constants.cc instead of reusing > > the one for the web notification tray. > > > > I would also call it "size" since you use it for both width and height. > > Done. > > https://codereview.chromium.org/1996563002/diff/440001/ash/common/system/chro... > ash/common/system/chromeos/ime_menu/ime_menu_tray.cc:37: : > TrayBackgroundView(wm_shelf), label_(nullptr), window_(nullptr) { > On 2016/06/30 21:15:46, James Cook wrote: > > inline label_(new ImeMenuLabel) > > Done. > > https://codereview.chromium.org/1996563002/diff/440001/ash/common/system/chro... > ash/common/system/chromeos/ime_menu/ime_menu_tray.cc:64: > SetDrawBackgroundAsActive(window_->IsActive()); > On 2016/06/30 21:15:47, James Cook wrote: > > Do you still want to do this if |window| is nullptr? > > > > If |window| is nullptr should you set |window_| to null? > > > > What does it mean to call SetImeWindow(nullptr)? I would document that in the > > header file. > > Put SetDrawBackgroundAsActive(window_->IsActive()); under the if(window) > condition. > > https://codereview.chromium.org/1996563002/diff/440001/ash/common/system/chro... > ash/common/system/chromeos/ime_menu/ime_menu_tray.cc:108: // Sets the background > color based on the visiablity of the window. > On 2016/06/30 21:15:46, James Cook wrote: > > nit: either visiability -> visibility, or just delete the comment > > Done. > > https://codereview.chromium.org/1996563002/diff/440001/ash/common/system/chro... > ash/common/system/chromeos/ime_menu/ime_menu_tray.cc:114: if (window_ && window_ > == window) { > On 2016/06/30 21:15:47, James Cook wrote: > > just check window_ == window > > Done. > > https://codereview.chromium.org/1996563002/diff/440001/ash/common/system/chro... > ash/common/system/chromeos/ime_menu/ime_menu_tray.cc:125: > delegate->GetCurrentIME(¤t_ime_); > On 2016/06/30 21:15:46, James Cook wrote: > > inline WmShell::Get()->system_tray_delegate()->GetCurrentIme() > > Done. > > https://codereview.chromium.org/1996563002/diff/440001/ash/common/system/chro... > File ash/common/system/chromeos/ime_menu/ime_menu_tray.h (right): > > https://codereview.chromium.org/1996563002/diff/440001/ash/common/system/chro... > ash/common/system/chromeos/ime_menu/ime_menu_tray.h:10: #include > "ash/common/system/tray/system_tray_delegate.h" > On 2016/06/30 21:15:47, James Cook wrote: > > not needed > > This is for IMEInfo. > > https://codereview.chromium.org/1996563002/diff/440001/ash/common/system/chro... > ash/common/system/chromeos/ime_menu/ime_menu_tray.h:15: namespace wm { > On 2016/06/30 21:15:47, James Cook wrote: > > namespace wm not needed, WmWindow is in namespace ash > > Done. > > https://codereview.chromium.org/1996563002/diff/440001/ash/common/system/chro... > ash/common/system/chromeos/ime_menu/ime_menu_tray.h:63: ash::IMEInfo > current_ime_; > On 2016/06/30 21:15:47, James Cook wrote: > > ash:: not needed > > Done. > > https://codereview.chromium.org/1996563002/diff/440001/ash/shelf/shelf.cc > File ash/shelf/shelf.cc (right): > > https://codereview.chromium.org/1996563002/diff/440001/ash/shelf/shelf.cc#new... > ash/shelf/shelf.cc:131: } > On 2016/06/30 21:15:47, James Cook wrote: > > nit: This might be easier to read with an early return for this case. You > could > > keep the original code for the non-ime-menu case. I think > > kShelfItemOnTrayForImeMenu is only used on chromeos, right? > > Yes, kShelfItemOnTrayForImeMenu is only for ChromeOs. > Adds early return here. > > https://codereview.chromium.org/1996563002/diff/440001/ash/test/test_system_t... > File ash/test/test_system_tray_delegate.cc (right): > > https://codereview.chromium.org/1996563002/diff/440001/ash/test/test_system_t... > ash/test/test_system_tray_delegate.cc:116: info->id = current_ime_.id; > On 2016/06/30 21:15:47, James Cook wrote: > > Can you use *info = current_ime_? If not, can you add an assignment operator > to > > IMEInfo? I think it could just be "= default" > > Done. > > https://codereview.chromium.org/1996563002/diff/440001/ash/test/test_system_t... > File ash/test/test_system_tray_delegate.h (right): > > https://codereview.chromium.org/1996563002/diff/440001/ash/test/test_system_t... > ash/test/test_system_tray_delegate.h:68: IMEInfo current_ime_; > On 2016/06/30 21:15:47, James Cook wrote: > > #include the header for this > > Done. > > https://codereview.chromium.org/1996563002/diff/440001/chrome/browser/ui/ash/... > File chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc > (right): > > https://codereview.chromium.org/1996563002/diff/440001/chrome/browser/ui/ash/... > chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:124: > // lanucher. Instead, is should show ImeMenuTray in the status tray. > On 2016/06/30 21:15:47, James Cook wrote: > > nit: lanucher -> launcher. Or did you mean shelf? > > > > Also "is should show" -> it should show > > Done. > > https://codereview.chromium.org/1996563002/diff/440001/chrome/browser/ui/ash/... > chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:130: > if (ime_menu_tray) { > On 2016/06/30 21:15:47, James Cook wrote: > > I think this pointer is never null when you have a status_area_widget. You can > > remove the if(). > > Done. Let's discuss more on email before we proceed.
Description was changed from ========== Make IME menu closer to the status tray. There has consistent spacing between panel windows with notification center/ status tray. This cl puts the IME menu icon on the status tray rather than in the shelf. BUG=570761, 624186 TEST=Verified on local build. ========== to ========== Adds ImeMenuTray element on status area widget for IME menu. It has the following behaviors: - Lays on the status tray, which has a consistent spacing between other shelf items. - Unmoveable by users, but automatically moving when status tray show/hide other items, like VK/web notification tray element. - Show/Hide the icon according to activation of new menu feature. - Show the short name of the current selected IME. - Change background color when clicked. BUG=570761, 624186 TEST=Verified on local build. ==========
> > Let's discuss more on email before we proceed. This cl has been updated. Please review it when you have time. Thanks!
https://codereview.chromium.org/1996563002/diff/480001/ash/common/system/chro... File ash/common/system/chromeos/ime_menu/ime_menu_tray.cc (right): https://codereview.chromium.org/1996563002/diff/480001/ash/common/system/chro... ash/common/system/chromeos/ime_menu/ime_menu_tray.cc:24: nit: ~ImeMenuLabel() override {} https://codereview.chromium.org/1996563002/diff/480001/ash/common/system/chro... ash/common/system/chromeos/ime_menu/ime_menu_tray.cc:77: if (is_activated) Do you need to clear is_active when is_activated == false? https://codereview.chromium.org/1996563002/diff/480001/ash/common/system/chro... ash/common/system/chromeos/ime_menu/ime_menu_tray.cc:93: int label_width = label_->width(); nit: const https://codereview.chromium.org/1996563002/diff/480001/ash/common/system/chro... ash/common/system/chromeos/ime_menu/ime_menu_tray.cc:94: int label_height = label_->height(); nit: ditto https://codereview.chromium.org/1996563002/diff/480001/ash/common/system/chro... File ash/common/system/chromeos/ime_menu/ime_menu_tray.h (right): https://codereview.chromium.org/1996563002/diff/480001/ash/common/system/chro... ash/common/system/chromeos/ime_menu/ime_menu_tray.h:22: class ASH_EXPORT ImeMenuTray : public TrayBackgroundView, public IMEObserver { nit: class comment please. https://codereview.chromium.org/1996563002/diff/480001/ash/common/system/chro... ash/common/system/chromeos/ime_menu/ime_menu_tray.h:50: bool is_active_; nit: Document what is active. The button (highlighted?). The menu (shown?). Both? https://codereview.chromium.org/1996563002/diff/480001/ash/common/system/chro... File ash/common/system/chromeos/ime_menu/ime_menu_tray_unittest.cc (right): https://codereview.chromium.org/1996563002/diff/480001/ash/common/system/chro... ash/common/system/chromeos/ime_menu/ime_menu_tray_unittest.cc:54: // activation of the IME menu. Thanks for writing comments about each test. It makes it much easier for me to read. https://codereview.chromium.org/1996563002/diff/480001/ash/common/system/chro... ash/common/system/chromeos/ime_menu/ime_menu_tray_unittest.cc:59: ASSERT_TRUE(IsVisible()); Use EXPECT_TRUE/EXPECT_FALSE for test expectations, unless the test would crash or completely fail if it continued running. https://codereview.chromium.org/1996563002/diff/480001/ash/common/system/chro... ash/common/system/chromeos/ime_menu/ime_menu_tray_unittest.cc:92: ASSERT_EQ(info2.short_name + base::UTF8ToUTF16("*"), GetTrayText()); optional nit: It's OK to copy/paste things like strings in tests. For example, I think it would be easier to read this if it was: EXPECT_EQ(base::UTF8ToUTF16("US*"), GetTrayText()) If that seems too long you could also do "using base::UTF8ToUTF16" to shorten the lines in these functions. https://codereview.chromium.org/1996563002/diff/480001/ash/common/system/chro... ash/common/system/chromeos/ime_menu/ime_menu_tray_unittest.cc:94: Can you add a test that checks that the label has a non-empty border? It doesn't need to check the specific size. views::View::GetInsets() or views::View::border() may be helpful here. https://codereview.chromium.org/1996563002/diff/480001/ash/common/system/chro... ash/common/system/chromeos/ime_menu/ime_menu_tray_unittest.cc:108: } Nice test suite. Easy to read. https://codereview.chromium.org/1996563002/diff/480001/ash/system/status_area... File ash/system/status_area_widget.h (right): https://codereview.chromium.org/1996563002/diff/480001/ash/system/status_area... ash/system/status_area_widget.h:61: ImeMenuTray* ime_menu_tray() { return ime_menu_tray_; } nit: Because this is only used in tests, rename to ime_menu_tray_for_testing() and move it down (just above the "private" section). But if you anticipate your next CL will need to access this pointer via StatusAreaWidget then this is fine.
https://codereview.chromium.org/1996563002/diff/480001/ash/common/system/chro... File ash/common/system/chromeos/ime_menu/ime_menu_tray.cc (right): https://codereview.chromium.org/1996563002/diff/480001/ash/common/system/chro... ash/common/system/chromeos/ime_menu/ime_menu_tray.cc:24: On 2016/07/12 17:53:37, James Cook wrote: > nit: ~ImeMenuLabel() override {} Done. https://codereview.chromium.org/1996563002/diff/480001/ash/common/system/chro... ash/common/system/chromeos/ime_menu/ime_menu_tray.cc:77: if (is_activated) On 2016/07/12 17:53:37, James Cook wrote: > Do you need to clear is_active when is_activated == false? Done. https://codereview.chromium.org/1996563002/diff/480001/ash/common/system/chro... ash/common/system/chromeos/ime_menu/ime_menu_tray.cc:93: int label_width = label_->width(); On 2016/07/12 17:53:37, James Cook wrote: > nit: const Removed this method. https://codereview.chromium.org/1996563002/diff/480001/ash/common/system/chro... File ash/common/system/chromeos/ime_menu/ime_menu_tray.h (right): https://codereview.chromium.org/1996563002/diff/480001/ash/common/system/chro... ash/common/system/chromeos/ime_menu/ime_menu_tray.h:22: class ASH_EXPORT ImeMenuTray : public TrayBackgroundView, public IMEObserver { On 2016/07/12 17:53:37, James Cook wrote: > nit: class comment please. Done. https://codereview.chromium.org/1996563002/diff/480001/ash/common/system/chro... ash/common/system/chromeos/ime_menu/ime_menu_tray.h:50: bool is_active_; On 2016/07/12 17:53:37, James Cook wrote: > nit: Document what is active. The button (highlighted?). The menu (shown?). > Both? Replaced this with function SetTrayActivation(bool is_active_); https://codereview.chromium.org/1996563002/diff/480001/ash/common/system/chro... File ash/common/system/chromeos/ime_menu/ime_menu_tray_unittest.cc (right): https://codereview.chromium.org/1996563002/diff/480001/ash/common/system/chro... ash/common/system/chromeos/ime_menu/ime_menu_tray_unittest.cc:59: ASSERT_TRUE(IsVisible()); On 2016/07/12 17:53:37, James Cook wrote: > Use EXPECT_TRUE/EXPECT_FALSE for test expectations, unless the test would crash > or completely fail if it continued running. Done. https://codereview.chromium.org/1996563002/diff/480001/ash/common/system/chro... ash/common/system/chromeos/ime_menu/ime_menu_tray_unittest.cc:92: ASSERT_EQ(info2.short_name + base::UTF8ToUTF16("*"), GetTrayText()); On 2016/07/12 17:53:37, James Cook wrote: > optional nit: It's OK to copy/paste things like strings in tests. For example, I > think it would be easier to read this if it was: > > EXPECT_EQ(base::UTF8ToUTF16("US*"), GetTrayText()) > > If that seems too long you could also do "using base::UTF8ToUTF16" to shorten > the lines in these functions. > Done. Thanks. https://codereview.chromium.org/1996563002/diff/480001/ash/common/system/chro... ash/common/system/chromeos/ime_menu/ime_menu_tray_unittest.cc:94: On 2016/07/12 17:53:37, James Cook wrote: > Can you add a test that checks that the label has a non-empty border? It doesn't > need to check the specific size. views::View::GetInsets() or > views::View::border() may be helpful here. Removed the codes of setting the label's border. It's used to adjust the label's height when the shelf alignment is changed to LEFT or RIGHT. Turns out this could be done by overriding the views::View::GetHeightForWidth(int width) method. https://codereview.chromium.org/1996563002/diff/480001/ash/system/status_area... File ash/system/status_area_widget.h (right): https://codereview.chromium.org/1996563002/diff/480001/ash/system/status_area... ash/system/status_area_widget.h:61: ImeMenuTray* ime_menu_tray() { return ime_menu_tray_; } On 2016/07/12 17:53:37, James Cook wrote: > nit: Because this is only used in tests, rename to ime_menu_tray_for_testing() > and move it down (just above the "private" section). > > But if you anticipate your next CL will need to access this pointer via > StatusAreaWidget then this is fine. Removed this as it has not been used for now. Will add it when needed.
LGTM with nits. https://codereview.chromium.org/1996563002/diff/500001/ash/common/system/chro... File ash/common/system/chromeos/ime_menu/ime_menu_tray.cc (right): https://codereview.chromium.org/1996563002/diff/500001/ash/common/system/chro... ash/common/system/chromeos/ime_menu/ime_menu_tray.cc:31: return GetPreferredSize().height(); nit: just return kTrayImeIconSize https://codereview.chromium.org/1996563002/diff/500001/ash/common/system/chro... ash/common/system/chromeos/ime_menu/ime_menu_tray.cc:93: void ImeMenuTray::SetTrayActivation(bool is_active_) { as above, is_active_ -> is_active https://codereview.chromium.org/1996563002/diff/500001/ash/common/system/chro... File ash/common/system/chromeos/ime_menu/ime_menu_tray.h (right): https://codereview.chromium.org/1996563002/diff/500001/ash/common/system/chro... ash/common/system/chromeos/ime_menu/ime_menu_tray.h:46: // Highlights the button and show the IME menu tray bubble if |is_active_| is nit: show -> shows, lowlight -> lowlights https://codereview.chromium.org/1996563002/diff/500001/ash/common/system/chro... ash/common/system/chromeos/ime_menu/ime_menu_tray.h:48: void SetTrayActivation(bool is_active_); nit: "is_active" not "is_active_" https://codereview.chromium.org/1996563002/diff/500001/ash/common/system/chro... File ash/common/system/chromeos/ime_menu/ime_menu_tray_unittest.cc (right): https://codereview.chromium.org/1996563002/diff/500001/ash/common/system/chro... ash/common/system/chromeos/ime_menu/ime_menu_tray_unittest.cc:54: bool IsTrayLabelHasBorder() { return ime_menu_tray_->label_->border(); } Not used - remove. https://codereview.chromium.org/1996563002/diff/500001/ash/common/system/chro... ash/common/system/chromeos/ime_menu/ime_menu_tray_unittest.cc:126: EXPECT_FALSE(IsTrayBackgroundActive()); Good idea to test this.
https://codereview.chromium.org/1996563002/diff/500001/ash/common/system/chro... File ash/common/system/chromeos/ime_menu/ime_menu_tray.cc (right): https://codereview.chromium.org/1996563002/diff/500001/ash/common/system/chro... ash/common/system/chromeos/ime_menu/ime_menu_tray.cc:31: return GetPreferredSize().height(); On 2016/07/14 15:47:35, James Cook wrote: > nit: just return kTrayImeIconSize Done. https://codereview.chromium.org/1996563002/diff/500001/ash/common/system/chro... ash/common/system/chromeos/ime_menu/ime_menu_tray.cc:93: void ImeMenuTray::SetTrayActivation(bool is_active_) { On 2016/07/14 15:47:35, James Cook wrote: > as above, is_active_ -> is_active Done. https://codereview.chromium.org/1996563002/diff/500001/ash/common/system/chro... File ash/common/system/chromeos/ime_menu/ime_menu_tray.h (right): https://codereview.chromium.org/1996563002/diff/500001/ash/common/system/chro... ash/common/system/chromeos/ime_menu/ime_menu_tray.h:46: // Highlights the button and show the IME menu tray bubble if |is_active_| is On 2016/07/14 15:47:35, James Cook wrote: > nit: show -> shows, lowlight -> lowlights Done. https://codereview.chromium.org/1996563002/diff/500001/ash/common/system/chro... ash/common/system/chromeos/ime_menu/ime_menu_tray.h:48: void SetTrayActivation(bool is_active_); On 2016/07/14 15:47:35, James Cook wrote: > nit: "is_active" not "is_active_" Done. https://codereview.chromium.org/1996563002/diff/500001/ash/common/system/chro... File ash/common/system/chromeos/ime_menu/ime_menu_tray_unittest.cc (right): https://codereview.chromium.org/1996563002/diff/500001/ash/common/system/chro... ash/common/system/chromeos/ime_menu/ime_menu_tray_unittest.cc:54: bool IsTrayLabelHasBorder() { return ime_menu_tray_->label_->border(); } On 2016/07/14 15:47:35, James Cook wrote: > Not used - remove. Done.
The CQ bit was checked by azurewei@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from shuchen@chromium.org, sadrul@chromium.org, jamescook@chromium.org Link to the patchset: https://codereview.chromium.org/1996563002/#ps520001 (title: "Addressed nits.")
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
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
The CQ bit was checked by azurewei@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from shuchen@chromium.org, sadrul@chromium.org, jamescook@chromium.org Link to the patchset: https://codereview.chromium.org/1996563002/#ps540001 (title: "Fix patch failure.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Adds ImeMenuTray element on status area widget for IME menu. It has the following behaviors: - Lays on the status tray, which has a consistent spacing between other shelf items. - Unmoveable by users, but automatically moving when status tray show/hide other items, like VK/web notification tray element. - Show/Hide the icon according to activation of new menu feature. - Show the short name of the current selected IME. - Change background color when clicked. BUG=570761, 624186 TEST=Verified on local build. ========== to ========== Adds ImeMenuTray element on status area widget for IME menu. It has the following behaviors: - Lays on the status tray, which has a consistent spacing between other shelf items. - Unmoveable by users, but automatically moving when status tray show/hide other items, like VK/web notification tray element. - Show/Hide the icon according to activation of new menu feature. - Show the short name of the current selected IME. - Change background color when clicked. BUG=570761, 624186 TEST=Verified on local build. ==========
Message was sent while issue was closed.
Committed patchset #28 (id:540001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Adds ImeMenuTray element on status area widget for IME menu. It has the following behaviors: - Lays on the status tray, which has a consistent spacing between other shelf items. - Unmoveable by users, but automatically moving when status tray show/hide other items, like VK/web notification tray element. - Show/Hide the icon according to activation of new menu feature. - Show the short name of the current selected IME. - Change background color when clicked. BUG=570761, 624186 TEST=Verified on local build. ========== to ========== Adds ImeMenuTray element on status area widget for IME menu. It has the following behaviors: - Lays on the status tray, which has a consistent spacing between other shelf items. - Unmoveable by users, but automatically moving when status tray show/hide other items, like VK/web notification tray element. - Show/Hide the icon according to activation of new menu feature. - Show the short name of the current selected IME. - Change background color when clicked. BUG=570761, 624186 TEST=Verified on local build. Committed: https://crrev.com/c0b0a4f3a8397cb257d975d947bf035e2b0d9a9e Cr-Commit-Position: refs/heads/master@{#405728} ==========
Message was sent while issue was closed.
Patchset 28 (id:??) landed as https://crrev.com/c0b0a4f3a8397cb257d975d947bf035e2b0d9a9e Cr-Commit-Position: refs/heads/master@{#405728} |
