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

Issue 2487373003: Disable background video track behind a feature flag (Closed)

Created:
4 years, 1 month ago by whywhat
Modified:
3 years, 9 months ago
CC:
apacible+watch_chromium.org, asvitkine+watch_chromium.org, blink-reviews, blink-reviews-api_chromium.org, blink-reviews-html_chromium.org, chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, dglazkov+blink, eric.carlson_apple.com, feature-media-reviews_chromium.org, fs, gasubic, jam, kinuko+watch, mcasas+watch+vc_chromium.org, miu+watch_chromium.org, mlamouri+watch-blink_chromium.org, mlamouri+watch-content_chromium.org, nasko+codewatch_chromium.org, posciak+watch_chromium.org, nessy, Srirama, vcarbune.chromium, xjz+watch_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Disable background video track behind a feature flag BUG=663999 TEST=unit test + manual testing with the flag on - see chrome://media-internals logging track selection Committed: https://crrev.com/39c1024093bdd21abd2c2ea3de5c05f7b9fee8ce Cr-Commit-Position: refs/heads/master@{#434255}

Patch Set 1 #

Patch Set 2 : Added the feature flag to histograms.xml #

Total comments: 9

Patch Set 3 : Moved the main logic into WMPI #

Patch Set 4 : Updated test files #

Total comments: 2

Patch Set 5 : Rebase + comment fix #

Unified diffs Side-by-side diffs Delta from patch set Stats (+104 lines, -21 lines) Patch
M chrome/app/generated_resources.grd View 1 2 3 4 1 chunk +8 lines, -0 lines 0 comments Download
M chrome/browser/about_flags.cc View 1 2 3 4 1 chunk +5 lines, -0 lines 0 comments Download
M content/browser/renderer_host/render_view_host_impl.cc View 1 2 2 chunks +4 lines, -0 lines 0 comments Download
M content/public/common/common_param_traits_macros.h View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download
M content/public/common/web_preferences.h View 1 2 3 4 1 chunk +3 lines, -0 lines 0 comments Download
M content/public/common/web_preferences.cc View 1 2 3 4 1 chunk +2 lines, -1 line 0 comments Download
M content/renderer/media/webmediaplayer_ms_unittest.cc View 1 2 3 1 chunk +4 lines, -0 lines 0 comments Download
M content/renderer/render_view_impl.cc View 1 2 3 4 1 chunk +2 lines, -0 lines 0 comments Download
M media/base/media_switches.h View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M media/base/media_switches.cc View 1 2 1 chunk +4 lines, -0 lines 0 comments Download
M media/blink/webmediaplayer_impl.h View 1 2 1 chunk +3 lines, -0 lines 0 comments Download
M media/blink/webmediaplayer_impl.cc View 1 2 6 chunks +25 lines, -4 lines 0 comments Download
M media/blink/webmediaplayer_impl_unittest.cc View 1 2 3 1 chunk +4 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/html/HTMLMediaElement.h View 1 2 2 chunks +3 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/html/HTMLMediaElement.cpp View 1 2 8 chunks +21 lines, -16 lines 0 comments Download
M third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/Source/web/WebRuntimeFeatures.cpp View 1 2 1 chunk +4 lines, -0 lines 0 comments Download
M third_party/WebKit/public/platform/WebMediaPlayerClient.h View 1 2 3 4 1 chunk +6 lines, -0 lines 0 comments Download
M third_party/WebKit/public/web/WebRuntimeFeatures.h View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M tools/metrics/histograms/histograms.xml View 1 2 3 4 2 chunks +2 lines, -0 lines 0 comments Download

Messages

Total messages: 58 (31 generated)
whywhat
PTaL
4 years, 1 month ago (2016-11-10 03:32:31 UTC) #2
whywhat
Added the feature flag to histograms.xml
4 years, 1 month ago (2016-11-10 16:15:32 UTC) #7
whywhat
+Dale
4 years, 1 month ago (2016-11-11 02:04:23 UTC) #13
mlamouri (slow - plz ping)
https://codereview.chromium.org/2487373003/diff/20001/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (left): https://codereview.chromium.org/2487373003/diff/20001/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp#oldcode2556 third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2556: DCHECK(RuntimeEnabledFeatures::audioVideoTracksEnabled()); Here and below, did you have to remove ...
4 years, 1 month ago (2016-11-12 23:35:48 UTC) #14
whywhat
https://codereview.chromium.org/2487373003/diff/20001/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (left): https://codereview.chromium.org/2487373003/diff/20001/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp#oldcode2556 third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2556: DCHECK(RuntimeEnabledFeatures::audioVideoTracksEnabled()); On 2016/11/12 at 23:35:48, mlamouri (slow) wrote: > ...
4 years, 1 month ago (2016-11-14 21:50:13 UTC) #15
DaleCurtis
=>sandersd for review since he's changing some of that media/ code right now. Overall this ...
4 years, 1 month ago (2016-11-14 21:52:49 UTC) #17
sandersd (OOO until July 31)
https://codereview.chromium.org/2487373003/diff/20001/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/2487373003/diff/20001/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp#newcode3188 third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3188: webMediaPlayer()->selectedVideoTrackChanged(nullptr); On 2016/11/14 21:50:13, whywhat wrote: > On 2016/11/12 ...
4 years, 1 month ago (2016-11-15 00:09:11 UTC) #18
whywhat
On 2016/11/15 at 00:09:11, sandersd wrote: > https://codereview.chromium.org/2487373003/diff/20001/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp > File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): > > https://codereview.chromium.org/2487373003/diff/20001/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp#newcode3188 ...
4 years, 1 month ago (2016-11-15 02:56:02 UTC) #19
whywhat
https://codereview.chromium.org/2487373003/diff/20001/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/2487373003/diff/20001/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp#newcode3188 third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3188: webMediaPlayer()->selectedVideoTrackChanged(nullptr); On 2016/11/15 at 00:09:11, sandersd wrote: > On ...
4 years, 1 month ago (2016-11-15 03:08:56 UTC) #20
mlamouri (slow - plz ping)
On 2016/11/15 at 03:08:56, avayvod wrote: > https://codereview.chromium.org/2487373003/diff/20001/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp > File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): > > https://codereview.chromium.org/2487373003/diff/20001/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp#newcode3188 ...
4 years, 1 month ago (2016-11-15 18:21:40 UTC) #21
sandersd (OOO until July 31)
> Maybe I could add a method to WMPClient to get the current video track ...
4 years, 1 month ago (2016-11-15 18:49:02 UTC) #22
whywhat
On 2016/11/15 at 18:21:40, mlamouri wrote: > On 2016/11/15 at 03:08:56, avayvod wrote: > > ...
4 years, 1 month ago (2016-11-15 20:08:55 UTC) #23
whywhat
On 2016/11/15 at 18:49:02, sandersd wrote: > > Maybe I could add a method to ...
4 years, 1 month ago (2016-11-15 20:09:26 UTC) #24
whywhat
Moved the main logic into WMPI
4 years, 1 month ago (2016-11-21 20:45:38 UTC) #25
whywhat
Dan, PTAL Note: still need to update the tests.
4 years, 1 month ago (2016-11-21 20:47:49 UTC) #30
sandersd (OOO until July 31)
WMPI changes LGTM. Can you add a section to your doc that shows what (if ...
4 years, 1 month ago (2016-11-21 21:23:25 UTC) #31
whywhat
PTAL +mkwst for common_param_traits_macros.h and histograms.xml +pfeldman for the rest of content/ & WebKit plumbing
4 years, 1 month ago (2016-11-22 17:05:54 UTC) #33
whywhat
Updated test files
4 years, 1 month ago (2016-11-22 17:07:44 UTC) #37
pfeldman
plumbing lgtm https://codereview.chromium.org/2487373003/diff/60001/third_party/WebKit/public/platform/WebMediaPlayerClient.h File third_party/WebKit/public/platform/WebMediaPlayerClient.h (right): https://codereview.chromium.org/2487373003/diff/60001/third_party/WebKit/public/platform/WebMediaPlayerClient.h#newcode106 third_party/WebKit/public/platform/WebMediaPlayerClient.h:106: // Returns the selected video track id ...
4 years, 1 month ago (2016-11-22 19:55:56 UTC) #38
whywhat
Rebase + comment fix
4 years, 1 month ago (2016-11-23 02:22:27 UTC) #43
whywhat
https://codereview.chromium.org/2487373003/diff/60001/third_party/WebKit/public/platform/WebMediaPlayerClient.h File third_party/WebKit/public/platform/WebMediaPlayerClient.h (right): https://codereview.chromium.org/2487373003/diff/60001/third_party/WebKit/public/platform/WebMediaPlayerClient.h#newcode106 third_party/WebKit/public/platform/WebMediaPlayerClient.h:106: // Returns the selected video track id (or an ...
4 years, 1 month ago (2016-11-23 02:23:38 UTC) #45
Mike West
adding the item to the IPC struct LGTM.
4 years ago (2016-11-23 13:01:14 UTC) #49
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2487373003/80001
4 years ago (2016-11-23 20:42:06 UTC) #52
commit-bot: I haz the power
Committed patchset #5 (id:80001)
4 years ago (2016-11-23 21:43:46 UTC) #54
commit-bot: I haz the power
Patchset 5 (id:??) landed as https://crrev.com/39c1024093bdd21abd2c2ea3de5c05f7b9fee8ce Cr-Commit-Position: refs/heads/master@{#434255}
4 years ago (2016-11-23 21:45:48 UTC) #56
servolk
On 2016/11/23 21:45:48, commit-bot: I haz the power wrote: > Patchset 5 (id:??) landed as ...
4 years ago (2016-12-05 19:35:40 UTC) #57
whywhat
4 years ago (2016-12-16 15:56:43 UTC) #58
Message was sent while issue was closed.
On 2016/12/05 at 19:35:40, servolk wrote:
> On 2016/11/23 21:45:48, commit-bot: I haz the power wrote:
> > Patchset 5 (id:??) landed as
> > https://crrev.com/39c1024093bdd21abd2c2ea3de5c05f7b9fee8ce
> > Cr-Commit-Position: refs/heads/master@{#434255}
> 
> Sorry, perhaps I'm missing something, but I can see a unit test in earlier
patchsets of this CL (in HTMLVideoElementTest.cpp), but I can't find anything
similar to that test in the latest patchset #5, which got landed. Has the test
been extracted to a separate CL?

Ah, the test went away after the implementation moved from HTMLMediaElement to
WMPI as the test was Blink-specific and I didn't find a good way to test this in
WMPI. If you have a suggestion for the test, I'm happy to add it.

Powered by Google App Engine
This is Rietveld 408576698