Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2769493002:
[MD settings] Scale avatar icon. (Closed)
|
Created:
3 years, 9 months ago by dschuyler Modified:
3 years, 9 months ago CC:
chromium-reviews, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, dbeam+watch-settings_chromium.org, stevenjb+watch-md-settings_chromium.org, arv+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionThis CL description became obsolete. The actual change scales the icon in c++ before passing it down. The border radius is still used. I'm leaving the description below intact since that is what will appear in the git log.
[MD settings] border radius to clip path on avatar icon
This CL changes the way the avatar icon is clipped so that it doesn't
appear to be cut off on the right edge.
BUG=625773
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2769493002
Cr-Commit-Position: refs/heads/master@{#458845}
Committed: https://chromium.googlesource.com/chromium/src/+/d5d274510522f75f93dfb4c79de24e1a6d301203
Patch Set 1 #
Total comments: 2
Patch Set 2 : aternate #
Total comments: 2
Patch Set 3 : merge with master #Messages
Total messages: 34 (23 generated)
Description was changed from ========== [MD settings] border radius to clip path on avatar icon This CL changes the way the avatar icon is clipped so that it doesn't appear to be cut off on the right edge. BUG=625773 ========== to ========== [MD settings] border radius to clip path on avatar icon This CL changes the way the avatar icon is clipped so that it doesn't appear to be cut off on the right edge. BUG=625773 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by dschuyler@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...
dschuyler@chromium.org changed reviewers: + tommycli@chromium.org
The clip radius is slightly smaller than half the image size so that there is room for anti-aliasing (it looks nicer than making it half the image size).
https://codereview.chromium.org/2769493002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/people_page/people_page.html (right): https://codereview.chromium.org/2769493002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/people_page/people_page.html:48: clip-path: circle(19px at center); Looks like the root cause is that the data URL image is 38x31, and not quite centered horizontally. Since you actually need a square, I recommend changing the call to GetAvatarIconForWebUI within ProfileInfoHandler to GetSizedAvatarIcon, and just specify the 40x40 image you actually want. I think that will fix this problem, as well as the image blurriness. In fact -- I'm not entirely sure why we even do the image scaling within C++... it seems entirely necessary. -- but that's a sidebar.
On 2017/03/21 22:29:53, tommycli wrote: > https://codereview.chromium.org/2769493002/diff/1/chrome/browser/resources/se... > File chrome/browser/resources/settings/people_page/people_page.html (right): > > https://codereview.chromium.org/2769493002/diff/1/chrome/browser/resources/se... > chrome/browser/resources/settings/people_page/people_page.html:48: clip-path: > circle(19px at center); > Looks like the root cause is that the data URL image is 38x31, and not quite > centered horizontally. > > Since you actually need a square, I recommend changing the call to > GetAvatarIconForWebUI within ProfileInfoHandler to GetSizedAvatarIcon, and just > specify the 40x40 image you actually want. > > I think that will fix this problem, as well as the image blurriness. > > In fact -- I'm not entirely sure why we even do the image scaling within C++... > it seems entirely necessary. -- but that's a sidebar. *seems entirely unnecessary
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 dschuyler@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 checked by dschuyler@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...
Patchset #2 (id:20001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
https://codereview.chromium.org/2769493002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/people_page/people_page.html (right): https://codereview.chromium.org/2769493002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/people_page/people_page.html:48: clip-path: circle(19px at center); On 2017/03/21 22:29:53, tommycli wrote: > Looks like the root cause is that the data URL image is 38x31, and not quite > centered horizontally. That's true, it's documented in the bug (in comment #9). > Since you actually need a square, I recommend changing the call to > GetAvatarIconForWebUI within ProfileInfoHandler to GetSizedAvatarIcon, and just > specify the 40x40 image you actually want. That appears to work too. Doing the scaling c++ is ok with me if you prefer. > I think that will fix this problem, as well as the image blurriness. It doesn't actually. It's about the same as patch #1. > In fact -- I'm not entirely sure why we even do the image scaling within C++... With this new change I think we are now scaling the image -- i.e. I'm not sure if I follow. We can chat about it off-line if it's not critical to this CL. done.
lgtm, and i suspect you will need to update a test. https://codereview.chromium.org/2769493002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/profile_info_handler.cc (right): https://codereview.chromium.org/2769493002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/profile_info_handler.cc:41: const int kAvatarIconSize = 40; For this particular variable it may need to be a static member of ProfileInfoHandler to be accessible in tests. And if not, should be in the anonymous namespace.
The CQ bit was checked by dschuyler@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: ios-simulator on master.tryserver.chromium.mac (JOB_TIMED_OUT, build hasn't started yet, builder probably lacks capacity) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_TIMED_OUT, build hasn't started yet, builder probably lacks capacity)
https://codereview.chromium.org/2769493002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/profile_info_handler.cc (right): https://codereview.chromium.org/2769493002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/profile_info_handler.cc:41: const int kAvatarIconSize = 40; On 2017/03/22 00:00:22, tommycli wrote: > For this particular variable it may need to be a static member of > ProfileInfoHandler to be accessible in tests. And if not, should be in the > anonymous namespace. I've instead moved it near the only place it is used and made it a constexpr.
The CQ bit was checked by dschuyler@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tommycli@chromium.org Link to the patchset: https://codereview.chromium.org/2769493002/#ps60001 (title: "merge with master")
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": 1490208912787130,
"parent_rev": "959aa7f6f37c53cd282c7e4a9495a648f43847e2", "commit_rev":
"d5d274510522f75f93dfb4c79de24e1a6d301203"}
Message was sent while issue was closed.
Description was changed from ========== [MD settings] border radius to clip path on avatar icon This CL changes the way the avatar icon is clipped so that it doesn't appear to be cut off on the right edge. BUG=625773 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [MD settings] border radius to clip path on avatar icon This CL changes the way the avatar icon is clipped so that it doesn't appear to be cut off on the right edge. BUG=625773 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2769493002 Cr-Commit-Position: refs/heads/master@{#458845} Committed: https://chromium.googlesource.com/chromium/src/+/d5d274510522f75f93dfb4c79de2... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001) as https://chromium.googlesource.com/chromium/src/+/d5d274510522f75f93dfb4c79de2...
Message was sent while issue was closed.
dbeam@chromium.org changed reviewers: + dbeam@chromium.org
Message was sent while issue was closed.
next time can you update the CL description as a review progresses?
Message was sent while issue was closed.
Description was changed from ========== [MD settings] border radius to clip path on avatar icon This CL changes the way the avatar icon is clipped so that it doesn't appear to be cut off on the right edge. BUG=625773 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2769493002 Cr-Commit-Position: refs/heads/master@{#458845} Committed: https://chromium.googlesource.com/chromium/src/+/d5d274510522f75f93dfb4c79de2... ========== to ========== This CL description became obsolete. The actual change scales the icon in c++ before passing it down. The border radius is still used. I'm leaving the description below intact since that is what will appear in the git log. [MD settings] border radius to clip path on avatar icon This CL changes the way the avatar icon is clipped so that it doesn't appear to be cut off on the right edge. BUG=625773 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2769493002 Cr-Commit-Position: refs/heads/master@{#458845} Committed: https://chromium.googlesource.com/chromium/src/+/d5d274510522f75f93dfb4c79de2... ==========
Message was sent while issue was closed.
On 2017/03/23 00:47:54, Dan Beam wrote: > next time can you update the CL description as a review progresses? I made a note about the description being out of date. Iiuc the horse is already out of the barn on this, since git will already have the old description(?).
Message was sent while issue was closed.
On 2017/03/23 00:57:32, dschuyler wrote: > On 2017/03/23 00:47:54, Dan Beam wrote: > > next time can you update the CL description as a review progresses? > > I made a note about the description being out of date. Iiuc the horse is already > out of the barn on this, since git will already have the old description(?). yeah, just for future horses in barns... |
