Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2818183002:
Open system menu when click the status tray with volume popup opend (Closed)
DescriptionOpen system menu when click the status tray with volume popup opened.
Should open the system menu with one click of the status tray when a
popup detailed view is visible. This is a regression issue related to
https://codereview.chromium.org/2784163003/
BUG=711330
Review-Url: https://codereview.chromium.org/2818183002
Cr-Commit-Position: refs/heads/master@{#465801}
Committed: https://chromium.googlesource.com/chromium/src/+/444e30cbb5d2bc60ea021b2405a6cfeea17ca14c
Patch Set 1 #Patch Set 2 : Update comment. #
Total comments: 8
Patch Set 3 : Address comments. #
Total comments: 1
Messages
Total messages: 25 (17 generated)
Description was changed from ========== Open system menu when click the status tray with volume popup opend BUG=711330 ========== to ========== Open system menu when click the status tray with volume popup opend BUG=711330 ==========
minch@chromium.org changed reviewers: + tdanderson@chromium.org
Hi, tdanderson@, I think this issue is related to my previous one https://bugs.chromium.org/p/chromium/issues/detail?id=690112. Can you help review? Thanks.
The CQ bit was checked by minch@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by minch@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2818183002/diff/20001/ash/system/tray/system_... File ash/system/tray/system_tray.cc (right): https://codereview.chromium.org/2818183002/diff/20001/ash/system/tray/system_... ash/system/tray/system_tray.cc:333: detailed_view_with_close_delay_ = close_delay > 0 ? true : false; I think you can just say = close_delay > 0 here instead of using a ternary. Also, I am concerned about this member staying up-to-date. Would you also want to modify this member in cases such as: * If SystemTray::SetDetailedViewCloseDelay() is called * (maybe) when the detailed view closes? And/or you should also update the member documentation to clarify that this member only has meaning IF a detailed view is currently open. https://codereview.chromium.org/2818183002/diff/20001/ash/system/tray/system_... ash/system/tray/system_tray.cc:692: !detailed_view_with_close_delay_)) { With this change, if you: * Open the system menu * Open one of the detailed views (a11y for instance) * Click the tray button What happens? From reading this it seems like the menu won't close, and this would be regressing the change you made in https://codereview.chromium.org/2784163003 https://codereview.chromium.org/2818183002/diff/20001/ash/system/tray/system_... File ash/system/tray/system_tray.h (right): https://codereview.chromium.org/2818183002/diff/20001/ash/system/tray/system_... ash/system/tray/system_tray.h:237: TrayTiles* tray_tiles_ = nullptr; // only used in material design. nit: can you please remove the "only used in material design" comments here and on line 238? (Not related to this CL, they are just leftover from previous cleanup)
Hi, tdanderson@, can you help check my reply? Thanks. https://codereview.chromium.org/2818183002/diff/20001/ash/system/tray/system_... File ash/system/tray/system_tray.cc (right): https://codereview.chromium.org/2818183002/diff/20001/ash/system/tray/system_... ash/system/tray/system_tray.cc:333: detailed_view_with_close_delay_ = close_delay > 0 ? true : false; On 2017/04/18 17:59:14, tdanderson wrote: > I think you can just say = close_delay > 0 here instead of using a ternary. > > Also, I am concerned about this member staying up-to-date. Would you also want > to modify this member in cases such as: > > * If SystemTray::SetDetailedViewCloseDelay() is called > * (maybe) when the detailed view closes? > > And/or you should also update the member documentation to clarify that this > member only has meaning IF a detailed view is currently open. I think the ternary here is necessary. Since every time 'ShowDetailedView' is called, the variable detailed_view_with_close_delay will be updated according to the type of the detailed view (popup with close_delay or detailed view in system menu without close_delay) https://codereview.chromium.org/2818183002/diff/20001/ash/system/tray/system_... ash/system/tray/system_tray.cc:692: !detailed_view_with_close_delay_)) { On 2017/04/18 17:59:14, tdanderson wrote: > With this change, if you: > > * Open the system menu > * Open one of the detailed views (a11y for instance) > * Click the tray button > > What happens? From reading this it seems like the menu won't close, and this > would be regressing the change you made in > https://codereview.chromium.org/2784163003 I tried this in my device. The system menu will be closed correctly in this scenario. Since detailed_view_with_close_delay_ is updated in ShowDetailedView.
minch@chromium.org changed reviewers: + xiaoyinh@chromium.org
https://codereview.chromium.org/2818183002/diff/20001/ash/system/tray/system_... File ash/system/tray/system_tray.cc (right): https://codereview.chromium.org/2818183002/diff/20001/ash/system/tray/system_... ash/system/tray/system_tray.cc:333: detailed_view_with_close_delay_ = close_delay > 0 ? true : false; On 2017/04/18 18:29:34, minch1 wrote: > On 2017/04/18 17:59:14, tdanderson wrote: > > I think you can just say = close_delay > 0 here instead of using a ternary. > > > > Also, I am concerned about this member staying up-to-date. Would you also want > > to modify this member in cases such as: > > > > * If SystemTray::SetDetailedViewCloseDelay() is called > > * (maybe) when the detailed view closes? > > > > And/or you should also update the member documentation to clarify that this > > member only has meaning IF a detailed view is currently open. > > I think the ternary here is necessary. Since every time 'ShowDetailedView' is > called, the variable detailed_view_with_close_delay will be updated according to > the type of the detailed view (popup with close_delay or detailed view in system > menu without close_delay) I agree that you should be updating |detailed_view_with_close_delay_| here, but my comment is that I think you can just write this: detailed_view_with_close_delay_ = close_delay > 0; instead of this: detailed_view_with_close_delay_ = close_delay > 0 ? true : false; https://codereview.chromium.org/2818183002/diff/20001/ash/system/tray/system_... ash/system/tray/system_tray.cc:692: !detailed_view_with_close_delay_)) { On 2017/04/18 18:29:34, minch1 wrote: > On 2017/04/18 17:59:14, tdanderson wrote: > > With this change, if you: > > > > * Open the system menu > > * Open one of the detailed views (a11y for instance) > > * Click the tray button > > > > What happens? From reading this it seems like the menu won't close, and this > > would be regressing the change you made in > > https://codereview.chromium.org/2784163003 > > I tried this in my device. The system menu will be closed correctly in this > scenario. Since detailed_view_with_close_delay_ is updated in ShowDetailedView. You're right, I was reading this incorrectly. What you have here will work. https://codereview.chromium.org/2818183002/diff/20001/ash/system/tray/system_... File ash/system/tray/system_tray.h (right): https://codereview.chromium.org/2818183002/diff/20001/ash/system/tray/system_... ash/system/tray/system_tray.h:226: bool full_system_tray_menu_ = false; Can you check to see if you can use the existing |full_system_tray_menu_| instead of introducing a new member? It seems like both members might be tracking the same things.
tdanderson@, thanks for your comments. You are right, we can just use the |full_system_tray_menu_| instead. Can you take a look again?
The CQ bit was checked by minch@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM. Once this lands on ToT and you have verified on canary, please request (and perform) a merge back into M-59. https://codereview.chromium.org/2818183002/diff/40001/ash/system/tray/system_... File ash/system/tray/system_tray.h (right): https://codereview.chromium.org/2818183002/diff/40001/ash/system/tray/system_... ash/system/tray/system_tray.h:5: #ifndef ASH_SYSTEM_TRAY_SYSTEM_TRAY_H_ nit: typo in the CL description (opend->opened) nit: Also please include a brief CL description, linking to the CL that caused this regression.
Description was changed from ========== Open system menu when click the status tray with volume popup opend BUG=711330 ========== to ========== Open system menu when click the status tray with volume popup opened. Should open the system menu with one click of the status tray when a popup detailed view is visible. This is a regression issue related to https://codereview.chromium.org/2784163003/ BUG=711330 ==========
The CQ bit was checked by minch@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1492642460365230,
"parent_rev": "25e4011a30bc7e0cbb2912ea1fe47defefa606f8", "commit_rev":
"444e30cbb5d2bc60ea021b2405a6cfeea17ca14c"}
Message was sent while issue was closed.
Description was changed from ========== Open system menu when click the status tray with volume popup opened. Should open the system menu with one click of the status tray when a popup detailed view is visible. This is a regression issue related to https://codereview.chromium.org/2784163003/ BUG=711330 ========== to ========== Open system menu when click the status tray with volume popup opened. Should open the system menu with one click of the status tray when a popup detailed view is visible. This is a regression issue related to https://codereview.chromium.org/2784163003/ BUG=711330 Review-Url: https://codereview.chromium.org/2818183002 Cr-Commit-Position: refs/heads/master@{#465801} Committed: https://chromium.googlesource.com/chromium/src/+/444e30cbb5d2bc60ea021b2405a6... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/444e30cbb5d2bc60ea021b2405a6... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
