Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2870613002:
Use defualt shadow elevation. (Closed)
DescriptionUse default shadow elevation.
* Setting medium always use medium regardless of the active
state.
BUG=719415
TEST=manual. Start ARC app and activate deactivate.
Review-Url: https://codereview.chromium.org/2870613002
Cr-Commit-Position: refs/heads/master@{#470156}
Committed: https://chromium.googlesource.com/chromium/src/+/34e7e8f91cf42bbe67ff86b75e3aa616046ccccd
Patch Set 1 #
Total comments: 1
Patch Set 2 : Use defualt shadow elevation. #Patch Set 3 : update unit tests to use DEFAULT #
Messages
Total messages: 24 (11 generated)
oshima@chromium.org changed reviewers: + reveman@chromium.org
https://codereview.chromium.org/2870613002/diff/1/components/exo/shell_surfac... File components/exo/shell_surface.cc (left): https://codereview.chromium.org/2870613002/diff/1/components/exo/shell_surfac... components/exo/shell_surface.cc:1732: shadow->SetRoundedCornerRadius(0); This is causing b/37254594
Arc windows don't have rounded corners so why doesn't radius 0 work? Is the shadow code broken? I'd like to keep it 0 so we avoid overdraw. Mostly to make overdraw debugging easier. It's confusing to see overdraw when it shouldn't be needed.
On 2017/05/08 11:27:12, reveman wrote: > Arc windows don't have rounded corners so why doesn't radius 0 work? Is the > shadow code broken? I'd like to keep it 0 so we avoid overdraw. Mostly to make > overdraw debugging easier. It's confusing to see overdraw when it shouldn't be > needed. This is used simply adjust the insets of the shadow. Setting 0 leaves the gap between the content and the shadow, as you can see it. This shouldn't cause overdraw because the shadow is below the window, right?
On 2017/05/08 at 11:33:22, oshima wrote: > On 2017/05/08 11:27:12, reveman wrote: > > Arc windows don't have rounded corners so why doesn't radius 0 work? Is the > > shadow code broken? I'd like to keep it 0 so we avoid overdraw. Mostly to make > > overdraw debugging easier. It's confusing to see overdraw when it shouldn't be > > needed. > > This is used simply adjust the insets of the shadow. Setting 0 leaves the gap between > the content and the shadow, as you can see it. This shouldn't cause overdraw because > the shadow is below the window, right? Why a gap when 0? I wouldn't expect from telling the shadow code that radius is 0. Unless the compositor has perfect occlusion culling code (it doesn't) or the window contents needs blending we'll see overdraw if we inset the shadow below the window.
Patchset #2 (id:20001) has been deleted
On 2017/05/08 11:48:16, reveman wrote: > On 2017/05/08 at 11:33:22, oshima wrote: > > On 2017/05/08 11:27:12, reveman wrote: > > > Arc windows don't have rounded corners so why doesn't radius 0 work? Is the > > > shadow code broken? I'd like to keep it 0 so we avoid overdraw. Mostly to > make > > > overdraw debugging easier. It's confusing to see overdraw when it shouldn't > be > > > needed. > > > > This is used simply adjust the insets of the shadow. Setting 0 leaves the gap > between > > the content and the shadow, as you can see it. This shouldn't cause overdraw > because > > the shadow is below the window, right? > > Why a gap when 0? I wouldn't expect from telling the shadow code that radius is > 0. > > Unless the compositor has perfect occlusion culling code (it doesn't) or the > window contents needs blending we'll see overdraw if we inset the shadow below > the window. Hmm, then there maybe a bug in shadow code. I uploaded a few example in b/37254594. Let me land the activation fix, and I'll look into the gap separately.
lgtm after updating the description to not mention rounded corners
Description was changed from ========== Use defualt shadow elevation. * Setting medium always use medium regardless of the active state. * Do not adjust the rounded corner for shadow. This creates gap between shadow and content. This should not be necessary because the shadow is under the main window. BUG=719415 TEST=manual. Start ARC app and activate deactivate. ========== to ========== Use default shadow elevation. * Setting medium always use medium regardless of the active state. BUG=719415 TEST=manual. Start ARC app and activate deactivate. ==========
On 2017/05/08 21:20:01, reveman wrote: > lgtm after updating the description to not mention rounded corners done
The CQ bit was checked by oshima@chromium.org
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: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by oshima@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from reveman@chromium.org Link to the patchset: https://codereview.chromium.org/2870613002/#ps60001 (title: "update unit tests to use DEFAULT")
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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by oshima@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": 60001, "attempt_start_ts": 1494293514456410,
"parent_rev": "b0433ed2c9ef88bec875e676b03e861ee6a1b601", "commit_rev":
"34e7e8f91cf42bbe67ff86b75e3aa616046ccccd"}
Message was sent while issue was closed.
Description was changed from ========== Use default shadow elevation. * Setting medium always use medium regardless of the active state. BUG=719415 TEST=manual. Start ARC app and activate deactivate. ========== to ========== Use default shadow elevation. * Setting medium always use medium regardless of the active state. BUG=719415 TEST=manual. Start ARC app and activate deactivate. Review-Url: https://codereview.chromium.org/2870613002 Cr-Commit-Position: refs/heads/master@{#470156} Committed: https://chromium.googlesource.com/chromium/src/+/34e7e8f91cf42bbe67ff86b75e3a... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001) as https://chromium.googlesource.com/chromium/src/+/34e7e8f91cf42bbe67ff86b75e3a... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
