[Media,Remoting] Remove the disable remoting button on Android

third_party/WebKit/Source/core/html/shadow/MediaRemotingElements.cpp

 // Copyright 2017 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "core/html/shadow/MediaRemotingElements.h"
 
 #include "core/dom/ClientRect.h"
 #include "core/dom/shadow/ShadowRoot.h"
 #include "core/events/MouseEvent.h"
 #include "core/html/HTMLVideoElement.h"
@@ skipping 46 matching lines @@
     MediaRemotingInterstitial& interstitial)
     : HTMLDivElement(interstitial.GetDocument()), interstitial_(interstitial) {
   listener_ = new MouseEventsListener(*this);
   SetShadowPseudoId(AtomicString("-internal-media-remoting-disable-button"));
   setInnerText(interstitial.GetVideoElement().GetLocale().QueryString(
                    WebLocalizedString::kMediaRemotingDisableText),
                ASSERT_NO_EXCEPTION);
 }
 
 void MediaRemotingExitButtonElement::OnShown() {
+  if (const ComputedStyle* style = NonLayoutObjectComputedStyle()) {
+    if (style->Display() == EDisplay::kNone)

[xjz, 2017/05/25 19:02:18]

Have you tested on this? I am not sure whether this works. IIUC, the
showing/hiding of the button is decided by the previous
RemoveInlineStyleProperty()/SetInlineStyleProperty() call:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/html/shad...

OnShown()/OnHidden() here are not to show/hide the button, but to
register/de-register the event handler.

[whywhat, 2017/05/25 20:26:29]

The hiding of the button happens via the mediacontrolsandroid.css that affects
the button only. The one you pointed to affects the whole interstitial, AFAIK.
Yes, my intention here is not to register/unregister the event handler in case
the button is not visible.
The names became a bit confusing though. It now mean more like
"OnInterstitialShown" rather than "OnExitButtonShown" :)

Mounir suggested that the event handlers won't get called for display:none
elements anyway, but still, registering them felt like a waste.

[xjz, 2017/05/25 21:20:34]

Yes, you are correct. When showing/hiding the interstitial, only the display
property of the whole interstitial (MediaRemotingInterstitial) is changed, not
the separate elements.

I still feel a little confusing to do the check here. Maybe we can do the check
in MediaRemotingInterstitial::Show()/Hide(). If the Display property of the
|exit_button_| is kNone (indicating that the button is not visible), don't call
this OnShown()/OnHidden(). WDYT?

[rune, 2017/05/26 11:17:57]

Querying the computed style requires that the computed style is up-to-date. I
don't think it is at this point because RemoveInlineStyleProperty() will cause
the computed style to be updated asynchronously.

It could be an idea to add/remove the event listener on
MediaRemotingExitButtonElement::AttachLayoutTree/DetachLayoutTree instead.

+      return;
+  }
   GetDocument().addEventListener(EventTypeNames::click, listener_, true);
 }
 
 void MediaRemotingExitButtonElement::OnHidden() {
+  if (const ComputedStyle* style = NonLayoutObjectComputedStyle()) {
+    if (style->Display() == EDisplay::kNone)
+      return;
+  }
   GetDocument().removeEventListener(EventTypeNames::click, listener_, true);
 }
 
 HTMLVideoElement& MediaRemotingExitButtonElement::GetVideoElement() const {
   return interstitial_->GetVideoElement();
 }
 
 DEFINE_TRACE(MediaRemotingExitButtonElement) {
   visitor->Trace(interstitial_);
   visitor->Trace(listener_);
@@ skipping 13 matching lines @@
 
 // ----------------------------
 
 MediaRemotingCastIconElement::MediaRemotingCastIconElement(
     MediaRemotingInterstitial& interstitial)
     : HTMLDivElement(interstitial.GetDocument()) {
   SetShadowPseudoId(AtomicString("-internal-media-remoting-cast-icon"));
 }
 
 }  // namespace blink