Extracted media mime type checks from net/base/ into media/base/

content/browser/frame_host/navigation_controller_impl.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/frame_host/navigation_controller_impl.h"
 
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/logging.h"
 #include "base/metrics/histogram.h"
@@ skipping 470 matching lines @@
 }
 
 NavigationEntryImpl* NavigationControllerImpl::GetLastCommittedEntry() const {
   if (last_committed_entry_index_ == -1)
     return NULL;
   return entries_[last_committed_entry_index_].get();
 }
 
 bool NavigationControllerImpl::CanViewSource() const {
   const std::string& mime_type = delegate_->GetContentsMimeType();
-  bool is_viewable_mime_type = net::IsSupportedNonImageMimeType(mime_type) &&
-      !net::IsSupportedMediaMimeType(mime_type);

[Ryan Sleevi, 2015/02/25 03:04:24]

This change is clearly a change in behaviour and logic.

Why is this correct?

[servolk, 2015/02/25 23:39:31]

You are talking about IsSupportedNonImageMimeType behavior and logic, right?
Aaron's original CL (https://codereview.chromium.org/401523002/) did that too
and so I assumed that's the right thing to do. And after looking at its usage
some more, I'm inclined to agree, and in fact I'd prefer to remove the
IsSupportedNonImageMimeType completely (although that would probably be done in
a separate CL). As far as I can tell this function is not formally specified
anywhere, it's a legacy that we got from WebKit. Any code that really needs to
get a subset of supported mime media types that are not images should be able to
do that trivially (IsSupportedMimeType && !IsSupportedImageMimeType). A few
places that are/were using IsSupportedNonImageMimeType either excluded media
types explicitly (like here) or incorrectly assumed that media mime types are
excluded from the set of non-image types. e.g. the
URLRequestHttpJob::IsCompressibleContent used to assume that, in the UMA code
that I recently removed, see crbug.com/456916. And another instance (still
active) where code doesn't expect media types to be included into non-image
types can be found at
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

Do we have any strong reasons to keep that function and/or preserve it's
behavior exactly as it was in the past?

[Ryan Sleevi, 2015/02/25 23:43:13]

No. The very clear deletion of "IsSupportedMediaMimeType"

Why is it appropriate to no longer check that?

I realize I may be coming across as short, but we seem to be having frequent
communication failures, so I'm being forced to pay extra attention to these CLs.
It would help if you can explain or justify logic whenever you're changing the
behaviour of code as to why it's OK to change.

+  bool is_viewable_mime_type = net::IsSupportedNonImageMimeType(mime_type);
   NavigationEntry* visible_entry = GetVisibleEntry();
   return visible_entry && !visible_entry->IsViewSourceMode() &&
       is_viewable_mime_type && !delegate_->GetInterstitialPage();
 }
 
 int NavigationControllerImpl::GetLastCommittedEntryIndex() const {
   return last_committed_entry_index_;
 }
 
 int NavigationControllerImpl::GetEntryCount() const {
@@ skipping 1311 matching lines @@
     }
   }
 }
 
 void NavigationControllerImpl::SetGetTimestampCallbackForTest(
     const base::Callback<base::Time()>& get_timestamp_callback) {
   get_timestamp_callback_ = get_timestamp_callback;
 }
 
 }  // namespace content