Removing x-x509-user-cert mime handler.

- Removing x-x509-user-cert mime-type handling for non-Android systems.

BUG=514767
Committed: https://crrev.com/3004135bd9660dd87d673a374def42e9bbec341a
Cr-Commit-Position: refs/heads/master@{#368746}

[svaldez, 2015-10-29 15:03:45]

svaldez@chromium.org changed reviewers:
+ mmenke@chromium.org, rsleevi@chromium.org

[svaldez, 2015-10-29 15:03:46]

svaldez@chromium.org changed required reviewers:
+ rsleevi@chromium.org

[svaldez, 2015-10-29 15:03:46]

Still need to finish cleaning up the Download Handler to open up the Certificate
Manager with the correct file.

[mmenke, 2015-10-29 15:34:56]

Please file a bug for this.  Should also have a test for the loader change
(Didn't look at the rest).

Also, is the description accurate?  Can't figure out how it relates to the
loader/ change.  This also seems to add a lot more code than it removes, and I
don't see anything that clearly maps to a removed "x-x509-user-cert mime
handler".

https://codereview.chromium.org/1423663012/diff/1/content/browser/loader/mime...
File content/browser/loader/mime_type_resource_handler.cc (right):

https://codereview.chromium.org/1423663012/diff/1/content/browser/loader/mime...
content/browser/loader/mime_type_resource_handler.cc:387:
save_info->suggested_name = base::UTF8ToUTF16("user_cert.crt");
Why not put this in GetSuggestedFilename?  Also, why are you setting more than
just the extension?

[svaldez, 2015-10-29 15:41:42]

Description was changed from

==========
Removing x-x509-user-cert mime handler.

- Removing x-x509-user-cert mime handler.
- Adding Client Certificate Import to Certificate Manager.

BUG=
==========

to

==========
- Removing x-x509-user-cert mime-type handling for non-Android systems.
- Adding Client Certificate import support to Certificate Manager.

BUG=
==========

[svaldez, 2015-10-29 16:32:19]

Cleaning up the code to keep the mime types while removing the handler for
non-Android uses.

https://codereview.chromium.org/1423663012/diff/1/content/browser/loader/mime...
File content/browser/loader/mime_type_resource_handler.cc (right):

https://codereview.chromium.org/1423663012/diff/1/content/browser/loader/mime...
content/browser/loader/mime_type_resource_handler.cc:387:
save_info->suggested_name = base::UTF8ToUTF16("user_cert.crt");
On 2015/10/29 15:34:56, mmenke wrote:
> Why not put this in GetSuggestedFilename?  Also, why are you setting more than
> just the extension?

Done.

[mmenke, 2015-10-29 16:40:46]

https://codereview.chromium.org/1423663012/diff/20001/net/base/mime_util.cc
File net/base/mime_util.cc (right):

https://codereview.chromium.org/1423663012/diff/20001/net/base/mime_util.cc#n...
net/base/mime_util.cc:364: *extension = "crt";
Shouldn't this just be in secondary_mappings, or primary_mappings?

[svaldez, 2015-10-29 16:55:00]

https://codereview.chromium.org/1423663012/diff/20001/net/base/mime_util.cc
File net/base/mime_util.cc (right):

https://codereview.chromium.org/1423663012/diff/20001/net/base/mime_util.cc#n...
net/base/mime_util.cc:364: *extension = "crt";
On 2015/10/29 16:40:46, mmenke wrote:
> Shouldn't this just be in secondary_mappings, or primary_mappings?

Putting it in primary/secondary_mappings restricts the extensions that are
allowed for the mime type, while the PreferredExtensionForMimeType is what
determines what to fallback to.

[svaldez, 2015-10-29 18:36:10]

Description was changed from

==========
- Removing x-x509-user-cert mime-type handling for non-Android systems.
- Adding Client Certificate import support to Certificate Manager.

BUG=
==========

to

==========
- Removing x-x509-user-cert mime-type handling for non-Android systems.
- Adding Client Certificate import support to Certificate Manager.

BUG=514767
==========

[Ryan Sleevi, 2015-10-30 23:33:47]

https://codereview.chromium.org/1423663012/diff/40001/chrome/browser/download...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

https://codereview.chromium.org/1423663012/diff/40001/chrome/browser/download...
chrome/browser/download/chrome_download_manager_delegate.cc:473:
ui::PAGE_TRANSITION_LINK, false);
I don't think this is right - this would imply it opens for all OSes, right?

https://codereview.chromium.org/1423663012/diff/40001/chrome/browser/download...
chrome/browser/download/chrome_download_manager_delegate.cc:742: if
(path.MatchesExtension(FILE_PATH_LITERAL(".crt"))) {
This doesn't handle the general mimetype, right?

(This type is possible to have extensions .pem, .crt, .cert, .p7b, .pfx, and
several others; I don't know what the "right" answer is here)

https://codereview.chromium.org/1423663012/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/certificate_manager_handler.cc (right):

https://codereview.chromium.org/1423663012/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/certificate_manager_handler.cc:721:
base::FilePath(), &file_type_info, 1, FILE_PATH_LITERAL("p12"),
BUG? the FILE_PATH_LITERAL("p12")

https://codereview.chromium.org/1423663012/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/certificate_manager_handler.cc:731:
"CertificateManager.importPersonalAskPassword");
Blergh; this is bugged even for .p12; there's no guarantee that a .p12 will be
password protected.

https://codereview.chromium.org/1423663012/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/certificate_manager_handler.cc:771: if
(file_path_.MatchesExtension(FILE_PATH_LITERAL(".p12"))) {
Again, no guarantee that .p12 == private

https://codereview.chromium.org/1423663012/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/certificate_manager_handler.cc:789:
net::X509Certificate::CreateFromBytes(data->c_str(), data->size());
BUG: Part of the goal of this change was to support multiple certificate types,
much like .p12 can. There's no guarantee that |cert| will be a singular
certificate (it could be a chain in a variety of formats)

https://codereview.chromium.org/1423663012/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/certificate_manager_handler.cc:792:
net::CertDatabase::GetInstance()->CheckUserCert(cert.get());
net::CertDatabase is pretty awful; ideally, we'll be able to move away from it
as much as possible.

We already have a net::NSSCertDatabase (cert_db_), via the
certificate_manager_model_, ideally we should be importing certificates against
that. Even if we don't, we want the model to be responsible for this sort of
logic, I think...

There's also no (strict) need to require an associated private key.

https://codereview.chromium.org/1423663012/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/certificate_manager_handler.cc:798: if
(cert_error != net::OK) {
BUG: You shouldn't assume that *any* error indicates a missing private key.
net::ERR_NO_PRIVATE_KEY_FOR_CERT is that error, but you can have other errors.

https://codereview.chromium.org/1423663012/diff/40001/content/browser/loader/...
File content/browser/loader/mime_type_resource_handler.cc (right):

https://codereview.chromium.org/1423663012/diff/40001/content/browser/loader/...
content/browser/loader/mime_type_resource_handler.cc:352: #endif
We should be able to delete this code; the intent should fire for the mime type,
AIUI - meaning we just treat it as a file download and let the inbuilt intent
handle it.

https://codereview.chromium.org/1423663012/diff/40001/net/base/mime_util.cc
File net/base/mime_util.cc (right):

https://codereview.chromium.org/1423663012/diff/40001/net/base/mime_util.cc#n...
net/base/mime_util.cc:368: #endif
BUG: Don't #ifdef this

*extension = FILE_PATH_LITERAL("crt");

[Ryan Sleevi, 2015-10-30 23:43:57]

More concrete steps as to possible next steps:

- Review
http://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/nsNSSCerti...
- this is Mozilla's key entry function for importing both through the UI and for
application/x-x509-user-cert handling
- Look at what we can do similarly in
https://code.google.com/p/chromium/codesearch#chromium/src/net/third_party/mo...
and
http://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/nsNSSCerti...
  * In particular, "getCertsFromPackage" is of relevance here, independent of
the extension
- Look at
http://mxr.mozilla.org/mozilla-central/source/security/manager/pki/resources/...
to see how extensions and such are handled

From our layering perspective, I believe the set of CLs should be:
- Improve our importer's handling w/r/t PKCS#12 vs (single certificates,
bundles)
-
  * [Ideal] Wire up this code to handle application/x-x509-user-cert as a MIME
type
  * [Acceptable] Munge the extension to force .crt in the download
- Remove the specific mime handler for application/x-x509-user-cert across all
platforms

I *think* that should be the right order, but curious if there are challenges.

[svaldez, 2015-11-02 16:27:38]

https://codereview.chromium.org/1423663012/diff/40001/chrome/browser/download...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

https://codereview.chromium.org/1423663012/diff/40001/chrome/browser/download...
chrome/browser/download/chrome_download_manager_delegate.cc:473:
ui::PAGE_TRANSITION_LINK, false);
On 2015/10/30 23:33:45, Ryan Sleevi wrote:
> I don't think this is right - this would imply it opens for all OSes, right?

This would only apply to OS_LINUX and OS_CHROMEOS due to the
IsOpenInBrowserPreferredForFile check below.

https://codereview.chromium.org/1423663012/diff/40001/chrome/browser/download...
chrome/browser/download/chrome_download_manager_delegate.cc:742: if
(path.MatchesExtension(FILE_PATH_LITERAL(".crt"))) {
On 2015/10/30 23:33:46, Ryan Sleevi wrote:
> This doesn't handle the general mimetype, right?
> 
> (This type is possible to have extensions .pem, .crt, .cert, .p7b, .pfx, and
> several others; I don't know what the "right" answer is here)

This is explicitly for dealing with downloaded files that result from
x-x509-user-cert, which will be '.crt' files. For other .pem, .cert, .p7b, .pfx
files, we would be handling them as before, where they are just a downloaded
file handled by the default system handlers.

https://codereview.chromium.org/1423663012/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/certificate_manager_handler.cc (right):

https://codereview.chromium.org/1423663012/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/certificate_manager_handler.cc:721:
base::FilePath(), &file_type_info, 1, FILE_PATH_LITERAL("p12"),
On 2015/10/30 23:33:47, Ryan Sleevi wrote:
> BUG? the FILE_PATH_LITERAL("p12")

We're defaulting to the existing case of unknown files being parsed as p12. This
is also ignored in most systems for SelectFile dialogs.

https://codereview.chromium.org/1423663012/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/certificate_manager_handler.cc:731:
"CertificateManager.importPersonalAskPassword");
On 2015/10/30 23:33:46, Ryan Sleevi wrote:
> Blergh; this is bugged even for .p12; there's no guarantee that a .p12 will be
> password protected.

It looked like it worked correctly if you just entered a blank password
previously.

https://codereview.chromium.org/1423663012/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/certificate_manager_handler.cc:771: if
(file_path_.MatchesExtension(FILE_PATH_LITERAL(".p12"))) {
On 2015/10/30 23:33:47, Ryan Sleevi wrote:
> Again, no guarantee that .p12 == private

Can't we assume as much since this is coming in from the user cert import
codepath, and if we fail to import the cert, then we just error out?

https://codereview.chromium.org/1423663012/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/certificate_manager_handler.cc:789:
net::X509Certificate::CreateFromBytes(data->c_str(), data->size());
On 2015/10/30 23:33:47, Ryan Sleevi wrote:
> BUG: Part of the goal of this change was to support multiple certificate
types,
> much like .p12 can. There's no guarantee that |cert| will be a singular
> certificate (it could be a chain in a variety of formats)

Done.

https://codereview.chromium.org/1423663012/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/certificate_manager_handler.cc:792:
net::CertDatabase::GetInstance()->CheckUserCert(cert.get());
On 2015/10/30 23:33:47, Ryan Sleevi wrote:
> net::CertDatabase is pretty awful; ideally, we'll be able to move away from it
> as much as possible.
> 
> We already have a net::NSSCertDatabase (cert_db_), via the
> certificate_manager_model_, ideally we should be importing certificates
against
> that. Even if we don't, we want the model to be responsible for this sort of
> logic, I think...
> 
> There's also no (strict) need to require an associated private key.

We'd need to modify the mozilla_security_manager to support this case, since
there currently doesn't exist an API to import a bare certificate, without also
providing the private key. Also, by spec, it looks like we are required to throw
an error on a missing private key.

https://codereview.chromium.org/1423663012/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/certificate_manager_handler.cc:798: if
(cert_error != net::OK) {
On 2015/10/30 23:33:47, Ryan Sleevi wrote:
> BUG: You shouldn't assume that *any* error indicates a missing private key.
> net::ERR_NO_PRIVATE_KEY_FOR_CERT is that error, but you can have other errors.

Done.

https://codereview.chromium.org/1423663012/diff/40001/content/browser/loader/...
File content/browser/loader/mime_type_resource_handler.cc (right):

https://codereview.chromium.org/1423663012/diff/40001/content/browser/loader/...
content/browser/loader/mime_type_resource_handler.cc:352: #endif
On 2015/10/30 23:33:47, Ryan Sleevi wrote:
> We should be able to delete this code; the intent should fire for the mime
type,
> AIUI - meaning we just treat it as a file download and let the inbuilt intent
> handle it.

Acknowledged.

https://codereview.chromium.org/1423663012/diff/40001/net/base/mime_util.cc
File net/base/mime_util.cc (right):

https://codereview.chromium.org/1423663012/diff/40001/net/base/mime_util.cc#n...
net/base/mime_util.cc:368: #endif
On 2015/10/30 23:33:47, Ryan Sleevi wrote:
> BUG: Don't #ifdef this
> 
> *extension = FILE_PATH_LITERAL("crt");

Done.

[svaldez, 2015-11-02 16:29:55]

On 2015/10/30 23:43:57, Ryan Sleevi wrote:
> More concrete steps as to possible next steps:
> 
> - Review
>
http://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/nsNSSCerti...
> - this is Mozilla's key entry function for importing both through the UI and
for
> application/x-x509-user-cert handling
> - Look at what we can do similarly in
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/third_party/mo...
> and
>
http://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/nsNSSCerti...
>   * In particular, "getCertsFromPackage" is of relevance here, independent of
> the extension
> - Look at
>
http://mxr.mozilla.org/mozilla-central/source/security/manager/pki/resources/...
> to see how extensions and such are handled
> 
> From our layering perspective, I believe the set of CLs should be:
> - Improve our importer's handling w/r/t PKCS#12 vs (single certificates,
> bundles)
> -
>   * [Ideal] Wire up this code to handle application/x-x509-user-cert as a MIME
> type
>   * [Acceptable] Munge the extension to force .crt in the download
> - Remove the specific mime handler for application/x-x509-user-cert across all
> platforms
> 
> I *think* that should be the right order, but curious if there are challenges.

For the second part of this, I don't think we'll be able to purely munge .crt
extensions, since it looks like some systems will turn a .crt extension into a
file mime type of application/pkix-cert. We can either also special-case that as
a valid mime-type, or we could try plumbing through the
application/x-x509-user-cert MIME type from the original download request.

For the first part, shouldn't CertificateListFromBytes be sufficient to support
most types of incoming certificates/cert bundles?

[svaldez, 2015-11-02 17:41:41]

On 2015/11/02 16:29:55, svaldez wrote:
> On 2015/10/30 23:43:57, Ryan Sleevi wrote:
> > More concrete steps as to possible next steps:
> > 
> > - Review
> >
>
http://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/nsNSSCerti...
> > - this is Mozilla's key entry function for importing both through the UI and
> for
> > application/x-x509-user-cert handling
> > - Look at what we can do similarly in
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/third_party/mo...
> > and
> >
>
http://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/nsNSSCerti...
> >   * In particular, "getCertsFromPackage" is of relevance here, independent
of
> > the extension
> > - Look at
> >
>
http://mxr.mozilla.org/mozilla-central/source/security/manager/pki/resources/...
> > to see how extensions and such are handled
> > 
> > From our layering perspective, I believe the set of CLs should be:
> > - Improve our importer's handling w/r/t PKCS#12 vs (single certificates,
> > bundles)
> > -
> >   * [Ideal] Wire up this code to handle application/x-x509-user-cert as a
MIME
> > type
> >   * [Acceptable] Munge the extension to force .crt in the download
> > - Remove the specific mime handler for application/x-x509-user-cert across
all
> > platforms
> > 
> > I *think* that should be the right order, but curious if there are
challenges.
> 
> For the second part of this, I don't think we'll be able to purely munge .crt
> extensions, since it looks like some systems will turn a .crt extension into a
> file mime type of application/pkix-cert. We can either also special-case that
as
> a valid mime-type, or we could try plumbing through the
> application/x-x509-user-cert MIME type from the original download request.
> 
> For the first part, shouldn't CertificateListFromBytes be sufficient to
support
> most types of incoming certificates/cert bundles?

Ah, didn't realize we're safe to modify our copy of nsNSSCertificateDB. In that
case I'll split off our certificate import code into another CL. Still not sure
what we want to do about plumbing the x509-user-cert info through to the
download action handler. The most general solution is probably to just make the
content-type a variable included through the DownloadItem, and check that at the
end? Then we wouldn't even need to deal with the mime-type handling.

[Ryan Sleevi, 2015-11-04 03:36:03]

On 2015/11/02 16:29:55, svaldez wrote:
> For the first part, shouldn't CertificateListFromBytes be sufficient to
support
> most types of incoming certificates/cert bundles?

Yeah, with FormatAuto it should be fine, and is the moral/spiritual equivalent
to what FF is doing.

I just recalled there's some gotcha's here (wanting to make sure that each of
the intermediates has a valid signed path to a trusted root, so that we're not
importing junk), so you're likely fine parsing with FORMAT_AUTO, grabbing the
first cert, and checking for the private key. We can worry about the
intermediate validation later (e.g. in another quarter :P)

[Ryan Sleevi, 2015-11-04 03:39:45]

On 2015/11/02 16:29:55, svaldez wrote:
> For the second part of this, I don't think we'll be able to purely munge .crt
> extensions, since it looks like some systems will turn a .crt extension into a
> file mime type of application/pkix-cert. We can either also special-case that
as
> a valid mime-type, or we could try plumbing through the
> application/x-x509-user-cert MIME type from the original download request.

I'm not sure I grok this concern?

Adding an association for .crt - whether it be application/pkix-cert or
application/x-x509-user-cert - should be fine. If it's a CMS blob w/ a user
cert, it'll be importable; otherwise, it won't (at least not until we add
support for multiple certificates). So in terms of user experience we should be
fine - it's more than what they can do today, easier - but doesn't break any
existing functionality.

Mapping application/x-x509-user-cert to be .crt should also be fine. Yes, it
means if the user uploads that file, they'll need to fixup the extension, but if
the server is having trouble with extension mangling, it can just drop/change
the mimetype and the extension will be preserved as it was originally. This is
perhaps less than ideal - so I'm not opposed to surfacing mime information, but
asanka@ and the folks who "know downloads" would know more about that.

[svaldez, 2015-11-04 15:25:40]

On 2015/11/04 03:39:45, Ryan Sleevi (OOO. no rvws plz) wrote:
> On 2015/11/02 16:29:55, svaldez wrote:
> > For the second part of this, I don't think we'll be able to purely munge
.crt
> > extensions, since it looks like some systems will turn a .crt extension into
a
> > file mime type of application/pkix-cert. We can either also special-case
that
> as
> > a valid mime-type, or we could try plumbing through the
> > application/x-x509-user-cert MIME type from the original download request.
> 
> I'm not sure I grok this concern?
> 
> Adding an association for .crt - whether it be application/pkix-cert or
> application/x-x509-user-cert - should be fine. If it's a CMS blob w/ a user
> cert, it'll be importable; otherwise, it won't (at least not until we add
> support for multiple certificates). So in terms of user experience we should
be
> fine - it's more than what they can do today, easier - but doesn't break any
> existing functionality.
> 
> Mapping application/x-x509-user-cert to be .crt should also be fine. Yes, it
> means if the user uploads that file, they'll need to fixup the extension, but
if
> the server is having trouble with extension mangling, it can just drop/change
> the mimetype and the extension will be preserved as it was originally. This is
> perhaps less than ideal - so I'm not opposed to surfacing mime information,
but
> asanka@ and the folks who "know downloads" would know more about that.

I guess my worry was more about handling file downloads that have a .crt ending,
but weren't x509-user-cert MIME types, though I suppose it doesn't break too
many
things to fail in this case.

[svaldez, 2015-11-23 21:51:26]

This should be the mime-handler part of the removal of the x-x509-user-cert.

[svaldez, 2015-11-30 17:27:53]

Description was changed from

==========
- Removing x-x509-user-cert mime-type handling for non-Android systems.
- Adding Client Certificate import support to Certificate Manager.

BUG=514767
==========

to

==========
- Removing x-x509-user-cert mime-type handling for non-Android systems.

BUG=514767
==========

[svaldez, 2015-11-30 18:01:41]

On 2015/11/23 21:51:26, svaldez wrote:
> This should be the mime-handler part of the removal of the x-x509-user-cert.

Now we're using the mime-type to check whether we want to bring up the settings.
We still need the extension munging, since otherwise we'll usually end up with
downloaded files without a valid extension, making them not show up in the
Import file selector by default.

[svaldez, 2015-12-01 19:40:16]

svaldez@chromium.org changed reviewers:
+ mattm@chromium.org
- mmenke@chromium.org

[Ryan Sleevi, 2015-12-01 22:45:47]

https://codereview.chromium.org/1423663012/diff/140001/content/browser/loader...
File content/browser/loader/mime_type_resource_handler.cc (right):

https://codereview.chromium.org/1423663012/diff/140001/content/browser/loader...
content/browser/loader/mime_type_resource_handler.cc:366:
!mime_util::IsSupportedCertificateMimeType(mime_type)) {
I'm not entirely clear why this is needed; shouldn't it be fine to force it as a
download?

https://codereview.chromium.org/1423663012/diff/140001/net/base/mime_util.cc
File net/base/mime_util.cc (right):

https://codereview.chromium.org/1423663012/diff/140001/net/base/mime_util.cc#...
net/base/mime_util.cc:362: // Force .crt extension for client certificates.
Shouldn't this only be for ChromeOS, and embedded in the logic of
platform_mime_util_linux.cc for OS_CHROMEOS?

[svaldez, 2015-12-01 22:53:36]

https://codereview.chromium.org/1423663012/diff/140001/content/browser/loader...
File content/browser/loader/mime_type_resource_handler.cc (right):

https://codereview.chromium.org/1423663012/diff/140001/content/browser/loader...
content/browser/loader/mime_type_resource_handler.cc:366:
!mime_util::IsSupportedCertificateMimeType(mime_type)) {
On 2015/12/01 22:45:47, Ryan Sleevi wrote:
> I'm not entirely clear why this is needed; shouldn't it be fine to force it as
a
> download?

This is the escape hatch for files with MimeTypes we can handle separately.
Since we don't want to do that with Certificate files, we have to add the check
here.

https://codereview.chromium.org/1423663012/diff/140001/net/base/mime_util.cc
File net/base/mime_util.cc (right):

https://codereview.chromium.org/1423663012/diff/140001/net/base/mime_util.cc#...
net/base/mime_util.cc:362: // Force .crt extension for client certificates.
On 2015/12/01 22:45:47, Ryan Sleevi wrote:
> Shouldn't this only be for ChromeOS, and embedded in the logic of
> platform_mime_util_linux.cc for OS_CHROMEOS?

Not entirely sure whether we want to limit this to only OS_CHROMEOS. Linux needs
it since there's no good default handler, and other OS might need it since
otherwise we are going to typically end up downloading files with no extension,
since the filename probably isn't set for application/x-x509-user-cert files.

[Ryan Sleevi, 2015-12-01 23:00:50]

https://codereview.chromium.org/1423663012/diff/140001/content/browser/loader...
File content/browser/loader/mime_type_resource_handler.cc (right):

https://codereview.chromium.org/1423663012/diff/140001/content/browser/loader...
content/browser/loader/mime_type_resource_handler.cc:366:
!mime_util::IsSupportedCertificateMimeType(mime_type)) {
On 2015/12/01 22:53:36, svaldez wrote:
> This is the escape hatch for files with MimeTypes we can handle separately.
> Since we don't want to do that with Certificate files, we have to add the
check
> here.

I guess I'm still confused. Why don't we want to do that for Certificate files?

I guess put differently: Why wouldn't we simply remove certificates entirely
from IsSupportedMimeType?

https://codereview.chromium.org/1423663012/diff/140001/net/base/mime_util.cc
File net/base/mime_util.cc (right):

https://codereview.chromium.org/1423663012/diff/140001/net/base/mime_util.cc#...
net/base/mime_util.cc:362: // Force .crt extension for client certificates.
On 2015/12/01 22:53:36, svaldez wrote:
> On 2015/12/01 22:45:47, Ryan Sleevi wrote:
> > Shouldn't this only be for ChromeOS, and embedded in the logic of
> > platform_mime_util_linux.cc for OS_CHROMEOS?
> 
> Not entirely sure whether we want to limit this to only OS_CHROMEOS. Linux
needs
> it since there's no good default handler,

My (opinionated, potentially wrong) thinking is that Linux, forcing a download
and having them import via chrome://settings/certificates is fine. The Linux UI
isn't the 'canonical' way to manage the NSS store, and there's plenty of other
ways depending on distro (p11-kit on RHEL-and-derivatives come to mind)

So since Linux is such a special snowflake of having use cases, it seems like we
should just let it be a download and let the user handle. Supporting device
enrollment for Linux is a bit of a 'non-goal' anyways.

> and other OS might need it since
> otherwise we are going to typically end up downloading files with no
extension,
> since the filename probably isn't set for application/x-x509-user-cert files.

Yeah, but that will have issues w/ other browsers anyways, so I'm inclined to
take the simple path of leaving the extension unmolested. I mention this because
a .crt extension will trigger different behaviour on different platforms
(Windows in particular), where you might be importing a PKCS#7 file (which
Windows likes to do as .p7b - to indicate PKCS#7 binary, *shrug*)

Is the extension mangling even necessary now, given
https://codereview.chromium.org/1423663012/diff/140001/chrome/browser/downloa...
? It doesn't seem like it is - we can just leave it extensionless (if the server
happened to not set one)

[svaldez, 2015-12-01 23:05:22]

https://codereview.chromium.org/1423663012/diff/140001/content/browser/loader...
File content/browser/loader/mime_type_resource_handler.cc (right):

https://codereview.chromium.org/1423663012/diff/140001/content/browser/loader...
content/browser/loader/mime_type_resource_handler.cc:366:
!mime_util::IsSupportedCertificateMimeType(mime_type)) {
On 2015/12/01 23:00:50, Ryan Sleevi wrote:
> On 2015/12/01 22:53:36, svaldez wrote:
> > This is the escape hatch for files with MimeTypes we can handle separately.
> > Since we don't want to do that with Certificate files, we have to add the
> check
> > here.
> 
> I guess I'm still confused. Why don't we want to do that for Certificate
files?
> 
> I guess put differently: Why wouldn't we simply remove certificates entirely
> from IsSupportedMimeType?

I believe that doesn't work since sites don't set Content-Disposition and the
browser ends up displaying the certificate instead of attempting to download it.

https://codereview.chromium.org/1423663012/diff/140001/net/base/mime_util.cc
File net/base/mime_util.cc (right):

https://codereview.chromium.org/1423663012/diff/140001/net/base/mime_util.cc#...
net/base/mime_util.cc:362: // Force .crt extension for client certificates.
On 2015/12/01 23:00:50, Ryan Sleevi wrote:
> On 2015/12/01 22:53:36, svaldez wrote:
> > On 2015/12/01 22:45:47, Ryan Sleevi wrote:
> > > Shouldn't this only be for ChromeOS, and embedded in the logic of
> > > platform_mime_util_linux.cc for OS_CHROMEOS?
> > 
> > Not entirely sure whether we want to limit this to only OS_CHROMEOS. Linux
> needs
> > it since there's no good default handler,
> 
> My (opinionated, potentially wrong) thinking is that Linux, forcing a download
> and having them import via chrome://settings/certificates is fine. The Linux
UI
> isn't the 'canonical' way to manage the NSS store, and there's plenty of other
> ways depending on distro (p11-kit on RHEL-and-derivatives come to mind)
> 
> So since Linux is such a special snowflake of having use cases, it seems like
we
> should just let it be a download and let the user handle. Supporting device
> enrollment for Linux is a bit of a 'non-goal' anyways.
> 
> > and other OS might need it since
> > otherwise we are going to typically end up downloading files with no
> extension,
> > since the filename probably isn't set for application/x-x509-user-cert
files.
> 
> Yeah, but that will have issues w/ other browsers anyways, so I'm inclined to
> take the simple path of leaving the extension unmolested. I mention this
because
> a .crt extension will trigger different behaviour on different platforms
> (Windows in particular), where you might be importing a PKCS#7 file (which
> Windows likes to do as .p7b - to indicate PKCS#7 binary, *shrug*)
> 
> Is the extension mangling even necessary now, given
>
https://codereview.chromium.org/1423663012/diff/140001/chrome/browser/downloa...
> ? It doesn't seem like it is - we can just leave it extensionless (if the
server
> happened to not set one)

The problem in that case is that since we'll end up with extensionless
downloads, the user would have to change the file type in the open dialog in the
certificate manager to be "All Files" instead of the default we have now. Though
I suppose that might encourage users of client certificates to start setting a
Content-Disposition on the files so that they download with the right file name.

[Ryan Sleevi, 2015-12-01 23:15:56]

https://codereview.chromium.org/1423663012/diff/140001/content/browser/loader...
File content/browser/loader/mime_type_resource_handler.cc (right):

https://codereview.chromium.org/1423663012/diff/140001/content/browser/loader...
content/browser/loader/mime_type_resource_handler.cc:366:
!mime_util::IsSupportedCertificateMimeType(mime_type)) {
On 2015/12/01 23:05:22, svaldez wrote:
> I believe that doesn't work since sites don't set Content-Disposition and the
> browser ends up displaying the certificate instead of attempting to download
it.

Well, not necessarily.

So, sorry, some hidden context here is the desire to get rid of the notion of
certificate mime types from being a special case, as much as possible. That's
the end goal, and any intermediate steps we can take to move towards that end
goal, rad.

I think it's not the end of the world if we have this check here, but the
alternative is that we remove it from IsSupportedMimeType(), and instead let it
be handled via the MustDownload (line 363). There, we let MustDownload() call to
the host (
https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro...
), which could then force it to be downloaded.

That said, I can see where embedders likely want to rely on the weird implicit
API contract we have now in //content (where they don't have to think about
certificates; which is a "bug" but *shrug*), so this is fine.

As an alternative way of thinking, would it make sense to mangle in a
Content-Disposition (and filename) if no C-D is present, at a lower layer in the
stack? I'm not sure if there's any clean opportunities to do that, but that'd be
another way of keeping some of that logic close.

All of this to say that, after the explanation and looking at the code more, I'm
able to live with the conditional as is and we can nuke it when we fully nuke
certificate support, but I'm also fond of removing as much as possible any
knowledge of certificates.

[svaldez, 2015-12-02 14:53:15]

On 2015/12/01 23:15:56, Ryan Sleevi wrote:
>
https://codereview.chromium.org/1423663012/diff/140001/content/browser/loader...
> File content/browser/loader/mime_type_resource_handler.cc (right):
> 
>
https://codereview.chromium.org/1423663012/diff/140001/content/browser/loader...
> content/browser/loader/mime_type_resource_handler.cc:366:
> !mime_util::IsSupportedCertificateMimeType(mime_type)) {
> On 2015/12/01 23:05:22, svaldez wrote:
> > I believe that doesn't work since sites don't set Content-Disposition and
the
> > browser ends up displaying the certificate instead of attempting to download
> it.
> 
> Well, not necessarily.
> 
> So, sorry, some hidden context here is the desire to get rid of the notion of
> certificate mime types from being a special case, as much as possible. That's
> the end goal, and any intermediate steps we can take to move towards that end
> goal, rad.
> 
> I think it's not the end of the world if we have this check here, but the
> alternative is that we remove it from IsSupportedMimeType(), and instead let
it
> be handled via the MustDownload (line 363). There, we let MustDownload() call
to
> the host (
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro...
> ), which could then force it to be downloaded.
> 
> That said, I can see where embedders likely want to rely on the weird implicit
> API contract we have now in //content (where they don't have to think about
> certificates; which is a "bug" but *shrug*), so this is fine.
> 
> As an alternative way of thinking, would it make sense to mangle in a
> Content-Disposition (and filename) if no C-D is present, at a lower layer in
the
> stack? I'm not sure if there's any clean opportunities to do that, but that'd
be
> another way of keeping some of that logic close.
> 
> All of this to say that, after the explanation and looking at the code more,
I'm
> able to live with the conditional as is and we can nuke it when we fully nuke
> certificate support, but I'm also fond of removing as much as possible any
> knowledge of certificates.

I actually like the approach of mangling in a C-D if there isn't one and the
content-type is x-x509-user-cert. We'd still have a couple lines of logic that
depends on the mime-type, but that does seem cleaner than tracking certificate
mime types globally. I'll try to implement something like that and see if
there's a good place to do so.

[svaldez, 2015-12-02 16:57:25]

On 2015/12/02 14:53:15, svaldez wrote:
> On 2015/12/01 23:15:56, Ryan Sleevi wrote:
> >
>
https://codereview.chromium.org/1423663012/diff/140001/content/browser/loader...
> > File content/browser/loader/mime_type_resource_handler.cc (right):
> > 
> >
>
https://codereview.chromium.org/1423663012/diff/140001/content/browser/loader...
> > content/browser/loader/mime_type_resource_handler.cc:366:
> > !mime_util::IsSupportedCertificateMimeType(mime_type)) {
> > On 2015/12/01 23:05:22, svaldez wrote:
> > > I believe that doesn't work since sites don't set Content-Disposition and
> the
> > > browser ends up displaying the certificate instead of attempting to
download
> > it.
> > 
> > Well, not necessarily.
> > 
> > So, sorry, some hidden context here is the desire to get rid of the notion
of
> > certificate mime types from being a special case, as much as possible.
That's
> > the end goal, and any intermediate steps we can take to move towards that
end
> > goal, rad.
> > 
> > I think it's not the end of the world if we have this check here, but the
> > alternative is that we remove it from IsSupportedMimeType(), and instead let
> it
> > be handled via the MustDownload (line 363). There, we let MustDownload()
call
> to
> > the host (
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro...
> > ), which could then force it to be downloaded.
> > 
> > That said, I can see where embedders likely want to rely on the weird
implicit
> > API contract we have now in //content (where they don't have to think about
> > certificates; which is a "bug" but *shrug*), so this is fine.
> > 
> > As an alternative way of thinking, would it make sense to mangle in a
> > Content-Disposition (and filename) if no C-D is present, at a lower layer in
> the
> > stack? I'm not sure if there's any clean opportunities to do that, but
that'd
> be
> > another way of keeping some of that logic close.
> > 
> > All of this to say that, after the explanation and looking at the code more,
> I'm
> > able to live with the conditional as is and we can nuke it when we fully
nuke
> > certificate support, but I'm also fond of removing as much as possible any
> > knowledge of certificates.
> 
> I actually like the approach of mangling in a C-D if there isn't one and the
> content-type is x-x509-user-cert. We'd still have a couple lines of logic that
> depends on the mime-type, but that does seem cleaner than tracking certificate
> mime types globally. I'll try to implement something like that and see if
> there's a good place to do so.

PTAL, I added the mangling into mime_type_handler, since that seems to be the
most reasonable place, but its a super hacky mangling, without keeping knowledge
of certificate mime types in mime_util.

[Ryan Sleevi, 2015-12-08 02:38:37]

LGTM % nits

https://codereview.chromium.org/1423663012/diff/160001/chrome/browser/downloa...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

https://codereview.chromium.org/1423663012/diff/160001/chrome/browser/downloa...
chrome/browser/download/chrome_download_manager_delegate.cc:470: params =
content::OpenURLParams(GURL("chrome://settings/certificates"),
There's no way to smuggle the file name or pre-warm the UI, right?

[I would imagine not, but just double checking; we may be able to hack something
with URL fragments or query strings, but that's probably not necessary yet]

https://codereview.chromium.org/1423663012/diff/160001/content/browser/loader...
File content/browser/loader/mime_type_resource_handler.cc (right):

https://codereview.chromium.org/1423663012/diff/160001/content/browser/loader...
content/browser/loader/mime_type_resource_handler.cc:348: "Content-Disposition:
attachment; filename=\"user.crt\"");
I'm still torn on this. Do we anticipate ever ripping off the bandaid here and
no longer synthesizing a C-D - and instead, expect the servers to?

If so, at what point?

My 'gut' is that we should remove this code when we remove <keygen>, in which
case, we should file a bug to track that, suitably forward-reaching, and add a
comment here to that effect

// https://crbug.com/foo - "Temporary" hack to handle servers that aren't
setting
// Content-Disposition and expecting the browser to automatically
// install certificates; this is being deprecated and will be removed
// in XXX.

https://codereview.chromium.org/1423663012/diff/160001/content/browser/loader...
content/browser/loader/mime_type_resource_handler.cc:374: if
(mime_util::IsSupportedMimeType(mime_type)) {
braces are unnecessary here; keep it consistent with rest of file (and net/ code
in general)

[svaldez, 2015-12-08 15:43:39]

https://codereview.chromium.org/1423663012/diff/160001/chrome/browser/downloa...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

https://codereview.chromium.org/1423663012/diff/160001/chrome/browser/downloa...
chrome/browser/download/chrome_download_manager_delegate.cc:470: params =
content::OpenURLParams(GURL("chrome://settings/certificates"),
On 2015/12/08 02:38:37, Ryan Sleevi wrote:
> There's no way to smuggle the file name or pre-warm the UI, right?
> 
> [I would imagine not, but just double checking; we may be able to hack
something
> with URL fragments or query strings, but that's probably not necessary yet]

We could pipe this through, though we'll end up having the full filename in the
URL fragment. If that's reasonable, I can create a CL for that and we can see
whether we want to use it.

https://codereview.chromium.org/1423663012/diff/160001/content/browser/loader...
File content/browser/loader/mime_type_resource_handler.cc (right):

https://codereview.chromium.org/1423663012/diff/160001/content/browser/loader...
content/browser/loader/mime_type_resource_handler.cc:348: "Content-Disposition:
attachment; filename=\"user.crt\"");
On 2015/12/08 02:38:37, Ryan Sleevi wrote:
> I'm still torn on this. Do we anticipate ever ripping off the bandaid here and
> no longer synthesizing a C-D - and instead, expect the servers to?
> 
> If so, at what point?
> 
> My 'gut' is that we should remove this code when we remove <keygen>, in which
> case, we should file a bug to track that, suitably forward-reaching, and add a
> comment here to that effect
> 
> // https://crbug.com/foo - "Temporary" hack to handle servers that aren't
> setting
> // Content-Disposition and expecting the browser to automatically
> // install certificates; this is being deprecated and will be removed
> // in XXX.

We could get rid of the filename part of this and then just have a check to
force download if we do end up adding the hack to get the certificate manager to
auto-populate the filename.

https://codereview.chromium.org/1423663012/diff/160001/content/browser/loader...
content/browser/loader/mime_type_resource_handler.cc:374: if
(mime_util::IsSupportedMimeType(mime_type)) {
On 2015/12/08 02:38:37, Ryan Sleevi wrote:
> braces are unnecessary here; keep it consistent with rest of file (and net/
code
> in general)

Done.

[svaldez, 2015-12-08 16:34:35]

The importPersonal hack CL:
https://codereview.chromium.org/1509673004/

https://codereview.chromium.org/1423663012/diff/160001/chrome/browser/downloa...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

https://codereview.chromium.org/1423663012/diff/160001/chrome/browser/downloa...
chrome/browser/download/chrome_download_manager_delegate.cc:470: params =
content::OpenURLParams(GURL("chrome://settings/certificates"),
On 2015/12/08 15:43:39, svaldez wrote:
> On 2015/12/08 02:38:37, Ryan Sleevi wrote:
> > There's no way to smuggle the file name or pre-warm the UI, right?
> > 
> > [I would imagine not, but just double checking; we may be able to hack
> something
> > with URL fragments or query strings, but that's probably not necessary yet]
> 
> We could pipe this through, though we'll end up having the full filename in
the
> URL fragment. If that's reasonable, I can create a CL for that and we can see
> whether we want to use it.

So attempting to pipe this through, there isn't really a good way of having the
UI default to a specific file. We can trigger the importPersonal file selector
with the correct file, but then the UX is that a Open File dialog popups and the
user really isn't informed of what the file dialog is for. If we bypass file
selection entirely, then there really isn't a chance for the user to
cancel/accept the import.

https://codereview.chromium.org/1423663012/diff/160001/content/browser/loader...
File content/browser/loader/mime_type_resource_handler.cc (right):

https://codereview.chromium.org/1423663012/diff/160001/content/browser/loader...
content/browser/loader/mime_type_resource_handler.cc:348: "Content-Disposition:
attachment; filename=\"user.crt\"");
On 2015/12/08 15:43:39, svaldez wrote:
> On 2015/12/08 02:38:37, Ryan Sleevi wrote:
> > I'm still torn on this. Do we anticipate ever ripping off the bandaid here
and
> > no longer synthesizing a C-D - and instead, expect the servers to?
> > 
> > If so, at what point?
> > 
> > My 'gut' is that we should remove this code when we remove <keygen>, in
which
> > case, we should file a bug to track that, suitably forward-reaching, and add
a
> > comment here to that effect
> > 
> > // https://crbug.com/foo - "Temporary" hack to handle servers that aren't
> > setting
> > // Content-Disposition and expecting the browser to automatically
> > // install certificates; this is being deprecated and will be removed
> > // in XXX.
> 
> We could get rid of the filename part of this and then just have a check to
> force download if we do end up adding the hack to get the certificate manager
to
> auto-populate the filename.

Should this be a separate bug from the Keygen deprecation, or can we use the
same bug for tracking this?

[Ryan Sleevi, 2015-12-09 01:27:24]

Still LG

https://codereview.chromium.org/1423663012/diff/160001/chrome/browser/downloa...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

https://codereview.chromium.org/1423663012/diff/160001/chrome/browser/downloa...
chrome/browser/download/chrome_download_manager_delegate.cc:470: params =
content::OpenURLParams(GURL("chrome://settings/certificates"),
On 2015/12/08 16:34:35, svaldez wrote:
> On 2015/12/08 15:43:39, svaldez wrote:
> > On 2015/12/08 02:38:37, Ryan Sleevi wrote:
> > > There's no way to smuggle the file name or pre-warm the UI, right?
> > > 
> > > [I would imagine not, but just double checking; we may be able to hack
> > something
> > > with URL fragments or query strings, but that's probably not necessary
yet]
> > 
> > We could pipe this through, though we'll end up having the full filename in
> the
> > URL fragment. If that's reasonable, I can create a CL for that and we can
see
> > whether we want to use it.
> 
> So attempting to pipe this through, there isn't really a good way of having
the
> UI default to a specific file. We can trigger the importPersonal file selector
> with the correct file, but then the UX is that a Open File dialog popups and
the
> user really isn't informed of what the file dialog is for. If we bypass file
> selection entirely, then there really isn't a chance for the user to
> cancel/accept the import.

Fair point. Short of reworking this UI, which we could do later, I think it's
fine as is.

https://codereview.chromium.org/1423663012/diff/160001/content/browser/loader...
File content/browser/loader/mime_type_resource_handler.cc (right):

https://codereview.chromium.org/1423663012/diff/160001/content/browser/loader...
content/browser/loader/mime_type_resource_handler.cc:348: "Content-Disposition:
attachment; filename=\"user.crt\"");
On 2015/12/08 16:34:35, svaldez wrote:
> On 2015/12/08 15:43:39, svaldez wrote:
> > On 2015/12/08 02:38:37, Ryan Sleevi wrote:
> > > I'm still torn on this. Do we anticipate ever ripping off the bandaid here
> and
> > > no longer synthesizing a C-D - and instead, expect the servers to?
> > > 
> > > If so, at what point?
> > > 
> > > My 'gut' is that we should remove this code when we remove <keygen>, in
> which
> > > case, we should file a bug to track that, suitably forward-reaching, and
add
> a
> > > comment here to that effect
> > > 
> > > // https://crbug.com/foo - "Temporary" hack to handle servers that aren't
> > > setting
> > > // Content-Disposition and expecting the browser to automatically
> > > // install certificates; this is being deprecated and will be removed
> > > // in XXX.
> > 
> > We could get rid of the filename part of this and then just have a check to
> > force download if we do end up adding the hack to get the certificate
manager
> to
> > auto-populate the filename.
> 
> Should this be a separate bug from the Keygen deprecation, or can we use the
> same bug for tracking this?

Separate; good to keep concrete tasks as distinct bugs.

https://codereview.chromium.org/1423663012/diff/180001/content/browser/loader...
File content/browser/loader/mime_type_resource_handler.cc (right):

https://codereview.chromium.org/1423663012/diff/180001/content/browser/loader...
content/browser/loader/mime_type_resource_handler.cc:346: // deprecated and will
be removed upon full <keygen> removal.
One last thought - we should add an UMA hook here to see how often it's hit.

That is, UMA that triggers on application/x-x509-user-cert and UMAs whether or
not a CD was present & was download/attachment.

That may give us a quicker path to nuking this.

[svaldez, 2015-12-09 21:16:03]

https://codereview.chromium.org/1423663012/diff/180001/content/browser/loader...
File content/browser/loader/mime_type_resource_handler.cc (right):

https://codereview.chromium.org/1423663012/diff/180001/content/browser/loader...
content/browser/loader/mime_type_resource_handler.cc:346: // deprecated and will
be removed upon full <keygen> removal.
On 2015/12/09 01:27:24, Ryan Sleevi wrote:
> One last thought - we should add an UMA hook here to see how often it's hit.
> 
> That is, UMA that triggers on application/x-x509-user-cert and UMAs whether or
> not a CD was present & was download/attachment.
> 
> That may give us a quicker path to nuking this.

Added bool based on CD presence.

[Ryan Sleevi, 2015-12-09 21:42:14]

rsleevi@chromium.org changed reviewers:
+ isherman@chromium.org

[Ryan Sleevi, 2015-12-09 21:42:15]

Still LGTM; adding Ilya for histograms

[Ilya Sherman, 2015-12-09 23:13:03]

histograms LGTM % nits

https://codereview.chromium.org/1423663012/diff/200001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1423663012/diff/200001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:50731: +        Distribution of
Content-Disposition headers send with x-x509-user-cert
nit: s/send/sent

https://codereview.chromium.org/1423663012/diff/200001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:50732: +    content types.
nit: Please fix the alignment (run the pretty print script)

[svaldez, 2015-12-11 16:18:35]

https://codereview.chromium.org/1423663012/diff/200001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1423663012/diff/200001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:50731: +        Distribution of
Content-Disposition headers send with x-x509-user-cert
On 2015/12/09 23:13:03, Ilya Sherman wrote:
> nit: s/send/sent

Done.

https://codereview.chromium.org/1423663012/diff/200001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:50732: +    content types.
On 2015/12/09 23:13:03, Ilya Sherman wrote:
> nit: Please fix the alignment (run the pretty print script)

Done, though for some reason pretty_print didn't fix it the first time.

[svaldez, 2015-12-11 16:19:21]

The CQ bit was checked by svaldez@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-11 16:20:06]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1423663012/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1423663012/220001

[commit-bot: I haz the power, 2015-12-11 16:23:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-11 16:23:16]

Dry run: Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[svaldez, 2016-01-05 22:28:53]

The CQ bit was checked by svaldez@chromium.org

[svaldez, 2016-01-05 22:28:53]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org, rsleevi@chromium.org
Link to the patchset: https://codereview.chromium.org/1423663012/#ps280001
(title: "Rebase.")

[commit-bot: I haz the power, 2016-01-05 22:30:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1423663012/280001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1423663012/280001

[commit-bot: I haz the power, 2016-01-05 22:35:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-01-05 22:35:48]

Try jobs failed on following builders:
  chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[svaldez, 2016-01-05 23:11:00]

The CQ bit was checked by svaldez@chromium.org

[svaldez, 2016-01-05 23:11:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org, rsleevi@chromium.org
Link to the patchset: https://codereview.chromium.org/1423663012/#ps300001
(title: "Rebase.")

[commit-bot: I haz the power, 2016-01-05 23:12:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1423663012/300001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1423663012/300001

[commit-bot: I haz the power, 2016-01-05 23:23:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-01-05 23:23:44]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[svaldez, 2016-01-05 23:27:37]

svaldez@chromium.org changed reviewers:
+ asanka@chromium.org, davidben@chromium.org

[svaldez, 2016-01-05 23:27:38]

asanka, davidben:

Could I get an OWNERS review on:
    chrome/browser/download/chrome_download_manager_delegate.cc
    content/browser/loader/certificate_resource_handler.cc
    content/browser/loader/certificate_resource_handler.h
    content/browser/loader/mime_type_resource_handler.cc

[asanka, 2016-01-06 01:55:43]

https://codereview.chromium.org/1423663012/diff/320001/chrome/browser/downloa...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

https://codereview.chromium.org/1423663012/diff/320001/chrome/browser/downloa...
chrome/browser/download/chrome_download_manager_delegate.cc:472: params =
content::OpenURLParams(GURL("chrome://settings/certificates"),
This just opens the settings page?

[svaldez, 2016-01-06 02:18:31]

On 2016/01/06 01:55:43, asanka wrote:
>
https://codereview.chromium.org/1423663012/diff/320001/chrome/browser/downloa...
> File chrome/browser/download/chrome_download_manager_delegate.cc (right):
> 
>
https://codereview.chromium.org/1423663012/diff/320001/chrome/browser/downloa...
> chrome/browser/download/chrome_download_manager_delegate.cc:472: params =
> content::OpenURLParams(GURL("chrome://settings/certificates"),
> This just opens the settings page?

Yeah, we consider having it auto-populate the import certificate dialog, but
that would require a lot of plumbing and need another confirmation dialog.

[davidben, 2016-01-06 03:53:10]

https://codereview.chromium.org/1423663012/diff/320001/content/browser/loader...
File content/browser/loader/mime_type_resource_handler.cc (right):

https://codereview.chromium.org/1423663012/diff/320001/content/browser/loader...
content/browser/loader/mime_type_resource_handler.cc:343: //
https://crbug.com/568184 - "Temporary" hack to handle servers that aren't
Nit: I'd probably drop the quotes. It's sort of confusing out of context what
the sarcastic-quotes on 'temporary' means here. :-)

https://codereview.chromium.org/1423663012/diff/320001/content/browser/loader...
content/browser/loader/mime_type_resource_handler.cc:346: // deprecated and will
be removed upon full <keygen> removal.
Comment should probably say whether you're doing this to fix the filename or to
force a download. I think you actually don't need it to force a download. It's
an unknown mimetype, which means we'll just download it, even in an iframe.

I could buy you need it for the filename, though I wonder if it that'd work
better putting it in the downloads subsystem? I'm not as familiar with that
code, but it's got to be less hacky than modifying the headers, right? (asanka?)

https://codereview.chromium.org/1423663012/diff/320001/content/browser/loader...
content/browser/loader/mime_type_resource_handler.cc:351: if
(!response_->head.headers->HasHeader("Content-Disposition")) {
Nit: bool has_content_disposition = ...?
Since you're using the same thing anyway.

[davidben, 2016-01-06 03:59:04]

https://codereview.chromium.org/1423663012/diff/320001/content/browser/loader...
File content/browser/loader/mime_type_resource_handler.cc (right):

https://codereview.chromium.org/1423663012/diff/320001/content/browser/loader...
content/browser/loader/mime_type_resource_handler.cc:346: // deprecated and will
be removed upon full <keygen> removal.
On 2016/01/06 03:53:10, davidben wrote:
> Comment should probably say whether you're doing this to fix the filename or
to
> force a download. I think you actually don't need it to force a download. It's
> an unknown mimetype, which means we'll just download it, even in an iframe.
> 
> I could buy you need it for the filename, though I wonder if it that'd work
> better putting it in the downloads subsystem? I'm not as familiar with that
> code, but it's got to be less hacky than modifying the headers, right?
(asanka?)

In fact, if you open https://davidben.net/cuttlefish-download (content-type
image/png + disposition attachement), the save file dialog fixes the filename.
And if I turn off prompting, it fixes the filename too. So something in this
stack is already fixing filenames based on mimetype. Probably using mimetypes
from the OS, but teaching it that application/x-x509-user-cert has a
non-operating-system default file extension seems natural.

[asanka, 2016-01-06 15:33:43]

On 2016/01/06 02:18:31, svaldez wrote:
> On 2016/01/06 01:55:43, asanka wrote:
> >
>
https://codereview.chromium.org/1423663012/diff/320001/chrome/browser/downloa...
> > File chrome/browser/download/chrome_download_manager_delegate.cc (right):
> > 
> >
>
https://codereview.chromium.org/1423663012/diff/320001/chrome/browser/downloa...
> > chrome/browser/download/chrome_download_manager_delegate.cc:472: params =
> > content::OpenURLParams(GURL("chrome://settings/certificates"),
> > This just opens the settings page?
> 
> Yeah, we consider having it auto-populate the import certificate dialog, but
> that would require a lot of plumbing and need another confirmation dialog.

Sure, but how about not handling the file at all? It seems confusing to open a
download and have a settings page open with no means to open the file that was
downloaded. The user would need to find some other means to open the file
anyway. Why not allow the platform open semantics deal with this?

[asanka, 2016-01-06 15:55:22]

https://codereview.chromium.org/1423663012/diff/320001/content/browser/loader...
File content/browser/loader/mime_type_resource_handler.cc (right):

https://codereview.chromium.org/1423663012/diff/320001/content/browser/loader...
content/browser/loader/mime_type_resource_handler.cc:346: // deprecated and will
be removed upon full <keygen> removal.
On 2016/01/06 03:59:04, davidben wrote:
> On 2016/01/06 03:53:10, davidben wrote:
> > Comment should probably say whether you're doing this to fix the filename or
> to
> > force a download. I think you actually don't need it to force a download.
It's
> > an unknown mimetype, which means we'll just download it, even in an iframe.
> > 
> > I could buy you need it for the filename, though I wonder if it that'd work
> > better putting it in the downloads subsystem? I'm not as familiar with that
> > code, but it's got to be less hacky than modifying the headers, right?
> (asanka?)
> 
> In fact, if you open https://davidben.net/cuttlefish-download (content-type
> image/png + disposition attachement), the save file dialog fixes the filename.
> And if I turn off prompting, it fixes the filename too. So something in this
> stack is already fixing filenames based on mimetype. Probably using mimetypes
> from the OS, but teaching it that application/x-x509-user-cert has a
> non-operating-system default file extension seems natural.

Yeah, if the Content-Type is not handled and the request is frame level request
(i.e. main frame or subframe navigation request), then the request is
automatically treated as a download.

We have a set of heuristics to determine the download filename if the server
doesn't send a Content-Disposition header. However, those heuristics will only
come up with a suitable file type if the underlying platform has a mapping from
the Content-Type to a preferred filename extension. For Linux and Chrome OS,
you'd need to add a mapping for 'application/x-x509-user-cert' to
platform_mime_util_linux.cc.

I also think we should let downloads come up with a filename rather than force
one here by inserting a Content-Disposition header. In general, if a
Content-Type is unsupported, I'd lean towards letting it go through the same
process as other unsupported resources.

[svaldez, 2016-01-06 16:19:15]

On 2016/01/06 15:55:22, asanka wrote:
>
https://codereview.chromium.org/1423663012/diff/320001/content/browser/loader...
> File content/browser/loader/mime_type_resource_handler.cc (right):
> 
>
https://codereview.chromium.org/1423663012/diff/320001/content/browser/loader...
> content/browser/loader/mime_type_resource_handler.cc:346: // deprecated and
will
> be removed upon full <keygen> removal.
> On 2016/01/06 03:59:04, davidben wrote:
> > On 2016/01/06 03:53:10, davidben wrote:
> > > Comment should probably say whether you're doing this to fix the filename
or
> > to
> > > force a download. I think you actually don't need it to force a download.
> It's
> > > an unknown mimetype, which means we'll just download it, even in an
iframe.
> > > 
> > > I could buy you need it for the filename, though I wonder if it that'd
work
> > > better putting it in the downloads subsystem? I'm not as familiar with
that
> > > code, but it's got to be less hacky than modifying the headers, right?
> > (asanka?)
> > 
> > In fact, if you open https://davidben.net/cuttlefish-download (content-type
> > image/png + disposition attachement), the save file dialog fixes the
filename.
> > And if I turn off prompting, it fixes the filename too. So something in this
> > stack is already fixing filenames based on mimetype. Probably using
mimetypes
> > from the OS, but teaching it that application/x-x509-user-cert has a
> > non-operating-system default file extension seems natural.
> 
> Yeah, if the Content-Type is not handled and the request is frame level
request
> (i.e. main frame or subframe navigation request), then the request is
> automatically treated as a download.
> 
> We have a set of heuristics to determine the download filename if the server
> doesn't send a Content-Disposition header. However, those heuristics will only
> come up with a suitable file type if the underlying platform has a mapping
from
> the Content-Type to a preferred filename extension. For Linux and Chrome OS,
> you'd need to add a mapping for 'application/x-x509-user-cert' to
> platform_mime_util_linux.cc.
> 
> I also think we should let downloads come up with a filename rather than force
> one here by inserting a Content-Disposition header. In general, if a
> Content-Type is unsupported, I'd lean towards letting it go through the same
> process as other unsupported resources.

The idea is that we need to direct the user to the "Import Client Certificate"
dialog, since otherwise the OS will default to doing the wrong thing. If we let
the underlying system handle it, we end up with it being interpreted as a
pkix-cert which is also incorrect for client certs. The intended user experience
is to click on the user cert, have the import dialog popup and then have them
navigate to the downloaded file.

[asanka, 2016-01-06 21:33:09]

Ok. Thanks for the explanation. I'm still on board with not injecting a
Content-Disposition header.

https://codereview.chromium.org/1423663012/diff/320001/chrome/browser/downloa...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

https://codereview.chromium.org/1423663012/diff/320001/chrome/browser/downloa...
chrome/browser/download/chrome_download_manager_delegate.cc:471: if
(download->GetOriginalMimeType() == "application/x-x509-user-cert") {
Use GetMimeType() instead. The original MIME type is only useful if you care
about the MIME type before the MimeTypeResourceHandler did its thing.

https://codereview.chromium.org/1423663012/diff/320001/chrome/browser/downloa...
chrome/browser/download/chrome_download_manager_delegate.cc:472: params =
content::OpenURLParams(GURL("chrome://settings/certificates"),
See chrome/browser/ui/chrome_pages.h for how to open settings pages. Apparently
it's still possible to get the settings page to open in a window. Either way,
use the canonical method for invoking settings pages.

[svaldez, 2016-01-11 18:02:26]

Should be fixed.

https://codereview.chromium.org/1423663012/diff/320001/chrome/browser/downloa...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

https://codereview.chromium.org/1423663012/diff/320001/chrome/browser/downloa...
chrome/browser/download/chrome_download_manager_delegate.cc:471: if
(download->GetOriginalMimeType() == "application/x-x509-user-cert") {
On 2016/01/06 21:33:09, asanka wrote:
> Use GetMimeType() instead. The original MIME type is only useful if you care
> about the MIME type before the MimeTypeResourceHandler did its thing.

Done.

https://codereview.chromium.org/1423663012/diff/320001/chrome/browser/downloa...
chrome/browser/download/chrome_download_manager_delegate.cc:472: params =
content::OpenURLParams(GURL("chrome://settings/certificates"),
On 2016/01/06 21:33:09, asanka wrote:
> See chrome/browser/ui/chrome_pages.h for how to open settings pages.
Apparently
> it's still possible to get the settings page to open in a window. Either way,
> use the canonical method for invoking settings pages.

Done.

https://codereview.chromium.org/1423663012/diff/320001/content/browser/loader...
File content/browser/loader/mime_type_resource_handler.cc (right):

https://codereview.chromium.org/1423663012/diff/320001/content/browser/loader...
content/browser/loader/mime_type_resource_handler.cc:343: //
https://crbug.com/568184 - "Temporary" hack to handle servers that aren't
On 2016/01/06 03:53:10, davidben (OOO until 1-11) wrote:
> Nit: I'd probably drop the quotes. It's sort of confusing out of context what
> the sarcastic-quotes on 'temporary' means here. :-)

Done.

https://codereview.chromium.org/1423663012/diff/320001/content/browser/loader...
content/browser/loader/mime_type_resource_handler.cc:346: // deprecated and will
be removed upon full <keygen> removal.
On 2016/01/06 15:55:22, asanka wrote:
> On 2016/01/06 03:59:04, davidben wrote:
> > On 2016/01/06 03:53:10, davidben wrote:
> > > Comment should probably say whether you're doing this to fix the filename
or
> > to
> > > force a download. I think you actually don't need it to force a download.
> It's
> > > an unknown mimetype, which means we'll just download it, even in an
iframe.
> > > 
> > > I could buy you need it for the filename, though I wonder if it that'd
work
> > > better putting it in the downloads subsystem? I'm not as familiar with
that
> > > code, but it's got to be less hacky than modifying the headers, right?
> > (asanka?)
> > 
> > In fact, if you open https://davidben.net/cuttlefish-download (content-type
> > image/png + disposition attachement), the save file dialog fixes the
filename.
> > And if I turn off prompting, it fixes the filename too. So something in this
> > stack is already fixing filenames based on mimetype. Probably using
mimetypes
> > from the OS, but teaching it that application/x-x509-user-cert has a
> > non-operating-system default file extension seems natural.
> 
> Yeah, if the Content-Type is not handled and the request is frame level
request
> (i.e. main frame or subframe navigation request), then the request is
> automatically treated as a download.
> 
> We have a set of heuristics to determine the download filename if the server
> doesn't send a Content-Disposition header. However, those heuristics will only
> come up with a suitable file type if the underlying platform has a mapping
from
> the Content-Type to a preferred filename extension. For Linux and Chrome OS,
> you'd need to add a mapping for 'application/x-x509-user-cert' to
> platform_mime_util_linux.cc.
> 
> I also think we should let downloads come up with a filename rather than force
> one here by inserting a Content-Disposition header. In general, if a
> Content-Type is unsupported, I'd lean towards letting it go through the same
> process as other unsupported resources.

Done.

https://codereview.chromium.org/1423663012/diff/320001/content/browser/loader...
content/browser/loader/mime_type_resource_handler.cc:351: if
(!response_->head.headers->HasHeader("Content-Disposition")) {
On 2016/01/06 03:53:10, davidben (OOO until 1-11) wrote:
> Nit: bool has_content_disposition = ...?
> Since you're using the same thing anyway.

Done.

[davidben, 2016-01-11 18:45:35]

content lgtm. Deferring to asanka on the net change since it's
downloads-related.

[asanka, 2016-01-11 21:16:34]

https://codereview.chromium.org/1423663012/diff/340001/net/base/filename_util...
File net/base/filename_util_internal.cc (right):

https://codereview.chromium.org/1423663012/diff/340001/net/base/filename_util...
net/base/filename_util_internal.cc:249: filename = "user.crt";
Let's move this logic to DownloadTargetDeterminer in //chrome. I.e. let the
DoGenerateTargetPath step in DTD override the filename or the extension based on
the MIME type.

Special casing of application/x-x509-user-cert doesn't belong in //net. The
special handling exists in //chrome.

[svaldez, 2016-01-11 21:34:55]

https://codereview.chromium.org/1423663012/diff/340001/net/base/filename_util...
File net/base/filename_util_internal.cc (right):

https://codereview.chromium.org/1423663012/diff/340001/net/base/filename_util...
net/base/filename_util_internal.cc:249: filename = "user.crt";
On 2016/01/11 21:16:34, asanka wrote:
> Let's move this logic to DownloadTargetDeterminer in //chrome. I.e. let the
> DoGenerateTargetPath step in DTD override the filename or the extension based
on
> the MIME type.
> 
> Special casing of application/x-x509-user-cert doesn't belong in //net. The
> special handling exists in //chrome.

Done.

[asanka, 2016-01-11 21:52:43]

Thanks. Looking much better.

You should be able to copy one of the simple tests in
download_target_determiner_unittest to test the behavior for
application/x-x509-user-cert and also how it interacts with suggested filenames
if you wish to go that route.

https://codereview.chromium.org/1423663012/diff/360001/chrome/browser/downloa...
File chrome/browser/download/download_target_determiner.cc (right):

https://codereview.chromium.org/1423663012/diff/360001/chrome/browser/downloa...
chrome/browser/download/download_target_determiner.cc:199: if
(download_->GetMimeType() == "application/x-x509-user-cert")
How about conditionalizing this on suggested_filename being empty? It would make
the behavior consistent if there's an actual suggested filename (e.g.: a
@download attribute on an anchor).

I don't have a strong opinion on this since the handling of this type is
evolving. So feel free to push back. But I think the eventual state should
respect filename suggestions.

[svaldez, 2016-01-11 22:32:28]

Done modulo seeing whether the test pass.

[asanka, 2016-01-11 22:58:45]

lgtm. thanks!

[svaldez, 2016-01-11 23:19:20]

The CQ bit was checked by svaldez@chromium.org

[svaldez, 2016-01-11 23:19:20]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org, rsleevi@chromium.org, davidben@chromium.org
Link to the patchset: https://codereview.chromium.org/1423663012/#ps380001
(title: "Adding tests.")

[commit-bot: I haz the power, 2016-01-11 23:21:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1423663012/380001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1423663012/380001

[commit-bot: I haz the power, 2016-01-11 23:38:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-01-11 23:39:01]

Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Ryan Sleevi, 2016-01-12 00:15:36]

The CQ bit was checked by rsleevi@chromium.org

[commit-bot: I haz the power, 2016-01-12 00:16:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1423663012/380001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1423663012/380001

[commit-bot: I haz the power, 2016-01-12 01:07:37]

Description was changed from

==========
- Removing x-x509-user-cert mime-type handling for non-Android systems.

BUG=514767
==========

to

==========
- Removing x-x509-user-cert mime-type handling for non-Android systems.

BUG=514767
==========

[commit-bot: I haz the power, 2016-01-12 01:07:38]

Committed patchset #20 (id:380001)

[commit-bot: I haz the power, 2016-01-12 01:09:02]

Description was changed from

==========
- Removing x-x509-user-cert mime-type handling for non-Android systems.

BUG=514767
==========

to

==========
- Removing x-x509-user-cert mime-type handling for non-Android systems.

BUG=514767

Committed: https://crrev.com/3004135bd9660dd87d673a374def42e9bbec341a
Cr-Commit-Position: refs/heads/master@{#368746}
==========

[commit-bot: I haz the power, 2016-01-12 01:09:03]

Patchset 20 (id:??) landed as
https://crrev.com/3004135bd9660dd87d673a374def42e9bbec341a
Cr-Commit-Position: refs/heads/master@{#368746}