Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2968143003:
Toolbar: Report offline state through icon and verbose text. (Closed)
DescriptionToolbar: Report offline state through icon and verbose text
On Android, pages may be shown from a downloaded offline cache. In this
case, the security chip text and icon change. Reflect this in
ToolbarModel.
With this new ability, render the Offline security information in VR.
BUG=735770
Review-Url: https://codereview.chromium.org/2968143003
Cr-Commit-Position: refs/heads/master@{#485520}
Committed: https://chromium.googlesource.com/chromium/src/+/1a0e2b694566154a5317f61f6d0adf521fb7f485
Patch Set 1 #Patch Set 2 : . #
Total comments: 10
Patch Set 3 : Fix typo in string description. #
Total comments: 20
Patch Set 4 : Address some initial comments. #Patch Set 5 : Fix header date. #
Total comments: 3
Messages
Total messages: 30 (13 generated)
Description was changed from ========== Toolbar: Report offline state through icon and verbose text On Android, pages may be shown from a downloaded offline cache. In this case, the security chip text and icon change. Reflect this in ToolbarModel. BUG=735770 ========== to ========== Toolbar: Report offline state through icon and verbose text On Android, pages may be shown from a downloaded offline cache. In this case, the security chip text and icon change. Reflect this in ToolbarModel. BUG=735770 ==========
Description was changed from ========== Toolbar: Report offline state through icon and verbose text On Android, pages may be shown from a downloaded offline cache. In this case, the security chip text and icon change. Reflect this in ToolbarModel. BUG=735770 ========== to ========== Toolbar: Report offline state through icon and verbose text On Android, pages may be shown from a downloaded offline cache. In this case, the security chip text and icon change. Reflect this in ToolbarModel. With this new ability, render the Offline security information in VR. BUG=735770 ==========
cjgrant@chromium.org changed reviewers: + pkasting@chromium.org
Peter, I talked to the team, and we aren't worried about the string translation deadline. Therefore, I went ahead with the better approach. I have this as a follow-on CL to adding IsOfflinePage(). If you'd like them merged into one, I can do that too.
Peter, I talked to the team, and we aren't worried about the string translation deadline. Therefore, I went ahead with the better approach. I have this as a follow-on CL to adding IsOfflinePage(). If you'd like them merged into one, I can do that too. https://codereview.chromium.org/2968143003/diff/20001/components/omnibox_stri... File components/omnibox_strings.grdp (right): https://codereview.chromium.org/2968143003/diff/20001/components/omnibox_stri... components/omnibox_strings.grdp:35: <message name="IDS_OFFLINE_VERBOSE_STATE" desc="Text for the Offline Omnibox Verbose srate. Displayed when the current page is loaded from a previously-downloaded cache."> Do the states here have to be documented somewhere else? https://codereview.chromium.org/2968143003/diff/20001/components/toolbar/BUIL... File components/toolbar/BUILD.gn (right): https://codereview.chromium.org/2968143003/diff/20001/components/toolbar/BUIL... components/toolbar/BUILD.gn:27: "offline_pin.icon", Do we need to worry about a 1x icon? I assume we'd add that if needed, later, if desktop picks this up. https://codereview.chromium.org/2968143003/diff/20001/components/toolbar/tool... File components/toolbar/toolbar_model_impl.cc (right): https://codereview.chromium.org/2968143003/diff/20001/components/toolbar/tool... components/toolbar/toolbar_model_impl.cc:82: if (IsOfflinePage()) I assume we want this check after the override above. That same override could actually supply the icon, but I think it's more correct to do here.
fgorski@chromium.org changed reviewers: + fgorski@chromium.org
few notes (to supplement our discussion) https://codereview.chromium.org/2968143003/diff/20001/chrome/browser/android/... File chrome/browser/android/vr_shell/textures/url_bar_texture.cc (right): https://codereview.chromium.org/2968143003/diff/20001/chrome/browser/android/... chrome/browser/android/vr_shell/textures/url_bar_texture.cc:64: return color_scheme.url_emphasized; per our discussion I am concerned about putting that in black and matching the color between chip/status and URL https://codereview.chromium.org/2968143003/diff/20001/chrome/browser/android/... chrome/browser/android/vr_shell/textures/url_bar_texture.cc:250: if (state_.offline_page && state_.should_display_url) { this will only kick in for offline pages if you && Is that intentional? https://codereview.chromium.org/2968143003/diff/20001/components/omnibox_stri... File components/omnibox_strings.grdp (right): https://codereview.chromium.org/2968143003/diff/20001/components/omnibox_stri... components/omnibox_strings.grdp:35: <message name="IDS_OFFLINE_VERBOSE_STATE" desc="Text for the Offline Omnibox Verbose srate. Displayed when the current page is loaded from a previously-downloaded cache."> typo: s/srate/state/
https://codereview.chromium.org/2968143003/diff/20001/chrome/browser/android/... File chrome/browser/android/vr_shell/textures/url_bar_texture.cc (right): https://codereview.chromium.org/2968143003/diff/20001/chrome/browser/android/... chrome/browser/android/vr_shell/textures/url_bar_texture.cc:64: return color_scheme.url_emphasized; On 2017/07/06 20:01:33, fgorski wrote: > per our discussion I am concerned about putting that in black and matching the > color between chip/status and URL Noted. I'll follow up with Josh Carpenter (our UX lead) and rachelis@, but a color tweak would likely be a follow-on CL. https://codereview.chromium.org/2968143003/diff/20001/chrome/browser/android/... chrome/browser/android/vr_shell/textures/url_bar_texture.cc:250: if (state_.offline_page && state_.should_display_url) { On 2017/07/06 20:01:34, fgorski wrote: > this will only kick in for offline pages if you && > Is that intentional? Yes. We added chip support for "Secure" and "Not secure", but did not enable it, because it consumes too much space. We rely on the red/green scheme and icon there. The "Offline" chip is a slightly different thing it looks like, and hence, I'm activating that dormant chip code only in the offline case. We might enable the "Not secure" chip for M-62. https://codereview.chromium.org/2968143003/diff/20001/components/omnibox_stri... File components/omnibox_strings.grdp (right): https://codereview.chromium.org/2968143003/diff/20001/components/omnibox_stri... components/omnibox_strings.grdp:35: <message name="IDS_OFFLINE_VERBOSE_STATE" desc="Text for the Offline Omnibox Verbose srate. Displayed when the current page is loaded from a previously-downloaded cache."> On 2017/07/06 20:01:34, fgorski wrote: > typo: > s/srate/state/ Oops! Nice catch, sorry.
https://codereview.chromium.org/2968143003/diff/20001/components/omnibox_stri... File components/omnibox_strings.grdp (right): https://codereview.chromium.org/2968143003/diff/20001/components/omnibox_stri... components/omnibox_strings.grdp:35: <message name="IDS_OFFLINE_VERBOSE_STATE" desc="Text for the Offline Omnibox Verbose srate. Displayed when the current page is loaded from a previously-downloaded cache."> On 2017/07/06 20:13:00, cjgrant wrote: > On 2017/07/06 20:01:34, fgorski wrote: > > typo: > > s/srate/state/ > > Oops! Nice catch, sorry. Done.
cjgrant@chromium.org changed reviewers: + jochen@chromium.org
jochen@chromium.org: Please review changes in components/omnibox_strings.grdp Thanks!
https://codereview.chromium.org/2968143003/diff/40001/chrome/browser/android/... File chrome/browser/android/vr_shell/textures/url_bar_texture.cc (right): https://codereview.chromium.org/2968143003/diff/40001/chrome/browser/android/... chrome/browser/android/vr_shell/textures/url_bar_texture.cc:60: SkColor GetSecurityChipColor(SecurityLevel level, Do we need some corresponding changes somewhere for non-Android? Similar question applies to a lot of the other modifications in this file. https://codereview.chromium.org/2968143003/diff/40001/chrome/browser/android/... chrome/browser/android/vr_shell/textures/url_bar_texture.cc:65: return GetSchemeColor(level, color_scheme); Nit: Simpler: return offline_page ? color_scheme.url_emphasized : GetSchemeColor(level, color_scheme); https://codereview.chromium.org/2968143003/diff/40001/chrome/browser/android/... chrome/browser/android/vr_shell/textures/url_bar_texture.cc:250: if (state_.offline_page && state_.should_display_url) { Is this correct? You only display text in the offline case, no other cases? https://codereview.chromium.org/2968143003/diff/40001/chrome/browser/android/... chrome/browser/android/vr_shell/textures/url_bar_texture.cc:314: format_types |= url_formatter::kFormatUrlExperimentalOmitHTTPS; Do Tommy Li and the omnibox folks know you're trying to rely on this for a non-experimental purpose? Please clear things through them before doing this. I don't know the code here well enough to understand what this is modifying in terms of how users interact, but stripping HTTPS from the omnibox itself is dangerous. If it affects the text that users actually interact with, it changes the meaning of the text and will cause interaction issues. If it only changes "display" text but not "editable" text, then there are other concerns about how the transition between those states looks. In short, this makes me really nervous and I'd want to understand the consequences deeply before OKing. https://codereview.chromium.org/2968143003/diff/40001/components/toolbar/BUIL... File components/toolbar/BUILD.gn (right): https://codereview.chromium.org/2968143003/diff/40001/components/toolbar/BUIL... components/toolbar/BUILD.gn:27: "offline_pin.icon", You should likely add distinct 1x and 2x versions, as we've done for these others. https://codereview.chromium.org/2968143003/diff/40001/components/toolbar/tool... File components/toolbar/toolbar_model_impl.cc (right): https://codereview.chromium.org/2968143003/diff/40001/components/toolbar/tool... components/toolbar/toolbar_model_impl.cc:83: return toolbar::kOfflinePinIcon; There should probably be unittest additions for these changes. https://codereview.chromium.org/2968143003/diff/40001/components/toolbar/vect... File components/toolbar/vector_icons/offline_pin.icon (right): https://codereview.chromium.org/2968143003/diff/40001/components/toolbar/vect... components/toolbar/vector_icons/offline_pin.icon:1: CANVAS_DIMENSIONS, 24, Looks like we still put a copyright header in these .icon files.
can we add a per-file owner for omnibox_strings.grdp? Maybe pkasting? is it possible to write a test for this feature?
On 2017/07/07 07:48:20, jochen wrote: > can we add a per-file owner for omnibox_strings.grdp? Maybe pkasting? I'm no longer the omnibox lead, so don't add me on any new omnibox ownership stuff :). Default to jdonnelly. (Can we move that .gdrp to some specific component dir instead of having it in components/?)
Peter, I've responded to your comments. Patchset is coming. https://codereview.chromium.org/2968143003/diff/40001/chrome/browser/android/... File chrome/browser/android/vr_shell/textures/url_bar_texture.cc (right): https://codereview.chromium.org/2968143003/diff/40001/chrome/browser/android/... chrome/browser/android/vr_shell/textures/url_bar_texture.cc:60: SkColor GetSecurityChipColor(SecurityLevel level, On 2017/07/07 01:21:55, Peter Kasting wrote: > Do we need some corresponding changes somewhere for non-Android? > > Similar question applies to a lot of the other modifications in this file. I don't believe so. This code just fills in logic that otherwise exists in OmniboxViewViews or the Android omnibox system, as is most of this file. I might be misinterpreting your question though. https://codereview.chromium.org/2968143003/diff/40001/chrome/browser/android/... chrome/browser/android/vr_shell/textures/url_bar_texture.cc:65: return GetSchemeColor(level, color_scheme); On 2017/07/07 01:21:55, Peter Kasting wrote: > Nit: Simpler: > > return offline_page ? color_scheme.url_emphasized > : GetSchemeColor(level, color_scheme); To explain, I like ternaries in trivial A/B cases. I didn't use that here because I think "exceptions to the rule" are more visible as early-returns (especially when multiple exceptions get checked in sequence). That said, I'll change this as it's trivial enough. https://codereview.chromium.org/2968143003/diff/40001/chrome/browser/android/... chrome/browser/android/vr_shell/textures/url_bar_texture.cc:250: if (state_.offline_page && state_.should_display_url) { On 2017/07/07 01:21:55, Peter Kasting wrote: > Is this correct? You only display text in the offline case, no other cases? That is correct. Filip also caught this, so I'll adjust the code to make this clear. crbug.com/734206 tracks VR security chips in general, where we're figuring out if they physically fit. crbug.com/735770 asks that we explicitly enable the Offline chip now (offline state isn't reflected in any red icons, https strike-through, etc). https://codereview.chromium.org/2968143003/diff/40001/chrome/browser/android/... chrome/browser/android/vr_shell/textures/url_bar_texture.cc:314: format_types |= url_formatter::kFormatUrlExperimentalOmitHTTPS; On 2017/07/07 01:21:55, Peter Kasting wrote: > Do Tommy Li and the omnibox folks know you're trying to rely on this for a > non-experimental purpose? Please clear things through them before doing this. > > I don't know the code here well enough to understand what this is modifying in > terms of how users interact, but stripping HTTPS from the omnibox itself is > dangerous. If it affects the text that users actually interact with, it changes > the meaning of the text and will cause interaction issues. If it only changes > "display" text but not "editable" text, then there are other concerns about how > the transition between those states looks. In short, this makes me really > nervous and I'd want to understand the consequences deeply before OKing. VR is trying to match Clank, which appears to strip the scheme from pages viewed in offline mode. I assume it is because there is no "connection" in offline mode, as the source is a cache, but I don't know for sure (Filip may know). There is probably no harm in continuing to show the original scheme, and certainly using an "experimental" format mode feels odd. I'll roll this back for now, and have filed crbug.com/740112 to investigate the best approach. https://codereview.chromium.org/2968143003/diff/40001/components/toolbar/BUIL... File components/toolbar/BUILD.gn (right): https://codereview.chromium.org/2968143003/diff/40001/components/toolbar/BUIL... components/toolbar/BUILD.gn:27: "offline_pin.icon", On 2017/07/07 01:21:55, Peter Kasting wrote: > You should likely add distinct 1x and 2x versions, as we've done for these > others. My understanding of 1x is that they're hand-tuned to render nicely onto a 16x16 canvas, if the icon has features that warrant it. As of now, there's no foreseeable need for a 1x icon, as only VR uses it. I've messaged Terry Anderson offline about this - he mentioned adding a mechanism for catching misuse of non-1x icons, I believe. https://codereview.chromium.org/2968143003/diff/40001/components/toolbar/tool... File components/toolbar/toolbar_model_impl.cc (right): https://codereview.chromium.org/2968143003/diff/40001/components/toolbar/tool... components/toolbar/toolbar_model_impl.cc:83: return toolbar::kOfflinePinIcon; On 2017/07/07 01:21:55, Peter Kasting wrote: > There should probably be unittest additions for these changes. I had checked for cases where we test the icons/strings returned, but didn't see anything (which makes sense as ToolbarModel is a source of truth (why test that the truth is true? :) )). I'll add a test to our VR URL bar texture to verify the icons and chips. Would you add a desktop equivalent? If so, which module gets it?
Peter, PTAL when you can. I've addressed the addressable comments, and posed questions on a few others. Thanks! https://codereview.chromium.org/2968143003/diff/40001/chrome/browser/android/... File chrome/browser/android/vr_shell/textures/url_bar_texture.cc (right): https://codereview.chromium.org/2968143003/diff/40001/chrome/browser/android/... chrome/browser/android/vr_shell/textures/url_bar_texture.cc:65: return GetSchemeColor(level, color_scheme); On 2017/07/07 15:08:00, cjgrant wrote: > On 2017/07/07 01:21:55, Peter Kasting wrote: > > Nit: Simpler: > > > > return offline_page ? color_scheme.url_emphasized > > : GetSchemeColor(level, color_scheme); > > To explain, I like ternaries in trivial A/B cases. I didn't use that here > because I think "exceptions to the rule" are more visible as early-returns > (especially when multiple exceptions get checked in sequence). That said, I'll > change this as it's trivial enough. Done. https://codereview.chromium.org/2968143003/diff/40001/chrome/browser/android/... chrome/browser/android/vr_shell/textures/url_bar_texture.cc:314: format_types |= url_formatter::kFormatUrlExperimentalOmitHTTPS; On 2017/07/07 15:08:00, cjgrant wrote: > On 2017/07/07 01:21:55, Peter Kasting wrote: > > Do Tommy Li and the omnibox folks know you're trying to rely on this for a > > non-experimental purpose? Please clear things through them before doing this. > > > > I don't know the code here well enough to understand what this is modifying in > > terms of how users interact, but stripping HTTPS from the omnibox itself is > > dangerous. If it affects the text that users actually interact with, it > changes > > the meaning of the text and will cause interaction issues. If it only changes > > "display" text but not "editable" text, then there are other concerns about > how > > the transition between those states looks. In short, this makes me really > > nervous and I'd want to understand the consequences deeply before OKing. > > VR is trying to match Clank, which appears to strip the scheme from pages viewed > in offline mode. I assume it is because there is no "connection" in offline > mode, as the source is a cache, but I don't know for sure (Filip may know). > There is probably no harm in continuing to show the original scheme, and > certainly using an "experimental" format mode feels odd. I'll roll this back > for now, and have filed crbug.com/740112 to investigate the best approach. Reverted. https://codereview.chromium.org/2968143003/diff/40001/components/toolbar/BUIL... File components/toolbar/BUILD.gn (right): https://codereview.chromium.org/2968143003/diff/40001/components/toolbar/BUIL... components/toolbar/BUILD.gn:27: "offline_pin.icon", On 2017/07/07 15:08:00, cjgrant wrote: > On 2017/07/07 01:21:55, Peter Kasting wrote: > > You should likely add distinct 1x and 2x versions, as we've done for these > > others. > > My understanding of 1x is that they're hand-tuned to render nicely onto a 16x16 > canvas, if the icon has features that warrant it. As of now, there's no > foreseeable need for a 1x icon, as only VR uses it. I've messaged Terry > Anderson offline about this - he mentioned adding a mechanism for catching > misuse of non-1x icons, I believe. After talking to Terry, I still think this should be okay. The icon is usable at any resolution, and a 1x icon could be introduced if required later. However, the check Terry mentioned is different - it ensures that a 1x icon isn't rendered at another size (not applicable here). https://codereview.chromium.org/2968143003/diff/40001/components/toolbar/vect... File components/toolbar/vector_icons/offline_pin.icon (right): https://codereview.chromium.org/2968143003/diff/40001/components/toolbar/vect... components/toolbar/vector_icons/offline_pin.icon:1: CANVAS_DIMENSIONS, 24, On 2017/07/07 01:21:55, Peter Kasting wrote: > Looks like we still put a copyright header in these .icon files. Done. (date is 2017 in latest patch set)
https://codereview.chromium.org/2968143003/diff/40001/components/toolbar/tool... File components/toolbar/toolbar_model_impl.cc (right): https://codereview.chromium.org/2968143003/diff/40001/components/toolbar/tool... components/toolbar/toolbar_model_impl.cc:83: return toolbar::kOfflinePinIcon; On 2017/07/07 15:08:00, cjgrant wrote: > On 2017/07/07 01:21:55, Peter Kasting wrote: > > There should probably be unittest additions for these changes. > > I had checked for cases where we test the icons/strings returned, but didn't see > anything (which makes sense as ToolbarModel is a source of truth (why test that > the truth is true? :) )). > > I'll add a test to our VR URL bar texture to verify the icons and chips. Would > you add a desktop equivalent? If so, which module gets it? Actually, I partially coded the test, and realized I'm actually only testing our VR ToolbarHelper class, not ToolbarModel. It sounds like you'd like a new ToolbarModel unit test, I think.
vollick@chromium.org changed reviewers: + jdonnelly@chromium.org - pkasting@chromium.org
On 2017/07/07 17:27:44, cjgrant wrote: > https://codereview.chromium.org/2968143003/diff/40001/components/toolbar/tool... > File components/toolbar/toolbar_model_impl.cc (right): > > https://codereview.chromium.org/2968143003/diff/40001/components/toolbar/tool... > components/toolbar/toolbar_model_impl.cc:83: return toolbar::kOfflinePinIcon; > On 2017/07/07 15:08:00, cjgrant wrote: > > On 2017/07/07 01:21:55, Peter Kasting wrote: > > > There should probably be unittest additions for these changes. > > > > I had checked for cases where we test the icons/strings returned, but didn't > see > > anything (which makes sense as ToolbarModel is a source of truth (why test > that > > the truth is true? :) )). > > > > I'll add a test to our VR URL bar texture to verify the icons and chips. > Would > > you add a desktop equivalent? If so, which module gets it? > > Actually, I partially coded the test, and realized I'm actually only testing our > VR ToolbarHelper class, not ToolbarModel. It sounds like you'd like a new > ToolbarModel unit test, I think. +jdonnelly. Can you PTAL?
pkasting@chromium.org changed reviewers: + pkasting@chromium.org
LGTM https://codereview.chromium.org/2968143003/diff/40001/chrome/browser/android/... File chrome/browser/android/vr_shell/textures/url_bar_texture.cc (right): https://codereview.chromium.org/2968143003/diff/40001/chrome/browser/android/... chrome/browser/android/vr_shell/textures/url_bar_texture.cc:60: SkColor GetSecurityChipColor(SecurityLevel level, On 2017/07/07 15:08:00, cjgrant wrote: > On 2017/07/07 01:21:55, Peter Kasting wrote: > > Do we need some corresponding changes somewhere for non-Android? > > > > Similar question applies to a lot of the other modifications in this file. > > I don't believe so. This code just fills in logic that otherwise exists in > OmniboxViewViews or the Android omnibox system, as is most of this file. I > might be misinterpreting your question though. Basically, I'm concerned about tweaking the OmniboxViewViews side of things to also have the right colors/styles/whatever for offline; ensuring we show the chip in the right cases; etc. I don't know whether it's possible for offline mode to be true on desktop today. https://codereview.chromium.org/2968143003/diff/40001/chrome/browser/android/... chrome/browser/android/vr_shell/textures/url_bar_texture.cc:65: return GetSchemeColor(level, color_scheme); On 2017/07/07 15:08:00, cjgrant wrote: > On 2017/07/07 01:21:55, Peter Kasting wrote: > > Nit: Simpler: > > > > return offline_page ? color_scheme.url_emphasized > > : GetSchemeColor(level, color_scheme); > > To explain, I like ternaries in trivial A/B cases. I didn't use that here > because I think "exceptions to the rule" are more visible as early-returns > (especially when multiple exceptions get checked in sequence). That's a reasonable response. Also note that anything I say "Nit" about is basically "up to you, feel free to decline". https://codereview.chromium.org/2968143003/diff/80001/chrome/browser/android/... File chrome/browser/android/vr_shell/textures/url_bar_texture.cc (right): https://codereview.chromium.org/2968143003/diff/80001/chrome/browser/android/... chrome/browser/android/vr_shell/textures/url_bar_texture.cc:250: // space, so they are currently disabled (see crbug.com/734206). The offline Nit: they are -> it is https://codereview.chromium.org/2968143003/diff/80001/chrome/browser/android/... chrome/browser/android/vr_shell/textures/url_bar_texture.cc:251: // chip is an exception, and must be shown (see crbug.com/735770). Nit: chip -> state https://codereview.chromium.org/2968143003/diff/80001/chrome/browser/android/... chrome/browser/android/vr_shell/textures/url_bar_texture.cc:317: Nit: Probably wouldn't add this blank line
On 2017/07/07 08:23:30, Peter Kasting wrote: > On 2017/07/07 07:48:20, jochen wrote: > > can we add a per-file owner for omnibox_strings.grdp? Maybe pkasting? omnibox_strings.grdp already has a per-file entry pointing to //components/omnibox/OWNERS.
The CQ bit was checked by vollick@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...
The CQ bit was unchecked by vollick@chromium.org
The CQ bit was checked by vollick@chromium.org
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": 80001, "attempt_start_ts": 1499742707643780,
"parent_rev": "b08b2bd5ae5132ba7d06325cc158d9165611e1b9", "commit_rev":
"1a0e2b694566154a5317f61f6d0adf521fb7f485"}
Message was sent while issue was closed.
Description was changed from ========== Toolbar: Report offline state through icon and verbose text On Android, pages may be shown from a downloaded offline cache. In this case, the security chip text and icon change. Reflect this in ToolbarModel. With this new ability, render the Offline security information in VR. BUG=735770 ========== to ========== Toolbar: Report offline state through icon and verbose text On Android, pages may be shown from a downloaded offline cache. In this case, the security chip text and icon change. Reflect this in ToolbarModel. With this new ability, render the Offline security information in VR. BUG=735770 Review-Url: https://codereview.chromium.org/2968143003 Cr-Commit-Position: refs/heads/master@{#485520} Committed: https://chromium.googlesource.com/chromium/src/+/1a0e2b694566154a5317f61f6d0a... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/1a0e2b694566154a5317f61f6d0a... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
