Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1996793002:
Fill the canvas with black background before painting wallpaper (Closed)
|
Created:
4 years, 7 months ago by xiaoyinh(OOO Sep 11-29) Modified:
4 years, 7 months ago CC:
chromium-reviews, kalyank, sadrul Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionThis change is to fill the canvas with black background every time before painting a new wallpaper, so that the new wallpaper with partial transparency won't be painted upon a previous wallpaper.
BUG=606434
Committed: https://crrev.com/5fb57605dc341abde4f2eace5ec578520b74b224
Cr-Commit-Position: refs/heads/master@{#395175}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Incorporated comments from xdai1@ #Messages
Total messages: 19 (9 generated)
xiaoyinh@chromium.org changed reviewers: + bshe@chromium.org, xdai@chromium.org
lgtm with nits Also please update the subject and description to better describe this issue: what's the problem and why you should do this. Thanks! And also please wait for bshe@'s owner review. https://codereview.chromium.org/1996793002/diff/1/ash/desktop_background/desk... File ash/desktop_background/desktop_background_view.cc (right): https://codereview.chromium.org/1996793002/diff/1/ash/desktop_background/desk... ash/desktop_background/desktop_background_view.cc:142: // Reset the canvas before rendering wallpaper. Might be better to say "Fill the canvas with black background to make it opaque before painting wallpaper"? https://codereview.chromium.org/1996793002/diff/1/ash/desktop_background/desk... ash/desktop_background/desktop_background_view.cc:144: Move this part after if(wallpaper.isNull())... https://codereview.chromium.org/1996793002/diff/1/ash/desktop_background/desk... ash/desktop_background/desktop_background_view.cc:145: if (wallpaper.isNull()) { No brace please https://codereview.chromium.org/1996793002/diff/1/ash/desktop_background/desk... ash/desktop_background/desktop_background_view.cc:184: canvas->FillRect(GetLocalBounds(), SK_ColorBLACK); Remove this line.
Description was changed from ========== format file reset background before painting wallpaper BUG=606434 ========== to ========== This change is to fill the canvas with black background every time before painting a new wallpaper, so that the new wallpaper with partial transparency won't be painted upon a previous wallpaper. BUG=606434 ==========
All the above comments has been incorporated. Thanks. https://codereview.chromium.org/1996793002/diff/1/ash/desktop_background/desk... File ash/desktop_background/desktop_background_view.cc (right): https://codereview.chromium.org/1996793002/diff/1/ash/desktop_background/desk... ash/desktop_background/desktop_background_view.cc:142: // Reset the canvas before rendering wallpaper. On 2016/05/19 23:27:55, xdai1 wrote: > Might be better to say "Fill the canvas with black background to make it opaque > before painting wallpaper"? Done. https://codereview.chromium.org/1996793002/diff/1/ash/desktop_background/desk... ash/desktop_background/desktop_background_view.cc:144: On 2016/05/19 23:27:55, xdai1 wrote: > Move this part after if(wallpaper.isNull())... Discussed with xdai1@, this will remain as is. https://codereview.chromium.org/1996793002/diff/1/ash/desktop_background/desk... ash/desktop_background/desktop_background_view.cc:145: if (wallpaper.isNull()) { On 2016/05/19 23:27:55, xdai1 wrote: > No brace please Done. https://codereview.chromium.org/1996793002/diff/1/ash/desktop_background/desk... ash/desktop_background/desktop_background_view.cc:184: canvas->FillRect(GetLocalBounds(), SK_ColorBLACK); On 2016/05/19 23:27:55, xdai1 wrote: > Remove this line. Done.
xiaoyinh@chromium.org changed reviewers: + achuith@chromium.org
achuith@, could you help take a look? Seems like bshe@ is OOO in the next few days. Thanks!
lgtm based on Daisy's review.
The CQ bit was checked by xiaoyinh@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/1996793002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1996793002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by xiaoyinh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xdai@chromium.org Link to the patchset: https://codereview.chromium.org/1996793002/#ps20001 (title: "Incorporated comments from xdai1@")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1996793002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1996793002/20001
Message was sent while issue was closed.
Description was changed from ========== This change is to fill the canvas with black background every time before painting a new wallpaper, so that the new wallpaper with partial transparency won't be painted upon a previous wallpaper. BUG=606434 ========== to ========== This change is to fill the canvas with black background every time before painting a new wallpaper, so that the new wallpaper with partial transparency won't be painted upon a previous wallpaper. BUG=606434 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== This change is to fill the canvas with black background every time before painting a new wallpaper, so that the new wallpaper with partial transparency won't be painted upon a previous wallpaper. BUG=606434 ========== to ========== This change is to fill the canvas with black background every time before painting a new wallpaper, so that the new wallpaper with partial transparency won't be painted upon a previous wallpaper. BUG=606434 Committed: https://crrev.com/5fb57605dc341abde4f2eace5ec578520b74b224 Cr-Commit-Position: refs/heads/master@{#395175} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/5fb57605dc341abde4f2eace5ec578520b74b224 Cr-Commit-Position: refs/heads/master@{#395175} |
