Add 'autoplay' content settings and expose it to Blink.

Add 'autoplay' content settings and expose it to Blink.

The new content settings is CONTENT_SETTINGS_TYPE_AUTOPLAY and it is exposed
to Blink via WebContentSettingsClient::allowAutoplay(). The client is kept is
sync by the browser process to avoid sync IPC or adding complex asynchronous
logic to HTMLMediaElement.

This CL doesn't add UI nor use the settings in order to keep things compartmented.
HTMLMediaElement will check the settings value in some situations.

More context in here:
https://docs.google.com/presentation/d/17ZXOdGosCso_SoMvQ9ckCsElvn_0YW2Q13y5ZqQ9dyU/edit

BUG=604751
Committed: https://crrev.com/709d8a936ef6aefde7411adb814744dc864c8d49
Cr-Commit-Position: refs/heads/master@{#390065}

[mlamouri (slow - plz ping), 2016-04-20 15:18:15]

mlamouri@chromium.org changed reviewers:
+ bauerb@chromium.org

[mlamouri (slow - plz ping), 2016-04-20 15:18:16]

+bauerb@ for early look.

[mlamouri (slow - plz ping), 2016-04-20 15:18:24]

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

[commit-bot: I haz the power, 2016-04-20 15:18:41]

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

[Bernhard Bauer, 2016-04-20 16:08:54]

Just as a data point for comparison: For plugins we do a synchronous IPC back to
the browser, and that hasn't created performance problems, so it might also be
fine for autoplay.

https://codereview.chromium.org/1906533002/diff/1/chrome/browser/chrome_conte...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/1906533002/diff/1/chrome/browser/chrome_conte...
chrome/browser/chrome_content_browser_client.cc:659: // By default, JavaScript
and images are enabled in guest content.
Update the comment.

[mlamouri (slow - plz ping), 2016-04-20 16:11:17]

On 2016/04/20 at 16:08:54, bauerb wrote:
> Just as a data point for comparison: For plugins we do a synchronous IPC back
to the browser, and that hasn't created performance problems, so it might also
be fine for autoplay.

I don't think adding sync IPC is an okay thing to do nowadays. As far as I know
the only alternative would be to have some async behaviour to check for autoplay
settings. We could do that but it would be heavy work given that
HTMLMediaElement basically checks for autoplay ability synchronously when play()
is called. We would need to delay playback until the browser process answers.
Doable but the overhead of using WebContentSettingsClient is way lower :)

[commit-bot: I haz the power, 2016-04-20 16:49:21]

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

[commit-bot: I haz the power, 2016-04-20 16:49:22]

Dry run: This issue passed the CQ dry run.

[mlamouri (slow - plz ping), 2016-04-20 18:34:44]

Description was changed from

==========
Add 'autoplay' content settings.

This is still WIP.

BUG=604751
==========

to

==========
Add 'autoplay' content settings and expose it to Blink.

The new content settings is CONTENT_SETTINGS_TYPE_AUTOPLAY and it is exposed
to Blink via WebContentSettingsClient::allowAutoplay(). The client is kept is
sync by the browser process to avoid sync IPC or adding complex asynchronous
logic to HTMLMediaElement.

This CL doesn't add UI nor use the settings in order to keep things
compartmented.
HTMLMediaElement will check the settings value in some situations.

More context in here:
https://docs.google.com/presentation/d/17ZXOdGosCso_SoMvQ9ckCsElvn_0YW2Q13y5Z...

BUG=604751
==========

[mlamouri (slow - plz ping), 2016-04-20 18:36:46]

mlamouri@chromium.org changed reviewers:
+ jochen@chromium.org, mkwst@chromium.org

[mlamouri (slow - plz ping), 2016-04-20 18:36:47]

Reviewers, can you PTAL at:

bauerb:
  chrome/browser/content_settings/host_content_settings_map_unittest.cc
  components/content_settings/*

jochen:
  chrome/browser/extensions/*
  chrome/renderer/*
  third_party/WebKit/*

mkwst:
  chrome/common/render_messages.h

Thanks! :)

https://codereview.chromium.org/1906533002/diff/1/chrome/browser/chrome_conte...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/1906533002/diff/1/chrome/browser/chrome_conte...
chrome/browser/chrome_content_browser_client.cc:659: // By default, JavaScript
and images are enabled in guest content.
On 2016/04/20 at 16:08:53, Bernhard Bauer wrote:
> Update the comment.

Done.

[mlamouri (slow - plz ping), 2016-04-20 18:36:55]

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

[commit-bot: I haz the power, 2016-04-20 18:37:22]

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

[commit-bot: I haz the power, 2016-04-20 20:57:18]

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

[commit-bot: I haz the power, 2016-04-20 20:57:19]

Dry run: This issue passed the CQ dry run.

[raymes, 2016-04-21 04:01:55]

raymes@chromium.org changed reviewers:
+ raymes@chromium.org

[raymes, 2016-04-21 04:01:56]

Just my thoughts :)

https://codereview.chromium.org/1906533002/diff/40001/chrome/renderer/content...
File chrome/renderer/content_settings_observer.cc (right):

https://codereview.chromium.org/1906533002/diff/40001/chrome/renderer/content...
chrome/renderer/content_settings_observer.cc:497:
frame->document().getSecurityOrigin().toString())) ==
Resolving content settings this way isn't great because it bypasses a lot of the
logic that lives in the browser. The code paths for figuring out what the actual
value is are different between browser/renderer (there are probably already
cases where it's wrong) and it means that if we implement new restrictions
around the setting in the browser, it won't get applied here. 

I know that it's hard to fix this because the value is needed synchronously (and
the existing code does this for other settings). Still, doing a sync IPC once
per navigation for all the values needed doesn't seem so bad if it means we
actually get correct values. We already have at least 3 sync IPCs in this file
for checking for access so we could potentially fold one (or more) of these IPCs
together and get all the info at once. For example, it looks like allowStorage
sends 2 sync IPCs already almost every navigation. So instead we could send 1
IPC and get the data for allowStorage as well as autoplay. This wouldn't
completely fix the existing problem for images/script because those may need to
check many origins per navigation...

[Bernhard Bauer, 2016-04-21 08:20:44]

https://codereview.chromium.org/1906533002/diff/40001/chrome/renderer/content...
File chrome/renderer/content_settings_observer.cc (right):

https://codereview.chromium.org/1906533002/diff/40001/chrome/renderer/content...
chrome/renderer/content_settings_observer.cc:497:
frame->document().getSecurityOrigin().toString())) ==
On 2016/04/21 04:01:56, raymes wrote:
> Resolving content settings this way isn't great because it bypasses a lot of
the
> logic that lives in the browser. The code paths for figuring out what the
actual
> value is are different between browser/renderer (there are probably already
> cases where it's wrong) and it means that if we implement new restrictions
> around the setting in the browser, it won't get applied here. 

Hm... it might be possible to make sure that the logic for doing these checks
renderer-side is the same as browser-side, if we do the separation between
content settings and permissions properly. If all the additional logic lives in
permissions, we could move it to common/ code, and abstract away the actual
mechanism that determines the raw content setting. Then in the browser the
content setting would be provided by HCSM, and in the renderer it would be
provided by GetContentSettingFromRules(). The logic inside of those would be the
same (HCSM just checks all the rules from all the providers in order).

> I know that it's hard to fix this because the value is needed synchronously
(and
> the existing code does this for other settings). Still, doing a sync IPC once
> per navigation for all the values needed doesn't seem so bad if it means we
> actually get correct values. We already have at least 3 sync IPCs in this file
> for checking for access so we could potentially fold one (or more) of these
IPCs
> together and get all the info at once. For example, it looks like allowStorage
> sends 2 sync IPCs already almost every navigation. So instead we could send 1
> IPC and get the data for allowStorage as well as autoplay. This wouldn't
> completely fix the existing problem for images/script because those may need
to
> check many origins per navigation...

[mlamouri (slow - plz ping), 2016-04-21 15:16:47]

I don't think we should add any new sync IPC. In the long term, we should get
ride of sync IPCs as far as I know. I think what Bernhard suggests make sense
and we should probably simply allow these checks to be done by the renderer if
needed. I would gladly tackle this in a follow-up.

[mlamouri (slow - plz ping), 2016-04-25 13:08:01]

Review ping.

[Bernhard Bauer, 2016-04-25 14:01:49]

LGTM

I'm still not a huge fan of pushing down the content setting rules (for
images/scripts that was basically a hack we did to not kill performance), but
OTOH sync IPCs aren't great either…

https://codereview.chromium.org/1906533002/diff/40001/chrome/renderer/content...
File chrome/renderer/content_settings_observer_browsertest.cc (right):

https://codereview.chromium.org/1906533002/diff/40001/chrome/renderer/content...
chrome/renderer/content_settings_observer_browsertest.cc:402:
::testing::Mock::VerifyAndClearExpectations(&observer);
The initial :: isn't necessary if you're in the global namespace.

[raymes, 2016-04-26 05:11:04]

I agree. I wasn't suggesting adding a new sync IPC, instead reusing an existing
one. 

I'm definitely in agreement that sync IPCs should be avoided. They've caused me
a lot of pain in pepper and I avoid them as much as possible :) I once gave a
talk in the Sydney office about the problems of sync IPCs! But in this case we
wouldn't be making matters worse. When/if we do the work to get rid of the other
sync IPCs from this file, we would also be able to remove the one in question.
Also, the number of sync IPC calls would stay the same (or go down with some
cleanup as suggested) and we would get correct behavior.

> Hm... it might be possible to make sure that the logic for doing these checks
> renderer-side is the same as browser-side, if we do the separation between
> content settings and permissions properly

I thought about something like this too. However given the current state of the
code health in permissions land, I'd be worried that trying to do something like
this right now would make things worse. I'd rather not try to share the code
between renderer/browser until we've addressed the current refactorings that are
in progress, even if we decide to go with the current CL. 

Thanks!

[raymes, 2016-04-26 09:28:39]

Btw please feel free to land this if you need to make progress and we can
continue the discussion or revisit after :)

On Tue, 26 Apr 2016, 3:11 PM <raymes@chromium.org> wrote:

> I agree. I wasn't suggesting adding a new sync IPC, instead reusing an
> existing
> one.
>
> I'm definitely in agreement that sync IPCs should be avoided. They've
> caused me
> a lot of pain in pepper and I avoid them as much as possible :) I once
> gave a
> talk in the Sydney office about the problems of sync IPCs! But in this
> case we
> wouldn't be making matters worse. When/if we do the work to get rid of the
> other
> sync IPCs from this file, we would also be able to remove the one in
> question.
> Also, the number of sync IPC calls would stay the same (or go down with
> some
> cleanup as suggested) and we would get correct behavior.
>
>
> > Hm... it might be possible to make sure that the logic for doing these
> checks
> > renderer-side is the same as browser-side, if we do the separation
> between
> > content settings and permissions properly
>
> I thought about something like this too. However given the current state
> of the
> code health in permissions land, I'd be worried that trying to do
> something like
> this right now would make things worse. I'd rather not try to share the
> code
> between renderer/browser until we've addressed the current refactorings
> that are
> in progress, even if we decide to go with the current CL.
>
> Thanks!
>
> https://codereview.chromium.org/1906533002/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[raymes, 2016-04-26 09:28:39]

Btw please feel free to land this if you need to make progress and we can
continue the discussion or revisit after :)

On Tue, 26 Apr 2016, 3:11 PM <raymes@chromium.org> wrote:

> I agree. I wasn't suggesting adding a new sync IPC, instead reusing an
> existing
> one.
>
> I'm definitely in agreement that sync IPCs should be avoided. They've
> caused me
> a lot of pain in pepper and I avoid them as much as possible :) I once
> gave a
> talk in the Sydney office about the problems of sync IPCs! But in this
> case we
> wouldn't be making matters worse. When/if we do the work to get rid of the
> other
> sync IPCs from this file, we would also be able to remove the one in
> question.
> Also, the number of sync IPC calls would stay the same (or go down with
> some
> cleanup as suggested) and we would get correct behavior.
>
>
> > Hm... it might be possible to make sure that the logic for doing these
> checks
> > renderer-side is the same as browser-side, if we do the separation
> between
> > content settings and permissions properly
>
> I thought about something like this too. However given the current state
> of the
> code health in permissions land, I'd be worried that trying to do
> something like
> this right now would make things worse. I'd rather not try to share the
> code
> between renderer/browser until we've addressed the current refactorings
> that are
> in progress, even if we decide to go with the current CL.
>
> Thanks!
>
> https://codereview.chromium.org/1906533002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[mlamouri (slow - plz ping), 2016-04-26 15:25:37]

jochen@ and mkwst@, can you PTAL :)

[jochen (gone - plz use gerrit), 2016-04-27 12:02:16]

lgtm

[Mike West, 2016-04-27 12:20:05]

IPC LGTM.

[mlamouri (slow - plz ping), 2016-04-27 12:51:49]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2016-04-27 12:51:58]

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

[commit-bot: I haz the power, 2016-04-27 14:25:09]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-04-27 14:26:16]

Description was changed from

==========
Add 'autoplay' content settings and expose it to Blink.

The new content settings is CONTENT_SETTINGS_TYPE_AUTOPLAY and it is exposed
to Blink via WebContentSettingsClient::allowAutoplay(). The client is kept is
sync by the browser process to avoid sync IPC or adding complex asynchronous
logic to HTMLMediaElement.

This CL doesn't add UI nor use the settings in order to keep things
compartmented.
HTMLMediaElement will check the settings value in some situations.

More context in here:
https://docs.google.com/presentation/d/17ZXOdGosCso_SoMvQ9ckCsElvn_0YW2Q13y5Z...

BUG=604751
==========

to

==========
Add 'autoplay' content settings and expose it to Blink.

The new content settings is CONTENT_SETTINGS_TYPE_AUTOPLAY and it is exposed
to Blink via WebContentSettingsClient::allowAutoplay(). The client is kept is
sync by the browser process to avoid sync IPC or adding complex asynchronous
logic to HTMLMediaElement.

This CL doesn't add UI nor use the settings in order to keep things
compartmented.
HTMLMediaElement will check the settings value in some situations.

More context in here:
https://docs.google.com/presentation/d/17ZXOdGosCso_SoMvQ9ckCsElvn_0YW2Q13y5Z...

BUG=604751

Committed: https://crrev.com/709d8a936ef6aefde7411adb814744dc864c8d49
Cr-Commit-Position: refs/heads/master@{#390065}
==========

[commit-bot: I haz the power, 2016-04-27 14:26:18]

Patchset 3 (id:??) landed as
https://crrev.com/709d8a936ef6aefde7411adb814744dc864c8d49
Cr-Commit-Position: refs/heads/master@{#390065}