Add flag for enabling/disabling video fullscreen detection

Add flag for enabling/disabling video fullscreen detection

This CL addes flag for enabling/disabling video fullscreen detection. So
we can reduce the overhead of MediaCustomControlsFullscreenDetector if
the client side doesn't need it.

The downstream change will be in the subclass of AppHooks.java.

BUG=699565
Review-Url: https://codereview.chromium.org/2738893002
Cr-Commit-Position: refs/heads/master@{#457412}
Committed: https://chromium.googlesource.com/chromium/src/+/93e959d18bd91a0ebd267561d44d8f29cb2561d9

[Zhiqiang Zhang (Slow), 2017-03-08 15:43:24]

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

[Zhiqiang Zhang (Slow), 2017-03-08 15:43:24]

PTAL

[Zhiqiang Zhang (Slow), 2017-03-08 15:43:29]

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

[commit-bot: I haz the power, 2017-03-08 15:43:55]

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

[Zhiqiang Zhang (Slow), 2017-03-08 15:46:20]

Description was changed from

==========
Add flag for enabling/disabling video fullscreen detection

This CL addes flag for enabling/disabling video fullscreen detection. So
we can reduce the overhead of MediaCustomControlsFullscreenDetector if
the client side doesn't need it.

BUG=699565
==========

to

==========
Add flag for enabling/disabling video fullscreen detection

This CL addes flag for enabling/disabling video fullscreen detection. So
we can reduce the overhead of MediaCustomControlsFullscreenDetector if
the client side doesn't need it.

The downstream change will be in the subclass of AppHooks.java.

BUG=699565
==========

[Zhiqiang Zhang (Slow), 2017-03-08 15:46:46]

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

[commit-bot: I haz the power, 2017-03-08 15:52:42]

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

[commit-bot: I haz the power, 2017-03-08 15:52:43]

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...)

[David Trainor- moved to gerrit, 2017-03-09 17:15:30]

lgtm

https://codereview.chromium.org/2738893002/diff/1/chrome/browser/android/app_...
File chrome/browser/android/app_hooks_helper.h (right):

https://codereview.chromium.org/2738893002/diff/1/chrome/browser/android/app_...
chrome/browser/android/app_hooks_helper.h:14: class AppHooksHelper {
Should this just be AppHooks?  I'm fine either way though.

[Zhiqiang Zhang (Slow), 2017-03-09 18:20:17]

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

[commit-bot: I haz the power, 2017-03-09 18:20:43]

Dry run: 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, 2017-03-09 21:12:26]

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

[commit-bot: I haz the power, 2017-03-09 21:12:27]

Dry run: This issue passed the CQ dry run.

[mlamouri (slow - plz ping), 2017-03-14 12:02:24]

https://codereview.chromium.org/2738893002/diff/20001/chrome/browser/android/...
File chrome/browser/android/app_hooks.h (right):

https://codereview.chromium.org/2738893002/diff/20001/chrome/browser/android/...
chrome/browser/android/app_hooks.h:20: ~AppHooks() {}
Can you implement these in the cc file and mark them as default?

https://codereview.chromium.org/2738893002/diff/20001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/2738893002/diff/20001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:2690: }
I don't think we need that flag. WebView doesn't use chrome/browser/ and the
pref is `false` by default. If someone doesn't want the feature.
`ShouldDetectVideoFullscreen()` should return `false`. If that's not called, it
would be `false` anyway.

https://codereview.chromium.org/2738893002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.json5 (right):

https://codereview.chromium.org/2738893002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/RuntimeEnabledFeatures.json5:1036: status:
"test",
Can we drop "test"? That would enable it for all LayoutTests. I would prefer us
to turn it on when we want to instead.

[Zhiqiang Zhang (Slow), 2017-03-14 13:15:28]

zqzhang@chromium.org changed reviewers:
+ clamy@chromium.org, mkwst@chromium.org

[Zhiqiang Zhang (Slow), 2017-03-14 13:15:29]

PTAL

+mkwst:
third_party/WebKit/
content/public/common/common_param_traints_macros.h

+clamy:
content/

https://codereview.chromium.org/2738893002/diff/1/chrome/browser/android/app_...
File chrome/browser/android/app_hooks_helper.h (right):

https://codereview.chromium.org/2738893002/diff/1/chrome/browser/android/app_...
chrome/browser/android/app_hooks_helper.h:14: class AppHooksHelper {
On 2017/03/09 at 17:15:30, David Trainor-ping if over 24h wrote:
> Should this just be AppHooks?  I'm fine either way though.

Done.

https://codereview.chromium.org/2738893002/diff/20001/chrome/browser/android/...
File chrome/browser/android/app_hooks.h (right):

https://codereview.chromium.org/2738893002/diff/20001/chrome/browser/android/...
chrome/browser/android/app_hooks.h:20: ~AppHooks() {}
On 2017/03/14 at 12:02:24, mlamouri wrote:
> Can you implement these in the cc file and mark them as default?

Done

https://codereview.chromium.org/2738893002/diff/20001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/2738893002/diff/20001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:2690: }
On 2017/03/14 at 12:02:24, mlamouri wrote:
> I don't think we need that flag. WebView doesn't use chrome/browser/ and the
pref is `false` by default. If someone doesn't want the feature.
`ShouldDetectVideoFullscreen()` should return `false`. If that's not called, it
would be `false` anyway.

Done.

https://codereview.chromium.org/2738893002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.json5 (right):

https://codereview.chromium.org/2738893002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/RuntimeEnabledFeatures.json5:1036: status:
"test",
On 2017/03/14 at 12:02:24, mlamouri wrote:
> Can we drop "test"? That would enable it for all LayoutTests. I would prefer
us to turn it on when we want to instead.

Done.

[Zhiqiang Zhang (Slow), 2017-03-14 13:15:37]

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

[commit-bot: I haz the power, 2017-03-14 13:16:04]

Dry run: 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, 2017-03-14 13:59:22]

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

[commit-bot: I haz the power, 2017-03-14 13:59:23]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Zhiqiang Zhang (Slow), 2017-03-14 15:13:19]

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

[commit-bot: I haz the power, 2017-03-14 15:14:03]

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

[mlamouri (slow - plz ping), 2017-03-14 15:36:15]

lgtm

https://codereview.chromium.org/2738893002/diff/60001/chrome/browser/android/...
File chrome/browser/android/app_hooks.h (right):

https://codereview.chromium.org/2738893002/diff/60001/chrome/browser/android/...
chrome/browser/android/app_hooks.h:14: class AppHooks {
Actually, just realised you could use = delete on the default ctor as a better
way to prevent the construction. Makes me think you might want to add a top
level comment explaining what the class is about.

https://codereview.chromium.org/2738893002/diff/60001/chrome/browser/android/...
chrome/browser/android/app_hooks.h:19: AppHooks();
so, as said above, shouldn't this be = delete?

[Mike West, 2017-03-14 15:47:54]

LGTM.

[commit-bot: I haz the power, 2017-03-14 16:26:56]

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

[commit-bot: I haz the power, 2017-03-14 16:26:57]

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

[Zhiqiang Zhang (Slow), 2017-03-14 17:11:57]

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

[commit-bot: I haz the power, 2017-03-14 17:12:24]

Dry run: 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, 2017-03-14 19:24:59]

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

[commit-bot: I haz the power, 2017-03-14 19:25:00]

Dry run: This issue passed the CQ dry run.

[Zhiqiang Zhang (Slow), 2017-03-15 16:32:44]

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

[alexmos, 2017-03-15 16:53:10]

content/ LGTM

[Zhiqiang Zhang (Slow), 2017-03-15 17:43:50]

The CQ bit was checked by zqzhang@chromium.org

[Zhiqiang Zhang (Slow), 2017-03-15 17:43:51]

The patchset sent to the CQ was uploaded after l-g-t-m from
dtrainor@chromium.org, mkwst@chromium.org, mlamouri@chromium.org
Link to the patchset: https://codereview.chromium.org/2738893002/#ps80001
(title: "fixed tests")

[commit-bot: I haz the power, 2017-03-15 17:44:20]

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, 2017-03-15 21:09:07]

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

[commit-bot: I haz the power, 2017-03-15 21:09:09]

Failed to apply patch for
third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetectorTest.cpp:
While running git apply --index -p1;
  error: patch failed:
third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetectorTest.cpp:7
  error:
third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetectorTest.cpp:
patch does not apply

Patch:      
third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetectorTest.cpp
Index:
third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetectorTest.cpp
diff --git
a/third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetectorTest.cpp
b/third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetectorTest.cpp
index
2d7fa04925713e13216932bc04000c41a317c787..24b389b9e2d22dc395f98682e6a4cc0daa0ddad3
100644
---
a/third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetectorTest.cpp
+++
b/third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetectorTest.cpp
@@ -7,6 +7,7 @@
 #include "core/EventTypeNames.h"
 #include "core/html/HTMLVideoElement.h"
 #include "core/testing/DummyPageHolder.h"
+#include "platform/RuntimeEnabledFeatures.h"
 #include "platform/geometry/IntRect.h"
 #include "public/platform/WebMediaPlayer.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -26,6 +27,11 @@ struct TestParam {
 class MediaCustomControlsFullscreenDetectorTest : public ::testing::Test {
  protected:
   void SetUp() override {
+    m_originalVideoFullscreenDetectionEnabled =
+        RuntimeEnabledFeatures::videoFullscreenDetectionEnabled();
+
+    RuntimeEnabledFeatures::setVideoFullscreenDetectionEnabled(true);
+
     m_pageHolder = DummyPageHolder::create();
     m_newPageHolder = DummyPageHolder::create();
 
@@ -33,6 +39,11 @@ class MediaCustomControlsFullscreenDetectorTest : public
::testing::Test {
     document().body()->appendChild(&videoElement());
   }
 
+  void TearDown() override {
+    RuntimeEnabledFeatures::setVideoFullscreenDetectionEnabled(
+        m_originalVideoFullscreenDetectionEnabled);
+  }
+
   HTMLVideoElement& videoElement() const { return *m_video; }
   MediaCustomControlsFullscreenDetector& fullscreenDetector() const {
     return *videoElement().m_customControlsFullscreenDetector;
@@ -66,6 +77,8 @@ class MediaCustomControlsFullscreenDetectorTest : public
::testing::Test {
   std::unique_ptr<DummyPageHolder> m_pageHolder;
   std::unique_ptr<DummyPageHolder> m_newPageHolder;
   Persistent<HTMLVideoElement> m_video;
+
+  bool m_originalVideoFullscreenDetectionEnabled;
 };
 
 TEST_F(MediaCustomControlsFullscreenDetectorTest, computeIsDominantVideo) {

[Zhiqiang Zhang (Slow), 2017-03-16 11:05:23]

The CQ bit was checked by zqzhang@chromium.org

[Zhiqiang Zhang (Slow), 2017-03-16 11:05:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
dtrainor@chromium.org, mkwst@chromium.org, alexmos@chromium.org,
mlamouri@chromium.org
Link to the patchset: https://codereview.chromium.org/2738893002/#ps100001
(title: "rebased")

[commit-bot: I haz the power, 2017-03-16 11:05:39]

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, 2017-03-16 13:10:24]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1489662323069790,
"parent_rev": "955f4364ffa5c275f04cb3c2d19f7ac63f7d8320", "commit_rev":
"93e959d18bd91a0ebd267561d44d8f29cb2561d9"}

[commit-bot: I haz the power, 2017-03-16 13:11:07]

Description was changed from

==========
Add flag for enabling/disabling video fullscreen detection

This CL addes flag for enabling/disabling video fullscreen detection. So
we can reduce the overhead of MediaCustomControlsFullscreenDetector if
the client side doesn't need it.

The downstream change will be in the subclass of AppHooks.java.

BUG=699565
==========

to

==========
Add flag for enabling/disabling video fullscreen detection

This CL addes flag for enabling/disabling video fullscreen detection. So
we can reduce the overhead of MediaCustomControlsFullscreenDetector if
the client side doesn't need it.

The downstream change will be in the subclass of AppHooks.java.

BUG=699565

Review-Url: https://codereview.chromium.org/2738893002
Cr-Commit-Position: refs/heads/master@{#457412}
Committed:
https://chromium.googlesource.com/chromium/src/+/93e959d18bd91a0ebd267561d44d...
==========

[commit-bot: I haz the power, 2017-03-16 13:11:08]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/93e959d18bd91a0ebd267561d44d...