Ability to block <audio> and <video> media.

This is the Blink-side code for a content privacy preference that blocks
HTML5 media elements <audio> and <video>. This is intended to work just
like the current setting for blocking images.

The LayoutTests in this CL rely on the Content Shell change in the CL
referenced below.

https://codereview.chromium.org/312103004/

BUG=50132
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=177015

[pwnall-personal, 2013-10-17 23:57:02]

Can you please take a look?

This works in a Chromium build, when used together with this Chromium-side CL:
https://codereview.chromium.org/27635002/

The Chromium-side CL still needs work (I used "video"/"sound" names all over the
place before deciding to use "media"), but it is decent enough to give this CL a
spin.

FWIW, Apple provides a <video> demo page that comes in handy for testing this
feature.
http://www.apple.com/html5/showcase/video/

I look forward to your feedback!

[jochen (gone - plz use gerrit), 2013-10-18 00:45:35]

can you please first reach consensus on the bug?

comment #11 seems to imply that we don't want this feature

[pwnall-personal, 2013-10-18 00:52:40]

On 2013/10/18 00:45:35, jochen wrote:
> can you please first reach consensus on the bug?
> 
> comment #11 seems to imply that we don't want this feature

Ah, sorry for misunderstanding and wasting your time!

I thought GoodFirstBug bugs are ready to be picked up and assumed the
"@google.com" later comment means this is desirable. My bad, should have asked
first :(

[jochen (gone - plz use gerrit), 2013-10-18 01:08:38]

I'm not saying we can't have this, we just should reach consensus first :)

[Bernhard Bauer, 2013-10-18 08:38:24]

On 2013/10/18 00:45:35, jochen wrote:
> can you please first reach consensus on the bug?
> 
> comment #11 seems to imply that we don't want this feature

I'm pretty sure that comment only referred to the UI. The plan at that time was
to move content settings completely to extension APIs, but that hasn't seen much
progress. Exposing these content settings via an extension API shouldn't be a
proble though, and then later we can reach consensus on whether to expose this
in the normal content settings UI.

[jochen (gone - plz use gerrit), 2013-10-18 22:53:55]

On 2013/10/18 08:38:24, Bernhard Bauer wrote:
> On 2013/10/18 00:45:35, jochen wrote:
> > can you please first reach consensus on the bug?
> > 
> > comment #11 seems to imply that we don't want this feature
> 
> I'm pretty sure that comment only referred to the UI. The plan at that time
was
> to move content settings completely to extension APIs, but that hasn't seen
much
> progress. Exposing these content settings via an extension API shouldn't be a
> proble though, and then later we can reach consensus on whether to expose this
> in the normal content settings UI.

sure. can we have the discussion on the bug?

[pwnall-personal, 2013-10-19 00:13:25]

On 2013/10/18 22:53:55, jochen wrote:
> On 2013/10/18 08:38:24, Bernhard Bauer wrote:
> > On 2013/10/18 00:45:35, jochen wrote:
> > > can you please first reach consensus on the bug?
> > > 
> > > comment #11 seems to imply that we don't want this feature
> > 
> > I'm pretty sure that comment only referred to the UI. The plan at that time
> was
> > to move content settings completely to extension APIs, but that hasn't seen
> much
> > progress. Exposing these content settings via an extension API shouldn't be
a
> > proble though, and then later we can reach consensus on whether to expose
this
> > in the normal content settings UI.
> 
> sure. can we have the discussion on the bug?

In the meantime, I'd be grateful for any feedback on the code.

I'm trying to learn more about how Blink and Chromium work, and I'm a hands-on
learner, so I've been writing patches to help me get there. Feedback would be
really, really helpful, even if this doesn't end up being committed.

[jochen (gone - plz use gerrit), 2013-10-22 08:25:30]

can you just reference existing media files in the layout tests instead of
copying them around?

https://codereview.chromium.org/27694002/diff/20001/LayoutTests/http/tests/pe...
File LayoutTests/http/tests/permissionclient/audio-permissions.html (right):

https://codereview.chromium.org/27694002/diff/20001/LayoutTests/http/tests/pe...
LayoutTests/http/tests/permissionclient/audio-permissions.html:1: <html>
<!DOCTYPE html>

https://codereview.chromium.org/27694002/diff/20001/LayoutTests/http/tests/pe...
LayoutTests/http/tests/permissionclient/audio-permissions.html:16: if
(window.testRunner) {
no { } for single-line if statements

https://codereview.chromium.org/27694002/diff/20001/Source/core/page/Settings.h
File Source/core/page/Settings.h (right):

https://codereview.chromium.org/27694002/diff/20001/Source/core/page/Settings...
Source/core/page/Settings.h:185: bool m_isMediaLoadingEnabled : 1;
i wouldn't introduce a new setting for this.

https://codereview.chromium.org/27694002/diff/20001/Source/web/FrameLoaderCli...
File Source/web/FrameLoaderClientImpl.h (right):

https://codereview.chromium.org/27694002/diff/20001/Source/web/FrameLoaderCli...
Source/web/FrameLoaderClientImpl.h:134: virtual bool allowMedia(bool
enabledPerSettings, const WebCore::KURL& mediaURL);
just allowMedia(mediaURL), I don't think we need a setting for this

[pwnall-personal, 2013-10-22 11:56:17]

https://codereview.chromium.org/27694002/diff/20001/LayoutTests/http/tests/pe...
File LayoutTests/http/tests/permissionclient/audio-permissions.html (right):

https://codereview.chromium.org/27694002/diff/20001/LayoutTests/http/tests/pe...
LayoutTests/http/tests/permissionclient/audio-permissions.html:1: <html>
On 2013/10/22 08:25:31, jochen wrote:
> <!DOCTYPE html>

Done.
Also added in the files that I drew inspiration from.

https://codereview.chromium.org/27694002/diff/20001/LayoutTests/http/tests/pe...
LayoutTests/http/tests/permissionclient/audio-permissions.html:16: if
(window.testRunner) {
On 2013/10/22 08:25:31, jochen wrote:
> no { } for single-line if statements

Done.

https://codereview.chromium.org/27694002/diff/20001/Source/core/page/Settings.h
File Source/core/page/Settings.h (right):

https://codereview.chromium.org/27694002/diff/20001/Source/core/page/Settings...
Source/core/page/Settings.h:185: bool m_isMediaLoadingEnabled : 1;
On 2013/10/22 08:25:31, jochen wrote:
> i wouldn't introduce a new setting for this.

Done.
Removed. Thank you!!

https://codereview.chromium.org/27694002/diff/20001/Source/web/FrameLoaderCli...
File Source/web/FrameLoaderClientImpl.h (right):

https://codereview.chromium.org/27694002/diff/20001/Source/web/FrameLoaderCli...
Source/web/FrameLoaderClientImpl.h:134: virtual bool allowMedia(bool
enabledPerSettings, const WebCore::KURL& mediaURL);
On 2013/10/22 08:25:31, jochen wrote:
> just allowMedia(mediaURL), I don't think we need a setting for this

Done.

[pwnall-personal, 2013-10-22 12:25:59]

Thank you very much for taking a look, jochen!


On 2013/10/22 08:25:30, jochen wrote:
> can you just reference existing media files in the layout tests instead of
> copying them around?

I removed the copies in LayoutTests/permissionclient.

I had to switch the file I was using in LayoutTests/http/permissionclient,
because it appears that only the LayoutTests/http directory is served by the
HTTP server. I changed the test to use a <video> instead of an <audio>, and used
a file I found in LayoutTests/http.

[jochen (gone - plz use gerrit), 2013-10-24 12:46:56]

ok

basically waiting for a decision what to do before we can continue here

[jochen (gone - plz use gerrit), 2013-10-24 13:55:48]

On 2013/10/24 12:46:56, jochen wrote:
> ok
> 
> basically waiting for a decision what to do before we can continue here

I've written Patrick a mail asking him to drive this to a decision. Will keep
you posted.

[eseidel, 2014-04-10 05:32:21]

Jochen: Was there a decision?  It's been 5 months. :/

[jochen (gone - plz use gerrit), 2014-04-10 07:30:29]

On 2014/04/10 05:32:21, eseidel wrote:
> Jochen: Was there a decision?  It's been 5 months. :/

UI review was requested (on the bug), but nothing happened - so I guess the old
decision that we don't want to have this feature still stands.

I'll ping the bug again (note it's a public bug, anybody could ping it).

[pwnall-personal, 2014-06-11 03:13:12]

On 2014/04/10 07:30:29, jochen traveling until Jun 23 wrote:
> On 2014/04/10 05:32:21, eseidel wrote:
> > Jochen: Was there a decision?  It's been 5 months. :/
> 
> UI review was requested (on the bug), but nothing happened - so I guess the
old
> decision that we don't want to have this feature still stands.
> 
> I'll ping the bug again (note it's a public bug, anybody could ping it).

The latest comments on the bug seem to indicate that the feature is desired, so
I'm resuming the work on it. I uploaded a rebased patch that seems to pass
tests.

The Content Shell's test_runner moved over to the Chromium repository, so I
pulled the test_runner changes that were originally in this CL to a new CL,
which landed.
https://codereview.chromium.org/312103004/ 

Can you please take another look?

[jochen (gone - plz use gerrit), 2014-06-12 14:44:53]

any reason you take the URL into account for the permission client (for plugins,
e.g., we don't)

I'd like Eric (or some other parser person) comment on the MediaDocumentParser
changes.

https://codereview.chromium.org/27694002/diff/554001/LayoutTests/http/tests/p...
File LayoutTests/http/tests/permissionclient/video-permissions.html (right):

https://codereview.chromium.org/27694002/diff/554001/LayoutTests/http/tests/p...
LayoutTests/http/tests/permissionclient/video-permissions.html:1: <html>
<!DOCTYPE html>

https://codereview.chromium.org/27694002/diff/554001/LayoutTests/http/tests/p...
LayoutTests/http/tests/permissionclient/video-permissions.html:15: function
done() {
nit. { on separate line

https://codereview.chromium.org/27694002/diff/554001/LayoutTests/http/tests/p...
LayoutTests/http/tests/permissionclient/video-permissions.html:21: var video =
document.querySelector('video');
nit. don't mix ' and "

https://codereview.chromium.org/27694002/diff/554001/LayoutTests/http/tests/p...
LayoutTests/http/tests/permissionclient/video-permissions.html:27: }
nit. add empty line

https://codereview.chromium.org/27694002/diff/554001/LayoutTests/permissioncl...
File LayoutTests/permissionclient/image-permissions.html (right):

https://codereview.chromium.org/27694002/diff/554001/LayoutTests/permissioncl...
LayoutTests/permissionclient/image-permissions.html:1: <!DOCTYPE html>
please don't change files unrelated to the CL

https://codereview.chromium.org/27694002/diff/554001/LayoutTests/permissioncl...
File LayoutTests/permissionclient/resources/audio.html (right):

https://codereview.chromium.org/27694002/diff/554001/LayoutTests/permissioncl...
LayoutTests/permissionclient/resources/audio.html:29: 
nit. no empty line here

[pwnall-personal, 2014-06-13 02:54:48]

Thank you for the feedback! Can you please take another look?

The main reason for having the URL in allowMedia() is consistency with
allowImage().

The extension API for image settings allows blocking images both by the frame
URL (primary pattern) and by the image URL (secondary pattern).
https://developer.chrome.com/extensions/contentSettings#property-images

I imagine having the media URL can be useful for e.g. blocking all media from a
site known for loud obnoxious video ads.

FWIW, the changes to MediaDocument.cpp parallel ImageDocument.cpp.
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

https://codereview.chromium.org/27694002/diff/554001/LayoutTests/http/tests/p...
File LayoutTests/http/tests/permissionclient/video-permissions.html (right):

https://codereview.chromium.org/27694002/diff/554001/LayoutTests/http/tests/p...
LayoutTests/http/tests/permissionclient/video-permissions.html:1: <html>
On 2014/06/12 14:44:53, jochen traveling until Jun 23 wrote:
> <!DOCTYPE html>

Done. Thank you!

https://codereview.chromium.org/27694002/diff/554001/LayoutTests/http/tests/p...
LayoutTests/http/tests/permissionclient/video-permissions.html:15: function
done() {
On 2014/06/12 14:44:53, jochen traveling until Jun 23 wrote:
> nit. { on separate line

Done.

https://codereview.chromium.org/27694002/diff/554001/LayoutTests/http/tests/p...
LayoutTests/http/tests/permissionclient/video-permissions.html:21: var video =
document.querySelector('video');
On 2014/06/12 14:44:53, jochen traveling until Jun 23 wrote:
> nit. don't mix ' and "

Done.

https://codereview.chromium.org/27694002/diff/554001/LayoutTests/http/tests/p...
LayoutTests/http/tests/permissionclient/video-permissions.html:27: }
On 2014/06/12 14:44:53, jochen traveling until Jun 23 wrote:
> nit. add empty line

Done.

https://codereview.chromium.org/27694002/diff/554001/LayoutTests/permissioncl...
File LayoutTests/permissionclient/image-permissions.html (right):

https://codereview.chromium.org/27694002/diff/554001/LayoutTests/permissioncl...
LayoutTests/permissionclient/image-permissions.html:1: <!DOCTYPE html>
On 2014/06/12 14:44:53, jochen traveling until Jun 23 wrote:
> please don't change files unrelated to the CL

Done. Sorry!

https://codereview.chromium.org/27694002/diff/554001/LayoutTests/permissioncl...
File LayoutTests/permissionclient/resources/audio.html (right):

https://codereview.chromium.org/27694002/diff/554001/LayoutTests/permissioncl...
LayoutTests/permissionclient/resources/audio.html:29: 
On 2014/06/12 14:44:53, jochen traveling until Jun 23 wrote:
> nit. no empty line here

Done.

[pwnall-personal, 2014-06-25 14:02:56]

jochen@: can you please take another look at this CL?

[jochen (gone - plz use gerrit), 2014-06-26 15:01:33]

lgtm

[pwnall-personal, 2014-06-26 15:19:17]

The CQ bit was checked by costan@gmail.com

[commit-bot: I haz the power, 2014-06-26 15:21:06]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/costan@gmail.com/27694002/614001

[commit-bot: I haz the power, 2014-06-26 15:21:15]

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

[commit-bot: I haz the power, 2014-06-26 15:21:16]

Failed to apply patch for Source/core/html/HTMLMediaElement.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/html/HTMLMediaElement.cpp
  Hunk #1 FAILED at 54.
  1 out of 1 hunk FAILED -- saving rejects to file
Source/core/html/HTMLMediaElement.cpp.rej

Patch:       Source/core/html/HTMLMediaElement.cpp
Index: Source/core/html/HTMLMediaElement.cpp
diff --git a/Source/core/html/HTMLMediaElement.cpp
b/Source/core/html/HTMLMediaElement.cpp
index
9c87d480a794990746763f004a4ea098f54cf084..d5508a418fd1e4021e88327e0ba4f1ab17682bcd
100644
--- a/Source/core/html/HTMLMediaElement.cpp
+++ b/Source/core/html/HTMLMediaElement.cpp
@@ -54,7 +54,6 @@
 #include "core/html/track/InbandTextTrack.h"
 #include "core/html/track/TextTrackCueList.h"
 #include "core/html/track/TextTrackList.h"
-#include "core/loader/FrameLoader.h"
 #include "core/rendering/RenderVideo.h"
 #include "core/rendering/RenderView.h"
 #include "core/rendering/compositing/RenderLayerCompositor.h"

[pwnall-personal, 2014-06-26 15:54:02]

The CQ bit was checked by costan@gmail.com

[commit-bot: I haz the power, 2014-06-26 15:55:47]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/costan@gmail.com/27694002/634001

[commit-bot: I haz the power, 2014-06-26 18:42:19]

Change committed as 177015

[pwnall-personal, 2014-06-26 19:21:26]

On 2014/06/26 15:01:33, jochen wrote:
> lgtm

Thank you for bearing with me through this long journey! :)