Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2550243002:
Color the VR omnibox security icons. (Closed)
DescriptionColor the VR omnibox security icons.
BUG=641508
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/e16ba97bc9e52eb3b58d26ca6d0bb216e7150812
Cr-Commit-Position: refs/heads/master@{#441154}
Patch Set 1 #
Total comments: 13
Patch Set 2 : Address some comments. #
Total comments: 1
Patch Set 3 : Return UI-specific image resources to the UI resource directory. #
Total comments: 9
Patch Set 4 : Rebase, and simplify the icon coloring CL wih dbeam@'s mask suggestion. #Patch Set 5 : Apply the correct red, green and grey colors. #Patch Set 6 : Arg. Fix style. #
Total comments: 1
Messages
Total messages: 36 (9 generated)
Description was changed from ========== Rework VrShell security warnings (and other improvements). - Move VrShell images to the same location as other webui resources. - Rename images for consistency. - Properly choose a security status icon based on security level. - Fix the omnibox icon size to not shrink with long URLs. - Simplify resource paths (localhost development no longer needs a hacked-up directory structure). BUG=641508 ========== to ========== Rework VrShell security warnings (and other improvements). - Move VrShell images to the same location as other webui resources. - Rename images for consistency. - Properly choose a security status icon based on security level. - Fix the omnibox icon size to not shrink with long URLs. - Simplify resource paths (localhost development no longer needs a hacked-up directory structure). BUG=641508 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
cjgrant@chromium.org changed reviewers: + bshe@chromium.org, mthiesse@chromium.org
https://codereview.chromium.org/2550243002/diff/1/chrome/browser/browser_reso... File chrome/browser/browser_resources.grd (left): https://codereview.chromium.org/2550243002/diff/1/chrome/browser/browser_reso... chrome/browser/browser_resources.grd:222: <include name="IDR_VR_SHELL_UI_HTML" file="resources\vr_shell\vr_shell_ui.html" allowexternalscript="true" flattenhtml="true" type="BINDATA" /> Note that alphabetization is happening here as well as additions. https://codereview.chromium.org/2550243002/diff/1/chrome/browser/resources/vr... File chrome/browser/resources/vr_shell/vr_shell_ui.css (right): https://codereview.chromium.org/2550243002/diff/1/chrome/browser/resources/vr... chrome/browser/resources/vr_shell/vr_shell_ui.css:103: background-image: url(images/back.svg); The original line was resolved through html-flattening, which is now disabled. This lets us manipulate images through Javascript, as well as avoiding possible size duplication (where we need the image file, and it gets embedded in the HTML as well). https://codereview.chromium.org/2550243002/diff/1/chrome/browser/resources/vr... File chrome/browser/resources/vr_shell/vr_shell_ui.js (right): https://codereview.chromium.org/2550243002/diff/1/chrome/browser/resources/vr... chrome/browser/resources/vr_shell/vr_shell_ui.js:364: // TODO(cjgrant): Clank appears to not handle this case. I'm investigating this, and will list a bug if warranted.
lgtm https://codereview.chromium.org/2550243002/diff/1/chrome/browser/browser_reso... File chrome/browser/browser_resources.grd (right): https://codereview.chromium.org/2550243002/diff/1/chrome/browser/browser_reso... chrome/browser/browser_resources.grd:225: <include name="IDR_VR_SHELL_UI_IMAGE_BACK" file="resources/vr_shell\images\back.svg" type="BINDATA" /> s/\//\\ https://codereview.chromium.org/2550243002/diff/1/chrome/browser/resources/vr... File chrome/browser/resources/vr_shell/vr_shell_ui.js (right): https://codereview.chromium.org/2550243002/diff/1/chrome/browser/resources/vr... chrome/browser/resources/vr_shell/vr_shell_ui.js:366: case 6: // DANGEROUS: no dangerous icon? Saving that for later?
https://codereview.chromium.org/2550243002/diff/1/chrome/browser/browser_reso... File chrome/browser/browser_resources.grd (right): https://codereview.chromium.org/2550243002/diff/1/chrome/browser/browser_reso... chrome/browser/browser_resources.grd:225: <include name="IDR_VR_SHELL_UI_IMAGE_BACK" file="resources/vr_shell\images\back.svg" type="BINDATA" /> I think you are likely to get push back from owner of this file. This is only intended for html/css/js files I believe. See: https://codereview.chromium.org/2363553003 https://codereview.chromium.org/2550243002/diff/1/chrome/browser/resources/vr... File chrome/browser/resources/vr_shell/vr_shell_ui.css (right): https://codereview.chromium.org/2550243002/diff/1/chrome/browser/resources/vr... chrome/browser/resources/vr_shell/vr_shell_ui.css:103: background-image: url(images/back.svg); On 2016/12/05 20:28:35, cjgrant wrote: > The original line was resolved through html-flattening, which is now disabled. > This lets us manipulate images through Javascript, as well as avoiding possible > size duplication (where we need the image file, and it gets embedded in the HTML > as well). hmm. I think flattening would make loading slightly faster. When is the svg size duplicated?
https://codereview.chromium.org/2550243002/diff/1/chrome/browser/browser_reso... File chrome/browser/browser_resources.grd (right): https://codereview.chromium.org/2550243002/diff/1/chrome/browser/browser_reso... chrome/browser/browser_resources.grd:225: <include name="IDR_VR_SHELL_UI_IMAGE_BACK" file="resources/vr_shell\images\back.svg" type="BINDATA" /> On 2016/12/05 21:08:38, bshe wrote: > I think you are likely to get push back from owner of this file. This is only > intended for html/css/js files I believe. > See: https://codereview.chromium.org/2363553003 Ahh, yes, right! After thinking more, I have a few questions. I'll post a new patch set, and loop in Dan Beam on the questions. https://codereview.chromium.org/2550243002/diff/1/chrome/browser/resources/vr... File chrome/browser/resources/vr_shell/vr_shell_ui.css (right): https://codereview.chromium.org/2550243002/diff/1/chrome/browser/resources/vr... chrome/browser/resources/vr_shell/vr_shell_ui.css:103: background-image: url(images/back.svg); On 2016/12/05 21:08:38, bshe wrote: > On 2016/12/05 20:28:35, cjgrant wrote: > > The original line was resolved through html-flattening, which is now disabled. > > > This lets us manipulate images through Javascript, as well as avoiding > possible > > size duplication (where we need the image file, and it gets embedded in the > HTML > > as well). > > hmm. I think flattening would make loading slightly faster. When is the svg size > duplicated? Example: The "i in a circle" image is used inline by HTML, but also set programmatically in Javascript (requiring a file). I thought it'd be better to not flatten, as our resources are loaded locally anyway. I could be wrong here though. Prior to this, I experimented with making 3 different divs, each with an icon, then using javascript to set which one is visible. That seemed a lot more cumbersome to me, but it works. https://codereview.chromium.org/2550243002/diff/1/chrome/browser/resources/vr... File chrome/browser/resources/vr_shell/vr_shell_ui.js (right): https://codereview.chromium.org/2550243002/diff/1/chrome/browser/resources/vr... chrome/browser/resources/vr_shell/vr_shell_ui.js:366: case 6: // DANGEROUS: On 2016/12/05 20:57:30, mthiesse wrote: > no dangerous icon? Saving that for later? It would use the warning image from line 352, but that was a stupid place for a default. I'll move it.
https://codereview.chromium.org/2550243002/diff/1/chrome/browser/browser_reso... File chrome/browser/browser_resources.grd (right): https://codereview.chromium.org/2550243002/diff/1/chrome/browser/browser_reso... chrome/browser/browser_resources.grd:225: <include name="IDR_VR_SHELL_UI_IMAGE_BACK" file="resources/vr_shell\images\back.svg" type="BINDATA" /> On 2016/12/05 22:10:17, cjgrant wrote: > On 2016/12/05 21:08:38, bshe wrote: > > I think you are likely to get push back from owner of this file. This is only > > intended for html/css/js files I believe. > > See: https://codereview.chromium.org/2363553003 > > Ahh, yes, right! After thinking more, I have a few questions. I'll post a new > patch set, and loop in Dan Beam on the questions. Done (in new location). https://codereview.chromium.org/2550243002/diff/1/chrome/browser/resources/vr... File chrome/browser/resources/vr_shell/vr_shell_ui.js (right): https://codereview.chromium.org/2550243002/diff/1/chrome/browser/resources/vr... chrome/browser/resources/vr_shell/vr_shell_ui.js:364: // TODO(cjgrant): Clank appears to not handle this case. On 2016/12/05 20:28:36, cjgrant wrote: > I'm investigating this, and will list a bug if warranted. Done (not a bug, only ChromeOS uses '5') https://codereview.chromium.org/2550243002/diff/1/chrome/browser/resources/vr... chrome/browser/resources/vr_shell/vr_shell_ui.js:366: case 6: // DANGEROUS: On 2016/12/05 22:10:17, cjgrant wrote: > On 2016/12/05 20:57:30, mthiesse wrote: > > no dangerous icon? Saving that for later? > > It would use the warning image from line 352, but that was a stupid place for a > default. I'll move it. Done. https://codereview.chromium.org/2550243002/diff/20001/ui/webui/resources/webu... File ui/webui/resources/webui_resources.grd (right): https://codereview.chromium.org/2550243002/diff/20001/ui/webui/resources/webu... ui/webui/resources/webui_resources.grd:200: <include name="IDR_VR_SHELL_UI_IMAGE_BACK" I hope to conditionally include these, after confirming with dbeam@ that this is the correct home.
On 2016/12/06 15:52:00, cjgrant wrote: > https://codereview.chromium.org/2550243002/diff/1/chrome/browser/browser_reso... > File chrome/browser/browser_resources.grd (right): > > https://codereview.chromium.org/2550243002/diff/1/chrome/browser/browser_reso... > chrome/browser/browser_resources.grd:225: <include > name="IDR_VR_SHELL_UI_IMAGE_BACK" file="resources/vr_shell\images\back.svg" > type="BINDATA" /> > On 2016/12/05 22:10:17, cjgrant wrote: > > On 2016/12/05 21:08:38, bshe wrote: > > > I think you are likely to get push back from owner of this file. This is > only > > > intended for html/css/js files I believe. > > > See: https://codereview.chromium.org/2363553003 > > > > Ahh, yes, right! After thinking more, I have a few questions. I'll post a > new > > patch set, and loop in Dan Beam on the questions. > > Done (in new location). > > https://codereview.chromium.org/2550243002/diff/1/chrome/browser/resources/vr... > File chrome/browser/resources/vr_shell/vr_shell_ui.js (right): > > https://codereview.chromium.org/2550243002/diff/1/chrome/browser/resources/vr... > chrome/browser/resources/vr_shell/vr_shell_ui.js:364: // TODO(cjgrant): Clank > appears to not handle this case. > On 2016/12/05 20:28:36, cjgrant wrote: > > I'm investigating this, and will list a bug if warranted. > > Done (not a bug, only ChromeOS uses '5') > > https://codereview.chromium.org/2550243002/diff/1/chrome/browser/resources/vr... > chrome/browser/resources/vr_shell/vr_shell_ui.js:366: case 6: // DANGEROUS: > On 2016/12/05 22:10:17, cjgrant wrote: > > On 2016/12/05 20:57:30, mthiesse wrote: > > > no dangerous icon? Saving that for later? > > > > It would use the warning image from line 352, but that was a stupid place for > a > > default. I'll move it. > > Done. > > https://codereview.chromium.org/2550243002/diff/20001/ui/webui/resources/webu... > File ui/webui/resources/webui_resources.grd (right): > > https://codereview.chromium.org/2550243002/diff/20001/ui/webui/resources/webu... > ui/webui/resources/webui_resources.grd:200: <include > name="IDR_VR_SHELL_UI_IMAGE_BACK" > I hope to conditionally include these, after confirming with dbeam@ that this is > the correct home. lgtm
cjgrant@chromium.org changed reviewers: + dbeam@chromium.org
Hi Dan, I have a few questions for you about this CL in general. Could you weigh in on these when you have time? - HTML flattening in grd file. This means things like SVGs can be embedded right in HTML, but then an image’s source can’t be varied by Javascript. Also, presumably, if an image is used in multiple places, flattening would bloat the HTML size. Is there a strong preference either way? - When setting an HTML image src attribute, is it better to use a URL like “chrome://resources/…”, or use AddResourcePath() then use a URL like “images/…”? - If we are building VrShell’s HTML-based UI, where would HTML/CSS/JS and images live? Right now, HTML/CSS/JS is in chrome/browser/resources, but images are in ui/webui/resources. This seems like an odd split. Are we doing it correctly? - I don’t see many conditionals used in webui_resources.grd. If our images are used only in Android VrShell-enabled builds, I assume we should use the same conditionals used for chrome/browser/resources. However, it seemed to me like the “is_android” conditional didn’t function properly in webui_resources.grd. Putting resources in that conditional failed to include them in my Clank build. Should it?
On 2016/12/06 19:02:13, cjgrant wrote: > Hi Dan, > > I have a few questions for you about this CL in general. Could you weigh in on > these when you have time? > > - HTML flattening in grd file. This means things like SVGs can be embedded > right in HTML, but then an image’s source can’t be varied by Javascript. technically you can use the CSSOM to look through a style sheet's style rules and change a property, but yes: it's much harder in this case. asking for an image by URL, it might hit disk separately from your original request the first time the URL is loaded. if you inline a resource, it might "paste" the contents in various places, but will be loaded in the same resource request (it's combined with the page it's inlined from). if you're using a resource once in a performance critical place, inlining might be right for you. if you're using a resource many places and it's likely to be cached, creating a URL and loading from there might be right for you. for in-between: ¯\_(ツ)_/¯ > Also, > presumably, if an image is used in multiple places, flattening would bloat the > HTML size. Is there a strong preference either way? potentially, yes. not really a consensus around loading from URL or flattening being better for chrome overall. > > - When setting an HTML image src attribute, is it better to use a URL like > “chrome://resources/…”, or use AddResourcePath() then use a URL like “images/…”? only use chrome://resources if your file is used in 2+ domains. > > - If we are building VrShell’s HTML-based UI, where would HTML/CSS/JS and images > live? Right now, HTML/CSS/JS is in chrome/browser/resources, but images are in > ui/webui/resources. This seems like an odd split. Are we doing it correctly? ui/webui/resources is the location for things used from multiple UIs, so probably not. > > - I don’t see many conditionals used in webui_resources.grd. If our images are > used only in Android VrShell-enabled builds, I assume we should use the same > conditionals used for chrome/browser/resources. However, it seemed to me like > the “is_android” conditional didn’t function properly in webui_resources.grd. > Putting resources in that conditional failed to include them in my Clank build. > Should it? is_android should succeed in any grd file (afaik). if it's not, you can probably fix it to.
On 2016/12/07 02:34:05, Dan Beam wrote: > On 2016/12/06 19:02:13, cjgrant wrote: > > Hi Dan, > > > > I have a few questions for you about this CL in general. Could you weigh in > on > > these when you have time? > > > > - HTML flattening in grd file. This means things like SVGs can be embedded > > right in HTML, but then an image’s source can’t be varied by Javascript. > > technically you can use the CSSOM to look through a style sheet's style rules > and change a property, but yes: it's much harder in this case. > > asking for an image by URL, it might hit disk separately from your original > request the first time the URL is loaded. if you inline a resource, it might > "paste" the contents in various places, but will be loaded in the same resource > request (it's combined with the page it's inlined from). > > if you're using a resource once in a performance critical place, inlining might > be right for you. > > if you're using a resource many places and it's likely to be cached, creating a > URL and loading from there might be right for you. > > for in-between: ¯\_(ツ)_/¯ > > > Also, > > presumably, if an image is used in multiple places, flattening would bloat the > > HTML size. Is there a strong preference either way? > > potentially, yes. not really a consensus around loading from URL or flattening > being better for chrome overall. > > > > > - When setting an HTML image src attribute, is it better to use a URL like > > “chrome://resources/…”, or use AddResourcePath() then use a URL like > “images/…”? > > only use chrome://resources if your file is used in 2+ domains. > > > > > - If we are building VrShell’s HTML-based UI, where would HTML/CSS/JS and > images > > live? Right now, HTML/CSS/JS is in chrome/browser/resources, but images are > in > > ui/webui/resources. This seems like an odd split. Are we doing it correctly? > > ui/webui/resources is the location for things used from multiple UIs, so > probably not. > > > > > - I don’t see many conditionals used in webui_resources.grd. If our images > are > > used only in Android VrShell-enabled builds, I assume we should use the same > > conditionals used for chrome/browser/resources. However, it seemed to me like > > the “is_android” conditional didn’t function properly in webui_resources.grd. > > Putting resources in that conditional failed to include them in my Clank > build. > > Should it? > > is_android should succeed in any grd file (afaik). if it's not, you can > probably fix it to. Awesome, thanks Dan. Only one clarification needed: Are you saying that we should indeed put our SVGs in chrome/browser/android/vr_shell? In an older CL, it was stated that only HTML, CSS and JS type files should go there. To me, it seems like the best home for the SVGs as well.
On 2016/12/07 15:29:46, cjgrant wrote: > On 2016/12/07 02:34:05, Dan Beam wrote: > > On 2016/12/06 19:02:13, cjgrant wrote: > > > Hi Dan, > > > > > > I have a few questions for you about this CL in general. Could you weigh in > > on > > > these when you have time? > > > > > > - HTML flattening in grd file. This means things like SVGs can be embedded > > > right in HTML, but then an image’s source can’t be varied by Javascript. > > > > technically you can use the CSSOM to look through a style sheet's style rules > > and change a property, but yes: it's much harder in this case. > > > > asking for an image by URL, it might hit disk separately from your original > > request the first time the URL is loaded. if you inline a resource, it might > > "paste" the contents in various places, but will be loaded in the same > resource > > request (it's combined with the page it's inlined from). > > > > if you're using a resource once in a performance critical place, inlining > might > > be right for you. > > > > if you're using a resource many places and it's likely to be cached, creating > a > > URL and loading from there might be right for you. > > > > for in-between: ¯\_(ツ)_/¯ > > > > > Also, > > > presumably, if an image is used in multiple places, flattening would bloat > the > > > HTML size. Is there a strong preference either way? > > > > potentially, yes. not really a consensus around loading from URL or > flattening > > being better for chrome overall. > > > > > > > > - When setting an HTML image src attribute, is it better to use a URL like > > > “chrome://resources/…”, or use AddResourcePath() then use a URL like > > “images/…”? > > > > only use chrome://resources if your file is used in 2+ domains. > > > > > > > > - If we are building VrShell’s HTML-based UI, where would HTML/CSS/JS and > > images > > > live? Right now, HTML/CSS/JS is in chrome/browser/resources, but images are > > in > > > ui/webui/resources. This seems like an odd split. Are we doing it > correctly? > > > > ui/webui/resources is the location for things used from multiple UIs, so > > probably not. > > > > > > > > - I don’t see many conditionals used in webui_resources.grd. If our images > > are > > > used only in Android VrShell-enabled builds, I assume we should use the same > > > conditionals used for chrome/browser/resources. However, it seemed to me > like > > > the “is_android” conditional didn’t function properly in > webui_resources.grd. > > > Putting resources in that conditional failed to include them in my Clank > > build. > > > Should it? > > > > is_android should succeed in any grd file (afaik). if it's not, you can > > probably fix it to. > > Awesome, thanks Dan. Only one clarification needed: Are you saying that we > should indeed put our SVGs in chrome/browser/android/vr_shell? In an older CL, > it was stated that only HTML, CSS and JS type files should go there. To me, it > seems like the best home for the SVGs as well. welll, you might not want to put them into a .grd file in ui/webui/resources with an IDR_, but storing the file in ui/webui/resources might be OK. to be honest, I didn't really understand the requirements of your previous reviewer (was it estade@?) about not putting images in chrome/browser/browser_resources.grd. tl;dr - ui/webui is generally used for stuff that's harnessed from multiple pages. putting your image as close to as where it's used (especially if it's in only 1 place) for now sounds more intuitive to me.
> welll, you might not want to put them into a .grd file in ui/webui/resources > with an IDR_, but storing the file in ui/webui/resources might be OK. to be > honest, I didn't really understand the requirements of your previous reviewer > (was it estade@?) about not putting images in > chrome/browser/browser_resources.grd. > > tl;dr - ui/webui is generally used for stuff that's harnessed from multiple > pages. putting your image as close to as where it's used (especially if it's in > only 1 place) for now sounds more intuitive to me. Yes, it was estade@, who said "+dbeam. To my knowledge, browser_resources isn't where we put pngs, just html/js/css. There are a few other pngs in there which I think might be a mistake." See here: https://codereview.chromium.org/2363553003/#msg21
On 2016/12/07 19:08:00, cjgrant wrote: > > welll, you might not want to put them into a .grd file in ui/webui/resources > > with an IDR_, but storing the file in ui/webui/resources might be OK. to be > > honest, I didn't really understand the requirements of your previous reviewer > > (was it estade@?) about not putting images in > > chrome/browser/browser_resources.grd. > > > > tl;dr - ui/webui is generally used for stuff that's harnessed from multiple > > pages. putting your image as close to as where it's used (especially if it's > in > > only 1 place) for now sounds more intuitive to me. > > Yes, it was estade@, who said "+dbeam. To my knowledge, browser_resources isn't > where we put pngs, just html/js/css. There are a few other pngs in there which > I think might be a mistake." See here: > > https://codereview.chromium.org/2363553003/#msg21 yeah, i think we just generally used inlining / flattening instead of loading the images by URL. in that case, the image wouldn't need to be in any .grd file
On 2016/12/07 22:02:16, Dan Beam wrote: > On 2016/12/07 19:08:00, cjgrant wrote: > > > welll, you might not want to put them into a .grd file in ui/webui/resources > > > with an IDR_, but storing the file in ui/webui/resources might be OK. to be > > > honest, I didn't really understand the requirements of your previous > reviewer > > > (was it estade@?) about not putting images in > > > chrome/browser/browser_resources.grd. > > > > > > tl;dr - ui/webui is generally used for stuff that's harnessed from multiple > > > pages. putting your image as close to as where it's used (especially if > it's > > in > > > only 1 place) for now sounds more intuitive to me. > > > > Yes, it was estade@, who said "+dbeam. To my knowledge, browser_resources > isn't > > where we put pngs, just html/js/css. There are a few other pngs in there > which > > I think might be a mistake." See here: > > > > https://codereview.chromium.org/2363553003/#msg21 > > yeah, i think we just generally used inlining / flattening instead of loading > the images by URL. in that case, the image wouldn't need to be in any .grd file So two things: If we inline/flatten, how would you switch icons? Keep doing what we're doing what we're doing, create multiple divs/images, and selectively only show the one we want, rather than changing image source? This seems brittle to me. If we opt to not inline/flatten, does this all mean we're fine to put our image files in chrome/browser/resources, with the HTML? Some (not all) of the images are currently shared with other webui's, but this will change - we want to customize our images for VR. Hence I'd rather assume we're not sharing anything at this time.
I talked to Biao, and we concluded on no flattening, and image resources alongside the HTML-related resources. Dan, can we get an lgtm for you on that?
https://codereview.chromium.org/2550243002/diff/40001/chrome/browser/resource... File chrome/browser/resources/vr_shell/images/lock.svg (right): https://codereview.chromium.org/2550243002/diff/40001/chrome/browser/resource... chrome/browser/resources/vr_shell/images/lock.svg:3: <path d="M36 16h-2v-4c0-5.52-4.48-10-10-10S14 6.48 14 12v4h-2c-2.21 0-4 1.79-4 4v20c0 2.21 1.79 4 4 4h24c2.21 0 4-1.79 4-4V20c0-2.21-1.79-4-4-4zM24 34c-2.21 0-4-1.79-4-4s1.79-4 4-4 4 1.79 4 4-1.79 4-4 4zm6.2-18H17.8v-4c0-3.42 2.78-6.2 6.2-6.2 3.42 0 6.2 2.78 6.2 6.2v4z"/> leave this in ui/webui, used by chrome/browser/resources/md_user_manager/user_manager.html https://codereview.chromium.org/2550243002/diff/40001/chrome/browser/resource... File chrome/browser/resources/vr_shell/images/warning.svg (right): https://codereview.chromium.org/2550243002/diff/40001/chrome/browser/resource... chrome/browser/resources/vr_shell/images/warning.svg:3: <path d="M2 42h44L24 4 2 42zm24-6h-4v-4h4v4zm0-8h-4v-8h4v8z"/> leave as ui/webui/resources/images/warning.svg as it's used from many other places https://codereview.chromium.org/2550243002/diff/40001/chrome/browser/resource... File chrome/browser/resources/vr_shell/vr_shell_ui.css (right): https://codereview.chromium.org/2550243002/diff/40001/chrome/browser/resource... chrome/browser/resources/vr_shell/vr_shell_ui.css:104: } #back .button, #reload .button { background-image: url(images/back.svg); } #forward .button { transform: scaleX(-1); } does less inlines https://codereview.chromium.org/2550243002/diff/40001/chrome/browser/resource... File chrome/browser/resources/vr_shell/vr_shell_ui.html (left): https://codereview.chromium.org/2550243002/diff/40001/chrome/browser/resource... chrome/browser/resources/vr_shell/vr_shell_ui.html:25: src="../../../../ui/webui/resources/images/i_circle.svg"> if you want to not inline but still shared, you can add i_circle.svg to ui/webui/resources/webui_resources.grd and load it via URL (i.e. chrome://resources/images/info.svg or something) https://codereview.chromium.org/2550243002/diff/40001/chrome/browser/resource... File chrome/browser/resources/vr_shell/vr_shell_ui.js (right): https://codereview.chromium.org/2550243002/diff/40001/chrome/browser/resource... chrome/browser/resources/vr_shell/vr_shell_ui.js:369: document.querySelector('#omni-icon').src = getSecurityIcon(this.level); nit: $('omni-icon').src if you're using util.js
Questions back to you, Dan. Sorry about the delay, I was OOO for a week. https://codereview.chromium.org/2550243002/diff/40001/chrome/browser/resource... File chrome/browser/resources/vr_shell/images/lock.svg (right): https://codereview.chromium.org/2550243002/diff/40001/chrome/browser/resource... chrome/browser/resources/vr_shell/images/lock.svg:3: <path d="M36 16h-2v-4c0-5.52-4.48-10-10-10S14 6.48 14 12v4h-2c-2.21 0-4 1.79-4 4v20c0 2.21 1.79 4 4 4h24c2.21 0 4-1.79 4-4V20c0-2.21-1.79-4-4-4zM24 34c-2.21 0-4-1.79-4-4s1.79-4 4-4 4 1.79 4 4-1.79 4-4 4zm6.2-18H17.8v-4c0-3.42 2.78-6.2 6.2-6.2 3.42 0 6.2 2.78 6.2 6.2v4z"/> On 2016/12/13 03:56:59, Dan Beam wrote: > leave this in ui/webui, used by > chrome/browser/resources/md_user_manager/user_manager.html Our lock image is green, whereas the existing lock image is grey. I don't think we want to share images, unless we go down the road of parsing and modifying SVG on the fly. https://codereview.chromium.org/2550243002/diff/40001/chrome/browser/resource... File chrome/browser/resources/vr_shell/images/warning.svg (right): https://codereview.chromium.org/2550243002/diff/40001/chrome/browser/resource... chrome/browser/resources/vr_shell/images/warning.svg:3: <path d="M2 42h44L24 4 2 42zm24-6h-4v-4h4v4zm0-8h-4v-8h4v8z"/> On 2016/12/13 03:56:59, Dan Beam wrote: > leave as ui/webui/resources/images/warning.svg as it's used from many other > places We'll eventually change this color as well. It looks like Android has a mechanism of coloring icons like this on the fly, but I don't know how we'd do that in HTML unless we parse the SVG and then attempt to modify its properties through the DOM. Thoughts? https://codereview.chromium.org/2550243002/diff/40001/chrome/browser/resource... File chrome/browser/resources/vr_shell/vr_shell_ui.html (left): https://codereview.chromium.org/2550243002/diff/40001/chrome/browser/resource... chrome/browser/resources/vr_shell/vr_shell_ui.html:25: src="../../../../ui/webui/resources/images/i_circle.svg"> On 2016/12/13 03:56:59, Dan Beam wrote: > if you want to not inline but still shared, you can add i_circle.svg to > ui/webui/resources/webui_resources.grd and load it via URL (i.e. > chrome://resources/images/info.svg or something) Deferring on this until we address the icon coloring question. https://codereview.chromium.org/2550243002/diff/40001/chrome/browser/resource... File chrome/browser/resources/vr_shell/vr_shell_ui.js (right): https://codereview.chromium.org/2550243002/diff/40001/chrome/browser/resource... chrome/browser/resources/vr_shell/vr_shell_ui.js:369: document.querySelector('#omni-icon').src = getSecurityIcon(this.level); On 2016/12/13 03:56:59, Dan Beam wrote: > nit: $('omni-icon').src if you're using util.js I can incorporate util.js, but we should be consistent and use it everywhere in this file. That feels like a separate change (which I'm happy to make!).
On 2016/12/19 21:19:10, cjgrant wrote: > Questions back to you, Dan. Sorry about the delay, I was OOO for a week. > > https://codereview.chromium.org/2550243002/diff/40001/chrome/browser/resource... > File chrome/browser/resources/vr_shell/images/lock.svg (right): > > https://codereview.chromium.org/2550243002/diff/40001/chrome/browser/resource... > chrome/browser/resources/vr_shell/images/lock.svg:3: <path d="M36 > 16h-2v-4c0-5.52-4.48-10-10-10S14 6.48 14 12v4h-2c-2.21 0-4 1.79-4 4v20c0 2.21 > 1.79 4 4 4h24c2.21 0 4-1.79 4-4V20c0-2.21-1.79-4-4-4zM24 34c-2.21 > 0-4-1.79-4-4s1.79-4 4-4 4 1.79 4 4-1.79 4-4 4zm6.2-18H17.8v-4c0-3.42 2.78-6.2 > 6.2-6.2 3.42 0 6.2 2.78 6.2 6.2v4z"/> > On 2016/12/13 03:56:59, Dan Beam wrote: > > leave this in ui/webui, used by > > chrome/browser/resources/md_user_manager/user_manager.html > > Our lock image is green, whereas the existing lock image is grey. I don't think > we want to share images, unless we go down the road of parsing and modifying SVG > on the fly. > > https://codereview.chromium.org/2550243002/diff/40001/chrome/browser/resource... > File chrome/browser/resources/vr_shell/images/warning.svg (right): > > https://codereview.chromium.org/2550243002/diff/40001/chrome/browser/resource... > chrome/browser/resources/vr_shell/images/warning.svg:3: <path d="M2 42h44L24 4 2 > 42zm24-6h-4v-4h4v4zm0-8h-4v-8h4v8z"/> > On 2016/12/13 03:56:59, Dan Beam wrote: > > leave as ui/webui/resources/images/warning.svg as it's used from many other > > places > > We'll eventually change this color as well. It looks like Android has a > mechanism of coloring icons like this on the fly, but I don't know how we'd do > that in HTML unless we parse the SVG and then attempt to modify its properties > through the DOM. Thoughts? > > https://codereview.chromium.org/2550243002/diff/40001/chrome/browser/resource... > File chrome/browser/resources/vr_shell/vr_shell_ui.html (left): > > https://codereview.chromium.org/2550243002/diff/40001/chrome/browser/resource... > chrome/browser/resources/vr_shell/vr_shell_ui.html:25: > src="../../../../ui/webui/resources/images/i_circle.svg"> > On 2016/12/13 03:56:59, Dan Beam wrote: > > if you want to not inline but still shared, you can add i_circle.svg to > > ui/webui/resources/webui_resources.grd and load it via URL (i.e. > > chrome://resources/images/info.svg or something) > > Deferring on this until we address the icon coloring question. > > https://codereview.chromium.org/2550243002/diff/40001/chrome/browser/resource... > File chrome/browser/resources/vr_shell/vr_shell_ui.js (right): > > https://codereview.chromium.org/2550243002/diff/40001/chrome/browser/resource... > chrome/browser/resources/vr_shell/vr_shell_ui.js:369: > document.querySelector('#omni-icon').src = getSecurityIcon(this.level); > On 2016/12/13 03:56:59, Dan Beam wrote: > > nit: $('omni-icon').src if you're using util.js > > I can incorporate util.js, but we should be consistent and use it everywhere in > this file. That feels like a separate change (which I'm happy to make!). I could be wrong, but I thought svg were colored via css. For example the media router cast icon (which is an svg) is either blue or black depending on if it's active. They use polymer (which confuses things a bit), but I think it holds for plain svg's as well. Media router cast icon: SVG definition (in a polymer html component) - https://cs.chromium.org/chromium/src/chrome/browser/resources/media_router/ic... References to the icon - https://cs.chromium.org/chromium/src/chrome/browser/resources/media_router/el... Code that calculates the css for the icon - https://cs.chromium.org/chromium/src/chrome/browser/resources/media_router/el... Css class for icon (note no color is specified and I think the default is black) - https://cs.chromium.org/chromium/src/chrome/browser/resources/media_router/el... and when it is active (only specifies color) - https://cs.chromium.org/chromium/src/chrome/browser/resources/media_router/el...
On 2016/12/19 22:22:02, amp wrote: > On 2016/12/19 21:19:10, cjgrant wrote: > > Questions back to you, Dan. Sorry about the delay, I was OOO for a week. > > > > > https://codereview.chromium.org/2550243002/diff/40001/chrome/browser/resource... > > File chrome/browser/resources/vr_shell/images/lock.svg (right): > > > > > https://codereview.chromium.org/2550243002/diff/40001/chrome/browser/resource... > > chrome/browser/resources/vr_shell/images/lock.svg:3: <path d="M36 > > 16h-2v-4c0-5.52-4.48-10-10-10S14 6.48 14 12v4h-2c-2.21 0-4 1.79-4 4v20c0 2.21 > > 1.79 4 4 4h24c2.21 0 4-1.79 4-4V20c0-2.21-1.79-4-4-4zM24 34c-2.21 > > 0-4-1.79-4-4s1.79-4 4-4 4 1.79 4 4-1.79 4-4 4zm6.2-18H17.8v-4c0-3.42 2.78-6.2 > > 6.2-6.2 3.42 0 6.2 2.78 6.2 6.2v4z"/> > > On 2016/12/13 03:56:59, Dan Beam wrote: > > > leave this in ui/webui, used by > > > chrome/browser/resources/md_user_manager/user_manager.html > > > > Our lock image is green, whereas the existing lock image is grey. I don't > think > > we want to share images, unless we go down the road of parsing and modifying > SVG > > on the fly. > > > > > https://codereview.chromium.org/2550243002/diff/40001/chrome/browser/resource... > > File chrome/browser/resources/vr_shell/images/warning.svg (right): > > > > > https://codereview.chromium.org/2550243002/diff/40001/chrome/browser/resource... > > chrome/browser/resources/vr_shell/images/warning.svg:3: <path d="M2 42h44L24 4 > 2 > > 42zm24-6h-4v-4h4v4zm0-8h-4v-8h4v8z"/> > > On 2016/12/13 03:56:59, Dan Beam wrote: > > > leave as ui/webui/resources/images/warning.svg as it's used from many other > > > places > > > > We'll eventually change this color as well. It looks like Android has a > > mechanism of coloring icons like this on the fly, but I don't know how we'd do > > that in HTML unless we parse the SVG and then attempt to modify its properties > > through the DOM. Thoughts? > > > > > https://codereview.chromium.org/2550243002/diff/40001/chrome/browser/resource... > > File chrome/browser/resources/vr_shell/vr_shell_ui.html (left): > > > > > https://codereview.chromium.org/2550243002/diff/40001/chrome/browser/resource... > > chrome/browser/resources/vr_shell/vr_shell_ui.html:25: > > src="../../../../ui/webui/resources/images/i_circle.svg"> > > On 2016/12/13 03:56:59, Dan Beam wrote: > > > if you want to not inline but still shared, you can add i_circle.svg to > > > ui/webui/resources/webui_resources.grd and load it via URL (i.e. > > > chrome://resources/images/info.svg or something) > > > > Deferring on this until we address the icon coloring question. > > > > > https://codereview.chromium.org/2550243002/diff/40001/chrome/browser/resource... > > File chrome/browser/resources/vr_shell/vr_shell_ui.js (right): > > > > > https://codereview.chromium.org/2550243002/diff/40001/chrome/browser/resource... > > chrome/browser/resources/vr_shell/vr_shell_ui.js:369: > > document.querySelector('#omni-icon').src = getSecurityIcon(this.level); > > On 2016/12/13 03:56:59, Dan Beam wrote: > > > nit: $('omni-icon').src if you're using util.js > > > > I can incorporate util.js, but we should be consistent and use it everywhere > in > > this file. That feels like a separate change (which I'm happy to make!). > > I could be wrong, but I thought svg were colored via css. > > For example the media router cast icon (which is an svg) is either blue or black > depending on if it's active. > > They use polymer (which confuses things a bit), but I think it holds for plain > svg's as well. > > Media router cast icon: > SVG definition (in a polymer html component) - > https://cs.chromium.org/chromium/src/chrome/browser/resources/media_router/ic... > > References to the icon - > https://cs.chromium.org/chromium/src/chrome/browser/resources/media_router/el... > > Code that calculates the css for the icon - > https://cs.chromium.org/chromium/src/chrome/browser/resources/media_router/el... > > Css class for icon (note no color is specified and I think the default is black) > - > https://cs.chromium.org/chromium/src/chrome/browser/resources/media_router/el... > and when it is active (only specifies color) - > https://cs.chromium.org/chromium/src/chrome/browser/resources/media_router/el... Thanks for the insights! I'll rework this change to share the icons, get this landed, then look at the coloring.
if you're waiting on me, btw, I believe you can change the color of an SVG with fill: currentcolor; in the SVG's CSS and set color from outside (polymer does this I'm pretty sure)
On 2016/12/22 17:30:45, Dan Beam wrote: > if you're waiting on me, btw, I believe you can change the color of an SVG with > fill: currentcolor; in the SVG's CSS and set color from outside (polymer does > this I'm pretty sure) Thanks. I had spent some time looking at Polymer. The thing that seems different for us is that we're setting an img src property, rather than using inline SVG (as polymer seems to do for the iron iconset). Setting the fill CSS property was among the first things I tried, and it only works if the SVG is directly included. I don't think we want to be using Polymer in this UI, but I also haven't found a working way to color the image files as is. It feels like we should be able to use flattening to do this, but I haven't gotten to investigating that.
On 2016/12/22 20:52:43, cjgrant wrote: > On 2016/12/22 17:30:45, Dan Beam wrote: > > if you're waiting on me, btw, I believe you can change the color of an SVG > with > > fill: currentcolor; in the SVG's CSS and set color from outside (polymer does > > this I'm pretty sure) > > Thanks. I had spent some time looking at Polymer. The thing that seems > different for us is that we're setting an img src property, rather than using > inline SVG (as polymer seems to do for the iron iconset). Setting the fill CSS > property was among the first things I tried, and it only works if the SVG is > directly included. > > I don't think we want to be using Polymer in this UI, but I also haven't found a > working way to color the image files as is. It feels like we should be able to > use flattening to do this, but I haven't gotten to investigating that. in that case you can use a fancy -webkit-mask-image, but it's a little involved: https://cs.chromium.org/chromium/src/ui/login/bubble.css?q=webkit-mask&sq=pac... let me know what you'd like to do
On 2016/12/22 20:58:17, Dan Beam wrote: > On 2016/12/22 20:52:43, cjgrant wrote: > > On 2016/12/22 17:30:45, Dan Beam wrote: > > > if you're waiting on me, btw, I believe you can change the color of an SVG > > with > > > fill: currentcolor; in the SVG's CSS and set color from outside (polymer > does > > > this I'm pretty sure) > > > > Thanks. I had spent some time looking at Polymer. The thing that seems > > different for us is that we're setting an img src property, rather than using > > inline SVG (as polymer seems to do for the iron iconset). Setting the fill > CSS > > property was among the first things I tried, and it only works if the SVG is > > directly included. > > > > I don't think we want to be using Polymer in this UI, but I also haven't found > a > > working way to color the image files as is. It feels like we should be able > to > > use flattening to do this, but I haven't gotten to investigating that. > > in that case you can use a fancy -webkit-mask-image, but it's a little involved: > https://cs.chromium.org/chromium/src/ui/login/bubble.css?q=webkit-mask&sq=pac... > > let me know what you'd like to do That mask business looks pretty cool. If it works, I'm all for it. I was also checking out the <embed> approach suggested by an online SVG Primer. Feel free to ignore this CL until after the holidays...
Description was changed from ========== Rework VrShell security warnings (and other improvements). - Move VrShell images to the same location as other webui resources. - Rename images for consistency. - Properly choose a security status icon based on security level. - Fix the omnibox icon size to not shrink with long URLs. - Simplify resource paths (localhost development no longer needs a hacked-up directory structure). BUG=641508 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Color the VR omnibox security icons. BUG=641508 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
On 2016/12/22 23:37:27, cjgrant wrote: > On 2016/12/22 20:58:17, Dan Beam wrote: > > On 2016/12/22 20:52:43, cjgrant wrote: > > > On 2016/12/22 17:30:45, Dan Beam wrote: > > > > if you're waiting on me, btw, I believe you can change the color of an SVG > > > with > > > > fill: currentcolor; in the SVG's CSS and set color from outside (polymer > > does > > > > this I'm pretty sure) > > > > > > Thanks. I had spent some time looking at Polymer. The thing that seems > > > different for us is that we're setting an img src property, rather than > using > > > inline SVG (as polymer seems to do for the iron iconset). Setting the fill > > CSS > > > property was among the first things I tried, and it only works if the SVG is > > > directly included. > > > > > > I don't think we want to be using Polymer in this UI, but I also haven't > found > > a > > > working way to color the image files as is. It feels like we should be able > > to > > > use flattening to do this, but I haven't gotten to investigating that. > > > > in that case you can use a fancy -webkit-mask-image, but it's a little > involved: > > > https://cs.chromium.org/chromium/src/ui/login/bubble.css?q=webkit-mask&sq=pac... > > > > let me know what you'd like to do > > That mask business looks pretty cool. If it works, I'm all for it. I was also > checking out the <embed> approach suggested by an online SVG Primer. > > Feel free to ignore this CL until after the holidays... Dan, the work was quicker than I thought. Your webkit-mask suggestion worked great. I need to plug in the exact green and red colors, but otherwise this change looks a lot better.
lgtm https://codereview.chromium.org/2550243002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/vr_shell/vr_shell_ui.css (left): https://codereview.chromium.org/2550243002/diff/100001/chrome/browser/resourc... chrome/browser/resources/vr_shell/vr_shell_ui.css:111: background-image: url(../../../../ui/webui/resources/images/vr_back.svg); optional nit: I found the old version is more readable. :)
The CQ bit was checked by cjgrant@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mthiesse@chromium.org Link to the patchset: https://codereview.chromium.org/2550243002/#ps100001 (title: "Arg. Fix style.")
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": 100001, "attempt_start_ts": 1483463274651830,
"parent_rev": "e2642286e03404f81e33ebb7604dbec3e804c3c8", "commit_rev":
"cde3f7a8e51ba8ad56acdb662cb1ce38c1ab2253"}
Message was sent while issue was closed.
Description was changed from ========== Color the VR omnibox security icons. BUG=641508 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Color the VR omnibox security icons. BUG=641508 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2550243002 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Color the VR omnibox security icons. BUG=641508 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2550243002 ========== to ========== Color the VR omnibox security icons. BUG=641508 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/e16ba97bc9e52eb3b58d26ca6d0bb216e7150812 Cr-Commit-Position: refs/heads/master@{#441154} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/e16ba97bc9e52eb3b58d26ca6d0bb216e7150812 Cr-Commit-Position: refs/heads/master@{#441154} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
