Add CastMediaBlocker and BrowserTest

Add CastMediaBlocker and BrowserTest

CastMediaBlocker is a class which can prevent media from being played
on a web page.

BUG=NONE
TEST=ChromecastShellMediaBlockingBrowserTest

Committed: https://crrev.com/502bcc946bd12e907572f8b22ef4a0abbc8190d0
Cr-Commit-Position: refs/heads/master@{#418897}

[derekjchow1, 2016-09-12 22:38:27]

derekjchow@chromium.org changed reviewers:
+ halliwell@chromium.org

[derekjchow1, 2016-09-12 22:38:28]

Will send the internal CL for review soon.

[derekjchow1, 2016-09-12 23:25:52]

derekjchow@chromium.org changed reviewers:
+ byungchul@chromium.org

[derekjchow1, 2016-09-12 23:25:52]

+Byungchul

[derekjchow1, 2016-09-12 23:26:04]

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

[commit-bot: I haz the power, 2016-09-12 23:26:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[byungchul, 2016-09-12 23:32:59]

https://codereview.chromium.org/2330243002/diff/20001/chromecast/browser/cast...
File chromecast/browser/cast_media_blocker.h (right):

https://codereview.chromium.org/2330243002/diff/20001/chromecast/browser/cast...
chromecast/browser/cast_media_blocker.h:47: // Whether or not this web app
should try to resume playing media. Should only
"resume playing media when media loading is unblocked"?

It's really confusing. Can we have a state machine and one integer flag for
state?

[commit-bot: I haz the power, 2016-09-12 23:42:26]

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

[commit-bot: I haz the power, 2016-09-12 23:42:27]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[derekjchow1, 2016-09-13 17:36:44]

https://codereview.chromium.org/2330243002/diff/20001/chromecast/browser/cast...
File chromecast/browser/cast_media_blocker.h (right):

https://codereview.chromium.org/2330243002/diff/20001/chromecast/browser/cast...
chromecast/browser/cast_media_blocker.h:47: // Whether or not this web app
should try to resume playing media. Should only
On 2016/09/12 23:32:59, byungchul wrote:
> "resume playing media when media loading is unblocked"?
> 
> It's really confusing. Can we have a state machine and one integer flag for
> state?

Done. Take a look at the most recent patchset.

[halliwell, 2016-09-13 23:59:47]

Add [Chromecast] to start of commit message.

https://codereview.chromium.org/2330243002/diff/40001/chromecast/browser/cast...
File chromecast/browser/cast_media_blocker.cc (right):

https://codereview.chromium.org/2330243002/diff/40001/chromecast/browser/cast...
chromecast/browser/cast_media_blocker.cc:25: state_ = (state_ == BLOCKED ?
UNBLOCKING : UNBLOCKED);
Can state_ be UNBLOCKING when this is called?  If so, why does it transition to
UNBLOCKED here?  I thought the UNBLOCKING->UNBLOCKED transition should only
happen when Resume is called?

https://codereview.chromium.org/2330243002/diff/40001/chromecast/browser/cast...
File chromecast/browser/cast_media_blocker.h (right):

https://codereview.chromium.org/2330243002/diff/40001/chromecast/browser/cast...
chromecast/browser/cast_media_blocker.h:18: explicit
CastMediaBlocker(content::WebContents* web_contents);
nit, forward-declare WebContents

[derekjchow1, 2016-09-14 00:12:59]

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

[commit-bot: I haz the power, 2016-09-14 00:13:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[derekjchow1, 2016-09-14 00:14:05]

Done to the commit message.

https://codereview.chromium.org/2330243002/diff/40001/chromecast/browser/cast...
File chromecast/browser/cast_media_blocker.cc (right):

https://codereview.chromium.org/2330243002/diff/40001/chromecast/browser/cast...
chromecast/browser/cast_media_blocker.cc:25: state_ = (state_ == BLOCKED ?
UNBLOCKING : UNBLOCKED);
On 2016/09/13 23:59:47, halliwell wrote:
> Can state_ be UNBLOCKING when this is called?  If so, why does it transition
to
> UNBLOCKED here?  I thought the UNBLOCKING->UNBLOCKED transition should only
> happen when Resume is called?

Good point. If we're unblocking, we should always go to the UNBLOCKING state,
unless we're already UNBLOCKED.

https://codereview.chromium.org/2330243002/diff/40001/chromecast/browser/cast...
File chromecast/browser/cast_media_blocker.h (right):

https://codereview.chromium.org/2330243002/diff/40001/chromecast/browser/cast...
chromecast/browser/cast_media_blocker.h:18: explicit
CastMediaBlocker(content::WebContents* web_contents);
On 2016/09/13 23:59:47, halliwell wrote:
> nit, forward-declare WebContents

Done.

[commit-bot: I haz the power, 2016-09-14 01:40:20]

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

[commit-bot: I haz the power, 2016-09-14 01:40:21]

Dry run: This issue passed the CQ dry run.

[halliwell, 2016-09-14 02:15:41]

On 2016/09/14 00:14:05, derekjchow1 wrote:
> Done to the commit message.
> 
>
https://codereview.chromium.org/2330243002/diff/40001/chromecast/browser/cast...
> File chromecast/browser/cast_media_blocker.cc (right):
> 
>
https://codereview.chromium.org/2330243002/diff/40001/chromecast/browser/cast...
> chromecast/browser/cast_media_blocker.cc:25: state_ = (state_ == BLOCKED ?
> UNBLOCKING : UNBLOCKED);
> On 2016/09/13 23:59:47, halliwell wrote:
> > Can state_ be UNBLOCKING when this is called?  If so, why does it transition
> to
> > UNBLOCKED here?  I thought the UNBLOCKING->UNBLOCKED transition should only
> > happen when Resume is called?
> 
> Good point. If we're unblocking, we should always go to the UNBLOCKING state,
> unless we're already UNBLOCKED.
> 
>
https://codereview.chromium.org/2330243002/diff/40001/chromecast/browser/cast...
> File chromecast/browser/cast_media_blocker.h (right):
> 
>
https://codereview.chromium.org/2330243002/diff/40001/chromecast/browser/cast...
> chromecast/browser/cast_media_blocker.h:18: explicit
> CastMediaBlocker(content::WebContents* web_contents);
> On 2016/09/13 23:59:47, halliwell wrote:
> > nit, forward-declare WebContents
> 
> Done.

lgtm.
Curious: does this test cover all the different edge cases that were found in
recent bugs?

[byungchul, 2016-09-14 02:20:17]

lgtm

and nits

https://codereview.chromium.org/2330243002/diff/60001/chromecast/browser/cast...
File chromecast/browser/cast_media_blocker.cc (right):

https://codereview.chromium.org/2330243002/diff/60001/chromecast/browser/cast...
chromecast/browser/cast_media_blocker.cc:25: state_ = (state_ == UNBLOCKED ?
UNBLOCKED : UNBLOCKING);
why not just:

} else if (state_ == BLOCKED) {
  state_ = UNBLOCKING;
}

?

https://codereview.chromium.org/2330243002/diff/60001/chromecast/browser/cast...
chromecast/browser/cast_media_blocker.cc:44: if (state_ == UNBLOCKING &&
suspended_) {
else if?

[derekjchow1, 2016-09-14 20:33:13]

> Curious: does this test cover all the different edge cases that were found in
> recent bugs?

I think this is sufficient. The edge case bugs we found were always in
unblocking of media, resulting in a crash or media not being paused.

https://codereview.chromium.org/2330243002/diff/60001/chromecast/browser/cast...
File chromecast/browser/cast_media_blocker.cc (right):

https://codereview.chromium.org/2330243002/diff/60001/chromecast/browser/cast...
chromecast/browser/cast_media_blocker.cc:25: state_ = (state_ == UNBLOCKED ?
UNBLOCKED : UNBLOCKING);
On 2016/09/14 02:20:17, byungchul wrote:
> why not just:
> 
> } else if (state_ == BLOCKED) {
>   state_ = UNBLOCKING;
> }
> 
> ?

Done.

https://codereview.chromium.org/2330243002/diff/60001/chromecast/browser/cast...
chromecast/browser/cast_media_blocker.cc:44: if (state_ == UNBLOCKING &&
suspended_) {
On 2016/09/14 02:20:17, byungchul wrote:
> else if?

Done.

[derekjchow1, 2016-09-15 16:50:14]

The CQ bit was checked by derekjchow@chromium.org

[derekjchow1, 2016-09-15 16:50:15]

The patchset sent to the CQ was uploaded after l-g-t-m from
byungchul@chromium.org, halliwell@chromium.org
Link to the patchset: https://codereview.chromium.org/2330243002/#ps80001
(title: "Use if-else")

[commit-bot: I haz the power, 2016-09-15 16:50:42]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-15 17:35:41]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-09-15 17:38:39]

Description was changed from

==========
Add CastMediaBlocker and BrowserTest

CastMediaBlocker is a class which can prevent media from being played
on a web page.

BUG=NONE
TEST=ChromecastShellMediaBlockingBrowserTest
==========

to

==========
Add CastMediaBlocker and BrowserTest

CastMediaBlocker is a class which can prevent media from being played
on a web page.

BUG=NONE
TEST=ChromecastShellMediaBlockingBrowserTest

Committed: https://crrev.com/502bcc946bd12e907572f8b22ef4a0abbc8190d0
Cr-Commit-Position: refs/heads/master@{#418897}
==========

[commit-bot: I haz the power, 2016-09-15 17:38:40]

Patchset 5 (id:??) landed as
https://crrev.com/502bcc946bd12e907572f8b22ef4a0abbc8190d0
Cr-Commit-Position: refs/heads/master@{#418897}

[jbroman, 2016-09-15 20:16:40]

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.chromium.org/2345053002/ by jbroman@chromium.org.

The reason for reverting is: Test is failing on Cast Linux:
https://build.chromium.org/p/chromium.linux/builders/Cast%20Linux/builds/31563.