Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(267)

Issue 877323009: Extracted media mime type checks from net/base/ into media/base/ (Closed)

Created:
5 years, 10 months ago by servolk
Modified:
5 years, 7 months ago
CC:
chromium-reviews, mlamouri+watch-content_chromium.org, cbentzel+watch_chromium.org, creis+watch_chromium.org, nasko+codewatch_chromium.org, jam, feature-media-reviews_chromium.org, darin-cc_chromium.org, mkwst+moarreviews-renderer_chromium.org, asanka, erickung1, sandersd (OOO until July 31)
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Extracted media mime type checks from net/base/ into media/base/ This is an old issue that has been discussed a while ago - net/ directory is probably not the right place for mime type checks. acolwell@ attempted to do some refactoring 6 months ago: https://codereview.chromium.org/401523002/ But that CL has never landed for some reason and acolwell@ is gone from Chromium. Unfortunately a lot of things have changed in the files touched by that CL in the past 6 months, so I was unable to do an easy cherry-pick. But I've tried to repeat some of that work. This CL is the first part of mime type refactoring. It moves media-related mime type checks from net/base/ into media/ but leaves the rest of the stuff in the net/base/ for now. One tricky thing to note here (same as in the previous CL) is that net::IsSupportedNonImageMimeType used to return true for media types and now it won't after this change. So we please pay special attention to that. I think ultimately we might want to move mime type checks out of net/ completely (perhaps to content/), but I'd prefer to save that for a separate CL/discussion. Looks like one of the reasons the previous CL didn't land was that Aaron tried to solve this problem in one shot, and so CL just kept growing and growing. BUG=318217

Patch Set 1 #

Patch Set 2 : Removed non_image_map_ from media/base/mime_util.cc #

Patch Set 3 : Update old references #

Patch Set 4 : Add dependency on media/base for mojo #

Patch Set 5 : Add mojo to the media/base visibility list #

Patch Set 6 : Make media/blink depend on media/base (due to using IsSupportedMediaMimeType) #

Patch Set 7 : Rebased changes onto ToT #

Patch Set 8 : Fixed android build #

Patch Set 9 : Added #include media/base/mime_util.h into shell_main_delegate.cc #

Total comments: 4

Patch Set 10 : Introduced content::IsSupportedMimeType and deprecated the net/ one #

Patch Set 11 : Build fixes #1 #

Total comments: 3
Unified diffs Side-by-side diffs Delta from patch set Stats (+1037 lines, -901 lines) Patch
M chrome/browser/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/DEPS View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/download/download_target_determiner.cc View 1 2 3 4 5 6 7 8 9 2 chunks +2 lines, -2 lines 0 comments Download
M chrome/browser/media_galleries/fileapi/media_path_filter.cc View 1 2 3 4 5 6 7 8 9 2 chunks +2 lines, -1 line 0 comments Download
M chrome/browser/media_galleries/fileapi/supported_audio_video_checker.cc View 1 2 3 4 5 6 7 8 9 2 chunks +3 lines, -2 lines 0 comments Download
M chrome/browser/ui/cocoa/drag_util.mm View 1 2 3 4 5 6 7 8 9 10 2 chunks +2 lines, -1 line 0 comments Download
M chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc View 1 2 3 4 5 6 7 8 9 2 chunks +2 lines, -1 line 0 comments Download
M content/browser/frame_host/navigation_controller_impl.cc View 1 2 3 4 5 6 1 chunk +1 line, -2 lines 0 comments Download
M content/browser/loader/buffered_resource_handler.cc View 1 2 3 4 5 6 7 8 9 3 chunks +3 lines, -2 lines 0 comments Download
M content/child/DEPS View 1 2 3 4 5 6 1 chunk +1 line, -1 line 0 comments Download
M content/child/assert_matching_enums.cc View 1 2 3 4 5 6 2 chunks +4 lines, -4 lines 0 comments Download
M content/child/blink_platform_impl.cc View 1 2 3 4 5 6 7 8 9 2 chunks +2 lines, -2 lines 0 comments Download
M content/child/simple_webmimeregistry_impl.cc View 1 2 3 4 5 6 7 8 9 2 chunks +2 lines, -1 line 0 comments Download
M content/child/web_url_loader_impl.cc View 1 2 3 4 5 6 7 8 9 3 chunks +2 lines, -2 lines 0 comments Download
A content/common/mime_util.h View 1 2 3 4 5 6 7 8 9 1 chunk +18 lines, -0 lines 0 comments Download
A content/common/mime_util.cc View 1 2 3 4 5 6 7 8 9 1 chunk +16 lines, -0 lines 0 comments Download
M content/content_common.gypi View 1 2 3 4 5 6 7 8 9 1 chunk +2 lines, -0 lines 0 comments Download
M content/renderer/renderer_blink_platform_impl.cc View 1 2 3 4 5 6 5 chunks +9 lines, -9 lines 0 comments Download
M content/shell/DEPS View 1 2 3 4 5 6 1 chunk +1 line, -0 lines 0 comments Download
M content/shell/app/shell_main_delegate.cc View 1 2 3 4 5 6 7 8 2 chunks +2 lines, -1 line 0 comments Download
M media/base/BUILD.gn View 1 2 3 4 5 6 3 chunks +7 lines, -1 line 0 comments Download
A media/base/mime_util.h View 1 2 3 4 5 6 1 chunk +73 lines, -0 lines 0 comments Download
A media/base/mime_util.cc View 1 2 3 4 5 6 1 chunk +683 lines, -0 lines 0 comments Download
A media/base/mime_util_unittest.cc View 1 2 3 4 5 6 1 chunk +154 lines, -0 lines 0 comments Download
M media/blink/webencryptedmediaclient_impl.cc View 1 2 3 4 5 6 3 chunks +4 lines, -4 lines 0 comments Download
M media/media.gyp View 1 2 3 4 5 6 2 chunks +3 lines, -0 lines 0 comments Download
M mojo/services/html_viewer/BUILD.gn View 1 2 3 4 5 6 1 chunk +1 line, -0 lines 0 comments Download
M mojo/services/html_viewer/DEPS View 1 2 3 4 5 6 7 8 9 10 1 chunk +1 line, -0 lines 0 comments Download
M mojo/services/html_viewer/webmimeregistry_impl.cc View 1 2 3 4 5 6 7 8 9 10 6 chunks +13 lines, -12 lines 0 comments Download
M net/base/mime_util.h View 1 2 3 4 5 6 7 8 9 3 chunks +5 lines, -56 lines 0 comments Download
M net/base/mime_util.cc View 1 2 3 4 5 6 7 8 9 15 chunks +6 lines, -654 lines 3 comments Download
M net/base/mime_util_unittest.cc View 1 2 3 4 5 6 7 8 9 2 chunks +10 lines, -142 lines 0 comments Download
M net/filter/filter.cc View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 62 (5 generated)
servolk
5 years, 10 months ago (2015-01-29 00:26:37 UTC) #2
Ryan Sleevi
On 2015/01/29 00:26:37, servolk wrote: At the risk of being a pain, I think we ...
5 years, 10 months ago (2015-01-29 00:45:29 UTC) #3
servolk
On 2015/01/29 00:45:29, Ryan Sleevi wrote: > On 2015/01/29 00:26:37, servolk wrote: > > At ...
5 years, 10 months ago (2015-01-29 01:02:54 UTC) #4
Ryan Sleevi
On 2015/01/29 01:02:54, servolk wrote: > On 2015/01/29 00:45:29, Ryan Sleevi wrote: > > On ...
5 years, 10 months ago (2015-01-29 01:16:07 UTC) #5
Ryan Sleevi
Randy: Payback time. Would you mind checking my previous comment re: SDCH & filter code ...
5 years, 10 months ago (2015-01-29 01:16:49 UTC) #7
Randy Smith (Not in Mondays)
[+asanka FHI for the download interaction with Filter::FixupEncodingTypes.] On 2015/01/29 01:16:07, Ryan Sleevi wrote: > ...
5 years, 10 months ago (2015-01-29 01:46:09 UTC) #8
Ryan Sleevi
On 2015/01/29 01:46:09, rdsmith wrote: > My inclination is to > think that we should ...
5 years, 10 months ago (2015-01-29 01:58:19 UTC) #9
Ryan Sleevi
+bengr for the UMA discussion to see if the Data Reduction Proxy is using these ...
5 years, 10 months ago (2015-01-29 01:58:41 UTC) #11
Randy Smith (Not in Mondays)
On 2015/01/29 01:58:19, Ryan Sleevi wrote: > On 2015/01/29 01:46:09, rdsmith wrote: > > My ...
5 years, 10 months ago (2015-01-29 18:47:54 UTC) #12
Randy Smith (Not in Mondays)
On 2015/01/29 18:47:54, rdsmith wrote: > On 2015/01/29 01:58:19, Ryan Sleevi wrote: > > On ...
5 years, 10 months ago (2015-01-29 18:49:44 UTC) #13
servolk
On 2015/01/29 18:49:44, rdsmith wrote: > On 2015/01/29 18:47:54, rdsmith wrote: > > On 2015/01/29 ...
5 years, 10 months ago (2015-01-29 19:02:38 UTC) #14
Randy Smith (Not in Mondays)
On 2015/01/29 19:02:38, servolk wrote: > On 2015/01/29 18:49:44, rdsmith wrote: > > On 2015/01/29 ...
5 years, 10 months ago (2015-01-29 19:18:07 UTC) #15
DaleCurtis
We have 4 top-level consumers using this code in different ways; it seems like a ...
5 years, 10 months ago (2015-01-29 19:27:04 UTC) #16
Randy Smith (Not in Mondays)
On 2015/01/29 19:27:04, DaleCurtis wrote: > We have 4 top-level consumers using this code in ...
5 years, 10 months ago (2015-01-29 19:31:26 UTC) #17
Ryan Sleevi
On 2015/01/29 19:31:26, rdsmith wrote: > I think the argument is that, to the extent ...
5 years, 10 months ago (2015-01-29 20:59:01 UTC) #18
Randy Smith (Not in Mondays)
On 2015/01/29 20:59:01, Ryan Sleevi wrote: > On 2015/01/29 19:31:26, rdsmith wrote: > > I ...
5 years, 10 months ago (2015-01-29 21:18:10 UTC) #19
Randy Smith (Not in Mondays)
On 2015/01/29 21:18:10, rdsmith wrote: > On 2015/01/29 20:59:01, Ryan Sleevi wrote: > > On ...
5 years, 10 months ago (2015-01-29 21:18:53 UTC) #20
bengr
On 2015/01/29 21:18:53, rdsmith wrote: > On 2015/01/29 21:18:10, rdsmith wrote: > > On 2015/01/29 ...
5 years, 10 months ago (2015-01-29 22:52:26 UTC) #21
Ryan Sleevi
On 2015/01/29 22:52:26, bengr wrote: > I'm late to the party. Are you asking if ...
5 years, 10 months ago (2015-01-30 00:48:56 UTC) #22
bengr
On 2015/01/30 00:48:56, Ryan Sleevi wrote: > On 2015/01/29 22:52:26, bengr wrote: > > I'm ...
5 years, 10 months ago (2015-01-30 17:41:47 UTC) #23
servolk
Ok, so after our recent discussions, I've rebased this CL onto ToT. Please take another ...
5 years, 10 months ago (2015-02-24 23:38:24 UTC) #25
Ryan Sleevi
https://codereview.chromium.org/877323009/diff/160001/content/browser/frame_host/navigation_controller_impl.cc File content/browser/frame_host/navigation_controller_impl.cc (left): https://codereview.chromium.org/877323009/diff/160001/content/browser/frame_host/navigation_controller_impl.cc#oldcode492 content/browser/frame_host/navigation_controller_impl.cc:492: !net::IsSupportedMediaMimeType(mime_type); This change is clearly a change in behaviour ...
5 years, 10 months ago (2015-02-25 03:04:25 UTC) #26
servolk
https://codereview.chromium.org/877323009/diff/160001/content/browser/frame_host/navigation_controller_impl.cc File content/browser/frame_host/navigation_controller_impl.cc (left): https://codereview.chromium.org/877323009/diff/160001/content/browser/frame_host/navigation_controller_impl.cc#oldcode492 content/browser/frame_host/navigation_controller_impl.cc:492: !net::IsSupportedMediaMimeType(mime_type); On 2015/02/25 03:04:24, Ryan Sleevi wrote: > This ...
5 years, 10 months ago (2015-02-25 23:39:31 UTC) #27
Ryan Sleevi
https://codereview.chromium.org/877323009/diff/160001/content/browser/frame_host/navigation_controller_impl.cc File content/browser/frame_host/navigation_controller_impl.cc (left): https://codereview.chromium.org/877323009/diff/160001/content/browser/frame_host/navigation_controller_impl.cc#oldcode492 content/browser/frame_host/navigation_controller_impl.cc:492: !net::IsSupportedMediaMimeType(mime_type); On 2015/02/25 23:39:31, servolk wrote: > On 2015/02/25 ...
5 years, 10 months ago (2015-02-25 23:43:13 UTC) #28
servolk
On 2015/02/25 23:43:13, Ryan Sleevi wrote: > https://codereview.chromium.org/877323009/diff/160001/content/browser/frame_host/navigation_controller_impl.cc > File content/browser/frame_host/navigation_controller_impl.cc (left): > > https://codereview.chromium.org/877323009/diff/160001/content/browser/frame_host/navigation_controller_impl.cc#oldcode492 ...
5 years, 10 months ago (2015-02-25 23:57:31 UTC) #29
Ryan Sleevi
Thanks for the explanation, totally makes sense now. I'd ask that you rework the commit ...
5 years, 10 months ago (2015-02-26 00:59:42 UTC) #30
Ryan Sleevi
Once those two concerns (ODR and CL description) are addressed, LGTM
5 years, 10 months ago (2015-02-26 01:00:45 UTC) #31
servolk
On 2015/02/26 01:00:45, Ryan Sleevi wrote: > Once those two concerns (ODR and CL description) ...
5 years, 10 months ago (2015-02-26 02:38:16 UTC) #32
Ryan Sleevi
On 2015/02/26 02:38:16, servolk wrote: > On 2015/02/26 01:00:45, Ryan Sleevi wrote: > > Once ...
5 years, 10 months ago (2015-02-26 05:03:32 UTC) #33
Ryan Sleevi
On 2015/02/26 02:38:16, servolk wrote: > On 2015/02/26 01:00:45, Ryan Sleevi wrote: > > Once ...
5 years, 10 months ago (2015-02-26 05:03:37 UTC) #34
servolk
On 2015/02/26 05:03:37, Ryan Sleevi wrote: > On 2015/02/26 02:38:16, servolk wrote: > > On ...
5 years, 10 months ago (2015-02-26 20:19:38 UTC) #35
Ryan Sleevi
> Could you help us find somebody > on the net/ team who can prioritize ...
5 years, 10 months ago (2015-02-26 20:26:03 UTC) #36
servolk
On 2015/02/26 20:26:03, Ryan Sleevi wrote: > > Could you help us find somebody > ...
5 years, 10 months ago (2015-02-26 20:37:32 UTC) #37
wolenetz
On 2015/02/26 20:37:32, servolk wrote: > On 2015/02/26 20:26:03, Ryan Sleevi wrote: > > > ...
5 years, 10 months ago (2015-02-26 21:33:42 UTC) #38
servolk
On 2015/02/26 21:33:42, wolenetz wrote: > On 2015/02/26 20:37:32, servolk wrote: > > On 2015/02/26 ...
5 years, 10 months ago (2015-02-26 21:47:24 UTC) #39
DaleCurtis
That was the idea I suggested during the meeting and Ryan seemed okay with; given ...
5 years, 10 months ago (2015-02-26 21:54:01 UTC) #40
Ryan Sleevi
On 2015/02/26 21:54:01, DaleCurtis wrote: > That was the idea I suggested during the meeting ...
5 years, 10 months ago (2015-02-26 23:32:34 UTC) #41
servolk
On 2015/02/26 23:32:34, Ryan Sleevi wrote: > On 2015/02/26 21:54:01, DaleCurtis wrote: > > That ...
5 years, 10 months ago (2015-02-27 00:17:19 UTC) #42
Ryan Sleevi
On 2015/02/27 00:17:19, servolk wrote: > Let's take the specific example of adding HEVC support. ...
5 years, 10 months ago (2015-02-27 00:32:25 UTC) #43
servolk
On 2015/02/27 00:32:25, Ryan Sleevi wrote: > On 2015/02/27 00:17:19, servolk wrote: > > Let's ...
5 years, 10 months ago (2015-02-27 01:00:53 UTC) #44
Ryan Sleevi
On 2015/02/27 01:00:53, servolk wrote: > Could you elaborate on why we can only use ...
5 years, 9 months ago (2015-02-27 02:17:02 UTC) #45
servolk
On 2015/02/27 02:17:02, Ryan Sleevi wrote: > On 2015/02/27 01:00:53, servolk wrote: > > Could ...
5 years, 9 months ago (2015-02-27 21:35:01 UTC) #46
Ryan Sleevi
On 2015/02/27 21:35:01, servolk wrote: > Wait, sorry, let me explain a bit more. Yes, ...
5 years, 9 months ago (2015-02-27 21:45:27 UTC) #47
servolk
On 2015/02/27 21:45:27, Ryan Sleevi wrote: > On 2015/02/27 21:35:01, servolk wrote: > > Wait, ...
5 years, 9 months ago (2015-02-27 22:20:59 UTC) #48
chromium-reviews
On Fri, Feb 27, 2015 at 1:45 PM, <rsleevi@chromium.org> wrote: > On 2015/02/27 21:35:01, servolk ...
5 years, 9 months ago (2015-02-27 22:21:04 UTC) #49
Ryan Sleevi
I still feel like we're not making progress on here, but I'm trying to find ...
5 years, 9 months ago (2015-02-27 23:18:45 UTC) #50
Ryan Sleevi
and note, all of this is better discussed on a bug. We're far from the ...
5 years, 9 months ago (2015-02-27 23:19:22 UTC) #51
servolk
On 2015/02/27 23:18:45, Ryan Sleevi wrote: > I still feel like we're not making progress ...
5 years, 9 months ago (2015-02-28 00:39:12 UTC) #52
Ryan Sleevi
On 2015/02/28 00:39:12, servolk wrote: > We may not be making much progress in terms ...
5 years, 9 months ago (2015-03-02 20:36:12 UTC) #53
ddorwin
Is the GYP/GN discussion blocking this specific CL? If so, what specific change is the ...
5 years, 9 months ago (2015-03-13 18:14:19 UTC) #54
servolk
On 2015/03/13 18:14:19, ddorwin wrote: > Is the GYP/GN discussion blocking this specific CL? If ...
5 years, 9 months ago (2015-03-13 20:20:58 UTC) #55
ddorwin
I added the specific proposals inline. See if reverting those 2 or 3 pieces of ...
5 years, 9 months ago (2015-03-13 20:44:59 UTC) #56
servolk
On 2015/03/13 20:44:59, ddorwin wrote: > I added the specific proposals inline. See if reverting ...
5 years, 9 months ago (2015-03-13 21:07:58 UTC) #57
Ryan Sleevi
On 2015/03/13 21:07:58, servolk wrote: > > This would break the dependency of net/ on ...
5 years, 9 months ago (2015-03-13 21:16:00 UTC) #58
ddorwin
On 2015/03/13 21:07:58, servolk wrote: > It might be trickier than it seems at first. ...
5 years, 9 months ago (2015-03-13 21:18:13 UTC) #59
servolk
On 2015/03/13 21:16:00, Ryan Sleevi wrote: > On 2015/03/13 21:07:58, servolk wrote: > > > ...
5 years, 9 months ago (2015-03-13 21:31:01 UTC) #60
ddorwin
5 years, 9 months ago (2015-03-13 21:47:41 UTC) #61
Talked to Ryan offline. The current plan SGTM. You can ignore my proposed
workaround. Sorry for the noise.

Powered by Google App Engine
This is Rietveld 408576698