Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2671593003:
Re-layout Chrome OS audio row when headphone connected (Closed)
DescriptionRe-layout Chrome OS audio row when headphone connected
Whenever visibility of device type icon in the audio row of system menu
changes, layout needs to be invalidated so that the row is re-layed out.
BUG=680171
TEST=manual
Review-Url: https://codereview.chromium.org/2671593003
Cr-Commit-Position: refs/heads/master@{#447879}
Committed: https://chromium.googlesource.com/chromium/src/+/74921d58b53f2333e9a3c921ecb87ca65721a902
Patch Set 1 #
Total comments: 2
Patch Set 2 : Update visibility iff necessary #Messages
Total messages: 18 (11 generated)
The CQ bit was checked by mohsen@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...
mohsen@chromium.org changed reviewers: + tdanderson@chromium.org
Please take a look...
sadrul@chromium.org changed reviewers: + sadrul@chromium.org
https://codereview.chromium.org/2671593003/diff/1/ash/common/system/chromeos/... File ash/common/system/chromeos/audio/volume_view.cc (right): https://codereview.chromium.org/2671593003/diff/1/ash/common/system/chromeos/... ash/common/system/chromeos/audio/volume_view.cc:281: device_type_->InvalidateLayout(); Setting the visibility can trigger a repaint. For example, if the View is visible, then we mark it as hidden first (line 262 above), then make it visible again. So although we don't actually need to repaint, we end up repainting anyway. May I suggest changing this code so this doesn't happen? For example: if (!show_more) return; bool should_be_visible = false; if (....) { ... should_be_visible = true; } else { ... should_be_visible = true; } if (should_be_visible != device_type_->visible()) { device_type_->SetVisible(should_be_visible); device_type_->InvalidateLayout(); } i.e. set the visibility only if we know that it needs to be changed.
The CQ bit was checked by mohsen@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...
https://codereview.chromium.org/2671593003/diff/1/ash/common/system/chromeos/... File ash/common/system/chromeos/audio/volume_view.cc (right): https://codereview.chromium.org/2671593003/diff/1/ash/common/system/chromeos/... ash/common/system/chromeos/audio/volume_view.cc:281: device_type_->InvalidateLayout(); On 2017/02/02 at 21:25:34, sadrul wrote: > Setting the visibility can trigger a repaint. For example, if the View is visible, then we mark it as hidden first (line 262 above), then make it visible again. So although we don't actually need to repaint, we end up repainting anyway. > > May I suggest changing this code so this doesn't happen? For example: > if (!show_more) > return; > bool should_be_visible = false; > if (....) { > ... > should_be_visible = true; > } else { > ... > should_be_visible = true; > } > if (should_be_visible != device_type_->visible()) { > device_type_->SetVisible(should_be_visible); > device_type_->InvalidateLayout(); > } > > > i.e. set the visibility only if we know that it needs to be changed. Sure. That makes a lot of sense. Done.
lgtm Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm also
The CQ bit was checked by mohsen@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": 20001, "attempt_start_ts": 1486078396227640,
"parent_rev": "f620a47cade760e6047bcaff139d12376f2f1014", "commit_rev":
"74921d58b53f2333e9a3c921ecb87ca65721a902"}
Message was sent while issue was closed.
Description was changed from ========== Re-layout Chrome OS audio row when headphone connected Whenever visibility of device type icon in the audio row of system menu changes, layout needs to be invalidated so that the row is re-layed out. BUG=680171 TEST=manual ========== to ========== Re-layout Chrome OS audio row when headphone connected Whenever visibility of device type icon in the audio row of system menu changes, layout needs to be invalidated so that the row is re-layed out. BUG=680171 TEST=manual Review-Url: https://codereview.chromium.org/2671593003 Cr-Commit-Position: refs/heads/master@{#447879} Committed: https://chromium.googlesource.com/chromium/src/+/74921d58b53f2333e9a3c921ecb8... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/74921d58b53f2333e9a3c921ecb8... |
