Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2363553003:
VrShell: implement insecure content warning display (Closed)
DescriptionWebVR: implement insecure content warning display
On WebVR pages loaded over insecure transport, show a small "Not secure"
warning overlay permanently, and a transient more verbose warning for
a short time on first entering the page.
It uses the information from bajones's "Expose secure origin state to
WebVR renderer" implementation.
The message strings were previously added to chrome/app/generated_resources.grd in https://codereview.chromium.org/2368443002/
BUG=389343, 644492
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/069c76c2ffc77d5fcebb84937e07df8e61c3ebaf
Cr-Commit-Position: refs/heads/master@{#422647}
Patch Set 1 #Patch Set 2 : VrShell: implement insecure content warning display #Patch Set 3 : Fix inconsistent types. #Patch Set 4 : Use CHECK to avoid "unused" warning in non-debug build #Patch Set 5 : Ready for review #
Total comments: 3
Patch Set 6 : Replace GIMP images with PNG file resources #
Total comments: 3
Patch Set 7 : Fixes: avoid img data copy, use time-based expiration #Patch Set 8 : Move warning text 2px down as per UI review #Patch Set 9 : Use HTML-rendered localized strings, delete PNGs #Patch Set 10 : Try to mark dependent CL #
Total comments: 2
Patch Set 11 : Use CSS styling instead of inline styles #
Total comments: 12
Patch Set 12 : Address Dan's review comments #
Total comments: 2
Patch Set 13 : address cjgrant's review comments #
Total comments: 2
Patch Set 14 : address cjgrant's review comments from #38 #Patch Set 15 : Rebase #Patch Set 16 : Upgrade to Josh's new message UI #
Total comments: 2
Patch Set 17 : Josh's requests: font weight=400, adjust sizes to 1px=1mm at 1m. #Patch Set 18 : Josh: lower permanent icon by half its height. #
Total comments: 5
Patch Set 19 : bshe#44: RTL-compatible CSS, comment cleanup. #Patch Set 20 : Rebase, prereq codereview.chromium.org/2374153002 is committed #Patch Set 21 : rebase, fix background for permanent message #Patch Set 22 : Rebase fixups: QuatToAxisAngle API, unused cursor_distance_ #
Total comments: 38
Patch Set 23 : Dan's review: use text_defaults_md, remove redundant CSS, use textDirection #Patch Set 24 : dbeam #58/#59: CSS fixes, move icon to separate resource file #
Total comments: 10
Patch Set 25 : mthiesse #51: use CHECK, add scaling comment. #Patch Set 26 : dbeam #60: fix CSS link, add comments, trust the magic #Patch Set 27 : Rebase to ToT (no changes to patched files) #Messages
Total messages: 77 (25 generated)
Description was changed from ========== VrShell: implement insecure content warning display On WebVR pages loaded over insecure transport, show a small "Not secure" warning overlay permanently, and a transient more verbose warning for a short time on first entering the page. It uses the information from bajones's "Expose secure origin state to WebVR renderer" implementation. This is intended to use the localized strings added to generated_resources.grd, but we can't currently render text in GL due to a not-yet-landed prerequisite patch. As a workaround, this patch uses compiled-in static bitmaps created via Gimp's "export as .c file" function. These bitmaps will be removed once we have text drawing available. BUG= ========== to ========== VrShell: implement insecure content warning display On WebVR pages loaded over insecure transport, show a small "Not secure" warning overlay permanently, and a transient more verbose warning for a short time on first entering the page. It uses the information from bajones's "Expose secure origin state to WebVR renderer" implementation. This is intended to use the localized strings added to generated_resources.grd, but we can't currently render text in GL due to a not-yet-landed prerequisite patch. As a workaround, this patch uses compiled-in static bitmaps created via Gimp's "export as .c file" function. These bitmaps will be removed once we have text drawing available. BUG= ==========
klausw@chromium.org changed reviewers: + bajones@chromium.org, blundell@chromium.org, bshe@chromium.org
blundell: please IM me in case of questions. We're trying to get the strings ready for translation by today's string freeze so that we're not blocked on those once we get the missing prerequisite sorted out. Let me know if this is not the right place to add them, the instructions I found were a bit confusing. Thanks, and sorry about the rush!
On 2016/09/22 21:33:24, klausw1 wrote: > blundell: please IM me in case of questions. We're trying to get the strings > ready for translation by today's string freeze so that we're not blocked on > those once we get the missing prerequisite sorted out. Let me know if this is > not the right place to add them, the instructions I found were a bit confusing. > Thanks, and sorry about the rush! Also, I can split this into two patches, one with just the strings and the rest for separate followup. Would that be preferable?
On 2016/09/22 21:36:04, klausw1 wrote: > On 2016/09/22 21:33:24, klausw1 wrote: > > blundell: please IM me in case of questions. We're trying to get the strings > > ready for translation by today's string freeze so that we're not blocked on > > those once we get the missing prerequisite sorted out. Let me know if this is > > not the right place to add them, the instructions I found were a bit > confusing. > > Thanks, and sorry about the rush! > > Also, I can split this into two patches, one with just the strings and the rest > for separate followup. Would that be preferable? I made https://codereview.chromium.org/2368443002/ with just the strings.
https://codereview.chromium.org/2363553003/diff/80001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_shell_icons.h (right): https://codereview.chromium.org/2363553003/diff/80001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell_icons.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. Given that these icons will probably be removed very soon let's move this file and the icons into a vr_shell/icons folder to keep things a bit cleaner. https://codereview.chromium.org/2363553003/diff/80001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell_icons.h:5: // Helper for loading bitmaps created by Gimp's "export as .c file" function. I hate to ask, but it seems like there's some license implications here? https://codereview.chromium.org/2363553003/diff/80001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_shell_renderer.h (right): https://codereview.chromium.org/2363553003/diff/80001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell_renderer.h:191: OverlayIconRenderer* GetOverlayIconRenderer() { Forgive my ignorance, but it seems like the TexturedQuadRenderer should work pretty well in this case, given that we're rendering quads with textures on them? What's the motivation for a new renderer type?
FYI, I've removed the string-related logic from this CL. The strings are in https://codereview.chromium.org/2368443002/ , and I'll do the loading from resources in a separate followup. On 2016/09/22 22:13:30, bajones wrote: > https://codereview.chromium.org/2363553003/diff/80001/chrome/browser/android/... > File chrome/browser/android/vr_shell/vr_shell_icons.h (right): > > https://codereview.chromium.org/2363553003/diff/80001/chrome/browser/android/... > chrome/browser/android/vr_shell/vr_shell_icons.h:1: // Copyright 2016 The > Chromium Authors. All rights reserved. > Given that these icons will probably be removed very soon let's move this file > and the icons into a vr_shell/icons folder to keep things a bit cleaner. I'm working now on loading from file resources which would avoid these headaches. Will follow up once that's done. > https://codereview.chromium.org/2363553003/diff/80001/chrome/browser/android/... > chrome/browser/android/vr_shell/vr_shell_icons.h:5: // Helper for loading > bitmaps created by Gimp's "export as .c file" function. > I hate to ask, but it seems like there's some license implications here? Best to avoid it, see above. Working on resource file loading. > > https://codereview.chromium.org/2363553003/diff/80001/chrome/browser/android/... > File chrome/browser/android/vr_shell/vr_shell_renderer.h (right): > > https://codereview.chromium.org/2363553003/diff/80001/chrome/browser/android/... > chrome/browser/android/vr_shell/vr_shell_renderer.h:191: OverlayIconRenderer* > GetOverlayIconRenderer() { > Forgive my ignorance, but it seems like the TexturedQuadRenderer should work > pretty well in this case, given that we're rendering quads with textures on > them? What's the motivation for a new renderer type? The shaders are incompatible, samplerExternalOES vs sampler2D. Once we're getting the textures from a background HTML page, we should be able to move to the existing renderer and ditch this one.
klausw@chromium.org changed reviewers: - blundell@chromium.org
On 2016/09/22 23:36:12, klausw1 wrote: > I'm working now on loading from file resources which would avoid these > headaches. Will follow up once that's done. Great, thanks. > The shaders are incompatible, samplerExternalOES vs sampler2D. Once we're > getting the textures from a background HTML page, we should be able to move to > the existing renderer and ditch this one. Forgot about the different sampler types. Makes perfect sense. Thanks for the explanation!
Description was changed from ========== VrShell: implement insecure content warning display On WebVR pages loaded over insecure transport, show a small "Not secure" warning overlay permanently, and a transient more verbose warning for a short time on first entering the page. It uses the information from bajones's "Expose secure origin state to WebVR renderer" implementation. This is intended to use the localized strings added to generated_resources.grd, but we can't currently render text in GL due to a not-yet-landed prerequisite patch. As a workaround, this patch uses compiled-in static bitmaps created via Gimp's "export as .c file" function. These bitmaps will be removed once we have text drawing available. BUG= ========== to ========== VrShell: implement insecure content warning display On WebVR pages loaded over insecure transport, show a small "Not secure" warning overlay permanently, and a transient more verbose warning for a short time on first entering the page. It uses the information from bajones's "Expose secure origin state to WebVR renderer" implementation. This is intended to use the localized strings added to generated_resources.grd, but we can't currently render text in GL due to a not-yet-landed prerequisite patch. As a workaround, this patch uses compiled-in static bitmaps created via Gimp's "export as .c file" function. These bitmaps will be removed once we have text drawing available. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== VrShell: implement insecure content warning display On WebVR pages loaded over insecure transport, show a small "Not secure" warning overlay permanently, and a transient more verbose warning for a short time on first entering the page. It uses the information from bajones's "Expose secure origin state to WebVR renderer" implementation. This is intended to use the localized strings added to generated_resources.grd, but we can't currently render text in GL due to a not-yet-landed prerequisite patch. As a workaround, this patch uses compiled-in static bitmaps created via Gimp's "export as .c file" function. These bitmaps will be removed once we have text drawing available. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== VrShell: implement insecure content warning display On WebVR pages loaded over insecure transport, show a small "Not secure" warning overlay permanently, and a transient more verbose warning for a short time on first entering the page. It uses the information from bajones's "Expose secure origin state to WebVR renderer" implementation. This is intended to use localized strings added separately to generated_resources.grd, but we can't currently render text in GL due to a not-yet-landed prerequisite patch. As a workaround, this patch uses PNG files from browser resources. These bitmaps will be removed once we have text drawing available. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
klausw@chromium.org changed reviewers: + estade@chromium.org
On 2016/09/22 23:51:27, bajones wrote: > On 2016/09/22 23:36:12, klausw1 wrote: > > I'm working now on loading from file resources which would avoid these > > headaches. Will follow up once that's done. > > Great, thanks. > > > The shaders are incompatible, samplerExternalOES vs sampler2D. Once we're > > getting the textures from a background HTML page, we should be able to move to > > the existing renderer and ditch this one. > > Forgot about the different sampler types. Makes perfect sense. Thanks for the > explanation! PTAL, updated to use resources loaded from PNG files.
what am I meant to be looking at? Do you care that the image will be scaled for non-100% device scale factors?
Description was changed from ========== VrShell: implement insecure content warning display On WebVR pages loaded over insecure transport, show a small "Not secure" warning overlay permanently, and a transient more verbose warning for a short time on first entering the page. It uses the information from bajones's "Expose secure origin state to WebVR renderer" implementation. This is intended to use localized strings added separately to generated_resources.grd, but we can't currently render text in GL due to a not-yet-landed prerequisite patch. As a workaround, this patch uses PNG files from browser resources. These bitmaps will be removed once we have text drawing available. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== WebVR: implement insecure content warning display On WebVR pages loaded over insecure transport, show a small "Not secure" warning overlay permanently, and a transient more verbose warning for a short time on first entering the page. It uses the information from bajones's "Expose secure origin state to WebVR renderer" implementation. This is intended to use localized strings added separately to generated_resources.grd, but we can't currently render text in GL due to a not-yet-landed prerequisite patch. As a workaround, this patch uses PNG files from browser resources. These bitmaps will be removed once we have text drawing available. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
On 2016/09/23 00:35:30, Evan Stade wrote:
> what am I meant to be looking at?
I think just the added resources, "git cl upload" said:
Missing OWNER reviewers for these files:
chrome/browser/browser_resources.grd
chrome/browser/resources/vr_shell/webvr_not_secure_permanent.png
chrome/browser/resources/vr_shell/webvr_not_secure_transient.png
Suggested OWNERS: (Use "git-cl owners" to interactively select owners.)
estade@chromium.org
> Do you care that the image will be scaled for non-100% device scale factors?
It isn't supposed to be scaled, but I am currently getting the expected unscaled
size back on a 3.5 pixel scale device when decoding. This is probably since I'm
directly decoding the raw resource, I need raw pixels for use as a GL texture.
If there's a more recommended way to do that, please let me know.
billorr@chromium.org changed reviewers: + billorr@chromium.org
I'm just learning the code review tools https://codereview.chromium.org/2363553003/diff/100001/chrome/browser/android... File chrome/browser/android/vr_shell/vr_shell.cc (right): https://codereview.chromium.org/2363553003/diff/100001/chrome/browser/android... chrome/browser/android/vr_shell/vr_shell.cc:504: webvr_warning_frames_ = warning_seconds * 60; how often do we miss frames? can we keep the warning up based on seconds instead of frames? https://codereview.chromium.org/2363553003/diff/100001/chrome/browser/android... File chrome/browser/android/vr_shell/vr_shell_renderer.cc (right): https://codereview.chromium.org/2363553003/diff/100001/chrome/browser/android... chrome/browser/android/vr_shell/vr_shell_renderer.cc:479: std::vector<unsigned char> data(data_str.begin(), data_str.end()); this is doing a copy, right? do we need to copy? https://codereview.chromium.org/2363553003/diff/100001/chrome/browser/android... chrome/browser/android/vr_shell/vr_shell_renderer.cc:486: gfx::PNGCodec::Decode(&data[0], data.size(), why not data.data()? I believe our coding conventions allow it
On 2016/09/23 17:52:11, billorr wrote: > I'm just learning the code review tools > > https://codereview.chromium.org/2363553003/diff/100001/chrome/browser/android... > File chrome/browser/android/vr_shell/vr_shell.cc (right): > > https://codereview.chromium.org/2363553003/diff/100001/chrome/browser/android... > chrome/browser/android/vr_shell/vr_shell.cc:504: webvr_warning_frames_ = > warning_seconds * 60; > how often do we miss frames? can we keep the warning up based on seconds > instead of frames? Yes, that's cleaner. Updated to use timing information instead using timestamps we're already getting from GVR. > https://codereview.chromium.org/2363553003/diff/100001/chrome/browser/android... > File chrome/browser/android/vr_shell/vr_shell_renderer.cc (right): > > https://codereview.chromium.org/2363553003/diff/100001/chrome/browser/android... > chrome/browser/android/vr_shell/vr_shell_renderer.cc:479: std::vector<unsigned > char> data(data_str.begin(), data_str.end()); > this is doing a copy, right? do we need to copy? Now using data.data() without copying. > https://codereview.chromium.org/2363553003/diff/100001/chrome/browser/android... > chrome/browser/android/vr_shell/vr_shell_renderer.cc:486: > gfx::PNGCodec::Decode(&data[0], data.size(), > why not data.data()? I believe our coding conventions allow it Done.
estade@chromium.org changed reviewers: + dbeam@chromium.org
+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. > > Do you care that the image will be scaled for non-100% device scale factors? > > It isn't supposed to be scaled, but I am currently getting the expected unscaled > size back on a 3.5 pixel scale device when decoding. This is probably since I'm > directly decoding the raw resource, I need raw pixels for use as a GL texture. > If there's a more recommended way to do that, please let me know. so the text is super tiny on a 3.5x scale device, or super huge on a 1x device? You can get different raw pixels for different scale factors if you add several versions (default_100_percent/, default_200_percent/, etc.) and use GetRawDataResourceForScale. But you'd still need to scale in your code because we won't have a specific version for every imaginable scale factor. How far off is that prereq patch? Might be better to wait for it.
On 2016/09/23 20:13:00, Evan Stade wrote: > so the text is super tiny on a 3.5x scale device, or super huge on a 1x device? This resource will be rendered in VR, which will present it at a consistent size regardless of device DPI. (In this case 0.15 meters wide at a distance of 0.7 meters) It's a significantly different rendering pipeline than traditional Chrome resources are subject to. That said, we do also intend to replace this with vector graphics in the near future for localization. That part of the rendering pipeline is simply not in place yet.
bshe@chromium.org changed reviewers: + cjgrant@chromium.org
+Chris Follow up discussion about vr shell ui here. Are you planning to use vr shell ui to get the texture or do you plan to use vr_shell ui framework to render too? If you just plan to get texture out, the TOT should support it already.
On 2016/09/23 20:13:00, Evan Stade wrote: > +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. fwiw: Evan is probably correct that the downloads-related pngs in browser_resources.grd are likely in the wrong place.
On 2016/09/26 18:26:12, Dan Beam wrote: > On 2016/09/23 20:13:00, Evan Stade wrote: > > +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. > > fwiw: Evan is probably correct that the downloads-related pngs in > browser_resources.grd are likely in the wrong place. PTAL, the missing dependency has landed and I've switched to using localized strings rendered via HTML. Dan and Evan, there's no more PNGs involved in this CL, can we deal with pre-existing PNGs as a separate issue? Open issues: - this CL depends on bajones's https://codereview.chromium.org/2361533003 as prerequisite which is still in review. (I can't find how to mark this dependency in the review tool.) - the styling isn't quite final yet for the messages, waiting for joshcarpenter@ to help me with final touches to ensure it exactly matches the images used for UI review.
https://codereview.chromium.org/2363553003/diff/180001/chrome/browser/resourc... File chrome/browser/resources/vr_shell/vr_shell_ui.html (right): https://codereview.chromium.org/2363553003/diff/180001/chrome/browser/resourc... chrome/browser/resources/vr_shell/vr_shell_ui.html:71: "><span>ⓘ</span> $i18n{insecureWebVrContentPermanent}</span></div> can you put these style="" attributes into a <style> tag instead? <style> #not-secure-verbose { background: transparent; position: absolute; } ... </style> https://codereview.chromium.org/2363553003/diff/180001/chrome/browser/ui/webu... File chrome/browser/ui/webui/vr_shell/vr_shell_ui_ui.cc (right): https://codereview.chromium.org/2363553003/diff/180001/chrome/browser/ui/webu... chrome/browser/ui/webui/vr_shell/vr_shell_ui_ui.cc:185: // DisableI18nAndUseGzipForAllPaths() comments. yeah, i'll fix this eventually
On 2016/09/27 20:13:57, klausw wrote: > On 2016/09/26 18:26:12, Dan Beam wrote: > > On 2016/09/23 20:13:00, Evan Stade wrote: > > > +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. > > > > fwiw: Evan is probably correct that the downloads-related pngs in > > browser_resources.grd are likely in the wrong place. > > PTAL, the missing dependency has landed and I've switched to using localized > strings rendered via HTML. > > Dan and Evan, there's no more PNGs involved in this CL, can we deal with > pre-existing PNGs as a separate issue? I'd say yes
On 2016/09/27 20:28:44, Dan Beam wrote: > https://codereview.chromium.org/2363553003/diff/180001/chrome/browser/resourc... > File chrome/browser/resources/vr_shell/vr_shell_ui.html (right): > > https://codereview.chromium.org/2363553003/diff/180001/chrome/browser/resourc... > chrome/browser/resources/vr_shell/vr_shell_ui.html:71: "><span>ⓘ</span> > $i18n{insecureWebVrContentPermanent}</span></div> > can you put these style="" attributes into a <style> tag instead? > > <style> > #not-secure-verbose { > background: transparent; > position: absolute; > } > ... > </style> I've moved the style settings to vr_shell_ui.css. > https://codereview.chromium.org/2363553003/diff/180001/chrome/browser/ui/webu... > File chrome/browser/ui/webui/vr_shell/vr_shell_ui_ui.cc (right): > > https://codereview.chromium.org/2363553003/diff/180001/chrome/browser/ui/webu... > chrome/browser/ui/webui/vr_shell/vr_shell_ui_ui.cc:185: // > DisableI18nAndUseGzipForAllPaths() comments. > yeah, i'll fix this eventually Thanks!
Description was changed from ========== WebVR: implement insecure content warning display On WebVR pages loaded over insecure transport, show a small "Not secure" warning overlay permanently, and a transient more verbose warning for a short time on first entering the page. It uses the information from bajones's "Expose secure origin state to WebVR renderer" implementation. This is intended to use localized strings added separately to generated_resources.grd, but we can't currently render text in GL due to a not-yet-landed prerequisite patch. As a workaround, this patch uses PNG files from browser resources. These bitmaps will be removed once we have text drawing available. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== WebVR: implement insecure content warning display On WebVR pages loaded over insecure transport, show a small "Not secure" warning overlay permanently, and a transient more verbose warning for a short time on first entering the page. It uses the information from bajones's "Expose secure origin state to WebVR renderer" implementation. BUG=2363553003 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== WebVR: implement insecure content warning display On WebVR pages loaded over insecure transport, show a small "Not secure" warning overlay permanently, and a transient more verbose warning for a short time on first entering the page. It uses the information from bajones's "Expose secure origin state to WebVR renderer" implementation. BUG=2363553003 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== WebVR: implement insecure content warning display On WebVR pages loaded over insecure transport, show a small "Not secure" warning overlay permanently, and a transient more verbose warning for a short time on first entering the page. It uses the information from bajones's "Expose secure origin state to WebVR renderer" implementation. The message strings were previously added to chrome/app/generated_resources.grd in https://codereview.chromium.org/2368443002/ BUG=2363553003 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
do you have any mocks for this UI? https://codereview.chromium.org/2363553003/diff/200001/chrome/browser/browser... File chrome/browser/browser_resources.grd (right): https://codereview.chromium.org/2363553003/diff/200001/chrome/browser/browser... chrome/browser/browser_resources.grd:208: <include name="IDR_VR_SHELL_UI_HTML" file="resources\vr_shell\vr_shell_ui.html" allowexternalscript="true" type="BINDATA" /> i think allowexternalscript only has an effect when flattenhtml="true", so it can now be removed https://codereview.chromium.org/2363553003/diff/200001/chrome/browser/browser... chrome/browser/browser_resources.grd:208: <include name="IDR_VR_SHELL_UI_HTML" file="resources\vr_shell\vr_shell_ui.html" allowexternalscript="true" type="BINDATA" /> I think if you remove flattenhtml="true", the <if> tags in the .html file will no longer be processed https://codereview.chromium.org/2363553003/diff/200001/chrome/browser/resourc... File chrome/browser/resources/vr_shell/vr_shell_ui.css (right): https://codereview.chromium.org/2363553003/diff/200001/chrome/browser/resourc... chrome/browser/resources/vr_shell/vr_shell_ui.css:19: div.webvr-message-box { the "div" probably isn't doing much for you here https://codereview.chromium.org/2363553003/diff/200001/chrome/browser/resourc... chrome/browser/resources/vr_shell/vr_shell_ui.css:20: background: rgba(0,0,0,0); nit: this is the same as "transparent", which is arguably easier to reason about. why are you setting a background to transparent? isn't that the effective default? https://codereview.chromium.org/2363553003/diff/200001/chrome/browser/resourc... chrome/browser/resources/vr_shell/vr_shell_ui.css:28: transform: translate(-50%, -50%); can you use flex box instead of left+top+position+transform? .webvr-message-box { align-items: center; display: flex; justify-content: center; } might do the trick for the contents to be vertically and horizontally centered https://codereview.chromium.org/2363553003/diff/200001/chrome/browser/resourc... chrome/browser/resources/vr_shell/vr_shell_ui.css:43: width: 251px; where are you getting this width from? a specific language? https://codereview.chromium.org/2363553003/diff/200001/chrome/browser/resourc... chrome/browser/resources/vr_shell/vr_shell_ui.css:49: font-size: 15px; you should use em or % for font-size measurements so it scales if users change their default font size in settings https://codereview.chromium.org/2363553003/diff/200001/chrome/browser/resourc... File chrome/browser/resources/vr_shell/vr_shell_ui.html (right): https://codereview.chromium.org/2363553003/diff/200001/chrome/browser/resourc... chrome/browser/resources/vr_shell/vr_shell_ui.html:5: --> why is this comment here? this is not common for .html files and might mess with <!DOCTYPE html> (if it's not the first line) https://codereview.chromium.org/2363553003/diff/200001/chrome/browser/resourc... chrome/browser/resources/vr_shell/vr_shell_ui.html:14: <meta http-equiv="pragma" content="no-cache"> why are you setting this caching-related <meta> tags? https://codereview.chromium.org/2363553003/diff/200001/chrome/browser/resourc... chrome/browser/resources/vr_shell/vr_shell_ui.html:17: <link rel="stylesheet" href="vr_shell_ui.css"> can you pull in text_defaults.css or text_defaults_md.css (if material design) to use the same font as all other places in chrome? https://codereview.chromium.org/2363553003/diff/200001/chrome/browser/resourc... chrome/browser/resources/vr_shell/vr_shell_ui.html:21: <div id="webvr-not-secure-transient" class="webvr-message-box"><div class="webvr-center"><span class="webvr-center">$i18n{insecureWebVrContentTransient}</span></div></div></body> we try to wrap at 80 columns in HTML as well https://codereview.chromium.org/2363553003/diff/200001/chrome/browser/ui/webu... File chrome/browser/ui/webui/vr_shell/vr_shell_ui_ui.cc (right): https://codereview.chromium.org/2363553003/diff/200001/chrome/browser/ui/webu... chrome/browser/ui/webui/vr_shell/vr_shell_ui_ui.cc:198: VrShellUIUI::VrShellUIUI(content::WebUI* web_ui) : WebUIController(web_ui) { UIUI? :(
can you update the BUG= line as well?
On 2016/09/27 22:33:40, Dan Beam wrote: > do you have any mocks for this UI? It's a bit tricky since it's intended for rendering in VR. Here's what the elements look like in isolation: https://screenshot.googleplex.com/zu40AwBNOzj In VR mode it looks something like this: https://screenshot.googleplex.com/HF8vecrAZqD . Please ignore the extra white borders, there's a known issue that transparency isn't working right yet for the floating overlays. > > https://codereview.chromium.org/2363553003/diff/200001/chrome/browser/browser... > File chrome/browser/browser_resources.grd (right): > > https://codereview.chromium.org/2363553003/diff/200001/chrome/browser/browser... > chrome/browser/browser_resources.grd:208: <include name="IDR_VR_SHELL_UI_HTML" > file="resources\vr_shell\vr_shell_ui.html" allowexternalscript="true" > type="BINDATA" /> > i think allowexternalscript only has an effect when flattenhtml="true", so it > can now be removed I added flattenhtml="true" back, I missed the "if" below which needs it. > https://codereview.chromium.org/2363553003/diff/200001/chrome/browser/browser... > chrome/browser/browser_resources.grd:208: <include name="IDR_VR_SHELL_UI_HTML" > file="resources\vr_shell\vr_shell_ui.html" allowexternalscript="true" > type="BINDATA" /> > I think if you remove flattenhtml="true", the <if> tags in the .html file will > no longer be processed Thanks for spotting it, re-added. > https://codereview.chromium.org/2363553003/diff/200001/chrome/browser/resourc... > File chrome/browser/resources/vr_shell/vr_shell_ui.css (right): > > https://codereview.chromium.org/2363553003/diff/200001/chrome/browser/resourc... > chrome/browser/resources/vr_shell/vr_shell_ui.css:19: div.webvr-message-box { > the "div" probably isn't doing much for you here There's two levels of centering here. The text is supposed to be vertically centered within the surrounding gray semitransparent "toast" box, and that toast is vertically centered within the fixed-size surrounding div. I couldn't get it working without two levels of nesting, though it now looks a bit cleaner by using your "flex" suggestion. > https://codereview.chromium.org/2363553003/diff/200001/chrome/browser/resourc... > chrome/browser/resources/vr_shell/vr_shell_ui.css:20: background: rgba(0,0,0,0); > nit: this is the same as "transparent", which is arguably easier to reason > about. > > why are you setting a background to transparent? isn't that the effective > default? Changed to "transparent". I felt it's clearer to be explicit here since the UI basically gets sliced apart into rectangles that then get rendered as floating elements in VR. > https://codereview.chromium.org/2363553003/diff/200001/chrome/browser/resourc... > chrome/browser/resources/vr_shell/vr_shell_ui.css:28: transform: translate(-50%, > -50%); > can you use flex box instead of left+top+position+transform? > > .webvr-message-box { > align-items: center; > display: flex; > justify-content: center; > } > > might do the trick for the contents to be vertically and horizontally centered That works, thanks! > https://codereview.chromium.org/2363553003/diff/200001/chrome/browser/resourc... > chrome/browser/resources/vr_shell/vr_shell_ui.css:43: width: 251px; > where are you getting this width from? a specific language? I added comments (basically it's the width minus padding), and changed the sizing to use min/max dimensions where appropriate to be more accommodating to differently sized content strings. > https://codereview.chromium.org/2363553003/diff/200001/chrome/browser/resourc... > chrome/browser/resources/vr_shell/vr_shell_ui.css:49: font-size: 15px; > you should use em or % for font-size measurements so it scales if users change > their default font size in settings No, this needs to be pixel based since it's used as the source of a GL texture with pixel-based dimensions. The result is rendered in VR as floating billboards, the final sizing is based on dimensions in meters so that it's device independent. The user font size preferences can't currently affect this, but the effective text size should be large enough to be easily readable. > https://codereview.chromium.org/2363553003/diff/200001/chrome/browser/resourc... > File chrome/browser/resources/vr_shell/vr_shell_ui.html (right): > > https://codereview.chromium.org/2363553003/diff/200001/chrome/browser/resourc... > chrome/browser/resources/vr_shell/vr_shell_ui.html:5: --> > why is this comment here? this is not common for .html files and might mess > with <!DOCTYPE html> (if it's not the first line) Removed the comments. > https://codereview.chromium.org/2363553003/diff/200001/chrome/browser/resourc... > chrome/browser/resources/vr_shell/vr_shell_ui.html:14: <meta http-equiv="pragma" > content="no-cache"> > why are you setting this caching-related <meta> tags? That wasn't me. Development versions of this code supported hosting the UI pages on a webserver for quick iteration while changing designs, I assume this was left over from that use case. > https://codereview.chromium.org/2363553003/diff/200001/chrome/browser/resourc... > chrome/browser/resources/vr_shell/vr_shell_ui.html:17: <link rel="stylesheet" > href="vr_shell_ui.css"> > can you pull in text_defaults.css or text_defaults_md.css (if material design) > to use the same font as all other places in chrome? The text is only shown in VR mode, so it doesn't appear alongside regular Chrome content, and the scaling / sizing has different requirements than screen viewing. (For example, resolution is based on pixels-per-degree, and this is far lower than typical monitors, so text needs to be quite large to be readable.. Also, Daydream team is developing their own styling for VR content, and AFAIK there's a goal to stay aligned with their styling, but I think that's not quite ready for internal sharing yet. For now I think it would be clearer to keep it separate. > https://codereview.chromium.org/2363553003/diff/200001/chrome/browser/resourc... > chrome/browser/resources/vr_shell/vr_shell_ui.html:21: <div > id="webvr-not-secure-transient" class="webvr-message-box"><div > class="webvr-center"><span > class="webvr-center">$i18n{insecureWebVrContentTransient}</span></div></div></body> > we try to wrap at 80 columns in HTML as well Done. > https://codereview.chromium.org/2363553003/diff/200001/chrome/browser/ui/webu... > File chrome/browser/ui/webui/vr_shell/vr_shell_ui_ui.cc (right): > > https://codereview.chromium.org/2363553003/diff/200001/chrome/browser/ui/webu... > chrome/browser/ui/webui/vr_shell/vr_shell_ui_ui.cc:198: > VrShellUIUI::VrShellUIUI(content::WebUI* web_ui) : WebUIController(web_ui) { > UIUI? :( Also pre-existing - it's following conventions according to mthiesse@. The HTML page provides UI elements for the VrShell browsing mode, and apparently the rule is to add an extra "UI" suffix for a wrapper class that provides the underlying content.
On 2016/09/27 22:34:18, Dan Beam wrote: > can you update the BUG= line as well? Done.
https://codereview.chromium.org/2363553003/diff/220001/chrome/browser/android... File chrome/browser/android/vr_shell/vr_shell.cc (right): https://codereview.chromium.org/2363553003/diff/220001/chrome/browser/android... chrome/browser/android/vr_shell/vr_shell.cc:468: Rectf copy_rect = {256.f / ui_tex_width_, 0.f / ui_tex_height_, Could you move these coordinates (here and below) to constants up at the top of the file? This will naturally break any time the HTML UI is changed. Optionally, I'd adding a private helper method on this class. Something like: Rectf copy_rect = MakeUiCopyRect(pixel_x, pixel_y, width, height), to handle int-to-float casting and division. https://codereview.chromium.org/2363553003/diff/220001/chrome/browser/ui/webu... File chrome/browser/ui/webui/vr_shell/vr_shell_ui_ui.cc (right): https://codereview.chromium.org/2363553003/diff/220001/chrome/browser/ui/webu... chrome/browser/ui/webui/vr_shell/vr_shell_ui_ui.cc:183: // We're localizing strings, so we can't use gzip since it's Line length and wrapping look wrong here.
On 2016/09/28 14:57:52, cjgrant wrote: > https://codereview.chromium.org/2363553003/diff/220001/chrome/browser/android... > File chrome/browser/android/vr_shell/vr_shell.cc (right): > > https://codereview.chromium.org/2363553003/diff/220001/chrome/browser/android... > chrome/browser/android/vr_shell/vr_shell.cc:468: Rectf copy_rect = {256.f / > ui_tex_width_, 0.f / ui_tex_height_, > Could you move these coordinates (here and below) to constants up at the top of > the file? This will naturally break any time the HTML UI is changed. > > Optionally, I'd adding a private helper method on this class. Something like: > > Rectf copy_rect = MakeUiCopyRect(pixel_x, pixel_y, width, height), > > to handle int-to-float casting and division. Done, using Recti constants so that there's just a single constant per rectangle. Having separate int constants for x/y/width/height would be tedious. > > https://codereview.chromium.org/2363553003/diff/220001/chrome/browser/ui/webu... > File chrome/browser/ui/webui/vr_shell/vr_shell_ui_ui.cc (right): > > https://codereview.chromium.org/2363553003/diff/220001/chrome/browser/ui/webu... > chrome/browser/ui/webui/vr_shell/vr_shell_ui_ui.cc:183: // We're localizing > strings, so we can't use gzip since it's > Line length and wrapping look wrong here. Fixed.
lgtm https://codereview.chromium.org/2363553003/diff/240001/chrome/browser/android... File chrome/browser/android/vr_shell/vr_shell.cc (right): https://codereview.chromium.org/2363553003/diff/240001/chrome/browser/android... chrome/browser/android/vr_shell/vr_shell.cc:340: Rectf copy_rect = {(float)pixel_rect.x / ui_tex_width_, nits: - Should use C++ casts as per style guide. - Can probably lost the intermediate variable, return Rectf({...}), if you wish. https://codereview.chromium.org/2363553003/diff/240001/chrome/browser/android... chrome/browser/android/vr_shell/vr_shell.cc:495: MakeUiPixelCopyRect(kWebVrWarningPermanentRect)); This isn't making a pixel copy-rect, it's making a GL copy-rect. Shouldn't it be MakeUiGlCopyRect?
On 2016/09/28 17:48:04, cjgrant wrote: > lgtm > > https://codereview.chromium.org/2363553003/diff/240001/chrome/browser/android... > File chrome/browser/android/vr_shell/vr_shell.cc (right): > > https://codereview.chromium.org/2363553003/diff/240001/chrome/browser/android... > chrome/browser/android/vr_shell/vr_shell.cc:340: Rectf copy_rect = > {(float)pixel_rect.x / ui_tex_width_, > nits: > - Should use C++ casts as per style guide. > - Can probably lost the intermediate variable, return Rectf({...}), if you wish. Done. (My C++ is rusty, thanks for the pointers!) > https://codereview.chromium.org/2363553003/diff/240001/chrome/browser/android... > chrome/browser/android/vr_shell/vr_shell.cc:495: > MakeUiPixelCopyRect(kWebVrWarningPermanentRect)); > This isn't making a pixel copy-rect, it's making a GL copy-rect. Shouldn't it > be MakeUiGlCopyRect? Agree, done.
On 2016/09/27 23:49:42, klausw wrote: > On 2016/09/27 22:34:18, Dan Beam wrote: > > can you update the BUG= line as well? > > Done. where was this done? 2363553003 isn't showing much on any tracker i know of
Description was changed from ========== WebVR: implement insecure content warning display On WebVR pages loaded over insecure transport, show a small "Not secure" warning overlay permanently, and a transient more verbose warning for a short time on first entering the page. It uses the information from bajones's "Expose secure origin state to WebVR renderer" implementation. The message strings were previously added to chrome/app/generated_resources.grd in https://codereview.chromium.org/2368443002/ BUG=2363553003 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== WebVR: implement insecure content warning display On WebVR pages loaded over insecure transport, show a small "Not secure" warning overlay permanently, and a transient more verbose warning for a short time on first entering the page. It uses the information from bajones's "Expose secure origin state to WebVR renderer" implementation. The message strings were previously added to chrome/app/generated_resources.grd in https://codereview.chromium.org/2368443002/ BUG=389343 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
On 2016/09/29 03:06:41, Dan Beam wrote: > On 2016/09/27 23:49:42, klausw wrote: > > On 2016/09/27 22:34:18, Dan Beam wrote: > > > can you update the BUG= line as well? > > > > Done. > > where was this done? 2363553003 isn't showing much on any tracker i know of Sorry, this hadn't worked right due to a cut&paste error, BUG= was pointing back to this code review. I've now set it to point to https://bugs.chromium.org/p/chromium/issues/detail?id=389343 "Implement WebVR for Android", same as the prerequisite issue https://codereview.chromium.org/2374153002/ "Expose secure origin state to WebVR renderer".
https://codereview.chromium.org/2363553003/diff/340001/chrome/browser/android... File chrome/browser/android/vr_shell/vr_shell.cc (right): https://codereview.chromium.org/2363553003/diff/340001/chrome/browser/android... chrome/browser/android/vr_shell/vr_shell.cc:382: */ nit: move comment to .h file? Also, this seems like java style comment https://codereview.chromium.org/2363553003/diff/340001/chrome/browser/android... chrome/browser/android/vr_shell/vr_shell.cc:491: // with head rotations. nit: this looks like comment for function. If so, move them to .h file? Also wrap when you are about exceed 80 characters. https://codereview.chromium.org/2363553003/diff/340001/chrome/browser/resourc... File chrome/browser/resources/vr_shell/vr_shell_ui.css (right): https://codereview.chromium.org/2363553003/diff/340001/chrome/browser/resourc... chrome/browser/resources/vr_shell/vr_shell_ui.css:23: flex-direction: column; why do you want to use column here. From the screenshot, it looks like you want to use row? https://codereview.chromium.org/2363553003/diff/340001/chrome/browser/resourc... chrome/browser/resources/vr_shell/vr_shell_ui.css:37: float: left; this is probably not going to work well with right-to-left language. If you use flex-direction to layout it, float shouldn't be necessary? https://codereview.chromium.org/2363553003/diff/340001/chrome/browser/ui/webu... File chrome/browser/ui/webui/vr_shell/vr_shell_ui_ui.cc (right): https://codereview.chromium.org/2363553003/diff/340001/chrome/browser/ui/webu... chrome/browser/ui/webui/vr_shell/vr_shell_ui_ui.cc:184: // i18n. See WebUIDataSource's DisableI18nAndUseGzipForAllPaths() comments. The above comment seems more relevant to the change you made in browser_resources.grd, where you actually removed gzip tag?
On 2016/09/29 13:50:56, bshe wrote: > https://codereview.chromium.org/2363553003/diff/340001/chrome/browser/android... > File chrome/browser/android/vr_shell/vr_shell.cc (right): > > https://codereview.chromium.org/2363553003/diff/340001/chrome/browser/android... > chrome/browser/android/vr_shell/vr_shell.cc:382: */ > nit: move comment to .h file? Also, this seems like java style comment Done. > https://codereview.chromium.org/2363553003/diff/340001/chrome/browser/android... > chrome/browser/android/vr_shell/vr_shell.cc:491: // with head rotations. > nit: this looks like comment for function. If so, move them to .h file? I think it makes more sense here since it's more of an explanation of the implementation as opposed to something that callers of the function need to know. > Also wrap when you are about exceed 80 characters. Done. > https://codereview.chromium.org/2363553003/diff/340001/chrome/browser/resourc... > File chrome/browser/resources/vr_shell/vr_shell_ui.css (right): > > https://codereview.chromium.org/2363553003/diff/340001/chrome/browser/resourc... > chrome/browser/resources/vr_shell/vr_shell_ui.css:23: flex-direction: column; > why do you want to use column here. From the screenshot, it looks like you want > to use row? No, column is correct, the content needs to be arranged vertically. > https://codereview.chromium.org/2363553003/diff/340001/chrome/browser/resourc... > chrome/browser/resources/vr_shell/vr_shell_ui.css:37: float: left; > this is probably not going to work well with right-to-left language. If you use > flex-direction to layout it, float shouldn't be necessary? Fixed, I've refactored the CSS to be symmetrical and tested with temporary "direction: rtl" to confirm that the icon then appears on the right with appropriate margins. > https://codereview.chromium.org/2363553003/diff/340001/chrome/browser/ui/webu... > File chrome/browser/ui/webui/vr_shell/vr_shell_ui_ui.cc (right): > > https://codereview.chromium.org/2363553003/diff/340001/chrome/browser/ui/webu... > chrome/browser/ui/webui/vr_shell/vr_shell_ui_ui.cc:184: // i18n. See > WebUIDataSource's DisableI18nAndUseGzipForAllPaths() comments. > The above comment seems more relevant to the change you made in > browser_resources.grd, where you actually removed gzip tag? There are currently no comments in browser_resources.grd, so it would look out of place there. I've converted the comment to a TODO that more specifically refers to the implementation here. I can also just remove it if you don't think it's helpful.
I've rebased to current origin/master now that the prerequisite patch https://codereview.chromium.org/2374153002/ is committed. dbeam@ or estade@, I still need OWNERS review for the browser_resources.grd change. It's just removing compress=gzip for input files that are now using l10n. (Earlier changes involving new PNG resources have been reverted.)
The CQ bit was checked by klausw@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 2016/09/29 19:00:33, klausw wrote: > I've rebased to current origin/master now that the prerequisite patch > https://codereview.chromium.org/2374153002/ is committed. > > dbeam@ or estade@, I still need OWNERS review for the browser_resources.grd > change. It's just removing compress=gzip for input files that are now using > l10n. (Earlier changes involving new PNG resources have been reverted.) don't you also need owners for chrome/browser/resources/?
On 2016/09/29 19:03:15, Dan Beam wrote: > On 2016/09/29 19:00:33, klausw wrote: > > I've rebased to current origin/master now that the prerequisite patch > > https://codereview.chromium.org/2374153002/ is committed. > > > > dbeam@ or estade@, I still need OWNERS review for the browser_resources.grd > > change. It's just removing compress=gzip for input files that are now using > > l10n. (Earlier changes involving new PNG resources have been reverted.) > > don't you also need owners for chrome/browser/resources/? Ah, I do indeed. I was under the mistaken impression that hoverboard team had OWNERS permission for the resources/vr_shell/ subdirectory, sorry.
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/09/29 19:39:59, bshe wrote: > lgtm Rebased to ToT origin/master. Transparency support has landed, I fixed a missing background color that became visible due to that.
mthiesse@chromium.org changed reviewers: + mthiesse@chromium.org
https://codereview.chromium.org/2363553003/diff/420001/chrome/browser/android... File chrome/browser/android/vr_shell/vr_shell.cc (right): https://codereview.chromium.org/2363553003/diff/420001/chrome/browser/android... chrome/browser/android/vr_shell/vr_shell.cc:398: DCHECK(IsUiTextureReady()); This is for a security warning, so CHECK is probably warranted over DCHECK. https://codereview.chromium.org/2363553003/diff/420001/chrome/browser/android... chrome/browser/android/vr_shell/vr_shell.cc:546: kWebVrWarningPermanentRect.width / 1000.f * kWebVrWarningDistance; What is this 1000.0f factor? https://codereview.chromium.org/2363553003/diff/420001/chrome/browser/android... chrome/browser/android/vr_shell/vr_shell.cc:626: const int64_t warning_seconds = 30; Move constant to top of file. https://codereview.chromium.org/2363553003/diff/420001/chrome/browser/android... chrome/browser/android/vr_shell/vr_shell.cc:628: webvr_warning_end_nanos_ = now + warning_seconds * 1000 * 1000 * 1000; Extract constant for seconds to nanos.
Description was changed from ========== WebVR: implement insecure content warning display On WebVR pages loaded over insecure transport, show a small "Not secure" warning overlay permanently, and a transient more verbose warning for a short time on first entering the page. It uses the information from bajones's "Expose secure origin state to WebVR renderer" implementation. The message strings were previously added to chrome/app/generated_resources.grd in https://codereview.chromium.org/2368443002/ BUG=389343 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== WebVR: implement insecure content warning display On WebVR pages loaded over insecure transport, show a small "Not secure" warning overlay permanently, and a transient more verbose warning for a short time on first entering the page. It uses the information from bajones's "Expose secure origin state to WebVR renderer" implementation. The message strings were previously added to chrome/app/generated_resources.grd in https://codereview.chromium.org/2368443002/ BUG=389343,644492 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
re: The text is only shown in VR mode, so it doesn't appear alongside regular Chrome content, and the scaling / sizing has different requirements than screen viewing. (For example, resolution is based on pixels-per-degree, and this is far lower than typical monitors, so text needs to be quite large to be readable.. Also, Daydream team is developing their own styling for VR content, and AFAIK there's a goal to stay aligned with their styling, but I think that's not quite ready for internal sharing yet. For now I think it would be clearer to keep it separate. I think we can only act on what we know now. Can you point me to the designer or PM that thinks different parts of Chrome should use different fonts? Because as far as I can tell, everyyyybody I've talked to in 5 years of UI development seems to think the exact opposite: that we should be more (not less) consistent with ourselves ;). additionally, if you're trying to minimize review latency, it will save me time if you comment in the review tool in the diff (double click to add a comment, or just click "Reply") rather than responding to all comments at once, inline (like an email). https://codereview.chromium.org/2363553003/diff/300001/chrome/browser/resourc... File chrome/browser/resources/vr_shell/vr_shell_ui.css (right): https://codereview.chromium.org/2363553003/diff/300001/chrome/browser/resourc... chrome/browser/resources/vr_shell/vr_shell_ui.css:39: margin: 20px 0 20px 20px; do you support RTL? because if so, this margin wont work https://codereview.chromium.org/2363553003/diff/420001/chrome/browser/resourc... File chrome/browser/resources/vr_shell/vr_shell_ui.css (right): https://codereview.chromium.org/2363553003/diff/420001/chrome/browser/resourc... chrome/browser/resources/vr_shell/vr_shell_ui.css:7: font-family: 'Roboto', sans-serif; can you use text_defaults.css or text_defaults_md.css? this will not match the rest of chrome in many cases. the Roboto font often does not exist as a font on users' platforms, and it takes a little bit of magic to load Chrome's copy and get different correct rendering in different weights, and also CJK support in Roboto is bad and the fallback font used by Chrome different per-platform. tl;dr - if you slap a <link rel="import" href="chrome://resources/css/text_defaults_md.css"> into vr_shell_ui.html like I mentioned earlier on, it should just work https://codereview.chromium.org/2363553003/diff/420001/chrome/browser/resourc... chrome/browser/resources/vr_shell/vr_shell_ui.css:10: .ui-button { this is a reallly generic name, so it's hard to understand where this is used or if it's duplicative with all the other code you're adding https://codereview.chromium.org/2363553003/diff/420001/chrome/browser/resourc... chrome/browser/resources/vr_shell/vr_shell_ui.css:21: background: transparent; why do you need this background: rule? https://codereview.chromium.org/2363553003/diff/420001/chrome/browser/resourc... chrome/browser/resources/vr_shell/vr_shell_ui.css:24: font-weight: 400; which font-weight are you overriding? note, 400 = normal https://codereview.chromium.org/2363553003/diff/420001/chrome/browser/resourc... chrome/browser/resources/vr_shell/vr_shell_ui.css:30: height: 128px; what's the natural size of the svg? https://codereview.chromium.org/2363553003/diff/420001/chrome/browser/resourc... chrome/browser/resources/vr_shell/vr_shell_ui.css:31: left: 0; i noticed you mentioned RTL earlier: are you sure using left: but not right: works in RTL? is this taking up the entire width of its parent? https://codereview.chromium.org/2363553003/diff/420001/chrome/browser/resourc... chrome/browser/resources/vr_shell/vr_shell_ui.css:44: margin: 20px 10.5px; why .5px? https://codereview.chromium.org/2363553003/diff/420001/chrome/browser/resourc... chrome/browser/resources/vr_shell/vr_shell_ui.css:51: box-shadow: 0 0 20px rgba(0,0,0,0.5); spaces after commas (i.e. rgba(0, 0, 0, 0.5);) https://codereview.chromium.org/2363553003/diff/420001/chrome/browser/resourc... chrome/browser/resources/vr_shell/vr_shell_ui.css:73: box-sizing: border-box; why do you need to change the box-sizing? https://codereview.chromium.org/2363553003/diff/420001/chrome/browser/resourc... chrome/browser/resources/vr_shell/vr_shell_ui.css:84: width: 512px; i don't understand where any of these widths come from. also, with such a small max/min width compared to a larger width, it seems like some of these styles are duplicative https://codereview.chromium.org/2363553003/diff/420001/chrome/browser/resourc... File chrome/browser/resources/vr_shell/vr_shell_ui.html (right): https://codereview.chromium.org/2363553003/diff/420001/chrome/browser/resourc... chrome/browser/resources/vr_shell/vr_shell_ui.html:22: <svg class="webvr-not-secure-icon" width="36px" height="36px" can we just use a background-image on #webvr-not-secure-permanent? https://codereview.chromium.org/2363553003/diff/420001/chrome/browser/resourc... chrome/browser/resources/vr_shell/vr_shell_ui.html:34: </svg> don't embed an svg directly https://codereview.chromium.org/2363553003/diff/420001/chrome/browser/resourc... chrome/browser/resources/vr_shell/vr_shell_ui.html:34: </svg> what does this look like? there's likely an existing .svg that you can re-use https://codereview.chromium.org/2363553003/diff/420001/chrome/browser/resourc... chrome/browser/resources/vr_shell/vr_shell_ui.html:39: <div> why do you need this outer <div>?
https://codereview.chromium.org/2363553003/diff/420001/chrome/browser/resourc... File chrome/browser/resources/vr_shell/vr_shell_ui.css (right): https://codereview.chromium.org/2363553003/diff/420001/chrome/browser/resourc... chrome/browser/resources/vr_shell/vr_shell_ui.css:71: background-color: rgba(26, 26, 26, 0.8); also note that this UI looks very much like a material design tooltip https://material.google.com/components/tooltips.html in which case you should be using 90% opacity and "grey 700" which can be accessed via var(--google-grey-700) if you import chrome://resources/polymer/v1_0/components-chromium/paper-styles/color.html or duplicate the equivalent of #616161 (or rgb(97, 97, 97)). https://material.google.com/style/color.html note that this is not the same as rgba(97, 97, 97, .9) as the text is rendered in different opacity
dbeam@ wrote: > I think we can only act on what we know now. Can you point me to the designer > or PM that thinks different parts of Chrome should use different fonts? Because > as far as I can tell, everyyyybody I've talked to in 5 years of UI development > seems to think the exact opposite: that we should be more (not less) consistent > with ourselves ;). Hopefully this is largely moot now due to including the text_defaults_mdd.css file. The point I was trying to make was that VR display is basically a different display device with its own unique characteristics such as very low angular pixel density, so even though it may be running on a smartphone it isn't necessarily appropriate to match Chrome's styling for a phone display if that would interfere with legibility. Daydream team is working on establishing best practices for Google's VR apps, and this is somewhat independent from Chrome at this point. > additionally, if you're trying to minimize review latency, it will save me time > if you comment in the review tool in the diff (double click to add a comment, or > just click "Reply") rather than responding to all comments at once, inline (like an email). I'm sorry, I had misunderstood how the tool works. Is this better? https://codereview.chromium.org/2363553003/diff/300001/chrome/browser/resourc... File chrome/browser/resources/vr_shell/vr_shell_ui.css (right): https://codereview.chromium.org/2363553003/diff/300001/chrome/browser/resourc... chrome/browser/resources/vr_shell/vr_shell_ui.css:39: margin: 20px 0 20px 20px; On 2016/10/03 18:12:48, Dan Beam wrote: > do you support RTL? because if so, this margin wont work Please see the current version of this file, this is now "margin: 20px 0;" as of patchset 19. https://codereview.chromium.org/2363553003/diff/420001/chrome/browser/resourc... File chrome/browser/resources/vr_shell/vr_shell_ui.css (right): https://codereview.chromium.org/2363553003/diff/420001/chrome/browser/resourc... chrome/browser/resources/vr_shell/vr_shell_ui.css:7: font-family: 'Roboto', sans-serif; On 2016/10/03 18:12:48, Dan Beam wrote: > can you use text_defaults.css or text_defaults_md.css? this will not match the > rest of chrome in many cases. > > the Roboto font often does not exist as a font on users' platforms, and it takes > a little bit of magic to load Chrome's copy and get different correct rendering > in different weights, and also CJK support in Roboto is bad and the fallback > font used by Chrome different per-platform. > > tl;dr - if you slap a > > <link rel="import" href="chrome://resources/css/text_defaults_md.css"> > > into vr_shell_ui.html like I mentioned earlier on, it should just work Done, and thank you. I got this working with a bit of extra plumbing. Does this look right in vr_shell_ui_ui.cc? const std::string& app_locale = g_browser_process->GetApplicationLocale(); webui::SetLoadTimeDataDefaults(app_locale, &localized_strings); https://codereview.chromium.org/2363553003/diff/420001/chrome/browser/resourc... chrome/browser/resources/vr_shell/vr_shell_ui.css:10: .ui-button { On 2016/10/03 18:12:48, Dan Beam wrote: > this is a reallly generic name, so it's hard to understand where this is used or > if it's duplicative with all the other code you're adding This CSS is for the pre-existing preliminary navigation buttons as used by VrShell (2D browsing in VR), the .js file can create buttons dynamically. The WebVR code doesn't use it. https://codereview.chromium.org/2363553003/diff/420001/chrome/browser/resourc... chrome/browser/resources/vr_shell/vr_shell_ui.css:21: background: transparent; On 2016/10/03 18:12:48, Dan Beam wrote: > why do you need this background: rule? Removed. https://codereview.chromium.org/2363553003/diff/420001/chrome/browser/resourc... chrome/browser/resources/vr_shell/vr_shell_ui.css:24: font-weight: 400; On 2016/10/03 18:12:48, Dan Beam wrote: > which font-weight are you overriding? note, 400 = normal Removed. This used to set font-weight: 300 in an earlier version, I didn't realize that the new value was the same as the default. https://codereview.chromium.org/2363553003/diff/420001/chrome/browser/resourc... chrome/browser/resources/vr_shell/vr_shell_ui.css:30: height: 128px; On 2016/10/03 18:12:48, Dan Beam wrote: > what's the natural size of the svg? The SVG is 36px x 36px and is used at that size. The dimensions here are for the overall message box which is an absolutely positioned DIV. The UI code uses fixed-sized pixel rectangles from the UI HTML page as textures for GL rendering, so it needs to know where they are placed and how big they are. https://codereview.chromium.org/2363553003/diff/420001/chrome/browser/resourc... chrome/browser/resources/vr_shell/vr_shell_ui.css:31: left: 0; On 2016/10/03 18:12:48, Dan Beam wrote: > i noticed you mentioned RTL earlier: are you sure using left: but not right: > works in RTL? is this taking up the entire width of its parent? This is intentionally text direction independent. It's an absolutely positioned fixed-size rectangle, only the content of the div needs to be RTL-compatible. https://codereview.chromium.org/2363553003/diff/420001/chrome/browser/resourc... chrome/browser/resources/vr_shell/vr_shell_ui.css:44: margin: 20px 10.5px; On 2016/10/03 18:12:48, Dan Beam wrote: > why .5px? I was trying to exactly replicate the UI review approved message image, both 10px and 11px didn't quite match. https://codereview.chromium.org/2363553003/diff/420001/chrome/browser/resourc... chrome/browser/resources/vr_shell/vr_shell_ui.css:51: box-shadow: 0 0 20px rgba(0,0,0,0.5); On 2016/10/03 18:12:48, Dan Beam wrote: > spaces after commas (i.e. rgba(0, 0, 0, 0.5);) Done. https://codereview.chromium.org/2363553003/diff/420001/chrome/browser/resourc... chrome/browser/resources/vr_shell/vr_shell_ui.css:71: background-color: rgba(26, 26, 26, 0.8); On 2016/10/03 18:19:19, Dan Beam wrote: > also note that this UI looks very much like a material design tooltip > > https://material.google.com/components/tooltips.html > > in which case you should be using 90% opacity and "grey 700" which can be > accessed via var(--google-grey-700) if you import > chrome://resources/polymer/v1_0/components-chromium/paper-styles/color.html or > duplicate the equivalent of #616161 (or rgb(97, 97, 97)). > > https://material.google.com/style/color.html > > note that this is not the same as rgba(97, 97, 97, .9) as the text is rendered > in different opacity I'll take this up with the UI designer, I'm reluctant to change it unilaterally due to the previous UI review. One possible concern is that the material design tooltip may have insufficient contrast since it's a lighter grey. https://codereview.chromium.org/2363553003/diff/420001/chrome/browser/resourc... chrome/browser/resources/vr_shell/vr_shell_ui.css:73: box-sizing: border-box; On 2016/10/03 18:12:49, Dan Beam wrote: > why do you need to change the box-sizing? AFAIK the goal here was to ensure that the rounded corners remain visible within the allotted width. This seemed less fragile than manually subtracting the padding from the width. https://codereview.chromium.org/2363553003/diff/420001/chrome/browser/resourc... chrome/browser/resources/vr_shell/vr_shell_ui.css:84: width: 512px; On 2016/10/03 18:12:48, Dan Beam wrote: > i don't understand where any of these widths come from. > > also, with such a small max/min width compared to a larger width, it seems like > some of these styles are duplicative The width: 512px matches the absolutely positioned div which is used as the source of the GL texture for rendering the message. The min and max height/width are intended to provide some flexibility for translated messages with varying text lengths. The overall space available is inflexible, but we want to make sure the message boxes are sufficiently large to be clearly visible even for short text while not being larger than necessary. https://codereview.chromium.org/2363553003/diff/420001/chrome/browser/resourc... File chrome/browser/resources/vr_shell/vr_shell_ui.html (right): https://codereview.chromium.org/2363553003/diff/420001/chrome/browser/resourc... chrome/browser/resources/vr_shell/vr_shell_ui.html:22: <svg class="webvr-not-secure-icon" width="36px" height="36px" On 2016/10/03 18:12:49, Dan Beam wrote: > can we just use a background-image on #webvr-not-secure-permanent? Would a background image work for RTL compatibility? This way, the inline-block display moves it to the right when shown alongside RTL text, see screenshot in https://bugs.chromium.org/p/chromium/issues/detail?id=644492#c9 https://codereview.chromium.org/2363553003/diff/420001/chrome/browser/resourc... chrome/browser/resources/vr_shell/vr_shell_ui.html:34: </svg> On 2016/10/03 18:12:49, Dan Beam wrote: > don't embed an svg directly Moved to ui/webui/resources/images/i_circle.svg, does that look appropriate? https://codereview.chromium.org/2363553003/diff/420001/chrome/browser/resourc... chrome/browser/resources/vr_shell/vr_shell_ui.html:34: </svg> On 2016/10/03 18:12:49, Dan Beam wrote: > what does this look like? there's likely an existing .svg that you can re-use I didn't find an existing one. It's an "i" with a circle around it to indicate an informational message. https://codereview.chromium.org/2363553003/diff/420001/chrome/browser/resourc... chrome/browser/resources/vr_shell/vr_shell_ui.html:39: <div> On 2016/10/03 18:12:49, Dan Beam wrote: > why do you need this outer <div>? This div provides the background and rounded corners and acts as a container for the text div which is vertically centered within it. Without it, I couldn't get the layout looking right while maintaining the min/max height and spacing.
https://codereview.chromium.org/2363553003/diff/460001/chrome/browser/resourc... File chrome/browser/resources/vr_shell/vr_shell_ui.css (right): https://codereview.chromium.org/2363553003/diff/460001/chrome/browser/resourc... chrome/browser/resources/vr_shell/vr_shell_ui.css:5: @import url(chrome://resources/css/text_defaults_md.css); sorry, i gave you bad advice <link rel="stylesheet" href="chrome://resources/css/text_defaults_md.css"> is what I meant. what you're doing works but it blocks this stylesheet from loading whereas turning this @import into a <link> lets things load in parallel https://codereview.chromium.org/2363553003/diff/460001/chrome/browser/resourc... chrome/browser/resources/vr_shell/vr_shell_ui.css:59: white-space: nowrap; do these widths work well for German? how does this elide? https://codereview.chromium.org/2363553003/diff/460001/chrome/browser/resourc... File chrome/browser/resources/vr_shell/vr_shell_ui.html (right): https://codereview.chromium.org/2363553003/diff/460001/chrome/browser/resourc... chrome/browser/resources/vr_shell/vr_shell_ui.html:22: <img class="webvr-not-secure-icon" width="36px" height="36px" are you sure this width/height is working? i think in width= and height= cases, you don't use "px" (i.e. height="36" width="36") https://codereview.chromium.org/2363553003/diff/460001/chrome/browser/ui/webu... File chrome/browser/ui/webui/vr_shell/vr_shell_ui_ui.cc (right): https://codereview.chromium.org/2363553003/diff/460001/chrome/browser/ui/webu... chrome/browser/ui/webui/vr_shell/vr_shell_ui_ui.cc:203: webui::SetLoadTimeDataDefaults(app_locale, &localized_strings); why are you adding this? SetLoadTimeDataDefaults() should be running through magic already https://codereview.chromium.org/2363553003/diff/460001/ui/webui/resources/web... File ui/webui/resources/webui_resources.grd (right): https://codereview.chromium.org/2363553003/diff/460001/ui/webui/resources/web... ui/webui/resources/webui_resources.grd:204: <include name="IDR_WEBUI_IMAGES_I_CIRCLE" btw, you only need to add something to this file if you address it via chrome://theme/IDR_WEBUI_IMAGES_I_CIRCLE or something or other. i don't see you doing that in this CL, so you probably can revert this file.
FYI, I had already extracted constants in patchset 22: https://codereview.chromium.org/2363553003/diff2/420001:460001/chrome/browser... https://codereview.chromium.org/2363553003/diff/420001/chrome/browser/android... File chrome/browser/android/vr_shell/vr_shell.cc (right): https://codereview.chromium.org/2363553003/diff/420001/chrome/browser/android... chrome/browser/android/vr_shell/vr_shell.cc:398: DCHECK(IsUiTextureReady()); On 2016/10/03 17:47:30, mthiesse wrote: > This is for a security warning, so CHECK is probably warranted over DCHECK. Done. Without the CHECK it would fail with a "division by zero" exception in the following lines. https://codereview.chromium.org/2363553003/diff/420001/chrome/browser/android... chrome/browser/android/vr_shell/vr_shell.cc:546: kWebVrWarningPermanentRect.width / 1000.f * kWebVrWarningDistance; On 2016/10/03 17:47:30, mthiesse wrote: > What is this 1000.0f factor? Added a comment: // The UI is designed in pixels with the assumption that 1px = 1mm at 1m // distance. Scale mm-to-m and adjust to keep the same angular size if the // distance changes. https://codereview.chromium.org/2363553003/diff/420001/chrome/browser/android... chrome/browser/android/vr_shell/vr_shell.cc:626: const int64_t warning_seconds = 30; On 2016/10/03 17:47:30, mthiesse wrote: > Move constant to top of file. Done. https://codereview.chromium.org/2363553003/diff/420001/chrome/browser/android... chrome/browser/android/vr_shell/vr_shell.cc:628: webvr_warning_end_nanos_ = now + warning_seconds * 1000 * 1000 * 1000; On 2016/10/03 17:47:30, mthiesse wrote: > Extract constant for seconds to nanos. Done.
The changes are included in patchset 30. https://codereview.chromium.org/2363553003/diff/460001/chrome/browser/resourc... File chrome/browser/resources/vr_shell/vr_shell_ui.css (right): https://codereview.chromium.org/2363553003/diff/460001/chrome/browser/resourc... chrome/browser/resources/vr_shell/vr_shell_ui.css:5: @import url(chrome://resources/css/text_defaults_md.css); On 2016/10/03 20:30:59, Dan Beam wrote: > sorry, i gave you bad advice > > <link rel="stylesheet" href="chrome://resources/css/text_defaults_md.css"> > > is what I meant. > > what you're doing works but it blocks this stylesheet from loading whereas > turning this @import into a <link> lets things load in parallel Done. https://codereview.chromium.org/2363553003/diff/460001/chrome/browser/resourc... chrome/browser/resources/vr_shell/vr_shell_ui.css:59: white-space: nowrap; On 2016/10/03 20:30:59, Dan Beam wrote: > do these widths work well for German? how does this elide? We don't have the translated strings yet. I tested manually, there's room for "Das ist sehr unsicher, Vorsicht bitte!" which would be a lot longer than the English "Not secure" message. (It'll probably be something like "Nicht sicher", the string info included a request to translators to keep it short.) In the unlikely case of too-long text, it currently simply truncates, but it'll be fairly obvious visually that the message is incomplete since it'll mash up right against the border of the element with no white padding. https://codereview.chromium.org/2363553003/diff/460001/chrome/browser/resourc... File chrome/browser/resources/vr_shell/vr_shell_ui.html (right): https://codereview.chromium.org/2363553003/diff/460001/chrome/browser/resourc... chrome/browser/resources/vr_shell/vr_shell_ui.html:22: <img class="webvr-not-secure-icon" width="36px" height="36px" On 2016/10/03 20:30:59, Dan Beam wrote: > are you sure this width/height is working? i think in width= and height= cases, > you don't use "px" (i.e. height="36" width="36") Fixed, thanks. (Chrome appears to ignore the suffix, "36" and "36px" and "36em" all look identical.) https://codereview.chromium.org/2363553003/diff/460001/chrome/browser/ui/webu... File chrome/browser/ui/webui/vr_shell/vr_shell_ui_ui.cc (right): https://codereview.chromium.org/2363553003/diff/460001/chrome/browser/ui/webu... chrome/browser/ui/webui/vr_shell/vr_shell_ui_ui.cc:203: webui::SetLoadTimeDataDefaults(app_locale, &localized_strings); On 2016/10/03 20:30:59, Dan Beam wrote: > why are you adding this? SetLoadTimeDataDefaults() should be running through > magic already Thank you, it does indeed work without it. FWIW, that's rather subtle magic, I didn't see any indication in https://cs.chromium.org/chromium/src/content/public/browser/web_ui_data_source.h that it does this, I only found it after reading the _impl file. https://codereview.chromium.org/2363553003/diff/460001/ui/webui/resources/web... File ui/webui/resources/webui_resources.grd (right): https://codereview.chromium.org/2363553003/diff/460001/ui/webui/resources/web... ui/webui/resources/webui_resources.grd:204: <include name="IDR_WEBUI_IMAGES_I_CIRCLE" On 2016/10/03 20:30:59, Dan Beam wrote: > btw, you only need to add something to this file if you address it via > chrome://theme/IDR_WEBUI_IMAGES_I_CIRCLE or something or other. i don't see you > doing that in this CL, so you probably can revert this file. Thanks, removed.
On 2016/10/03 21:28:19, klausw wrote: > The changes are included in patchset 30. Make that patchset 26, haven't gotten quite that far yet.
On 2016/10/03 21:28:19, klausw wrote: > The changes are included in patchset 30. Make that patchset 26, haven't gotten quite that far yet.
though i think some of the HTML/CSS could be refined just a little bit (right now you're doing some vertical centering with display: inline-block; and some with flex box + justify-content/align-items), this seems fine for now lgtm
The CQ bit was checked by klausw@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from cjgrant@chromium.org, bshe@chromium.org, dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/2363553003/#ps520001 (title: "Rebase to ToT (no changes to patched files)")
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
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by klausw@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== WebVR: implement insecure content warning display On WebVR pages loaded over insecure transport, show a small "Not secure" warning overlay permanently, and a transient more verbose warning for a short time on first entering the page. It uses the information from bajones's "Expose secure origin state to WebVR renderer" implementation. The message strings were previously added to chrome/app/generated_resources.grd in https://codereview.chromium.org/2368443002/ BUG=389343,644492 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== WebVR: implement insecure content warning display On WebVR pages loaded over insecure transport, show a small "Not secure" warning overlay permanently, and a transient more verbose warning for a short time on first entering the page. It uses the information from bajones's "Expose secure origin state to WebVR renderer" implementation. The message strings were previously added to chrome/app/generated_resources.grd in https://codereview.chromium.org/2368443002/ BUG=389343,644492 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Message was sent while issue was closed.
Committed patchset #27 (id:520001)
Message was sent while issue was closed.
Description was changed from ========== WebVR: implement insecure content warning display On WebVR pages loaded over insecure transport, show a small "Not secure" warning overlay permanently, and a transient more verbose warning for a short time on first entering the page. It uses the information from bajones's "Expose secure origin state to WebVR renderer" implementation. The message strings were previously added to chrome/app/generated_resources.grd in https://codereview.chromium.org/2368443002/ BUG=389343,644492 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== WebVR: implement insecure content warning display On WebVR pages loaded over insecure transport, show a small "Not secure" warning overlay permanently, and a transient more verbose warning for a short time on first entering the page. It uses the information from bajones's "Expose secure origin state to WebVR renderer" implementation. The message strings were previously added to chrome/app/generated_resources.grd in https://codereview.chromium.org/2368443002/ BUG=389343,644492 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/069c76c2ffc77d5fcebb84937e07df8e61c3ebaf Cr-Commit-Position: refs/heads/master@{#422647} ==========
Message was sent while issue was closed.
Patchset 27 (id:??) landed as https://crrev.com/069c76c2ffc77d5fcebb84937e07df8e61c3ebaf Cr-Commit-Position: refs/heads/master@{#422647} |
