Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2839953005:
Round the corners of bookmark button ink drop effects. (Closed)
|
Created:
3 years, 7 months ago by Evan Stade Modified:
3 years, 7 months ago CC:
chromium-reviews, tfarina, bruthig+ink_drop_chromium.org, dcheng Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionRound the corners of bookmark button ink drop effects.
This applies to Linux and Chrome OS (and Mac Views) until
713359 is fixed.
BUG=602756, 713359
Review-Url: https://codereview.chromium.org/2839953005
Cr-Commit-Position: refs/heads/master@{#468090}
Committed: https://chromium.googlesource.com/chromium/src/+/6fe9396d6935924e6bff4345269293ed3ffd430e
Patch Set 1 #Patch Set 2 : no windows #
Total comments: 4
Patch Set 3 : couple of fixes #Patch Set 4 : rearrange #
Total comments: 5
Patch Set 5 : move macro #
Messages
Total messages: 37 (21 generated)
Description was changed from ========== Round the corners of bookmark button ink drop effects. BUG=602756 ========== to ========== Round the corners of bookmark button ink drop effects. This applies to Linux and Chrome OS (and Mac View) until 713359 is fixed. BUG=602756,713359 ==========
estade@chromium.org changed reviewers: + bruthig@chromium.org
The CQ bit was checked by estade@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/2839953005/diff/20001/ui/views/animation/ink_... File ui/views/animation/ink_drop_host_view.cc (left): https://codereview.chromium.org/2839953005/diff/20001/ui/views/animation/ink_... ui/views/animation/ink_drop_host_view.cc:137: ink_drop_layer->SetMaskLayer(ink_drop_mask_->layer()); Did you mean to replace lines 135-137 with a call to InstallInkDropMask? There are many buttons that don't extend LabelButton who are already using an InkDropMask. Am I missing something or won't this effectively remove the mask from those surfaces? https://codereview.chromium.org/2839953005/diff/20001/ui/views/animation/ink_... File ui/views/animation/ink_drop_host_view.h (right): https://codereview.chromium.org/2839953005/diff/20001/ui/views/animation/ink_... ui/views/animation/ink_drop_host_view.h:55: InkDropMode ink_drop_mode() const { return ink_drop_mode_; } Where is this needed?
https://codereview.chromium.org/2839953005/diff/20001/ui/views/animation/ink_... File ui/views/animation/ink_drop_host_view.cc (left): https://codereview.chromium.org/2839953005/diff/20001/ui/views/animation/ink_... ui/views/animation/ink_drop_host_view.cc:137: ink_drop_layer->SetMaskLayer(ink_drop_mask_->layer()); On 2017/04/26 20:45:40, bruthig wrote: > Did you mean to replace lines 135-137 with a call to InstallInkDropMask? oops, yes. https://codereview.chromium.org/2839953005/diff/20001/ui/views/animation/ink_... File ui/views/animation/ink_drop_host_view.h (right): https://codereview.chromium.org/2839953005/diff/20001/ui/views/animation/ink_... ui/views/animation/ink_drop_host_view.h:55: InkDropMode ink_drop_mode() const { return ink_drop_mode_; } On 2017/04/26 20:45:40, bruthig wrote: > Where is this needed? ah, this is from a different CL. Will remove.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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/...)
updated, ptal
lgtm https://codereview.chromium.org/2839953005/diff/60001/ui/views/animation/ink_... File ui/views/animation/ink_drop_host_view.cc (right): https://codereview.chromium.org/2839953005/diff/60001/ui/views/animation/ink_... ui/views/animation/ink_drop_host_view.cc:148: ink_drop_mask_.reset(); nit: Should this call ResetInkDropMask()?
estade@chromium.org changed reviewers: + sky@chromium.org
+sky, ptal https://codereview.chromium.org/2839953005/diff/60001/ui/views/animation/ink_... File ui/views/animation/ink_drop_host_view.cc (right): https://codereview.chromium.org/2839953005/diff/60001/ui/views/animation/ink_... ui/views/animation/ink_drop_host_view.cc:148: ink_drop_mask_.reset(); On 2017/04/27 20:22:45, bruthig wrote: > nit: Should this call ResetInkDropMask()? I don't have a strong opinion but if I read that it might make me think that it was doing something more complicated than a one liner
The CQ bit was checked by estade@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: Try jobs failed on following builders: linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2839953005/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right): https://codereview.chromium.org/2839953005/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:383: #if !defined(OS_WIN) Are we generally doing ifdefs for this? Is it worth a PlatformStyle value?
https://codereview.chromium.org/2839953005/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right): https://codereview.chromium.org/2839953005/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:383: #if !defined(OS_WIN) On 2017/04/27 22:32:04, sky wrote: > Are we generally doing ifdefs for this? Is it worth a PlatformStyle value? I think this may be the first time we're doing this. The #ifdef might be better moved to InstallInkDropMask so it applies to all masks and we won't have to add ifdefs in the future either. Hopefully that bug gets fixed and then we can enable them on Windows.
https://codereview.chromium.org/2839953005/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right): https://codereview.chromium.org/2839953005/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:383: #if !defined(OS_WIN) On 2017/04/27 22:36:31, Evan Stade wrote: > On 2017/04/27 22:32:04, sky wrote: > > Are we generally doing ifdefs for this? Is it worth a PlatformStyle value? > > I think this may be the first time we're doing this. The #ifdef might be better > moved to InstallInkDropMask so it applies to all masks and we won't have to add > ifdefs in the future either. > > Hopefully that bug gets fixed and then we can enable them on Windows. So, would it be better to move the ifdef as you say in this patch?
On 2017/04/27 22:54:09, sky wrote: > https://codereview.chromium.org/2839953005/diff/60001/chrome/browser/ui/views... > File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right): > > https://codereview.chromium.org/2839953005/diff/60001/chrome/browser/ui/views... > chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:383: #if !defined(OS_WIN) > On 2017/04/27 22:36:31, Evan Stade wrote: > > On 2017/04/27 22:32:04, sky wrote: > > > Are we generally doing ifdefs for this? Is it worth a PlatformStyle value? > > > > I think this may be the first time we're doing this. The #ifdef might be > better > > moved to InstallInkDropMask so it applies to all masks and we won't have to > add > > ifdefs in the future either. > > > > Hopefully that bug gets fixed and then we can enable them on Windows. > > So, would it be better to move the ifdef as you say in this patch? yes, sorry for being cryptic. Should have finished off that thought with "wdyt". Moved the ifdef.
The CQ bit was checked by estade@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: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
LGTM - assuming InkDropHostView::InstallInkDropMask() for the windows case doesn't leak.
On 2017/04/28 17:24:44, sky wrote: > LGTM - assuming InkDropHostView::InstallInkDropMask() for the windows case > doesn't leak. which object are you worried about leaking?
Description was changed from ========== Round the corners of bookmark button ink drop effects. This applies to Linux and Chrome OS (and Mac View) until 713359 is fixed. BUG=602756,713359 ========== to ========== Round the corners of bookmark button ink drop effects. This applies to Linux and Chrome OS (and Mac Views) until 713359 is fixed. BUG=602756,713359 ==========
The CQ bit was checked by estade@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bruthig@chromium.org Link to the patchset: https://codereview.chromium.org/2839953005/#ps80001 (title: "move macro")
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": 80001, "attempt_start_ts": 1493404815554580,
"parent_rev": "6eacb1e7be2be27916d9c329fbd9859fc5c2bf01", "commit_rev":
"6fe9396d6935924e6bff4345269293ed3ffd430e"}
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1493404815554580,
"parent_rev": "6eacb1e7be2be27916d9c329fbd9859fc5c2bf01", "commit_rev":
"6fe9396d6935924e6bff4345269293ed3ffd430e"}
Message was sent while issue was closed.
Description was changed from ========== Round the corners of bookmark button ink drop effects. This applies to Linux and Chrome OS (and Mac Views) until 713359 is fixed. BUG=602756,713359 ========== to ========== Round the corners of bookmark button ink drop effects. This applies to Linux and Chrome OS (and Mac Views) until 713359 is fixed. BUG=602756,713359 Review-Url: https://codereview.chromium.org/2839953005 Cr-Commit-Position: refs/heads/master@{#468090} Committed: https://chromium.googlesource.com/chromium/src/+/6fe9396d6935924e6bff43452692... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/6fe9396d6935924e6bff43452692...
Message was sent while issue was closed.
Sorry for not being clear. The Layer that is passed to InkDropHostView::InstallInkDropMask. On Fri, Apr 28, 2017 at 11:39 AM, <estade@chromium.org> wrote: > On 2017/04/28 17:24:44, sky wrote: > > LGTM - assuming InkDropHostView::InstallInkDropMask() for the windows > case > > doesn't leak. > > which object are you worried about leaking? > > https://codereview.chromium.org/2839953005/ > -- 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.
Message was sent while issue was closed.
On 2017/04/28 21:59:26, sky wrote: > Sorry for not being clear. The Layer that is passed > to InkDropHostView::InstallInkDropMask. yea, not leaking. That param is a weak reference. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
