Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2016203002:
Widget opacity goes from 0 to 1, not 0 to 255. (Closed)
|
Created:
4 years, 7 months ago by Peter Kasting Modified:
4 years, 6 months ago CC:
chromium-reviews, tfarina, cc-bugs_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionWidget opacity goes from 0 to 1, not 0 to 255.
This change was made in r395980, but missed a lot of callsites.
This also adds DCHECKs for this to the compositor in hopes of catching similar errors sooner in the future. These DCHECKs caught most of the places touched by this patch.
BUG=610039
TEST=Trigger new backspace shortcut bubble; it should fade in and out, not flicker and mostly be white.
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
Committed: https://crrev.com/d28e06bee3c0807483aed283bafd4c6f72198961
Cr-Commit-Position: refs/heads/master@{#396590}
Patch Set 1 #Patch Set 2 : Fix more problems #Patch Set 3 : One more fix #Patch Set 4 : Resync #
Messages
Total messages: 35 (12 generated)
Description was changed from ========== Opacity goes from 0 to 1, not 0 to 255. This also adds DCHECKs for this to the compositor in hopes of catching similar errors sooner in the future. BUG=610039 TEST=Trigger new backspace shortcut bubble; it should fade in and out, not flicker and mostly be white. ========== to ========== Opacity goes from 0 to 1, not 0 to 255. This also adds DCHECKs for this to the compositor in hopes of catching similar errors sooner in the future. BUG=610039 TEST=Trigger new backspace shortcut bubble; it should fade in and out, not flicker and mostly be white. CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
pkasting@chromium.org changed reviewers: + mgiuca@chromium.org, vmpstr@chromium.org
mgiuca: c/b/ui/views vmpstr: cc/layers
lgtm. Please change the CL description prefixing "NewBackShortcutBubble: ..." so it's clear that you're only fixing this one case, not making a global change. You could also note that this was introduced by r395980 and r396138 landing in quick succession and conflicting.
Description was changed from ========== Opacity goes from 0 to 1, not 0 to 255. This also adds DCHECKs for this to the compositor in hopes of catching similar errors sooner in the future. BUG=610039 TEST=Trigger new backspace shortcut bubble; it should fade in and out, not flicker and mostly be white. CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== Widget opacity goes from 0 to 1, not 0 to 255. This change was made in r395980. The NewBackShortcutBubble code landed just after, in r396138, but still used the old values. This also adds DCHECKs for this to the compositor in hopes of catching similar errors sooner in the future. BUG=610039 TEST=Trigger new backspace shortcut bubble; it should fade in and out, not flicker and mostly be white. CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
On 2016/05/27 03:37:04, Matt Giuca wrote: > lgtm. > > Please change the CL description prefixing "NewBackShortcutBubble: ..." so it's > clear that you're only fixing this one case, not making a global change. I updated the description some.
On 2016/05/27 03:46:53, Peter Kasting wrote: > On 2016/05/27 03:37:04, Matt Giuca wrote: > > lgtm. > > > > Please change the CL description prefixing "NewBackShortcutBubble: ..." so > it's > > clear that you're only fixing this one case, not making a global change. > > I updated the description some. Thanks. Why not include NBSB in the subject line? Also nit: Please wrap description to 72 chars.
Description was changed from ========== Widget opacity goes from 0 to 1, not 0 to 255. This change was made in r395980. The NewBackShortcutBubble code landed just after, in r396138, but still used the old values. This also adds DCHECKs for this to the compositor in hopes of catching similar errors sooner in the future. BUG=610039 TEST=Trigger new backspace shortcut bubble; it should fade in and out, not flicker and mostly be white. CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== Widget opacity goes from 0 to 1, not 0 to 255. This change was made in r395980, but missed a lot of callsites. This also adds DCHECKs for this to the compositor in hopes of catching similar errors sooner in the future. These DCHECKs caught most of the places touched by this patch. BUG=610039 TEST=Trigger new backspace shortcut bubble; it should fade in and out, not flicker and mostly be white. CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
pkasting@chromium.org changed reviewers: + sky@chromium.org
+sky for OWNERS of various directories I'm now touching a lot more places (and will continue to add more as the trybots point them out). Reworked the CL description again, to be more general, as a result. I didn't manually wrap to 72 cols. That normally happens automatically AFAIK, except on the initial description line, or something.
OK slgtm.
LGTM - I manually inspected all the sites and it seems you found them.
cc lgtm
The CQ bit was checked by pkasting@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mgiuca@chromium.org Link to the patchset: https://codereview.chromium.org/2016203002/#ps40001 (title: "One more fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2016203002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2016203002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_blink_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/8...) chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by pkasting@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vmpstr@chromium.org, sky@chromium.org, mgiuca@chromium.org Link to the patchset: https://codereview.chromium.org/2016203002/#ps60001 (title: "Resync")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2016203002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2016203002/60001
Message was sent while issue was closed.
Description was changed from ========== Widget opacity goes from 0 to 1, not 0 to 255. This change was made in r395980, but missed a lot of callsites. This also adds DCHECKs for this to the compositor in hopes of catching similar errors sooner in the future. These DCHECKs caught most of the places touched by this patch. BUG=610039 TEST=Trigger new backspace shortcut bubble; it should fade in and out, not flicker and mostly be white. CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== Widget opacity goes from 0 to 1, not 0 to 255. This change was made in r395980, but missed a lot of callsites. This also adds DCHECKs for this to the compositor in hopes of catching similar errors sooner in the future. These DCHECKs caught most of the places touched by this patch. BUG=610039 TEST=Trigger new backspace shortcut bubble; it should fade in and out, not flicker and mostly be white. CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Widget opacity goes from 0 to 1, not 0 to 255. This change was made in r395980, but missed a lot of callsites. This also adds DCHECKs for this to the compositor in hopes of catching similar errors sooner in the future. These DCHECKs caught most of the places touched by this patch. BUG=610039 TEST=Trigger new backspace shortcut bubble; it should fade in and out, not flicker and mostly be white. CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== Widget opacity goes from 0 to 1, not 0 to 255. This change was made in r395980, but missed a lot of callsites. This also adds DCHECKs for this to the compositor in hopes of catching similar errors sooner in the future. These DCHECKs caught most of the places touched by this patch. BUG=610039 TEST=Trigger new backspace shortcut bubble; it should fade in and out, not flicker and mostly be white. CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel Committed: https://crrev.com/d28e06bee3c0807483aed283bafd4c6f72198961 Cr-Commit-Position: refs/heads/master@{#396590} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/d28e06bee3c0807483aed283bafd4c6f72198961 Cr-Commit-Position: refs/heads/master@{#396590}
Message was sent while issue was closed.
On 2016/05/27 06:05:01, Peter Kasting wrote: > I didn't manually wrap to 72 cols. That normally happens automatically AFAIK, > except on the initial description line, or something. FYI, automatic wrapping didn't happen: https://crrev.com/396590
Message was sent while issue was closed.
On Thu, May 26, 2016 at 11:05 PM, <pkasting@chromium.org> wrote: > +sky for OWNERS of various directories > > I'm now touching a lot more places (and will continue to add more as the > trybots > point them out). Reworked the CL description again, to be more general, as > a > result. > > I didn't manually wrap to 72 cols. That normally happens automatically > AFAIK, > except on the initial description line, or something. > I don't think the CL description is changed automatically. -- 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 2016/05/31 20:11:38, danakj wrote: > On Thu, May 26, 2016 at 11:05 PM, <mailto:pkasting@chromium.org> wrote: > > > +sky for OWNERS of various directories > > > > I'm now touching a lot more places (and will continue to add more as the > > trybots > > point them out). Reworked the CL description again, to be more general, as > > a > > result. > > > > I didn't manually wrap to 72 cols. That normally happens automatically > > AFAIK, > > except on the initial description line, or something. > > > > I don't think the CL description is changed automatically. It's not. I was wrong. Most reviewers don't look at this; Matt asked for 72; Erik Chen asked a different developer for 80 on another bug. At the risk of wasting time on an issue that might not deserve it, or being insensitive to people with a different workflow than mine, here's what I wrote on that code review: "I kinda feel like we shouldn't ask people to waste their time doing this manually, as long as there is a reasonable summary in the first line, which is fully visible in all various git tools. "If lines beyond that are too long, I sort of feel like people can read the CLs on the web, or git tools should suck less, or something. In the limit, if this is vitally important, the code review site should be automatically doing it or warning people or something. I don't think it's important enough for that to happen, and thus I don't think it's important enough that we should ask people to do this. It doesn't feel nearly as beneficial as e.g. the 80-column limit in source code." "This is super trivial and I'm only speaking up because another reviewer brought up the exact same issue on a CL of mine a couple days ago."
Message was sent while issue was closed.
On Tue, May 31, 2016 at 1:15 PM, <pkasting@chromium.org> wrote: > On 2016/05/31 20:11:38, danakj wrote: > > On Thu, May 26, 2016 at 11:05 PM, <mailto:pkasting@chromium.org> wrote: > > > > > +sky for OWNERS of various directories > > > > > > I'm now touching a lot more places (and will continue to add more as > the > > > trybots > > > point them out). Reworked the CL description again, to be more > general, as > > > a > > > result. > > > > > > I didn't manually wrap to 72 cols. That normally happens automatically > > > AFAIK, > > > except on the initial description line, or something. > > > > > > > I don't think the CL description is changed automatically. > > It's not. I was wrong. > > Most reviewers don't look at this; Matt asked for 72; Erik Chen asked a > different developer for 80 on another bug. At the risk of wasting time on > an > issue that might not deserve it, or being insensitive to people with a > different > workflow than mine, here's what I wrote on that code review: > > "I kinda feel like we shouldn't ask people to waste their time doing this > manually, as long as there is a reasonable summary in the first line, > which is > fully visible in all various git tools. > > "If lines beyond that are too long, I sort of feel like people can read > the CLs > on the web, or git tools should suck less, or something. In the limit, if > this > is vitally important, the code review site should be automatically doing > it or > warning people or something. I don't think it's important enough for that > to > happen, and thus I don't think it's important enough that we should ask > people > to do this. It doesn't feel nearly as beneficial as e.g. the 80-column > limit in > source code." > > "This is super trivial and I'm only speaking up because another reviewer > brought > up the exact same issue on a CL of mine a couple days ago." > I just always link to http://chris.beams.io/posts/git-commit/ > > https://codereview.chromium.org/2016203002/ > -- 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 Tue, May 31, 2016 at 1:22 PM, Dana Jansens <danakj@chromium.org> wrote: > I just always link to http://chris.beams.io/posts/git-commit/ > Most of his recommendations there I have no issue with. The only one I have an exception with is wrapping the body, and when I click through to the section on it, he basically doesn't justify this with why not wrapping causes a hardship. (By comparison, "wrap code at 80 columns" has, among other arguments, "this allows putting multiple columns of code onscreen side-by-side for comparison", and comparing columns of code is something developers do all the time.) Generally, when I write my commit messages, I try to wrap at 80 characters. But if I edit them in Rietveld, it uses a proportional font and has no column indicator, so wrapping at 80 columns is nontrivial. Since I rarely do this I rarely run into these issues, but I kind of feel like the onus is on people who read commit messages in a terminal or something to at least make wrapping easy in Rietveld if not automatic, if they want to ensure the rest of us don't make their lives annoying. I have been reluctant to say this because even for me as someone happy to debate almost everything this feels like a bikeshed, and as a Windows user who debugs using MSVC I'm keenly aware of the plight of "most people on the team don't use my workflow and so don't have sympathy for me", so I don't want to be a jerk. But it's really hard for me to follow a reviewer's suggestion when I don't think reviewers should be looking for the problem to begin with, and we should fix it in a less manual way if it's worth fixing. I will shut up now. PK -- 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 Tue, May 31, 2016 at 1:49 PM, Peter Kasting <pkasting@chromium.org> wrote: > On Tue, May 31, 2016 at 1:22 PM, Dana Jansens <danakj@chromium.org> wrote: > >> I just always link to http://chris.beams.io/posts/git-commit/ >> > > Most of his recommendations there I have no issue with. The only one I > have an exception with is wrapping the body, and when I click through to > the section on it, he basically doesn't justify this with why not wrapping > causes a hardship. (By comparison, "wrap code at 80 columns" has, among > other arguments, "this allows putting multiple columns of code onscreen > side-by-side for comparison", and comparing columns of code is something > developers do all the time.) > I think it's premised on people have 80column wide windows. And then 72 is better than 80 because you can revert and reland and stay in the same range. > > Generally, when I write my commit messages, I try to wrap at 80 > characters. But if I edit them in Rietveld, it uses a proportional font > and has no column indicator, so wrapping at 80 columns is nontrivial. > Since I rarely do this I rarely run into these issues, but I kind of feel > like the onus is on people who read commit messages in a terminal or > something to at least make wrapping easy in Rietveld if not automatic, if > they want to ensure the rest of us don't make their lives annoying. > Ya, I think everyone who has ever thought about this is sad rietveld isn't more helpful. The most we got is a comment header at the top of the commit log message saying to wrap at 72 columns and pointing out where it is. I just tend to wrap an some arbitrary point when doing so with a non-wrapped message in rietveld and just hope. > > I have been reluctant to say this because even for me as someone happy to > debate almost everything this feels like a bikeshed, and as a Windows user > who debugs using MSVC I'm keenly aware of the plight of "most people on the > team don't use my workflow and so don't have sympathy for me", so I don't > want to be a jerk. But it's really hard for me to follow a reviewer's > suggestion when I don't think reviewers should be looking for the problem > to begin with, and we should fix it in a less manual way if it's worth > fixing. > > I will shut up now. > Normally when I see people didn't wrap stuff, I just do it myself in their CL in rietveld (to some arbitrary limits). > > PK > -- 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.
Hi, sorry to have started this long debate about something which, I agree, is not very important. I do think it's worth doing, though. (72 is not my preference, it's something that I think is generally followed on Chrome repo though, and I'm not exactly sure where it came from.) I don't care if you wrap 72 or 80 or 95 but I think wrapping the body to something around that range is useful for reading in a terminal. If you type "git log" in a terminal on the Chromium master, you'll see that nearly every commit is wrapped to 72-or-so characters and the handful that don't wrap stick out and are generally harder to read. I agree that it would be best if our tools did this for us (maybe better than Reitveld doing it would be if the CQ did it), but in lieu of someone writing that code, I think it's a simple thing for us to do by hand. (I use git cl description which lets me use vim to edit the description and wrap with vim).
Message was sent while issue was closed.
I didn't know about git cl description. I can't guarantee I'll remember it, but if I do I'll try to use it. Also, to correct myself (I hate being wrong): Rietveld doesn't use a proportional font, it uses a fixed-width one, so doing this in-browser is easier than I made out. I think the reason I forget to wrap is because the default "edit issue" textbox is very narrow. Perhaps if this box were, say, 80 chars wide, it would make it easier to remember to wrap and to actually do so. It seems like changing that width ought not to be too hard for the infra folks. Do you guys think this would help? If so I can try to file a bug in wherever the appropriate spot is.
Message was sent while issue was closed.
On 2016/06/01 01:11:20, Peter Kasting wrote: > I didn't know about git cl description. I can't guarantee I'll remember it, but > if I do I'll try to use it. > > Also, to correct myself (I hate being wrong): Rietveld doesn't use a > proportional font, it uses a fixed-width one, so doing this in-browser is easier > than I made out. > > I think the reason I forget to wrap is because the default "edit issue" textbox > is very narrow. Perhaps if this box were, say, 80 chars wide, it would make it > easier to remember to wrap and to actually do so. It seems like changing that > width ought not to be too hard for the infra folks. Do you guys think this > would help? If so I can try to file a bug in wherever the appropriate spot is. Yeah that would help.
Message was sent while issue was closed.
On 2016/06/01 01:14:32, Matt Giuca wrote: > On 2016/06/01 01:11:20, Peter Kasting wrote: > > I didn't know about git cl description. I can't guarantee I'll remember it, > but > > if I do I'll try to use it. > > > > Also, to correct myself (I hate being wrong): Rietveld doesn't use a > > proportional font, it uses a fixed-width one, so doing this in-browser is > easier > > than I made out. > > > > I think the reason I forget to wrap is because the default "edit issue" > textbox > > is very narrow. Perhaps if this box were, say, 80 chars wide, it would make > it > > easier to remember to wrap and to actually do so. It seems like changing that > > width ought not to be too hard for the infra folks. Do you guys think this > > would help? If so I can try to file a bug in wherever the appropriate spot > is. > > Yeah that would help. Cool, filed https://bugs.chromium.org/p/chromium/issues/detail?id=616325 . |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
