Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2728063002:
cros: Fix immersive fullscreen tabstrip revealing does not draw well (Closed)
|
Created:
3 years, 9 months ago by Qiang(Joe) Xu Modified:
3 years, 9 months ago CC:
chromium-reviews, kalyank, sadrul, tfarina Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptioncros: Fix immersive fullscreen tabstrip revealing does not draw well
Changes:
When OnImmersiveRevealStarted(), if top container's layer is not marked as not filling bounds opaquely, the tabstrip may not draw correctly. It happens when user has theme installed or hovers mouse onto tabstrip.
Regression comes from: https://codereview.chromium.org/2674813002.
BUG=697099
TEST=device test bug does not happen
Review-Url: https://codereview.chromium.org/2728063002
Cr-Commit-Position: refs/heads/master@{#454896}
Committed: https://chromium.googlesource.com/chromium/src/+/bc2aafec33859883cdd69a43b301a40a79ce9327
Patch Set 1 #Patch Set 2 : DCHECK to warning in Label::PaintText #
Total comments: 4
Patch Set 3 : rebase and feedback #Patch Set 4 : delete the change in header_view #
Messages
Total messages: 25 (13 generated)
Description was changed from ========== Revert of 'Leave immersive bar header layers as opaque' BUG=697099 TEST=device test bug does not happen ========== to ========== Revert of 'Leave immersive bar header layers as opaque' Revert reason: When OnImmersiveRevealStarted(), if top container's layer is not marked as not filling bounds opaquely, the tabstrip may not draw correctly. It happens when user has theme installed or hover mouse onto tabstrip. Revert the CL: https://codereview.chromium.org/2674813002 to get rid of this bad effect before having a proper fix. BUG=697099 TEST=device test bug does not happen ==========
warx@chromium.org changed reviewers: + estade@chromium.org, pkotwicz@chromium.org
Description was changed from ========== Revert of 'Leave immersive bar header layers as opaque' Revert reason: When OnImmersiveRevealStarted(), if top container's layer is not marked as not filling bounds opaquely, the tabstrip may not draw correctly. It happens when user has theme installed or hover mouse onto tabstrip. Revert the CL: https://codereview.chromium.org/2674813002 to get rid of this bad effect before having a proper fix. BUG=697099 TEST=device test bug does not happen ========== to ========== Revert of 'Leave immersive bar header layers as opaque' Revert reason: When OnImmersiveRevealStarted(), if top container's layer is not marked as not filling bounds opaquely, the tabstrip may not draw correctly. It happens when user has theme installed or hovers mouse onto tabstrip. Revert the CL: https://codereview.chromium.org/2674813002 to get rid of this bad effect before having a proper fix. BUG=697099 TEST=device test bug does not happen ==========
warx@chromium.org changed reviewers: + sky@chromium.org
+sky@ since this revert also touches your added mash code
lgtm
You might have to also temporarily disable the dcheck added in Label::PaintText. On Mar 2, 2017 10:49 PM, <pkotwicz@chromium.org> wrote: > lgtm > > > > https://codereview.chromium.org/2728063002/ > -- 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.
LGTM
The CQ bit was checked by warx@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...
Hi Evan, new patch uploaded based on your comments, ptal, thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2728063002/diff/20001/ui/views/controls/label.cc File ui/views/controls/label.cc (left): https://codereview.chromium.org/2728063002/diff/20001/ui/views/controls/label... ui/views/controls/label.cc:458: #if DCHECK_IS_ON() Why ar eyou removing the DCHECK_IS_ON conditional? https://codereview.chromium.org/2728063002/diff/20001/ui/views/controls/label.cc File ui/views/controls/label.cc (right): https://codereview.chromium.org/2728063002/diff/20001/ui/views/controls/label... ui/views/controls/label.cc:470: LOG(WARNING) << "Ancestor view has a non-opaque layer: " DLOG at best.
https://codereview.chromium.org/2728063002/diff/20001/ui/views/controls/label.cc File ui/views/controls/label.cc (left): https://codereview.chromium.org/2728063002/diff/20001/ui/views/controls/label... ui/views/controls/label.cc:458: #if DCHECK_IS_ON() On 2017/03/03 19:15:39, sky wrote: > Why ar eyou removing the DCHECK_IS_ON conditional? Done. I thought we were going to remove the following DCHECK. https://codereview.chromium.org/2728063002/diff/20001/ui/views/controls/label.cc File ui/views/controls/label.cc (right): https://codereview.chromium.org/2728063002/diff/20001/ui/views/controls/label... ui/views/controls/label.cc:470: LOG(WARNING) << "Ancestor view has a non-opaque layer: " On 2017/03/03 19:15:39, sky wrote: > DLOG at best. Done.
thanks, lgtm with one question. Are you sure both cases are necessary? i.e. is the bug not fixed by just restoring one of the two SetDrawsBoundsOpaquely calls?
Description was changed from ========== Revert of 'Leave immersive bar header layers as opaque' Revert reason: When OnImmersiveRevealStarted(), if top container's layer is not marked as not filling bounds opaquely, the tabstrip may not draw correctly. It happens when user has theme installed or hovers mouse onto tabstrip. Revert the CL: https://codereview.chromium.org/2674813002 to get rid of this bad effect before having a proper fix. BUG=697099 TEST=device test bug does not happen ========== to ========== cros: Fix immersive fullscreen tabstrip revealing does not draw well Changes: When OnImmersiveRevealStarted(), if top container's layer is not marked as not filling bounds opaquely, the tabstrip may not draw correctly. It happens when user has theme installed or hovers mouse onto tabstrip. Regression comes from: https://codereview.chromium.org/2674813002. BUG=697099 TEST=device test bug does not happen ==========
On 2017/03/03 21:26:44, Evan Stade wrote: > thanks, lgtm with one question. Are you sure both cases are necessary? i.e. is > the bug not fixed by just restoring one of the two SetDrawsBoundsOpaquely calls? Hmm, only browser_view_->top_container()->layer()->SetFillsBoundsOpaquely(false) matters, because tabstrip is the child view of top_container(). The ash//header_view seems ok. I guess because it doesn't interact with tabs. I updated the patch, issue title and description. Please take another look, thanks!
The CQ bit was checked by warx@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkotwicz@chromium.org, sky@chromium.org, estade@chromium.org Link to the patchset: https://codereview.chromium.org/2728063002/#ps60001 (title: "delete the change in header_view")
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": 1488821922939780,
"parent_rev": "0f16147c62ef8635f594ed77c415cc9de2ece5a6", "commit_rev":
"bc2aafec33859883cdd69a43b301a40a79ce9327"}
Message was sent while issue was closed.
Description was changed from ========== cros: Fix immersive fullscreen tabstrip revealing does not draw well Changes: When OnImmersiveRevealStarted(), if top container's layer is not marked as not filling bounds opaquely, the tabstrip may not draw correctly. It happens when user has theme installed or hovers mouse onto tabstrip. Regression comes from: https://codereview.chromium.org/2674813002. BUG=697099 TEST=device test bug does not happen ========== to ========== cros: Fix immersive fullscreen tabstrip revealing does not draw well Changes: When OnImmersiveRevealStarted(), if top container's layer is not marked as not filling bounds opaquely, the tabstrip may not draw correctly. It happens when user has theme installed or hovers mouse onto tabstrip. Regression comes from: https://codereview.chromium.org/2674813002. BUG=697099 TEST=device test bug does not happen Review-Url: https://codereview.chromium.org/2728063002 Cr-Commit-Position: refs/heads/master@{#454896} Committed: https://chromium.googlesource.com/chromium/src/+/bc2aafec33859883cdd69a43b301... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/bc2aafec33859883cdd69a43b301... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
