[Presentation API] Adding a flag to disable gesture check for starting presentations

[Presentation API] Adding a flag to disable gesture check for starting presentations

This patch adds a new command line flag to disable user gesture checks
for testing purposes. Before, a presentation session can only be started
only when triggered by a user gesture.

BUG=585404
Committed: https://crrev.com/1215ad421f1a69cea271d6f89d21e00bcf646db3
Cr-Commit-Position: refs/heads/master@{#374650}

[Zhiqiang Zhang (Slow), 2016-02-04 20:12:38]

zqzhang@chromium.org changed reviewers:
+ avayvod@chromium.org, mlamouri@chromium.org

[Zhiqiang Zhang (Slow), 2016-02-04 20:12:39]

PTAL.

* I've tested the patch on Android and it works (should be OK for other
platforms?)
* I'm not sure whether I've added the flag to all necessary places (e.g. there's
one in histograms.xml for "disable-gesture-requirement-for-media-playback", but
I'm didn't add the flag there)

[Zhiqiang Zhang (Slow), 2016-02-04 20:12:51]

Description was changed from

==========
[Presentation API]Adding a flag to disable gesture check for starting
presentations

This patch adds a new command line flag to disable user gesture checks
for testing purposes. Before, a presentation session can only be started
only when triggered by a user gesture.

BUG=
==========

to

==========
[Presentation API] Adding a flag to disable gesture check for starting
presentations

This patch adds a new command line flag to disable user gesture checks
for testing purposes. Before, a presentation session can only be started
only when triggered by a user gesture.

BUG=
==========

[whywhat, 2016-02-04 20:38:57]

On 2016/02/04 at 20:12:39, zqzhang wrote:
> PTAL.
> 
> * I've tested the patch on Android and it works (should be OK for other
platforms?)
> * I'm not sure whether I've added the flag to all necessary places (e.g.
there's one in histograms.xml for
"disable-gesture-requirement-for-media-playback", but I'm didn't add the flag
there)

Hm, it might not work as you stopped rejecting the promise if the value of the
setting is true (PresentationRequest.cpp:98).

[whywhat, 2016-02-04 20:39:08]

https://codereview.chromium.org/1660193007/diff/1/chrome/app/generated_resour...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1660193007/diff/1/chrome/app/generated_resour...
chrome/app/generated_resources.grd:5623: +        User gesture requirement for
playing media elements. Disabling this will allow auto-starting presentations.
I don't think we need chrome://flags for automated tests. You should probably
revert your changes for that (this file and about_flags.cc).

https://codereview.chromium.org/1660193007/diff/1/chromecast/browser/cast_bro...
File chromecast/browser/cast_browser_main_parts.cc (right):

https://codereview.chromium.org/1660193007/diff/1/chromecast/browser/cast_bro...
chromecast/browser/cast_browser_main_parts.cc:198: {
switches::kDisableGestureRequirementForMediaPlayback, ""},
what's up with this change? (the switch should be there but you shouldn't change
the file :)

https://codereview.chromium.org/1660193007/diff/1/content/public/android/java...
File
content/public/android/java/src/org/chromium/content/common/ContentSwitches.java
(right):

https://codereview.chromium.org/1660193007/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/common/ContentSwitches.java:67:
public static final String DISABLE_GESTURE_REQUIREMENT_FOR_PRESENTATION =
I don't think you need this change (do we use it from any Java file?)

https://codereview.chromium.org/1660193007/diff/1/content/shell/app/shell_mai...
File content/shell/app/shell_main_delegate.cc (right):

https://codereview.chromium.org/1660193007/diff/1/content/shell/app/shell_mai...
content/shell/app/shell_main_delegate.cc:177: command_line.AppendSwitch(
This is not used I think? It would disable the requirement for content shell
tests. Let's add it when we have the tests relying on it.

https://codereview.chromium.org/1660193007/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/presentation/PresentationRequest.cpp
(right):

https://codereview.chromium.org/1660193007/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/presentation/PresentationRequest.cpp:97:
Settings* settings = getSettings(executionContext());
I wonder if this can be null? Does similar code check for Settings being null?

https://codereview.chromium.org/1660193007/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/presentation/PresentationRequest.cpp:98: bool
bypassGestureCheck = settings && settings->presentationRequiresUserGesture();
I think the name of the variable means the opposite to its value.
isUserGestureRequired or something?

[Zhiqiang Zhang (Slow), 2016-02-05 16:12:38]

Patchset #2 (id:20001) has been deleted

[Zhiqiang Zhang (Slow), 2016-02-05 16:20:34]

PTAL.

* addressed avayvod@'s comments, see replies
* fixed several problems in the old patch

https://codereview.chromium.org/1660193007/diff/1/chrome/app/generated_resour...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1660193007/diff/1/chrome/app/generated_resour...
chrome/app/generated_resources.grd:5623: +        User gesture requirement for
playing media elements. Disabling this will allow auto-starting presentations.
On 2016/02/04 20:39:08, whywhat wrote:
> I don't think we need chrome://flags for automated tests. You should probably
> revert your changes for that (this file and about_flags.cc).

Done.

https://codereview.chromium.org/1660193007/diff/1/chromecast/browser/cast_bro...
File chromecast/browser/cast_browser_main_parts.cc (right):

https://codereview.chromium.org/1660193007/diff/1/chromecast/browser/cast_bro...
chromecast/browser/cast_browser_main_parts.cc:198: {
switches::kDisableGestureRequirementForMediaPlayback, ""},
On 2016/02/04 20:39:08, whywhat wrote:
> what's up with this change? (the switch should be there but you shouldn't
change
> the file :)

Done.

https://codereview.chromium.org/1660193007/diff/1/content/public/android/java...
File
content/public/android/java/src/org/chromium/content/common/ContentSwitches.java
(right):

https://codereview.chromium.org/1660193007/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/common/ContentSwitches.java:67:
public static final String DISABLE_GESTURE_REQUIREMENT_FOR_PRESENTATION =
On 2016/02/04 20:39:08, whywhat wrote:
> I don't think you need this change (do we use it from any Java file?)

Done.

https://codereview.chromium.org/1660193007/diff/1/content/shell/app/shell_mai...
File content/shell/app/shell_main_delegate.cc (right):

https://codereview.chromium.org/1660193007/diff/1/content/shell/app/shell_mai...
content/shell/app/shell_main_delegate.cc:177: command_line.AppendSwitch(
On 2016/02/04 20:39:08, whywhat wrote:
> This is not used I think? It would disable the requirement for content shell
> tests. Let's add it when we have the tests relying on it.

Done.

https://codereview.chromium.org/1660193007/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/presentation/PresentationRequest.cpp
(right):

https://codereview.chromium.org/1660193007/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/presentation/PresentationRequest.cpp:97:
Settings* settings = getSettings(executionContext());
On 2016/02/04 20:39:08, whywhat wrote:
> I wonder if this can be null? Does similar code check for Settings being null?

See here:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

The comment says that Document::settings() can be null.

https://codereview.chromium.org/1660193007/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/presentation/PresentationRequest.cpp:98: bool
bypassGestureCheck = settings && settings->presentationRequiresUserGesture();
On 2016/02/04 20:39:08, whywhat wrote:
> I think the name of the variable means the opposite to its value.
> isUserGestureRequired or something?

Done.

[Zhiqiang Zhang (Slow), 2016-02-05 17:10:33]

zqzhang@chromium.org changed reviewers:
+ jochen@chromium.org, kenrb@chromium.org

[Zhiqiang Zhang (Slow), 2016-02-05 17:10:34]

kenrb@chromium.org: Please review changes in 
    content/public/common/common_param_traits_macros.h

jochen@chromium.org: Please review changes in 
    content/*
    third_party/WebKit/*

[Zhiqiang Zhang (Slow), 2016-02-08 11:53:06]

zqzhang@chromium.org changed reviewers:
+ avi@chromium.org, philipj@opera.com

[Zhiqiang Zhang (Slow), 2016-02-08 11:53:06]

Since jochen@ is OOO, add other available reviewers.
avi@ PTAL at:
content/*

philipj@ PTAL at:
third_party/WebKit

[kenrb, 2016-02-08 15:14:15]

ipc lgtm

[Avi (use Gerrit), 2016-02-08 15:40:07]

lgtm

[philipj_slow, 2016-02-09 07:41:09]

third_party/WebKit LGTM % naming nit, and can you also fill in the BUG= for
context? I assume that this will be followed by another CL to actually use the
setting in the same bug.

https://codereview.chromium.org/1660193007/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/presentation/PresentationRequest.cpp
(right):

https://codereview.chromium.org/1660193007/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/presentation/PresentationRequest.cpp:40:
Settings* getSettings(ExecutionContext* executionContext)
If one calls this a getter,
https://www.chromium.org/blink/coding-style#TOC-Names thinks it should be called
settings(). That would better match the above presentationClient().
Alternatively, make this a bool isUserGestureRequired(ExecutionContext*
executionContext) to avoid the issue entirely.

[Zhiqiang Zhang (Slow), 2016-02-09 11:53:41]

Description was changed from

==========
[Presentation API] Adding a flag to disable gesture check for starting
presentations

This patch adds a new command line flag to disable user gesture checks
for testing purposes. Before, a presentation session can only be started
only when triggered by a user gesture.

BUG=
==========

to

==========
[Presentation API] Adding a flag to disable gesture check for starting
presentations

This patch adds a new command line flag to disable user gesture checks
for testing purposes. Before, a presentation session can only be started
only when triggered by a user gesture.

BUG=585404
==========

[Zhiqiang Zhang (Slow), 2016-02-09 12:15:06]

Addressed phlipj@'s comments.

PTAL for avayvod@ for PresentationRequest.cpp

+pfeldman@ to take a look at:
third_party/webkit/source/web/*

https://codereview.chromium.org/1660193007/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/presentation/PresentationRequest.cpp
(right):

https://codereview.chromium.org/1660193007/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/presentation/PresentationRequest.cpp:40:
Settings* getSettings(ExecutionContext* executionContext)
On 2016/02/09 07:41:09, philipj_UTC7 wrote:
> If one calls this a getter,
> https://www.chromium.org/blink/coding-style#TOC-Names thinks it should be
called
> settings(). That would better match the above presentationClient().
> Alternatively, make this a bool isUserGestureRequired(ExecutionContext*
> executionContext) to avoid the issue entirely.

Done.

[Zhiqiang Zhang (Slow), 2016-02-09 12:15:29]

zqzhang@chromium.org changed reviewers:
+ pfeldman@chromium.org

[Zhiqiang Zhang (Slow), 2016-02-09 12:15:30]

Addressed phlipj@'s comments.

PTAL for avayvod@ for PresentationRequest.cpp

+pfeldman@ to take a look at:
third_party/webkit/source/web/*

[pfeldman, 2016-02-09 20:18:09]

public web lgtm

[Zhiqiang Zhang (Slow), 2016-02-10 11:26:32]

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

[commit-bot: I haz the power, 2016-02-10 11:26:41]

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

[whywhat, 2016-02-10 12:08:36]

PresentationRequest lgtm

[commit-bot: I haz the power, 2016-02-10 13:00:29]

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

[commit-bot: I haz the power, 2016-02-10 13:00:29]

Dry run: This issue passed the CQ dry run.

[Zhiqiang Zhang (Slow), 2016-02-10 13:22:25]

The CQ bit was checked by zqzhang@chromium.org

[Zhiqiang Zhang (Slow), 2016-02-10 13:22:26]

The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org,
kenrb@chromium.org, philipj@opera.com
Link to the patchset: https://codereview.chromium.org/1660193007/#ps60001
(title: "addressed philipj@'s comment")

[commit-bot: I haz the power, 2016-02-10 13:22:35]

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

[commit-bot: I haz the power, 2016-02-10 13:27:29]

Description was changed from

==========
[Presentation API] Adding a flag to disable gesture check for starting
presentations

This patch adds a new command line flag to disable user gesture checks
for testing purposes. Before, a presentation session can only be started
only when triggered by a user gesture.

BUG=585404
==========

to

==========
[Presentation API] Adding a flag to disable gesture check for starting
presentations

This patch adds a new command line flag to disable user gesture checks
for testing purposes. Before, a presentation session can only be started
only when triggered by a user gesture.

BUG=585404
==========

[commit-bot: I haz the power, 2016-02-10 13:27:30]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-02-10 13:28:37]

Description was changed from

==========
[Presentation API] Adding a flag to disable gesture check for starting
presentations

This patch adds a new command line flag to disable user gesture checks
for testing purposes. Before, a presentation session can only be started
only when triggered by a user gesture.

BUG=585404
==========

to

==========
[Presentation API] Adding a flag to disable gesture check for starting
presentations

This patch adds a new command line flag to disable user gesture checks
for testing purposes. Before, a presentation session can only be started
only when triggered by a user gesture.

BUG=585404

Committed: https://crrev.com/1215ad421f1a69cea271d6f89d21e00bcf646db3
Cr-Commit-Position: refs/heads/master@{#374650}
==========

[commit-bot: I haz the power, 2016-02-10 13:28:38]

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