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

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 2110653003: CUPS: MD Settings printers subpage (Closed)

Created:
4 years, 5 months ago by xdai1
Modified:
4 years, 5 months ago
Reviewers:
stevenjb, michaelpg
CC:
chromium-reviews, michaelpg+watch-md-settings_chromium.org, jlklein+watch-closure_chromium.org, michaelpg+watch-md-ui_chromium.org, arv+watch_chromium.org, vitalyp+closure_chromium.org, dbeam+watch-settings_chromium.org, stevenjb+watch-md-settings_chromium.org, dbeam+watch-closure_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

CUPS: MD Settings printers subpage - Add a placeholder for CUPS printers subpage - Move the google cloud printing page to the Cloud Printers subpage. See the mock: https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20Inner%20Pages/03-Settings/preview/flow#%2F101_Printing.png%3Fz=width BUG=626752, 625364 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/0f8070caad35c77f19748ff92ec0aa6670509bef Cr-Commit-Position: refs/heads/master@{#405604}

Patch Set 1 #

Patch Set 2 : fix format. #

Patch Set 3 : Use customized google cloud printing icon. #

Patch Set 4 : git cl format #

Patch Set 5 : Fix the failed trybot #

Patch Set 6 : Only enable the CUPS printers option in ChromeOS. #

Total comments: 6

Patch Set 7 : Address stevenjb@'s comments. #

Total comments: 11

Patch Set 8 : Address michaelpg@'s comments. #

Total comments: 5

Patch Set 9 : Format the Google cloud printing icon. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+143 lines, -33 lines) Patch
M chrome/app/settings_strings.grdp View 1 2 3 4 5 6 7 1 chunk +8 lines, -0 lines 0 comments Download
M chrome/browser/resources/settings/advanced_page/advanced_page.html View 1 chunk +3 lines, -1 line 0 comments Download
M chrome/browser/resources/settings/icons.html View 1 2 3 4 5 6 7 8 1 chunk +5 lines, -0 lines 0 comments Download
A + chrome/browser/resources/settings/printing_page/cloud_printers.html View 2 chunks +3 lines, -3 lines 0 comments Download
A + chrome/browser/resources/settings/printing_page/cloud_printers.js View 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/resources/settings/printing_page/compiled_resources2.gyp View 1 2 3 4 5 6 7 1 chunk +11 lines, -0 lines 0 comments Download
A chrome/browser/resources/settings/printing_page/cups_printers.html View 1 2 1 chunk +13 lines, -0 lines 0 comments Download
A + chrome/browser/resources/settings/printing_page/cups_printers.js View 1 2 3 4 5 6 7 1 chunk +3 lines, -1 line 0 comments Download
M chrome/browser/resources/settings/printing_page/printing_page.html View 1 2 3 4 5 1 chunk +39 lines, -19 lines 0 comments Download
M chrome/browser/resources/settings/printing_page/printing_page.js View 1 2 3 4 5 6 7 1 chunk +16 lines, -2 lines 0 comments Download
M chrome/browser/resources/settings/settings_page/settings_router.js View 1 2 3 4 5 6 7 1 chunk +14 lines, -0 lines 0 comments Download
M chrome/browser/resources/settings/settings_resources.grd View 1 2 3 4 5 6 7 1 chunk +17 lines, -0 lines 0 comments Download
M chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc View 1 2 3 4 5 6 7 1 chunk +10 lines, -6 lines 0 comments Download

Messages

Total messages: 37 (15 generated)
xdai1
Hey Steven, could you help take a look at this CL please? I'm not familiar ...
4 years, 5 months ago (2016-07-11 18:32:28 UTC) #7
stevenjb
On 2016/07/11 18:32:28, xdai1 wrote: > Hey Steven, could you help take a look at ...
4 years, 5 months ago (2016-07-11 18:41:07 UTC) #8
stevenjb
On 2016/07/11 18:41:07, stevenjb wrote: > On 2016/07/11 18:32:28, xdai1 wrote: > > Hey Steven, ...
4 years, 5 months ago (2016-07-11 18:41:36 UTC) #9
xdai1
On 2016/07/11 18:41:07, stevenjb wrote: > On 2016/07/11 18:32:28, xdai1 wrote: > > Hey Steven, ...
4 years, 5 months ago (2016-07-11 21:12:30 UTC) #10
michaelpg
I've reworded the title, because at first I thought this was about *printing* the MD ...
4 years, 5 months ago (2016-07-11 23:53:16 UTC) #12
xdai1
On 2016/07/11 23:53:16, michaelpg wrote: > I've reworded the title, because at first I thought ...
4 years, 5 months ago (2016-07-12 00:19:13 UTC) #13
xdai1
stevenjb@, kindly ping?
4 years, 5 months ago (2016-07-13 16:13:05 UTC) #15
stevenjb
This lgtm. michaelpg@ - can you take a quick look at the routing at least, ...
4 years, 5 months ago (2016-07-13 17:34:38 UTC) #17
xdai1
stevenjb@, I've addressed your comments. Thanks for your review. https://codereview.chromium.org/2110653003/diff/160001/chrome/browser/resources/settings/icons.html File chrome/browser/resources/settings/icons.html (right): https://codereview.chromium.org/2110653003/diff/160001/chrome/browser/resources/settings/icons.html#newcode21 chrome/browser/resources/settings/icons.html:21: ...
4 years, 5 months ago (2016-07-13 17:45:44 UTC) #18
michaelpg
https://codereview.chromium.org/2110653003/diff/180001/chrome/browser/resources/settings/icons.html File chrome/browser/resources/settings/icons.html (right): https://codereview.chromium.org/2110653003/diff/180001/chrome/browser/resources/settings/icons.html#newcode22 chrome/browser/resources/settings/icons.html:22: <!-- See https://drive.google.com/corp/drive/u/0/folders/0B_2Uyb2Rhx2OM1MzUmY0TEtzak0 for more info. --> Probably shouldn't ...
4 years, 5 months ago (2016-07-13 21:14:50 UTC) #19
xdai1
michaelpg@, I've addressed your comments. Please take another look, thanks! https://codereview.chromium.org/2110653003/diff/180001/chrome/browser/resources/settings/icons.html File chrome/browser/resources/settings/icons.html (right): https://codereview.chromium.org/2110653003/diff/180001/chrome/browser/resources/settings/icons.html#newcode22 ...
4 years, 5 months ago (2016-07-14 17:48:33 UTC) #20
michaelpg
lgtm w/ comments on icon https://codereview.chromium.org/2110653003/diff/200001/chrome/browser/resources/settings/icons.html File chrome/browser/resources/settings/icons.html (right): https://codereview.chromium.org/2110653003/diff/200001/chrome/browser/resources/settings/icons.html#newcode22 chrome/browser/resources/settings/icons.html:22: <g id="cloud-printing" transform="scale(0.6)"> remove ...
4 years, 5 months ago (2016-07-14 17:57:40 UTC) #21
xdai1
michealpg@, thanks for your review! I have a question and hope you can help with ...
4 years, 5 months ago (2016-07-14 18:23:49 UTC) #22
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/2110653003/220001
4 years, 5 months ago (2016-07-14 18:31:04 UTC) #25
commit-bot: I haz the power
Your CL relies on deprecated CQ feature(s): * Specifying master names in CQ_INCLUDE_TRYBOTS part of ...
4 years, 5 months ago (2016-07-14 18:31:06 UTC) #26
commit-bot: I haz the power
Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_ng/builds/260243)
4 years, 5 months ago (2016-07-14 19:48:40 UTC) #28
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/2110653003/220001
4 years, 5 months ago (2016-07-14 21:14:27 UTC) #30
commit-bot: I haz the power
Your CL relies on deprecated CQ feature(s): * Specifying master names in CQ_INCLUDE_TRYBOTS part of ...
4 years, 5 months ago (2016-07-14 21:14:29 UTC) #31
commit-bot: I haz the power
Committed patchset #9 (id:220001)
4 years, 5 months ago (2016-07-14 22:24:55 UTC) #33
commit-bot: I haz the power
CQ bit was unchecked.
4 years, 5 months ago (2016-07-14 22:25:37 UTC) #34
commit-bot: I haz the power
Patchset 9 (id:??) landed as https://crrev.com/0f8070caad35c77f19748ff92ec0aa6670509bef Cr-Commit-Position: refs/heads/master@{#405604}
4 years, 5 months ago (2016-07-14 22:26:52 UTC) #36
michaelpg
4 years, 5 months ago (2016-07-14 23:35:36 UTC) #37
Message was sent while issue was closed. 
https://codereview.chromium.org/2110653003/diff/200001/chrome/browser/resourc...
File chrome/browser/resources/settings/icons.html (right):

https://codereview.chromium.org/2110653003/diff/200001/chrome/browser/resourc...
chrome/browser/resources/settings/icons.html:23: <path d="M28
34H12V24h16v10zm4.25-21.934C31.116 6.316 26.066 2 20 2c-4.816 0-9 2.734-11.084
6.734C3.9 9.266 0 13.516 0 18.666c0 4.832 3.44 8.87 8
9.798V38h24v-9.368c4.442-.178 8-3.812 8-8.298 0-4.4-3.416-7.968-7.75-8.268z"
fill="#000" fill-rule="evenodd"></path>
On 2016/07/14 18:23:49, xdai1 wrote:
> On 2016/07/14 17:57:40, michaelpg (slow) wrote:
> > <path d="M16.8 20.4H7.2v-6h9.6v6zm2.55-13.16C18.67 3.79 15.64 1.2 12
1.2c-2.89
> > 0-5.4 1.64-6.65 4.04C2.34 5.56 0 8.11 0 11.2c0 2.9 2.064 5.322 4.8
> > 5.878V22.8h14.4v-5.62c2.665-.108 4.8-2.288 4.8-4.98
> 0-2.64-2.05-4.78-4.65-4.96z"
> > fill-rule="evenodd"/>
> 
> Thanks! How to get this cleaner icon path? I followed the instruction in your
> previous comment: paste the SVG into an SVG cleanup tool SVGOMG, and that's
what
> I got. What surprised me is originally the icon color is black but with your
> modification suggestion, the icon now has the same color as with the other
icons
> (e.g., print icon, in dark gray). 

Looks like SVGOMG has different results depending on whether the <g
transform="scale"> goes. I investigated further and found that iron-iconset-svg
actually respects viewBox, so we don't have to scale ourselves:

    <g id="cloud-printing" viewBox="0 0 40 40">...</g>

would have worked instead of using the transform.

I've simplified the instructions, mentioning that using the original viewBox
value works too: 
https://sites.google.com/a/chromium.org/dev/developers/updating-webui-for-mat...

I don't know when SVGOMG removes colors, but we definitely want to remove them
so they can be styled like the rest of our icons, like you noticed. Otherwise
CSS doesn't work, apparently.

Thanks for pointing out both issues!

Powered by Google App Engine
This is Rietveld 408576698