Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2807373003:
MD Settings: get the avatar icon the same way the user switcher does (Closed)
|
Created:
3 years, 8 months ago by Dan Beam Modified:
3 years, 8 months ago Reviewers:
dschuyler CC:
chromium-reviews, michaelpg+watch-md-settings_chromium.org, dbeam+watch-settings_chromium.org, stevenjb+watch-md-settings_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionMD Settings: get the avatar icon the same way the user switcher does
R=dschuyler@chromium.org
BUG=708863
Review-Url: https://codereview.chromium.org/2807373003
Cr-Commit-Position: refs/heads/master@{#463861}
Committed: https://chromium.googlesource.com/chromium/src/+/42652efd4bbbaffa83de98b4bcf57e270d0e8ccb
Patch Set 1 #
Total comments: 2
Patch Set 2 : update todo #Patch Set 3 : fix tests #
Total comments: 2
Patch Set 4 : dschuyler@ review #
Messages
Total messages: 22 (12 generated)
p1 -> p3
The CQ bit was checked by dbeam@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...
On 2017/04/11 22:54:07, Dan Beam wrote: > p1 -> p3 How about a note in the description like: This CL trades a p1 bug for a p3 bug at crbug://url/to/bug
https://codereview.chromium.org/2807373003/diff/1/chrome/browser/ui/webui/set... File chrome/browser/ui/webui/settings/profile_info_handler.cc (right): https://codereview.chromium.org/2807373003/diff/1/chrome/browser/ui/webui/set... chrome/browser/ui/webui/settings/profile_info_handler.cc:210: // theme_source.cc. Could we say something about why, like: to avoid using a heavy data url here.
lgtm
https://codereview.chromium.org/2807373003/diff/1/chrome/browser/ui/webui/set... File chrome/browser/ui/webui/settings/profile_info_handler.cc (right): https://codereview.chromium.org/2807373003/diff/1/chrome/browser/ui/webui/set... chrome/browser/ui/webui/settings/profile_info_handler.cc:210: // theme_source.cc. On 2017/04/11 22:58:54, dschuyler wrote: > Could we say something about why, like: to avoid using a heavy data url here. Done.
The CQ bit was checked by dbeam@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/2807373003/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/profile_info_handler_unittest.cc (right): https://codereview.chromium.org/2807373003/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/profile_info_handler_unittest.cc:84: EXPECT_TRUE(net::DataURL::Parse(GURL(icon_url), &mime, &charset, &data)); How about something like the tests done here that check the mimetype and the png data (i.e. doing something similar here) https://cs.chromium.org/chromium/src/content/browser/accessibility/accessibil...
https://codereview.chromium.org/2807373003/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/profile_info_handler_unittest.cc (right): https://codereview.chromium.org/2807373003/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/profile_info_handler_unittest.cc:84: EXPECT_TRUE(net::DataURL::Parse(GURL(icon_url), &mime, &charset, &data)); On 2017/04/11 23:36:48, dschuyler wrote: > How about something like the tests done here that check the mimetype and the png > data (i.e. doing something similar here) > https://cs.chromium.org/chromium/src/content/browser/accessibility/accessibil... Done.
The CQ bit was checked by dbeam@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...
slgtm
The CQ bit was unchecked by dbeam@chromium.org
The CQ bit was checked by dbeam@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dschuyler@chromium.org Link to the patchset: https://codereview.chromium.org/2807373003/#ps60001 (title: "dschuyler@ review")
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": 1491956030560590,
"parent_rev": "c4439066f7e35f0d81b3ce82d06ac9b8facb0d78", "commit_rev":
"42652efd4bbbaffa83de98b4bcf57e270d0e8ccb"}
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1491956030560590,
"parent_rev": "c4439066f7e35f0d81b3ce82d06ac9b8facb0d78", "commit_rev":
"42652efd4bbbaffa83de98b4bcf57e270d0e8ccb"}
Message was sent while issue was closed.
Description was changed from ========== MD Settings: get the avatar icon the same way the user switcher does R=dschuyler@chromium.org BUG=708863 ========== to ========== MD Settings: get the avatar icon the same way the user switcher does R=dschuyler@chromium.org BUG=708863 Review-Url: https://codereview.chromium.org/2807373003 Cr-Commit-Position: refs/heads/master@{#463861} Committed: https://chromium.googlesource.com/chromium/src/+/42652efd4bbbaffa83de98b4bcf5... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/42652efd4bbbaffa83de98b4bcf5... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
