Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(82)

Issue 2947913003: Updated Ads icon for content settings. (Closed)

Created:
3 years, 6 months ago by shivanisha
Modified:
3 years, 5 months ago
Reviewers:
Lei Zhang, dschuyler, dpapad
CC:
arv+watch_chromium.org, bettes, chromium-reviews, Charlie Harrison, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, stevenjb+watch-md-settings_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Updated Ads icon for content settings. Screenshot attached in comment#56 in the associated bug. BUG=689992 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2947913003 Cr-Commit-Position: refs/heads/master@{#482873} Committed: https://chromium.googlesource.com/chromium/src/+/6e26093ba629b7aa843f7284c8751d1f752f4b1e

Patch Set 1 #

Total comments: 2

Patch Set 2 : Removed id and fill attributes #

Patch Set 3 : Rebased with refs/heads/master@{#481935} #

Patch Set 4 : Updated icon as received from bettes@ #

Total comments: 4

Patch Set 5 : Updated icon definition with <rect> #

Total comments: 2

Patch Set 6 : Indentation #

Unified diffs Side-by-side diffs Delta from patch set Stats (+8 lines, -1 line) Patch
M chrome/browser/resources/settings/icons.html View 1 2 3 4 5 1 chunk +7 lines, -0 lines 0 comments Download
M chrome/browser/resources/settings/site_settings_page/site_settings_page.html View 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 57 (28 generated)
shivanisha
thestig@, PTAL, Thanks!
3 years, 6 months ago (2017-06-20 18:54:22 UTC) #6
Lei Zhang
Deferring to someone from chrome/browser/resources/settings/OWNERS.
3 years, 6 months ago (2017-06-20 21:05:56 UTC) #10
dschuyler
https://codereview.chromium.org/2947913003/diff/1/chrome/browser/resources/settings/icons.html File chrome/browser/resources/settings/icons.html (right): https://codereview.chromium.org/2947913003/diff/1/chrome/browser/resources/settings/icons.html#newcode13 chrome/browser/resources/settings/icons.html:13: <path d="M15,3.5 L15,12.5 C15,13.325 14.3077778,14 13.4444444,14 L2.55555556,14 C1.69222222,14 1,13.325 ...
3 years, 6 months ago (2017-06-20 21:15:51 UTC) #11
shivanisha
Feedback addressed, PTAL, Thanks! https://codereview.chromium.org/2947913003/diff/1/chrome/browser/resources/settings/icons.html File chrome/browser/resources/settings/icons.html (right): https://codereview.chromium.org/2947913003/diff/1/chrome/browser/resources/settings/icons.html#newcode13 chrome/browser/resources/settings/icons.html:13: <path d="M15,3.5 L15,12.5 C15,13.325 14.3077778,14 ...
3 years, 6 months ago (2017-06-21 07:11:56 UTC) #14
shivanisha
On 2017/06/21 at 07:11:56, shivanisha wrote: > Feedback addressed, PTAL, Thanks! > > https://codereview.chromium.org/2947913003/diff/1/chrome/browser/resources/settings/icons.html > ...
3 years, 6 months ago (2017-06-22 15:55:32 UTC) #17
shivanisha
On 2017/06/22 at 15:55:32, shivanisha wrote: > On 2017/06/21 at 07:11:56, shivanisha wrote: > > ...
3 years, 6 months ago (2017-06-23 01:06:50 UTC) #18
shivanisha
On 2017/06/23 at 01:06:50, shivanisha wrote: > On 2017/06/22 at 15:55:32, shivanisha wrote: > > ...
3 years, 6 months ago (2017-06-23 18:25:40 UTC) #19
Lei Zhang
+dpapad to either review or help find the right reviewer.
3 years, 6 months ago (2017-06-23 18:40:42 UTC) #21
dpapad
On 2017/06/23 at 18:40:42, thestig wrote: > +dpapad to either review or help find the ...
3 years, 6 months ago (2017-06-23 18:58:35 UTC) #22
Lei Zhang
On 2017/06/23 18:58:35, dpapad wrote: > On 2017/06/23 at 18:40:42, thestig wrote: > > +dpapad ...
3 years, 6 months ago (2017-06-23 19:08:03 UTC) #23
shivanisha
On 2017/06/23 at 19:08:03, thestig wrote: > On 2017/06/23 18:58:35, dpapad wrote: > > On ...
3 years, 6 months ago (2017-06-23 19:11:31 UTC) #24
shivanisha
On 2017/06/23 at 19:11:31, shivanisha wrote: > On 2017/06/23 at 19:08:03, thestig wrote: > > ...
3 years, 6 months ago (2017-06-23 19:13:06 UTC) #25
dpapad
On 2017/06/23 at 19:08:03, thestig wrote: > On 2017/06/23 18:58:35, dpapad wrote: > > On ...
3 years, 6 months ago (2017-06-23 19:24:53 UTC) #26
shivanisha
On 2017/06/23 at 19:24:53, dpapad wrote: > On 2017/06/23 at 19:08:03, thestig wrote: > > ...
3 years, 6 months ago (2017-06-23 19:50:33 UTC) #27
dpapad
On 2017/06/23 at 19:50:33, shivanisha wrote: > On 2017/06/23 at 19:24:53, dpapad wrote: > > ...
3 years, 6 months ago (2017-06-23 20:28:56 UTC) #28
shivanisha
On 2017/06/23 at 20:28:56, dpapad wrote: > On 2017/06/23 at 19:50:33, shivanisha wrote: > > ...
3 years, 5 months ago (2017-06-26 15:20:35 UTC) #29
shivanisha
dpapad@, PTAL , Thanks! Added the icon as received from bettes@ Screenshots attached in comment#55 ...
3 years, 5 months ago (2017-06-27 19:17:43 UTC) #32
shivanisha
On 2017/06/27 at 19:17:43, shivanisha wrote: > dpapad@, PTAL , Thanks! > Added the icon ...
3 years, 5 months ago (2017-06-27 19:59:34 UTC) #33
dpapad
On 2017/06/27 at 19:59:34, shivanisha wrote: > On 2017/06/27 at 19:17:43, shivanisha wrote: > > ...
3 years, 5 months ago (2017-06-27 20:25:10 UTC) #34
shivanisha
On 2017/06/27 at 20:25:10, dpapad wrote: > On 2017/06/27 at 19:59:34, shivanisha wrote: > > ...
3 years, 5 months ago (2017-06-27 20:32:17 UTC) #35
dschuyler
https://codereview.chromium.org/2947913003/diff/60001/chrome/browser/resources/settings/icons.html File chrome/browser/resources/settings/icons.html (right): https://codereview.chromium.org/2947913003/diff/60001/chrome/browser/resources/settings/icons.html#newcode13 chrome/browser/resources/settings/icons.html:13: <path d="M19,3H5C3.9,3,3,3.9,3,5v14c0,1.1,0.9,2,2,2h14c1.1,0,2-0.9,2-2V5C21,3.9,20.1,3,19,3z M11,15H9.5v-1.5h-2V15H6v-4.5V9h1.5h2H10h1V15z M18,14c0,0.5-0.5,1-1,1h-4V9h4c0.5,0,1,0.5,1,1V14z"</path> Please end the <path ...
3 years, 5 months ago (2017-06-27 20:48:17 UTC) #38
dschuyler
https://codereview.chromium.org/2947913003/diff/60001/chrome/browser/resources/settings/icons.html File chrome/browser/resources/settings/icons.html (right): https://codereview.chromium.org/2947913003/diff/60001/chrome/browser/resources/settings/icons.html#newcode12 chrome/browser/resources/settings/icons.html:12: <g id="ads"> It looks like the middles of the ...
3 years, 5 months ago (2017-06-27 20:57:40 UTC) #39
dschuyler
On 2017/06/27 20:25:10, dpapad wrote: > On 2017/06/27 at 19:59:34, shivanisha wrote: > > On ...
3 years, 5 months ago (2017-06-27 21:16:48 UTC) #40
shivanisha
dschuyler@, As discussed on email, PTAL, Thanks! comment#56 in the bug has the updated screenshots. ...
3 years, 5 months ago (2017-06-27 23:46:28 UTC) #43
dpapad
Deferring to @dschuyler who has more experienced with SVG icons in Settings.
3 years, 5 months ago (2017-06-28 00:09:27 UTC) #45
dschuyler
Code in CL LGTM. Opinion on the icon: the icon itself is not great. A ...
3 years, 5 months ago (2017-06-28 00:11:02 UTC) #47
shivanisha
dschuyler@, Thanks! https://codereview.chromium.org/2947913003/diff/80001/chrome/browser/resources/settings/icons.html File chrome/browser/resources/settings/icons.html (right): https://codereview.chromium.org/2947913003/diff/80001/chrome/browser/resources/settings/icons.html#newcode15 chrome/browser/resources/settings/icons.html:15: <rect x="14.5" y="10.5" width="2" height="3"></rect> On 2017/06/28 ...
3 years, 5 months ago (2017-06-28 01:55:45 UTC) #51
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2947913003/100001
3 years, 5 months ago (2017-06-28 01:56:09 UTC) #54
commit-bot: I haz the power
3 years, 5 months ago (2017-06-28 03:41:03 UTC) #57
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/6e26093ba629b7aa843f7284c875...

Powered by Google App Engine
This is Rietveld 408576698