chrome/blink: Add functionality for in-product help for media elements.

chrome/blink: Add functionality for in-product help for media elements.

IPH (In-product help) UI is added to highlight and assist in the
discovery of features in the browser. This change adds this
functionality for the download feature on media elements in blink. This
involves the following additions:

1) A public WebMediaIPH interface in blink that can be used to ask the
embedder to display IPH widgets for an element.

2) A mojo MediaIPHService to bridge these requests to the browser where
the UI can be shown using android pop-ups overlayed on the content.

3) An addition to the features supported by the FeatureEngagementTracker
for media download IPH.

The feature is currently gated behind a finch flag which is disabled by
default.

BUG=715185
Review-Url: https://codereview.chromium.org/2943983003
Cr-Commit-Position: refs/heads/master@{#496361}
Committed: https://chromium.googlesource.com/chromium/src/+/c5447db2943848192c0452c7b8e766a943335f07

[Khushal, 2017-06-20 09:19:27]

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

[Khushal, 2017-06-20 09:19:28]

mlamouri@, could you take a look at the blink side of changes and see if the
approach looks correct. Thanks.

[Khushal, 2017-06-20 09:20:54]

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

[Khushal, 2017-06-20 09:20:55]

+dtrainor for chrome and feature_engagement_tracker.

[Khushal, 2017-06-20 22:27:06]

Description was changed from

==========
chrome/blink: Add functionality for in-product help for media elements.

IPH (In-product help) UI is added to highlight and assist in the
discovery of features in the browser. This change adds this
functionality for the download feature on media elements in blink. This
involves the following additions:

1) A public WebMediaIPH interface in blink that can be used to ask the
embedder to display IPH widgets for an element.

2) A mojo MediaIPHService to bridge these requests to the browser where
the UI can be shown using android pop-ups overlayed on the content.

3) An addition to the features supported by the FeatureEngagementTracker
for media download IPH.

The feature is currently gated behind a finch flag which is disabled by
default.

BUG=715185
==========

to

==========
chrome/blink: Add functionality for in-product help for media elements.

IPH (In-product help) UI is added to highlight and assist in the
discovery of features in the browser. This change adds this
functionality for the download feature on media elements in blink. This
involves the following additions:

1) A public WebMediaIPH interface in blink that can be used to ask the
embedder to display IPH widgets for an element.

2) A mojo MediaIPHService to bridge these requests to the browser where
the UI can be shown using android pop-ups overlayed on the content.

3) An addition to the features supported by the FeatureEngagementTracker
for media download IPH.

The feature is currently gated behind a finch flag which is disabled by
default.

BUG=715185
==========

[Khushal, 2017-06-20 22:27:06]

khushalsagar@chromium.org changed reviewers:
+ nyquist@chromium.org

[Khushal, 2017-06-21 20:07:36]

khushalsagar@chromium.org changed reviewers:
+ chrishtr@chromium.org

[Khushal, 2017-06-21 20:07:37]

+chrishtr

[Khushal, 2017-06-22 20:00:54]

pingy?

[chrishtr, 2017-06-23 00:39:45]

https://codereview.chromium.org/2943983003/diff/1/chrome/browser/android/medi...
File chrome/browser/android/media_iph_manager.h (right):

https://codereview.chromium.org/2943983003/diff/1/chrome/browser/android/medi...
chrome/browser/android/media_iph_manager.h:23: virtual void
UpdateMediaDownloadIPHPosition(
You can remove this method and pass the rect to ShowMediaDownloadIPH.

https://codereview.chromium.org/2943983003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/MediaIPHManager.h (right):

https://codereview.chromium.org/2943983003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/MediaIPHManager.h:18: class CORE_EXPORT
MediaIPHManager final
I'm not so sure you need this class at all. Proposal:

There is only one in-product help promo shown for the whole WebView. Any media
element can request to show it, and provide its Element* as a key.

The WebView handles these requests. If the in-product help is already shown for
that
Element*, or is keyed by another Element*, it does nothing. Otherwise it shows
it and stores off that Element* as an active key.

The WebView also listens for signals to hide the in-product help due to
non-Blink
signals like touch gesture.

When media controls want to hide, they query the WebView about whether there
is an active in-product help for it, and if so ask the WebView to dismiss it,
then schedule hiding. I don't think you will need the logic around calls back
into Blink to dismiss the in-product help before hiding the controls. If a
layout
occurs without user gesture that messes things up, it's ok for a brief delay of
IPC (if visible at all).

https://codereview.chromium.org/2943983003/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/media_controls/MediaControlsImpl.cpp
(right):

https://codereview.chromium.org/2943983003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/media_controls/MediaControlsImpl.cpp:1235:
MaybeShow();
Why call MaybeShow and MakeOpaque?

https://codereview.chromium.org/2943983003/diff/1/third_party/WebKit/public/w...
File third_party/WebKit/public/web/WebFrameClient.h (right):

https://codereview.chromium.org/2943983003/diff/1/third_party/WebKit/public/w...
third_party/WebKit/public/web/WebFrameClient.h:824: virtual
std::unique_ptr<WebMediaIPH> CreateMediaIPH() { return nullptr; }
CreateMediaInProductHelp

https://codereview.chromium.org/2943983003/diff/1/third_party/WebKit/public/w...
File third_party/WebKit/public/web/WebMediaIPH.h (right):

https://codereview.chromium.org/2943983003/diff/1/third_party/WebKit/public/w...
third_party/WebKit/public/web/WebMediaIPH.h:5: #ifndef WebMediaIPH_h
InProductHelp, here and everywhere else in the CL.

https://codereview.chromium.org/2943983003/diff/1/third_party/WebKit/public/w...
third_party/WebKit/public/web/WebMediaIPH.h:23: class BLINK_EXPORT WebMediaIPH {
Can you adjust add these methods to WebViewClient instead?

[Khushal, 2017-06-23 02:11:06]

https://codereview.chromium.org/2943983003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/MediaIPHManager.h (right):

https://codereview.chromium.org/2943983003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/MediaIPHManager.h:18: class CORE_EXPORT
MediaIPHManager final
On 2017/06/23 00:39:44, chrishtr wrote:
> I'm not so sure you need this class at all. Proposal:
> 
> There is only one in-product help promo shown for the whole WebView. Any media
> element can request to show it, and provide its Element* as a key.
> 
> The WebView handles these requests. If the in-product help is already shown
for
> that
> Element*, or is keyed by another Element*, it does nothing. Otherwise it shows
> it and stores off that Element* as an active key.

The reason I put it on Document was because I thought this should be per-frame.
Each frame knows if the in-product help is active for an element in the frame
and has an IPC bridge to the browser, which knows if its active for any frame.
This fits with the OOPIF model too. Putting it on WebView/RenderViewImpl, I am
not sure which frame to tie the mojo service to.

> 
> The WebView also listens for signals to hide the in-product help due to
> non-Blink
> signals like touch gesture.

We will definitely need a way for the browser to inform the renderer when the UI
is dismissed because the renderer does not receive all signals that could
initiate it. The user could touch any browser UI, like the toolbar, which would
also initiate hiding it.

Also the process of showing the in-product help itself needs the browser to
allow the request. There are heuristics which limit the number of times we can
show it over a period, so the renderer needs to be informed if the request is
rejected in order to not keep the controls visible indefinitely.

> 
> When media controls want to hide, they query the WebView about whether there
> is an active in-product help for it, and if so ask the WebView to dismiss it,
> then schedule hiding. I don't think you will need the logic around calls back
> into Blink to dismiss the in-product help before hiding the controls. If a
> layout
> occurs without user gesture that messes things up, it's ok for a brief delay
of
> IPC (if visible at all).

There is no logic here that waits for the in-product help to be dismissed before
hiding here. The state tracking in this class is because both the browser and
renderer could have events that dismiss the UI, so the renderer waits for an ack
to its hide notification to come to a consistent state. My bad with the naming,
may be kHideRequested should be kNotifyControlsHidden.

https://codereview.chromium.org/2943983003/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/media_controls/MediaControlsImpl.cpp
(right):

https://codereview.chromium.org/2943983003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/media_controls/MediaControlsImpl.cpp:1235:
MaybeShow();
On 2017/06/23 00:39:44, chrishtr wrote:
> Why call MaybeShow and MakeOpaque?

MakeOpaque is unnecessary. There should be an assert instead that MaybeShow does
show the controls.

https://codereview.chromium.org/2943983003/diff/1/third_party/WebKit/public/w...
File third_party/WebKit/public/web/WebMediaIPH.h (right):

https://codereview.chromium.org/2943983003/diff/1/third_party/WebKit/public/w...
third_party/WebKit/public/web/WebMediaIPH.h:23: class BLINK_EXPORT WebMediaIPH {
On 2017/06/23 00:39:45, chrishtr wrote:
> Can you adjust add these methods to WebViewClient instead?

I'm just following the advice on RenderViewImpl which explicitly states that
code should be added to RenderFrameImpl. :) And this should be a per-frame
concept.

[Khushal, 2017-06-27 00:05:19]

On 2017/06/23 02:11:06, Khushal wrote:
>
https://codereview.chromium.org/2943983003/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/dom/MediaIPHManager.h (right):
> 
>
https://codereview.chromium.org/2943983003/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/dom/MediaIPHManager.h:18: class CORE_EXPORT
> MediaIPHManager final
> On 2017/06/23 00:39:44, chrishtr wrote:
> > I'm not so sure you need this class at all. Proposal:
> > 
> > There is only one in-product help promo shown for the whole WebView. Any
media
> > element can request to show it, and provide its Element* as a key.
> > 
> > The WebView handles these requests. If the in-product help is already shown
> for
> > that
> > Element*, or is keyed by another Element*, it does nothing. Otherwise it
shows
> > it and stores off that Element* as an active key.
> 
> The reason I put it on Document was because I thought this should be
per-frame.
> Each frame knows if the in-product help is active for an element in the frame
> and has an IPC bridge to the browser, which knows if its active for any frame.
> This fits with the OOPIF model too. Putting it on WebView/RenderViewImpl, I am
> not sure which frame to tie the mojo service to.

Chris, does the frame vs view thing make sense? I can move it to another frame
scoped class if that would be better.

> 
> > 
> > The WebView also listens for signals to hide the in-product help due to
> > non-Blink
> > signals like touch gesture.
> 
> We will definitely need a way for the browser to inform the renderer when the
UI
> is dismissed because the renderer does not receive all signals that could
> initiate it. The user could touch any browser UI, like the toolbar, which
would
> also initiate hiding it.
> 
> Also the process of showing the in-product help itself needs the browser to
> allow the request. There are heuristics which limit the number of times we can
> show it over a period, so the renderer needs to be informed if the request is
> rejected in order to not keep the controls visible indefinitely.
> 
> > 
> > When media controls want to hide, they query the WebView about whether there
> > is an active in-product help for it, and if so ask the WebView to dismiss
it,
> > then schedule hiding. I don't think you will need the logic around calls
back
> > into Blink to dismiss the in-product help before hiding the controls. If a
> > layout
> > occurs without user gesture that messes things up, it's ok for a brief delay
> of
> > IPC (if visible at all).
> 
> There is no logic here that waits for the in-product help to be dismissed
before
> hiding here. The state tracking in this class is because both the browser and
> renderer could have events that dismiss the UI, so the renderer waits for an
ack
> to its hide notification to come to a consistent state. My bad with the
naming,
> may be kHideRequested should be kNotifyControlsHidden.
> 
>
https://codereview.chromium.org/2943983003/diff/1/third_party/WebKit/Source/m...
> File third_party/WebKit/Source/modules/media_controls/MediaControlsImpl.cpp
> (right):
> 
>
https://codereview.chromium.org/2943983003/diff/1/third_party/WebKit/Source/m...
> third_party/WebKit/Source/modules/media_controls/MediaControlsImpl.cpp:1235:
> MaybeShow();
> On 2017/06/23 00:39:44, chrishtr wrote:
> > Why call MaybeShow and MakeOpaque?
> 
> MakeOpaque is unnecessary. There should be an assert instead that MaybeShow
does
> show the controls.
> 
>
https://codereview.chromium.org/2943983003/diff/1/third_party/WebKit/public/w...
> File third_party/WebKit/public/web/WebMediaIPH.h (right):
> 
>
https://codereview.chromium.org/2943983003/diff/1/third_party/WebKit/public/w...
> third_party/WebKit/public/web/WebMediaIPH.h:23: class BLINK_EXPORT WebMediaIPH
{
> On 2017/06/23 00:39:45, chrishtr wrote:
> > Can you adjust add these methods to WebViewClient instead?
> 
> I'm just following the advice on RenderViewImpl which explicitly states that
> code should be added to RenderFrameImpl. :) And this should be a per-frame
> concept.

[chrishtr, 2017-06-27 18:22:20]

My suggestions are all targeted around trying to reduce the complexity of this
patch, and
maintaining state outside of just a per-frame global.

https://codereview.chromium.org/2943983003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/MediaIPHManager.h (right):

https://codereview.chromium.org/2943983003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/MediaIPHManager.h:18: class CORE_EXPORT
MediaIPHManager final
On 2017/06/23 at 02:11:06, Khushal wrote:
> On 2017/06/23 00:39:44, chrishtr wrote:
> > I'm not so sure you need this class at all. Proposal:
> > 
> > There is only one in-product help promo shown for the whole WebView. Any
media
> > element can request to show it, and provide its Element* as a key.
> > 
> > The WebView handles these requests. If the in-product help is already shown
for
> > that
> > Element*, or is keyed by another Element*, it does nothing. Otherwise it
shows
> > it and stores off that Element* as an active key.
> 
> The reason I put it on Document was because I thought this should be
per-frame. Each frame knows if the in-product help is active for an element in
the frame and has an IPC bridge to the browser, which knows if its active for
any frame. This fits with the OOPIF model too. Putting it on
WebView/RenderViewImpl, I am not sure which frame to tie the mojo service to.
> 
> > 
> > The WebView also listens for signals to hide the in-product help due to
> > non-Blink
> > signals like touch gesture.
> 
> We will definitely need a way for the browser to inform the renderer when the
UI is dismissed because the renderer does not receive all signals that could
initiate it. The user could touch any browser UI, like the toolbar, which would
also initiate hiding it.
> 
> Also the process of showing the in-product help itself needs the browser to
allow the request. There are heuristics which limit the number of times we can
show it over a period, so the renderer needs to be informed if the request is
rejected in order to not keep the controls visible indefinitely.
> 
> > 
> > When media controls want to hide, they query the WebView about whether there
> > is an active in-product help for it, and if so ask the WebView to dismiss
it,
> > then schedule hiding. I don't think you will need the logic around calls
back
> > into Blink to dismiss the in-product help before hiding the controls. If a
> > layout
> > occurs without user gesture that messes things up, it's ok for a brief delay
of
> > IPC (if visible at all).
> 
> There is no logic here that waits for the in-product help to be dismissed
before hiding here. The state tracking in this class is because both the browser
and renderer could have events that dismiss the UI, so the renderer waits for an
ack to its hide notification to come to a consistent state. My bad with the
naming, may be kHideRequested should be kNotifyControlsHidden.

That sounds like waiting for the browser to me...

https://codereview.chromium.org/2943983003/diff/1/third_party/WebKit/public/w...
File third_party/WebKit/public/web/WebMediaIPH.h (right):

https://codereview.chromium.org/2943983003/diff/1/third_party/WebKit/public/w...
third_party/WebKit/public/web/WebMediaIPH.h:23: class BLINK_EXPORT WebMediaIPH {
On 2017/06/23 at 02:11:06, Khushal wrote:
> On 2017/06/23 00:39:45, chrishtr wrote:
> > Can you adjust add these methods to WebViewClient instead?
> 
> I'm just following the advice on RenderViewImpl which explicitly states that
code should be added to RenderFrameImpl. :) And this should be a per-frame
concept.

Ok then move them to WebFrameClient?

[Khushal, 2017-06-28 05:32:37]

khushalsagar@chromium.org changed reviewers:
+ haraken@chromium.org

[Khushal, 2017-06-28 05:32:38]

+haraken for the mojom bits. I have added the interface in platform/ for now,
until we find a place better suited.

https://codereview.chromium.org/2943983003/diff/1/chrome/browser/android/medi...
File chrome/browser/android/media_iph_manager.h (right):

https://codereview.chromium.org/2943983003/diff/1/chrome/browser/android/medi...
chrome/browser/android/media_iph_manager.h:23: virtual void
UpdateMediaDownloadIPHPosition(
On 2017/06/23 00:39:44, chrishtr wrote:
> You can remove this method and pass the rect to ShowMediaDownloadIPH.

Done.

https://codereview.chromium.org/2943983003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/MediaIPHManager.h (right):

https://codereview.chromium.org/2943983003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/MediaIPHManager.h:18: class CORE_EXPORT
MediaIPHManager final
On 2017/06/27 18:22:20, chrishtr wrote:
> On 2017/06/23 at 02:11:06, Khushal wrote:
> > On 2017/06/23 00:39:44, chrishtr wrote:
> > > I'm not so sure you need this class at all. Proposal:
> > > 
> > > There is only one in-product help promo shown for the whole WebView. Any
> media
> > > element can request to show it, and provide its Element* as a key.
> > > 
> > > The WebView handles these requests. If the in-product help is already
shown
> for
> > > that
> > > Element*, or is keyed by another Element*, it does nothing. Otherwise it
> shows
> > > it and stores off that Element* as an active key.
> > 
> > The reason I put it on Document was because I thought this should be
> per-frame. Each frame knows if the in-product help is active for an element in
> the frame and has an IPC bridge to the browser, which knows if its active for
> any frame. This fits with the OOPIF model too. Putting it on
> WebView/RenderViewImpl, I am not sure which frame to tie the mojo service to.
> > 
> > > 
> > > The WebView also listens for signals to hide the in-product help due to
> > > non-Blink
> > > signals like touch gesture.
> > 
> > We will definitely need a way for the browser to inform the renderer when
the
> UI is dismissed because the renderer does not receive all signals that could
> initiate it. The user could touch any browser UI, like the toolbar, which
would
> also initiate hiding it.
> > 
> > Also the process of showing the in-product help itself needs the browser to
> allow the request. There are heuristics which limit the number of times we can
> show it over a period, so the renderer needs to be informed if the request is
> rejected in order to not keep the controls visible indefinitely.
> > 
> > > 
> > > When media controls want to hide, they query the WebView about whether
there
> > > is an active in-product help for it, and if so ask the WebView to dismiss
> it,
> > > then schedule hiding. I don't think you will need the logic around calls
> back
> > > into Blink to dismiss the in-product help before hiding the controls. If a
> > > layout
> > > occurs without user gesture that messes things up, it's ok for a brief
delay
> of
> > > IPC (if visible at all).
> > 
> > There is no logic here that waits for the in-product help to be dismissed
> before hiding here. The state tracking in this class is because both the
browser
> and renderer could have events that dismiss the UI, so the renderer waits for
an
> ack to its hide notification to come to a consistent state. My bad with the
> naming, may be kHideRequested should be kNotifyControlsHidden.
> 
> That sounds like waiting for the browser to me...

Restructured like you suggested except the tracking is on LocalFrame instead of
WebView.

I just restricted showing the UI to once per frame to simplify all of this. I'm
sure the heuristics on the browser would have done that anyway.

https://codereview.chromium.org/2943983003/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/media_controls/MediaControlsImpl.cpp
(right):

https://codereview.chromium.org/2943983003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/media_controls/MediaControlsImpl.cpp:1235:
MaybeShow();
On 2017/06/23 02:11:06, Khushal wrote:
> On 2017/06/23 00:39:44, chrishtr wrote:
> > Why call MaybeShow and MakeOpaque?
> 
> MakeOpaque is unnecessary. There should be an assert instead that MaybeShow
does
> show the controls.

Done.

https://codereview.chromium.org/2943983003/diff/1/third_party/WebKit/public/w...
File third_party/WebKit/public/web/WebMediaIPH.h (right):

https://codereview.chromium.org/2943983003/diff/1/third_party/WebKit/public/w...
third_party/WebKit/public/web/WebMediaIPH.h:5: #ifndef WebMediaIPH_h
On 2017/06/23 00:39:45, chrishtr wrote:
> InProductHelp, here and everywhere else in the CL.

Done.

[Khushal, 2017-06-28 07:44:25]

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

[commit-bot: I haz the power, 2017-06-28 07:44:36]

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-06-28 07:47:04]

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

[commit-bot: I haz the power, 2017-06-28 07:47:05]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[haraken, 2017-06-28 08:36:32]

haraken@chromium.org changed reviewers:
+ sammc@chromium.org

[haraken, 2017-06-28 08:36:33]

(I chatted with Sam about this change. Sam has some thoughts about this.)

+sammc

[Sam McNally, 2017-06-28 09:19:58]

https://codereview.chromium.org/2943983003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/2943983003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/LocalFrame.cpp:1279: void
LocalFrame::ShowDownloadMediaInProductHelp(HTMLMediaElement& elem) {
Could this be handled within MediaControlDownloadButtonElement or
MediaControlsImpl? It doesn't feel like LocalFrame should need to know about
this. It seems to me that the browser is already enforcing one help widget per
WebContents so repeating that here doesn't seem necessary. If the delegate
interface is replaced with a response then the tracking of the element isn't
necessary either.

https://codereview.chromium.org/2943983003/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/media_in_product_help.mojom (right):

https://codereview.chromium.org/2943983003/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/media_in_product_help.mojom:9: interface
MediaInProductHelpService {
We don't name mojo interfaces with Service suffixes anymore.

https://codereview.chromium.org/2943983003/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/media_in_product_help.mojom:13:
ShowInProductHelpWidget(int32 x, int32 y, int32 width, int32 height);
This should use a gfx.mojom.Rect.

https://codereview.chromium.org/2943983003/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/media_in_product_help.mojom:24:
WidgetDismissed();
Could this message be a response to ShowInProductHelpWidget() instead? Then
HideInProductHelpWidget() could be replaced with the renderer closing the
connection before that response is sent.

[Khushal, 2017-06-28 22:28:46]

https://codereview.chromium.org/2943983003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/2943983003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/LocalFrame.cpp:1279: void
LocalFrame::ShowDownloadMediaInProductHelp(HTMLMediaElement& elem) {
On 2017/06/28 09:19:58, Sam McNally wrote:
> Could this be handled within MediaControlDownloadButtonElement or
> MediaControlsImpl? It doesn't feel like LocalFrame should need to know about
> this. It seems to me that the browser is already enforcing one help widget per
> WebContents so repeating that here doesn't seem necessary. If the delegate
> interface is replaced with a response then the tracking of the element isn't
> necessary either.

The element had to be tracked at some place which hears about Layout changes to
send the updated position to the browser.

https://codereview.chromium.org/2943983003/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/media_in_product_help.mojom (right):

https://codereview.chromium.org/2943983003/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/media_in_product_help.mojom:9: interface
MediaInProductHelpService {
On 2017/06/28 09:19:58, Sam McNally wrote:
> We don't name mojo interfaces with Service suffixes anymore.

Done.

https://codereview.chromium.org/2943983003/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/media_in_product_help.mojom:13:
ShowInProductHelpWidget(int32 x, int32 y, int32 width, int32 height);
On 2017/06/28 09:19:58, Sam McNally wrote:
> This should use a gfx.mojom.Rect.

Done.

https://codereview.chromium.org/2943983003/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/media_in_product_help.mojom:24:
WidgetDismissed();
On 2017/06/28 09:19:58, Sam McNally wrote:
> Could this message be a response to ShowInProductHelpWidget() instead?

For the initial request, yes. But the browser can dismiss the widget from any
other event after the response has been dispatched. Touching anywhere on the
screen for instance.

> HideInProductHelpWidget() could be replaced with the renderer closing the
> connection before that response is sent.

I'm not sure I understood this one. This hiding will be the result of a layout
change which removes the button from the page in the renderer.

Did you mean keeping the MediaInProductHelpPtr in LocalFrame for the duration
the renderer wants to show the UI and destroying it to close the connection and
using that as a notification to hide it in the browser instead of
HideInProductHelpWidget?

[nyquist, 2017-06-29 02:10:24]

I mostly looked at //components/feature_engagement_tracker and //chrome, and
that part looks reasonable to me.

https://codereview.chromium.org/2943983003/diff/40001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right):

https://codereview.chromium.org/2943983003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:3114: public
void showMediaDownloadInProductHelp(int x, int y, int width, int height) {
Nit: Could this and the method below be private?

https://codereview.chromium.org/2943983003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:3125:
mDownloadIPHBubble.show();
Optional nit: It's probably just me, but it feels weird when this isn't the last
line...

https://codereview.chromium.org/2943983003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:3136:
nativeDownloadMediaInProductHelpDismissed(mNativeTabAndroid);
Note to self, this is in the same callstack as show

https://codereview.chromium.org/2943983003/diff/40001/chrome/browser/android/...
File chrome/browser/android/media_in_product_help_manager.cc (right):

https://codereview.chromium.org/2943983003/diff/40001/chrome/browser/android/...
chrome/browser/android/media_in_product_help_manager.cc:48:
MediaInProductHelpManager* manager_;
Nit: Could you write a comment about lifetime / ownership of these raw pointers?

https://codereview.chromium.org/2943983003/diff/40001/chrome/browser/android/...
chrome/browser/android/media_in_product_help_manager.cc:60:
client_->DismissMediaDownloadInProductHelp();
In the API description, it's stated that a call to this method requires a call
to MediaInProductHelpManager::WidgetDismissed(), but in this case, that'd be
after |this| is destructed?

Would the DCHECK(media_iph_manager_) in
TabAndroid::DownloadMediaInProductHelpDismissed hit?

https://codereview.chromium.org/2943983003/diff/40001/chrome/browser/android/...
File chrome/browser/android/media_in_product_help_manager.h (right):

https://codereview.chromium.org/2943983003/diff/40001/chrome/browser/android/...
chrome/browser/android/media_in_product_help_manager.h:61:
MediaInProductHelpManagerClient* client_;
Nit: Could you add a comment describing the ownership / lifetime of this raw
pointer?

https://codereview.chromium.org/2943983003/diff/40001/components/feature_enga...
File components/feature_engagement_tracker/public/feature_constants.h (right):

https://codereview.chromium.org/2943983003/diff/40001/components/feature_enga...
components/feature_engagement_tracker/public/feature_constants.h:27: extern
const base::Feature kIPHMediaDownloadFeature;
Could you add the metrics for this new feature as well?
They are all described here now:
https://chromium.googlesource.com/chromium/src/+/master/components/feature_en...

[Sam McNally, 2017-07-03 09:45:41]

https://codereview.chromium.org/2943983003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/2943983003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/LocalFrame.cpp:1279: void
LocalFrame::ShowDownloadMediaInProductHelp(HTMLMediaElement& elem) {
On 2017/06/28 22:28:46, Khushal wrote:
> On 2017/06/28 09:19:58, Sam McNally wrote:
> > Could this be handled within MediaControlDownloadButtonElement or
> > MediaControlsImpl? It doesn't feel like LocalFrame should need to know about
> > this. It seems to me that the browser is already enforcing one help widget
per
> > WebContents so repeating that here doesn't seem necessary. If the delegate
> > interface is replaced with a response then the tracking of the element isn't
> > necessary either.
> 
> The element had to be tracked at some place which hears about Layout changes
to
> send the updated position to the browser.

MediaControlsImpl should be able to observe layout via its ResizeObserver.
Having every interested party add some code in LocalFrame for observing layout
doesn't seem very scalable. I'm not sure this captures all changes that might
matter, e.g. CSS visibility changes or scrolling, but I don't think observing
layout in LocalFrame would either.

https://codereview.chromium.org/2943983003/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/media_in_product_help.mojom (right):

https://codereview.chromium.org/2943983003/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/media_in_product_help.mojom:24:
WidgetDismissed();
On 2017/06/28 22:28:46, Khushal wrote:
> On 2017/06/28 09:19:58, Sam McNally wrote:
> > Could this message be a response to ShowInProductHelpWidget() instead?
> 
> For the initial request, yes. But the browser can dismiss the widget from any
> other event after the response has been dispatched. Touching anywhere on the
> screen for instance.

In that case, the browser could close the connection and rely on the renderer
observing the close.
> 
> > HideInProductHelpWidget() could be replaced with the renderer closing the
> > connection before that response is sent.
> 
> I'm not sure I understood this one. This hiding will be the result of a layout
> change which removes the button from the page in the renderer.
> 
> Did you mean keeping the MediaInProductHelpPtr in LocalFrame for the duration
> the renderer wants to show the UI and destroying it to close the connection
and
> using that as a notification to hide it in the browser instead of
> HideInProductHelpWidget?

I mean keeping the MediaInProductHelpPtr on one of the classes in
modules/media_controls and then each side can close the connection to indicate
that it wants the UI to be hidden.

[Khushal, 2017-07-07 04:18:14]

https://codereview.chromium.org/2943983003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/2943983003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/LocalFrame.cpp:1279: void
LocalFrame::ShowDownloadMediaInProductHelp(HTMLMediaElement& elem) {
On 2017/07/03 09:45:40, Sam McNally wrote:
> On 2017/06/28 22:28:46, Khushal wrote:
> > On 2017/06/28 09:19:58, Sam McNally wrote:
> > > Could this be handled within MediaControlDownloadButtonElement or
> > > MediaControlsImpl? It doesn't feel like LocalFrame should need to know
about
> > > this. It seems to me that the browser is already enforcing one help widget
> per
> > > WebContents so repeating that here doesn't seem necessary. If the delegate
> > > interface is replaced with a response then the tracking of the element
isn't
> > > necessary either.
> > 
> > The element had to be tracked at some place which hears about Layout changes
> to
> > send the updated position to the browser.
> 
> MediaControlsImpl should be able to observe layout via its ResizeObserver.
> Having every interested party add some code in LocalFrame for observing layout
> doesn't seem very scalable. I'm not sure this captures all changes that might
> matter, e.g. CSS visibility changes or scrolling, but I don't think observing
> layout in LocalFrame would either.

The ResizeObserver only hears about size changes in the video element, so it
captures less than what a Layout event on the Frame would have. But you're right
that it still ignores multiple changes (scrolling, etc.). Since we dismiss the
UI on any user interaction, in most cases the position of the button remains
static. I've moved the code for sending the rect to MediaControlsImpl and
restricted the update to dismissing the UI when the controls or the button are
hidden.

https://codereview.chromium.org/2943983003/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/media_in_product_help.mojom (right):

https://codereview.chromium.org/2943983003/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/media_in_product_help.mojom:24:
WidgetDismissed();
On 2017/07/03 09:45:40, Sam McNally wrote:
> On 2017/06/28 22:28:46, Khushal wrote:
> > On 2017/06/28 09:19:58, Sam McNally wrote:
> > > Could this message be a response to ShowInProductHelpWidget() instead?
> > 
> > For the initial request, yes. But the browser can dismiss the widget from
any
> > other event after the response has been dispatched. Touching anywhere on the
> > screen for instance.
> 
> In that case, the browser could close the connection and rely on the renderer
> observing the close.
> > 
> > > HideInProductHelpWidget() could be replaced with the renderer closing the
> > > connection before that response is sent.
> > 
> > I'm not sure I understood this one. This hiding will be the result of a
layout
> > change which removes the button from the page in the renderer.
> > 
> > Did you mean keeping the MediaInProductHelpPtr in LocalFrame for the
duration
> > the renderer wants to show the UI and destroying it to close the connection
> and
> > using that as a notification to hide it in the browser instead of
> > HideInProductHelpWidget?
> 
> I mean keeping the MediaInProductHelpPtr on one of the classes in
> modules/media_controls and then each side can close the connection to indicate
> that it wants the UI to be hidden.

Thanks for the inputs. Isolating the code to the controls and using connection
notification callbacks does simplify this a lot.

https://codereview.chromium.org/2943983003/diff/40001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right):

https://codereview.chromium.org/2943983003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:3114: public
void showMediaDownloadInProductHelp(int x, int y, int width, int height) {
On 2017/06/29 02:10:23, nyquist (OOO until 6-10) wrote:
> Nit: Could this and the method below be private?

Done.

https://codereview.chromium.org/2943983003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:3125:
mDownloadIPHBubble.show();
On 2017/06/29 02:10:23, nyquist (OOO until 6-10) wrote:
> Optional nit: It's probably just me, but it feels weird when this isn't the
last
> line...

Done.

https://codereview.chromium.org/2943983003/diff/40001/chrome/browser/android/...
File chrome/browser/android/media_in_product_help_manager.cc (right):

https://codereview.chromium.org/2943983003/diff/40001/chrome/browser/android/...
chrome/browser/android/media_in_product_help_manager.cc:48:
MediaInProductHelpManager* manager_;
On 2017/06/29 02:10:23, nyquist (OOO until 6-10) wrote:
> Nit: Could you write a comment about lifetime / ownership of these raw
pointers?

Done.

https://codereview.chromium.org/2943983003/diff/40001/chrome/browser/android/...
chrome/browser/android/media_in_product_help_manager.cc:60:
client_->DismissMediaDownloadInProductHelp();
On 2017/06/29 02:10:23, nyquist (OOO until 6-10) wrote:
> In the API description, it's stated that a call to this method requires a call
> to MediaInProductHelpManager::WidgetDismissed(), but in this case, that'd be
> after |this| is destructed?

It should happen in the same call stack. The call to the client synchronously
dismisses the widget and informs this class.

> 
> Would the DCHECK(media_iph_manager_) in
> TabAndroid::DownloadMediaInProductHelpDismissed hit?

I was hitting that DCHECK in the case of shutdown. Didn't realize that the UI
had to be cleaned up in Tab.destroy, before the native WebContents is destroyed.

https://codereview.chromium.org/2943983003/diff/40001/chrome/browser/android/...
File chrome/browser/android/media_in_product_help_manager.h (right):

https://codereview.chromium.org/2943983003/diff/40001/chrome/browser/android/...
chrome/browser/android/media_in_product_help_manager.h:61:
MediaInProductHelpManagerClient* client_;
On 2017/06/29 02:10:24, nyquist (OOO until 6-10) wrote:
> Nit: Could you add a comment describing the ownership / lifetime of this raw
> pointer?

Done.

https://codereview.chromium.org/2943983003/diff/40001/components/feature_enga...
File components/feature_engagement_tracker/public/feature_constants.h (right):

https://codereview.chromium.org/2943983003/diff/40001/components/feature_enga...
components/feature_engagement_tracker/public/feature_constants.h:27: extern
const base::Feature kIPHMediaDownloadFeature;
On 2017/06/29 02:10:24, nyquist (OOO until 6-10) wrote:
> Could you add the metrics for this new feature as well?
> They are all described here now:
>
https://chromium.googlesource.com/chromium/src/+/master/components/feature_en...

Done.

https://codereview.chromium.org/2943983003/diff/120001/chrome/browser/android...
File chrome/browser/android/media_in_product_help_manager.cc (right):

https://codereview.chromium.org/2943983003/diff/120001/chrome/browser/android...
chrome/browser/android/media_in_product_help_manager.cc:90: if
(media_in_product_help_)
sammc@, is this the correct way to ignore requests from multiple frames? And
from different elements in the same frame?

[Khushal, 2017-07-07 04:18:24]

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

[commit-bot: I haz the power, 2017-07-07 04:18:37]

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-07-07 04:42:29]

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

[commit-bot: I haz the power, 2017-07-07 04:42:30]

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

[Khushal, 2017-07-07 05:09:21]

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

[commit-bot: I haz the power, 2017-07-07 05:09:38]

Dry run: CQ is trying da patch.

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

[Sam McNally, 2017-07-07 05:22:58]

mojom and related files LGTM.

https://codereview.chromium.org/2943983003/diff/120001/chrome/browser/android...
File chrome/browser/android/media_in_product_help_manager.cc (right):

https://codereview.chromium.org/2943983003/diff/120001/chrome/browser/android...
chrome/browser/android/media_in_product_help_manager.cc:49: };
DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/2943983003/diff/120001/chrome/browser/android...
chrome/browser/android/media_in_product_help_manager.cc:90: if
(media_in_product_help_)
On 2017/07/07 04:18:14, Khushal wrote:
> sammc@, is this the correct way to ignore requests from multiple frames? And
> from different elements in the same frame?

Yes. There will be a race between an existing connection's closure being
detected and a new connection request arriving, but I suspect this won't be a
problem in practice.

https://codereview.chromium.org/2943983003/diff/120001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/media_in_product_help.mojom (right):

https://codereview.chromium.org/2943983003/diff/120001/third_party/WebKit/pub...
third_party/WebKit/public/platform/media_in_product_help.mojom:14: // The
request_id denotes the element making the request.
request_id doesn't seem to exist anymore.

[commit-bot: I haz the power, 2017-07-07 06:56:47]

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

[commit-bot: I haz the power, 2017-07-07 06:56:49]

Dry run: This issue passed the CQ dry run.

[mlamouri (slow - plz ping), 2017-07-07 14:52:10]

Two high level comments:
- I would prefer if we could have the service hooking in the download button
code instead of MediaControlsImpl;
- It seems that there is a lot of logic around whether the download button is
visible. However, we are going to drop the download button from being one click
away (and will be in a menu), I'm afraid that we could reduce by a lot the
complexity if the IPH was on the <audio>/<video> instead of the download button
itself (that solution would be future compatible).

[whywhat, 2017-07-10 15:29:27]

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

[whywhat, 2017-07-10 15:29:28]

a drive-by nit I had a couple of weeks ago (the rest seems to have been
resolved)

https://codereview.chromium.org/2943983003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/media_controls/MediaControlsImpl.cpp
(right):

https://codereview.chromium.org/2943983003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/media_controls/MediaControlsImpl.cpp:1230:
void MediaControlsImpl::MaybeDispatchDownloadIPHTrigger() {
nit: I think we should either use IPH or InProductHelp everywhere consistently,
no exceptions.

[nyquist, 2017-07-10 20:09:42]

On 2017/07/07 14:52:10, mlamouri (slow - plz ping) wrote:
> Two high level comments:
> - I would prefer if we could have the service hooking in the download button
> code instead of MediaControlsImpl;
> - It seems that there is a lot of logic around whether the download button is
> visible. However, we are going to drop the download button from being one
click
> away (and will be in a menu), I'm afraid that we could reduce by a lot the
> complexity if the IPH was on the <audio>/<video> instead of the download
button
> itself (that solution would be future compatible).

To this last point; if that's the case, I believe we would have to make this an
approach where we tell the user to first open the menu and then highlight the
button to tell the user to tap there, when that change happens.
Since the download button (or the menu item as well I guess) is not always
there, it would still have to be guarded in the same way to be able to only show
it when the download action is in fact available. So from my naive view of the
world it feels like we would somehow have to still track whether the download
action is available.

[Khushal, 2017-07-10 20:35:41]

On 2017/07/10 20:09:42, nyquist (OOO until 6-10) wrote:
> On 2017/07/07 14:52:10, mlamouri (slow - plz ping) wrote:
> > Two high level comments:
> > - I would prefer if we could have the service hooking in the download button
> > code instead of MediaControlsImpl;

Done.

> > - It seems that there is a lot of logic around whether the download button
is
> > visible. However, we are going to drop the download button from being one
> click
> > away (and will be in a menu), I'm afraid that we could reduce by a lot the
> > complexity if the IPH was on the <audio>/<video> instead of the download
> button
> > itself (that solution would be future compatible).
> 
> To this last point; if that's the case, I believe we would have to make this
an
> approach where we tell the user to first open the menu and then highlight the
> button to tell the user to tap there, when that change happens.

Yeah, if the download option is inside the menu, we would probably take an
approach similar to highlighting options in Chrome's UI menu. Dispatching the
trigger itself will always be dependent on whether the download option itself is
available or not.

> Since the download button (or the menu item as well I guess) is not always
> there, it would still have to be guarded in the same way to be able to only
show
> it when the download action is in fact available. So from my naive view of the
> world it feels like we would somehow have to still track whether the download
> action is available.

[Khushal, 2017-07-10 20:38:02]

https://codereview.chromium.org/2943983003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/media_controls/MediaControlsImpl.cpp
(right):

https://codereview.chromium.org/2943983003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/media_controls/MediaControlsImpl.cpp:1230:
void MediaControlsImpl::MaybeDispatchDownloadIPHTrigger() {
On 2017/07/10 15:29:28, whywhat wrote:
> nit: I think we should either use IPH or InProductHelp everywhere
consistently,
> no exceptions.

Done.

https://codereview.chromium.org/2943983003/diff/160001/chrome/browser/android...
File chrome/browser/android/media_in_product_help_manager.h (right):

https://codereview.chromium.org/2943983003/diff/160001/chrome/browser/android...
chrome/browser/android/media_in_product_help_manager.h:28: class
MediaInProductHelpManager : public content::WebContentsObserver {
This class isn't really doing anything useful anymore.
dtrainor@/nyquist@, lmk if it would be better to merge it with tab_android and
make it a WebContentsObserver.

[David Trainor- moved to gerrit, 2017-07-11 16:34:42]

lgtm!

https://codereview.chromium.org/2943983003/diff/160001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right):

https://codereview.chromium.org/2943983003/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:3144:
mDownloadIPHBubble.setAnchorRect(rect);
Does it make sense to do this before the show() call?

https://codereview.chromium.org/2943983003/diff/160001/chrome/browser/android...
File chrome/browser/android/media_in_product_help_manager.h (right):

https://codereview.chromium.org/2943983003/diff/160001/chrome/browser/android...
chrome/browser/android/media_in_product_help_manager.h:28: class
MediaInProductHelpManager : public content::WebContentsObserver {
On 2017/07/10 20:38:02, Khushal wrote:
> This class isn't really doing anything useful anymore.
> dtrainor@/nyquist@, lmk if it would be better to merge it with tab_android and
> make it a WebContentsObserver.

Merging it makes sense to me :)

[Khushal, 2017-07-11 20:34:36]

Thanks Dave.

https://codereview.chromium.org/2943983003/diff/160001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right):

https://codereview.chromium.org/2943983003/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:3144:
mDownloadIPHBubble.setAnchorRect(rect);
On 2017/07/11 16:34:42, David Trainor-ping after 12h wrote:
> Does it make sense to do this before the show() call?

Done.

https://codereview.chromium.org/2943983003/diff/160001/chrome/browser/android...
File chrome/browser/android/media_in_product_help_manager.h (right):

https://codereview.chromium.org/2943983003/diff/160001/chrome/browser/android...
chrome/browser/android/media_in_product_help_manager.h:28: class
MediaInProductHelpManager : public content::WebContentsObserver {
On 2017/07/11 16:34:42, David Trainor-ping after 12h wrote:
> On 2017/07/10 20:38:02, Khushal wrote:
> > This class isn't really doing anything useful anymore.
> > dtrainor@/nyquist@, lmk if it would be better to merge it with tab_android
and
> > make it a WebContentsObserver.
> 
> Merging it makes sense to me :)

Done.

[nyquist, 2017-07-11 22:49:19]

I looked at //chrome, //components and //content, and it lgtm.
I also looked at the //tools/metrics parts without being an owner, and it also
looks good.

[Khushal, 2017-07-11 23:04:24]

Thanks nyquist.

ping to mlamouri@ for media_controls/.

[mlamouri (slow - plz ping), 2017-07-13 21:14:27]

See some comments below. Would it be possible to add unit test for this in the
media controls?

https://codereview.chromium.org/2943983003/diff/200001/chrome/android/java/st...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://codereview.chromium.org/2943983003/diff/200001/chrome/android/java/st...
chrome/android/java/strings/android_chrome_strings.grd:3082: Download videos to
watch later using the Download button
Are you showing this only for videos?

https://codereview.chromium.org/2943983003/diff/200001/content/public/common/...
File content/public/common/web_preferences.cc (right):

https://codereview.chromium.org/2943983003/diff/200001/content/public/common/...
content/public/common/web_preferences.cc:209:
enable_media_in_product_help(false),
`enable_media_download_in_product_help`?

https://codereview.chromium.org/2943983003/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/media_controls/MediaControlsImpl.cpp
(right):

https://codereview.chromium.org/2943983003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/media_controls/MediaControlsImpl.cpp:584:
download_button_->UpdateInProductHelpState();
`MakeOpaque()` is what you care about here. I'm afraid you might end up showing
the IPH without the controls being visible.

https://codereview.chromium.org/2943983003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/media_controls/MediaControlsImpl.cpp:1228:
StartHideMediaControlsTimer();
There is at least one other place with this logic. Can you refactor?

Maybe rename the method `StartHideMediaControlsIfNecessary()`

https://codereview.chromium.org/2943983003/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/media_controls/MediaControlsImpl.h
(right):

https://codereview.chromium.org/2943983003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/media_controls/MediaControlsImpl.h:124: void
HideControlsIfNecessary();
Having the buttons being able to hide the control bar is bothering but I can't
find a better alternative. Could you maybe add a comment explaining why these
two methods are public?

https://codereview.chromium.org/2943983003/diff/200001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/media_controls/elements/MediaControlDownloadButtonElement.cpp
(right):

https://codereview.chromium.org/2943983003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/media_controls/elements/MediaControlDownloadButtonElement.cpp:141:
void MediaControlDownloadButtonElement::MaybeDispatchInProductHelpTrigger() {
For what I can see, this setting up the mojo pipe. Is it triggering anything?

https://codereview.chromium.org/2943983003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/media_controls/elements/MediaControlDownloadButtonElement.cpp:147:
->GetMediaDownloadInProductHelpEnabled()) {
For readability, can you do an early return?

https://codereview.chromium.org/2943983003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/media_controls/elements/MediaControlDownloadButtonElement.cpp:155:
.GetFrame()
Can you check that `GetFrame()` doesn't return `nullptr`?

https://codereview.chromium.org/2943983003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/media_controls/elements/MediaControlDownloadButtonElement.cpp:163:
GetMediaControls().MaybeShow();
Does that mean that when a download button will be added, we will show the
controls if they are not already showing?

https://codereview.chromium.org/2943983003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/media_controls/elements/MediaControlDownloadButtonElement.cpp:167:
void MediaControlDownloadButtonElement::UpdateInProductHelpState() {
Isn't this method actually triggering the UI?

https://codereview.chromium.org/2943983003/diff/200001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/media_in_product_help.mojom (right):

https://codereview.chromium.org/2943983003/diff/200001/third_party/WebKit/pub...
third_party/WebKit/public/platform/media_in_product_help.mojom:11: interface
MediaInProductHelp {
MediaDownloadInProductHelp

[Khushal, 2017-07-17 17:52:21]

Thanks Mounir. I added a few tests in MediaControlsImplTest. Let me know if
there is something I missed.

https://codereview.chromium.org/2943983003/diff/200001/chrome/android/java/st...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://codereview.chromium.org/2943983003/diff/200001/chrome/android/java/st...
chrome/android/java/strings/android_chrome_strings.grd:3082: Download videos to
watch later using the Download button
On 2017/07/13 21:14:27, mlamouri (slow - plz ping) wrote:
> Are you showing this only for videos?

Yes. I added an if check for video element in MayBeDispatchInProductHelpTrigger.

https://codereview.chromium.org/2943983003/diff/200001/content/public/common/...
File content/public/common/web_preferences.cc (right):

https://codereview.chromium.org/2943983003/diff/200001/content/public/common/...
content/public/common/web_preferences.cc:209:
enable_media_in_product_help(false),
On 2017/07/13 21:14:27, mlamouri (slow - plz ping) wrote:
> `enable_media_download_in_product_help`?

Done.

https://codereview.chromium.org/2943983003/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/media_controls/MediaControlsImpl.cpp
(right):

https://codereview.chromium.org/2943983003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/media_controls/MediaControlsImpl.cpp:584:
download_button_->UpdateInProductHelpState();
On 2017/07/13 21:14:27, mlamouri (slow - plz ping) wrote:
> `MakeOpaque()` is what you care about here. I'm afraid you might end up
showing
> the IPH without the controls being visible.

So I think I wasn't understanding the 2 states on the controls clearly. The
element enables/disables showing of the controls using MaybeShow/Hide. And then
the controls internally manage making themselves visible using
MakeOpaque/transparent. We don't want to show IPH if the controls have been
hidden using Hide(). But we can make them visible for the purpose of showing it.

I updated the MayBeDispatchInProductHelpTrigger to happen only when the panel
IsWanted. And we need to dismiss this IPH if the controls are hidden so Hide is
the only thing that needs to dismiss it. Restructured the code in the button as
well to make this obvious.

https://codereview.chromium.org/2943983003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/media_controls/MediaControlsImpl.cpp:1228:
StartHideMediaControlsTimer();
On 2017/07/13 21:14:27, mlamouri (slow - plz ping) wrote:
> There is at least one other place with this logic. Can you refactor?
> 
> Maybe rename the method `StartHideMediaControlsIfNecessary()`

Done.

https://codereview.chromium.org/2943983003/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/media_controls/MediaControlsImpl.h
(right):

https://codereview.chromium.org/2943983003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/media_controls/MediaControlsImpl.h:124: void
HideControlsIfNecessary();
On 2017/07/13 21:14:27, mlamouri (slow - plz ping) wrote:
> Having the buttons being able to hide the control bar is bothering but I can't
> find a better alternative. Could you maybe add a comment explaining why these
> two methods are public?

The controls will still check the internal state and only hide if it is valid to
do so? Is there a reason why it would still be a concern to have this function
public?

https://codereview.chromium.org/2943983003/diff/200001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/media_controls/elements/MediaControlDownloadButtonElement.cpp
(right):

https://codereview.chromium.org/2943983003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/media_controls/elements/MediaControlDownloadButtonElement.cpp:141:
void MediaControlDownloadButtonElement::MaybeDispatchInProductHelpTrigger() {
On 2017/07/13 21:14:27, mlamouri (slow - plz ping) wrote:
> For what I can see, this setting up the mojo pipe. Is it triggering anything?

MaybeShow would have called UpdateInProductHelpState which triggers showing. Now
this triggers it directly.

https://codereview.chromium.org/2943983003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/media_controls/elements/MediaControlDownloadButtonElement.cpp:147:
->GetMediaDownloadInProductHelpEnabled()) {
On 2017/07/13 21:14:27, mlamouri (slow - plz ping) wrote:
> For readability, can you do an early return?

Done.

https://codereview.chromium.org/2943983003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/media_controls/elements/MediaControlDownloadButtonElement.cpp:155:
.GetFrame()
On 2017/07/13 21:14:27, mlamouri (slow - plz ping) wrote:
> Can you check that `GetFrame()` doesn't return `nullptr`?

Done.

https://codereview.chromium.org/2943983003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/media_controls/elements/MediaControlDownloadButtonElement.cpp:163:
GetMediaControls().MaybeShow();
On 2017/07/13 21:14:27, mlamouri (slow - plz ping) wrote:
> Does that mean that when a download button will be added, we will show the
> controls if they are not already showing?

I updated the code to take the CanShow aspect into account, but yes, if we can
show the controls and a download button is added, this will make them visible.

https://codereview.chromium.org/2943983003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/media_controls/elements/MediaControlDownloadButtonElement.cpp:167:
void MediaControlDownloadButtonElement::UpdateInProductHelpState() {
On 2017/07/13 21:14:27, mlamouri (slow - plz ping) wrote:
> Isn't this method actually triggering the UI?

Restructured to move this into MaybeDispatchInProductHelpTrigger.

[Khushal, 2017-07-19 23:48:48]

khushalsagar@chromium.org changed reviewers:
+ isherman@chromium.org

[Khushal, 2017-07-19 23:48:50]

+jochen for chrome/, content/, third_party/Webkit.
+dcheng for mojo IPC review.
pingy to mlamouri@ for media_controls.
+isherman for tools/

[Ilya Sherman, 2017-07-20 00:05:19]

metrics lgtm

[jochen (gone - plz use gerrit), 2017-07-24 10:53:09]

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

[jochen (gone - plz use gerrit), 2017-07-24 10:53:10]

I reviewed content/, chrome/ (except for files in android/ directories), and
WebKit/Source/core and public/

those changes lgtm

for the rest, please ask other reviewers

https://codereview.chromium.org/2943983003/diff/240001/chrome/browser/chrome_...
File chrome/browser/chrome_content_browser_manifest_overlay.json (right):

https://codereview.chromium.org/2943983003/diff/240001/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_manifest_overlay.json:59:
"blink::mojom::MediaDownloadInProductHelp",
please alpha-sort

[mlamouri (slow - plz ping), 2017-07-25 17:29:09]

https://codereview.chromium.org/2943983003/diff/240001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right):

https://codereview.chromium.org/2943983003/diff/240001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:3129: if
(tracker.shouldTriggerHelpUI(FeatureConstants.MEDIA_DOWNLOAD_FEATURE)) {
Maybe you could do:
```
if (!tracker.shouldTriggerHelpUI(FeatureConstants.MEDIA_DOWNLOAD_FEATURE)) {
  nativeMediaDownloadInProductHelpDismissed(mNativeTabAndroid);
  return;
}

// the rest without that indentation

https://codereview.chromium.org/2943983003/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/media_controls/MediaControlsImpl.h
(right):

https://codereview.chromium.org/2943983003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/media_controls/MediaControlsImpl.h:123: bool
CanShow() const;
What does that mean?

https://codereview.chromium.org/2943983003/diff/240001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/media_controls/elements/MediaControlDownloadButtonElement.cpp
(right):

https://codereview.chromium.org/2943983003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/media_controls/elements/MediaControlDownloadButtonElement.cpp:95:
DismissInProductHelp();
That code should no longer be here, you will have to rebase. Most likely, you
update the IPH state in ::UpdateShownState()

https://codereview.chromium.org/2943983003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/media_controls/elements/MediaControlDownloadButtonElement.cpp:158:
return;
Looking at this and what follows, I wonder if it would make sense to have an
object for IPH instead of MediaControlDownloadButton receiving calls to deal
with the IPH. If the object was owned by the MediaControls, it could only be
created when needed (ie. for video element, when the setting was on). It would
also simplify the logic as MediaControls would makes the call to the IPH wrapper
instead of calling methods on MediaControlDownloadButton to update the IPH. I
think the only issue is that the MediaControlDownloadButton visibility would
need to notify the IPH. Maybe the object could be accessible via a getter? WDYT?

https://codereview.chromium.org/2943983003/diff/240001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/media_controls/elements/MediaControlElementBase.h
(right):

https://codereview.chromium.org/2943983003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/media_controls/elements/MediaControlElementBase.h:25:
class MODULES_EXPORT MediaControlElementBase : public GarbageCollectedMixin {
Why is this required?

[Khushal, 2017-08-03 02:41:22]

https://codereview.chromium.org/2943983003/diff/240001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right):

https://codereview.chromium.org/2943983003/diff/240001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:3129: if
(tracker.shouldTriggerHelpUI(FeatureConstants.MEDIA_DOWNLOAD_FEATURE)) {
On 2017/07/25 17:29:08, mlamouri (slow - plz ping) wrote:
> Maybe you could do:
> ```
> if (!tracker.shouldTriggerHelpUI(FeatureConstants.MEDIA_DOWNLOAD_FEATURE)) {
>   nativeMediaDownloadInProductHelpDismissed(mNativeTabAndroid);
>   return;
> }
> 
> // the rest without that indentation

Done.

https://codereview.chromium.org/2943983003/diff/240001/chrome/browser/chrome_...
File chrome/browser/chrome_content_browser_manifest_overlay.json (right):

https://codereview.chromium.org/2943983003/diff/240001/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_manifest_overlay.json:59:
"blink::mojom::MediaDownloadInProductHelp",
On 2017/07/24 10:53:10, jochen (gone - plz use gerrit) wrote:
> please alpha-sort

Done.

https://codereview.chromium.org/2943983003/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/media_controls/MediaControlsImpl.h
(right):

https://codereview.chromium.org/2943983003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/media_controls/MediaControlsImpl.h:123: bool
CanShow() const;
On 2017/07/25 17:29:08, mlamouri (slow - plz ping) wrote:
> What does that mean?

It returns the status of the panel. That is, whether the controls have been
hidden externally using Hide versus internal timer-based hiding. If its the
former, then we don't show the IPH. But if its the latter, then we can
internally make the controls visible for showing it.

https://codereview.chromium.org/2943983003/diff/240001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/media_controls/elements/MediaControlDownloadButtonElement.cpp
(right):

https://codereview.chromium.org/2943983003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/media_controls/elements/MediaControlDownloadButtonElement.cpp:95:
DismissInProductHelp();
On 2017/07/25 17:29:08, mlamouri (slow - plz ping) wrote:
> That code should no longer be here, you will have to rebase. Most likely, you
> update the IPH state in ::UpdateShownState()

Done.

https://codereview.chromium.org/2943983003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/media_controls/elements/MediaControlDownloadButtonElement.cpp:158:
return;
On 2017/07/25 17:29:08, mlamouri (slow - plz ping) wrote:
> Looking at this and what follows, I wonder if it would make sense to have an
> object for IPH instead of MediaControlDownloadButton receiving calls to deal
> with the IPH. If the object was owned by the MediaControls, it could only be
> created when needed (ie. for video element, when the setting was on). It would
> also simplify the logic as MediaControls would makes the call to the IPH
wrapper
> instead of calling methods on MediaControlDownloadButton to update the IPH. I
> think the only issue is that the MediaControlDownloadButton visibility would
> need to notify the IPH. Maybe the object could be accessible via a getter?
WDYT?

We can move this code to a seperate class owned by MediaControlsImpl but it
needs to poke at some internals of the controls.

1) Whether the download button IsWanted and CanFit. Since the dispatch trigger
comes when the pause state changes or download button state changes, it needs to
know that all the conditions are satisfied.

2) Also needs the download button to compute its position.

3) Whether the panel IsWanted. If it was hidden using Hide(), then we don't show
the IPH.

4) When the IPH is dismissed from the browser, it needs to poke the controls to
start the hide timer.

I'm not sure if you'd prefer to expose all of this from the controls? Or may be
it would be cleaner to restore this in the controls and just add one method on
MediaControlsImpl that the download button can call:

void MayBeDispatchDownloadInProductHelp()

from UpdateShownState.

[Khushal, 2017-08-16 04:10:30]

I've moved all of the state tracking to a seperate
MediaDownloadInProductHelpManager with some methods on MediaControlsImpl for
showing the controls when it is dismissed. PTAL.

[mlamouri (slow - plz ping), 2017-08-16 13:09:31]

lgtm with comments applied. I was also wondering what would happen if the user
were to scroll or interact with the page while the IPH is showing. Would the IPH
disappear? Where is this implemented?

https://codereview.chromium.org/2943983003/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/media_controls/MediaControlsImpl.cpp
(right):

https://codereview.chromium.org/2943983003/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/media_controls/MediaControlsImpl.cpp:301:
CreateDownloadInProductHelp();
Can you move this in ::Create()?

https://codereview.chromium.org/2943983003/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/media_controls/MediaControlsImpl.cpp:577:
download_iph_manager_->SetControlsState(true);
Similar to the download button "state", could this be "SetControlsVisibility"? I
assume this is what it's actually used for, is that correct?

https://codereview.chromium.org/2943983003/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/media_controls/MediaControlsImpl.cpp:976:
download_iph_manager_->SetPlayState(true);
This should probably be called "Playback" instead of "Play" or "SetIsPlaying()"
would work too.

https://codereview.chromium.org/2943983003/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/media_controls/MediaControlsImplTest.cpp
(right):

https://codereview.chromium.org/2943983003/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/media_controls/MediaControlsImplTest.cpp:543:
class MediaControlsImplInProductHelpTest : public MediaControlsImplTest {
Would it make sense to create a different unit test file for the IPH tests?

https://codereview.chromium.org/2943983003/diff/300001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/media_controls/MediaDownloadInProductHelpManager.cpp
(right):

https://codereview.chromium.org/2943983003/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/media_controls/MediaDownloadInProductHelpManager.cpp:62:
if (button_rect.IsEmpty())
What if it is partially hidden?

https://codereview.chromium.org/2943983003/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/media_controls/MediaDownloadInProductHelpManager.cpp:100:
return false;
You might want to merge this into one `return controls_can_show_ &&
button_con_show_ && is_playing_;` with a big comment above.

https://codereview.chromium.org/2943983003/diff/300001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/media_controls/elements/MediaControlDownloadButtonElement.cpp
(right):

https://codereview.chromium.org/2943983003/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/media_controls/elements/MediaControlDownloadButtonElement.cpp:9:
#include "core/frame/LocalFrameClient.h"
Looks like you don't need this include anymore.

https://codereview.chromium.org/2943983003/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/media_controls/elements/MediaControlDownloadButtonElement.cpp:18:
#include "platform/wtf/Functional.h"
Do you still need Functional.h?

https://codereview.chromium.org/2943983003/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/media_controls/elements/MediaControlDownloadButtonElement.cpp:102:
auto* iph_manager = static_cast<MediaControlsImpl&>(GetMediaControls())
You shouldn't have to static_cast anymore. Also, do you really need the `auto`
here?

https://codereview.chromium.org/2943983003/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/media_controls/elements/MediaControlDownloadButtonElement.cpp:105:
iph_manager->SetDownloadButtonState(IsWanted() && DoesFit());
Maybe you should call this `SetDownloadButtonVisible()` because it wasn't clear
to me which state you were passing until I read the implementation.

https://codereview.chromium.org/2943983003/diff/300001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/media_controls/elements/MediaControlDownloadButtonElement.h
(right):

https://codereview.chromium.org/2943983003/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/media_controls/elements/MediaControlDownloadButtonElement.h:16:
class MODULES_EXPORT MediaControlDownloadButtonElement final
Do you still need the export?

https://codereview.chromium.org/2943983003/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/media_controls/elements/MediaControlDownloadButtonElement.h:34:
void UpdateShownState() override;
Make it `final`?

https://codereview.chromium.org/2943983003/diff/300001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/media_controls/elements/MediaControlInputElement.h
(right):

https://codereview.chromium.org/2943983003/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/media_controls/elements/MediaControlInputElement.h:50:
void UpdateShownState();
Maybe swap `final` with `override` instead of nothing?

[Khushal, 2017-08-16 19:01:20]

khushalsagar@chromium.org changed reviewers:
+ dcheng@chromium.org

[Khushal, 2017-08-16 19:01:23]

Thanks mlamouri!

+dcheng for IPC review.

https://codereview.chromium.org/2943983003/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/media_controls/MediaControlsImpl.cpp
(right):

https://codereview.chromium.org/2943983003/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/media_controls/MediaControlsImpl.cpp:301:
CreateDownloadInProductHelp();
On 2017/08/16 13:09:30, mlamouri (slow - plz ping) wrote:
> Can you move this in ::Create()?

Done.

https://codereview.chromium.org/2943983003/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/media_controls/MediaControlsImpl.cpp:577:
download_iph_manager_->SetControlsState(true);
On 2017/08/16 13:09:30, mlamouri (slow - plz ping) wrote:
> Similar to the download button "state", could this be "SetControlsVisibility"?
I
> assume this is what it's actually used for, is that correct?

I wasn't sure if I should call it SetControlsVisibility since it states whether
the controls can show, not necessarily that they are. But the param passed is
|can_show| so I guess that should make it obvious.

https://codereview.chromium.org/2943983003/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/media_controls/MediaControlsImpl.cpp:976:
download_iph_manager_->SetPlayState(true);
On 2017/08/16 13:09:30, mlamouri (slow - plz ping) wrote:
> This should probably be called "Playback" instead of "Play" or
"SetIsPlaying()"
> would work too.

Done.

https://codereview.chromium.org/2943983003/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/media_controls/MediaControlsImplTest.cpp
(right):

https://codereview.chromium.org/2943983003/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/media_controls/MediaControlsImplTest.cpp:543:
class MediaControlsImplInProductHelpTest : public MediaControlsImplTest {
On 2017/08/16 13:09:30, mlamouri (slow - plz ping) wrote:
> Would it make sense to create a different unit test file for the IPH tests?

There is a lot test setup here that I wanted to re-use.

https://codereview.chromium.org/2943983003/diff/300001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/media_controls/MediaDownloadInProductHelpManager.cpp
(right):

https://codereview.chromium.org/2943983003/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/media_controls/MediaDownloadInProductHelpManager.cpp:62:
if (button_rect.IsEmpty())
On 2017/08/16 13:09:30, mlamouri (slow - plz ping) wrote:
> What if it is partially hidden?

That's a good point. I could look at button size and compare it with the rect to
see if its clipped. But I think this case should be rare.

For the hidden case, I was seeing instances where we would position the widget
at the origin in the browser because the rect was empty.

https://codereview.chromium.org/2943983003/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/media_controls/MediaDownloadInProductHelpManager.cpp:100:
return false;
On 2017/08/16 13:09:30, mlamouri (slow - plz ping) wrote:
> You might want to merge this into one `return controls_can_show_ &&
> button_con_show_ && is_playing_;` with a big comment above.

Done.

https://codereview.chromium.org/2943983003/diff/300001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/media_controls/elements/MediaControlDownloadButtonElement.cpp
(right):

https://codereview.chromium.org/2943983003/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/media_controls/elements/MediaControlDownloadButtonElement.cpp:9:
#include "core/frame/LocalFrameClient.h"
On 2017/08/16 13:09:30, mlamouri (slow - plz ping) wrote:
> Looks like you don't need this include anymore.

Done.

https://codereview.chromium.org/2943983003/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/media_controls/elements/MediaControlDownloadButtonElement.cpp:18:
#include "platform/wtf/Functional.h"
On 2017/08/16 13:09:30, mlamouri (slow - plz ping) wrote:
> Do you still need Functional.h?

Nope.

https://codereview.chromium.org/2943983003/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/media_controls/elements/MediaControlDownloadButtonElement.cpp:102:
auto* iph_manager = static_cast<MediaControlsImpl&>(GetMediaControls())
On 2017/08/16 13:09:30, mlamouri (slow - plz ping) wrote:
> You shouldn't have to static_cast anymore. Also, do you really need the `auto`
> here?

That's great. Remove the auto. I had it earlier because I didn't want to do the
ugly static_cast multiple times.

https://codereview.chromium.org/2943983003/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/media_controls/elements/MediaControlDownloadButtonElement.cpp:105:
iph_manager->SetDownloadButtonState(IsWanted() && DoesFit());
On 2017/08/16 13:09:30, mlamouri (slow - plz ping) wrote:
> Maybe you should call this `SetDownloadButtonVisible()` because it wasn't
clear
> to me which state you were passing until I read the implementation.

Done.

https://codereview.chromium.org/2943983003/diff/300001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/media_controls/elements/MediaControlDownloadButtonElement.h
(right):

https://codereview.chromium.org/2943983003/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/media_controls/elements/MediaControlDownloadButtonElement.h:16:
class MODULES_EXPORT MediaControlDownloadButtonElement final
On 2017/08/16 13:09:30, mlamouri (slow - plz ping) wrote:
> Do you still need the export?

The tests call SetIsWanted on the button to check that logic, so need the export
for that.

https://codereview.chromium.org/2943983003/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/media_controls/elements/MediaControlDownloadButtonElement.h:34:
void UpdateShownState() override;
On 2017/08/16 13:09:30, mlamouri (slow - plz ping) wrote:
> Make it `final`?

Done.

https://codereview.chromium.org/2943983003/diff/300001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/media_controls/elements/MediaControlInputElement.h
(right):

https://codereview.chromium.org/2943983003/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/media_controls/elements/MediaControlInputElement.h:50:
void UpdateShownState();
On 2017/08/16 13:09:30, mlamouri (slow - plz ping) wrote:
> Maybe swap `final` with `override` instead of nothing?

Done.

[Khushal, 2017-08-16 21:02:06]

https://codereview.chromium.org/2943983003/diff/340001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right):

https://codereview.chromium.org/2943983003/diff/340001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:3131:
mDownloadIPHBubble.setDismissOnTouchInteraction(true);
Oh and about the user scrolling/interacting with the page. That would dismiss
the in-product-help, that logic is here. We have a listener to observer any
touch events on the window and dismisses it.

[dcheng, 2017-08-19 00:30:15]

https://codereview.chromium.org/2943983003/diff/340001/chrome/browser/android...
File chrome/browser/android/tab_android.cc (right):

https://codereview.chromium.org/2943983003/diff/340001/chrome/browser/android...
chrome/browser/android/tab_android.cc:137: content::RenderFrameHost*
render_frame_host_;
Nit: content::RenderFrameHost* const

https://codereview.chromium.org/2943983003/diff/340001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/media_controls/MediaDownloadInProductHelpManager.h
(right):

https://codereview.chromium.org/2943983003/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/media_controls/MediaDownloadInProductHelpManager.h:24:
MediaDownloadInProductHelpManager(MediaControlsImpl&);
Nit: explicit

https://codereview.chromium.org/2943983003/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/media_controls/MediaDownloadInProductHelpManager.h:47:
std::unique_ptr<mojom::blink::MediaDownloadInProductHelpPtr>
No need to wrap the interface pointer in a unique_ptr (the code can check
is_bound(), etc to get the same effect)

[Khushal, 2017-08-21 23:27:34]

Thanks Daniel.

https://codereview.chromium.org/2943983003/diff/340001/chrome/browser/android...
File chrome/browser/android/tab_android.cc (right):

https://codereview.chromium.org/2943983003/diff/340001/chrome/browser/android...
chrome/browser/android/tab_android.cc:137: content::RenderFrameHost*
render_frame_host_;
On 2017/08/19 00:30:15, dcheng wrote:
> Nit: content::RenderFrameHost* const

Done.

https://codereview.chromium.org/2943983003/diff/340001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/media_controls/MediaDownloadInProductHelpManager.h
(right):

https://codereview.chromium.org/2943983003/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/media_controls/MediaDownloadInProductHelpManager.h:24:
MediaDownloadInProductHelpManager(MediaControlsImpl&);
On 2017/08/19 00:30:15, dcheng wrote:
> Nit: explicit

Done.

https://codereview.chromium.org/2943983003/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/media_controls/MediaDownloadInProductHelpManager.h:47:
std::unique_ptr<mojom::blink::MediaDownloadInProductHelpPtr>
On 2017/08/19 00:30:15, dcheng wrote:
> No need to wrap the interface pointer in a unique_ptr (the code can check
> is_bound(), etc to get the same effect)

Done.

[Khushal, 2017-08-21 23:27:44]

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

[commit-bot: I haz the power, 2017-08-21 23:27:51]

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-08-22 01:55:02]

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

[commit-bot: I haz the power, 2017-08-22 01:55:03]

Dry run: This issue passed the CQ dry run.

[dcheng, 2017-08-22 08:01:47]

lgtm

(for future CLs, it's helpful to upload separate patchsets for rebasing and for
responding to CR comments)

[Khushal, 2017-08-22 17:44:06]

On 2017/08/22 08:01:47, dcheng wrote:
> lgtm
> 
> (for future CLs, it's helpful to upload separate patchsets for rebasing and
for
> responding to CR comments)

Thanks Daniel. Sorry about the rebase mixup. I'll make a note of this in the
future.

[Khushal, 2017-08-22 17:44:17]

The CQ bit was checked by khushalsagar@chromium.org

[Khushal, 2017-08-22 17:44:19]

The patchset sent to the CQ was uploaded after l-g-t-m from sammc@chromium.org,
dtrainor@chromium.org, nyquist@chromium.org, jochen@chromium.org,
isherman@chromium.org, mlamouri@chromium.org
Link to the patchset: https://codereview.chromium.org/2943983003/#ps360001
(title: "..")

[commit-bot: I haz the power, 2017-08-22 17:44:30]

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-08-22 17:53:04]

CQ is committing da patch.

Bot data: {"patchset_id": 360001, "attempt_start_ts": 1503423857914460,
"parent_rev": "0fbacfb81345c38e0caa9d1d71edae9372065f6b", "commit_rev":
"c5447db2943848192c0452c7b8e766a943335f07"}

[commit-bot: I haz the power, 2017-08-22 17:53:33]

Description was changed from

==========
chrome/blink: Add functionality for in-product help for media elements.

IPH (In-product help) UI is added to highlight and assist in the
discovery of features in the browser. This change adds this
functionality for the download feature on media elements in blink. This
involves the following additions:

1) A public WebMediaIPH interface in blink that can be used to ask the
embedder to display IPH widgets for an element.

2) A mojo MediaIPHService to bridge these requests to the browser where
the UI can be shown using android pop-ups overlayed on the content.

3) An addition to the features supported by the FeatureEngagementTracker
for media download IPH.

The feature is currently gated behind a finch flag which is disabled by
default.

BUG=715185
==========

to

==========
chrome/blink: Add functionality for in-product help for media elements.

IPH (In-product help) UI is added to highlight and assist in the
discovery of features in the browser. This change adds this
functionality for the download feature on media elements in blink. This
involves the following additions:

1) A public WebMediaIPH interface in blink that can be used to ask the
embedder to display IPH widgets for an element.

2) A mojo MediaIPHService to bridge these requests to the browser where
the UI can be shown using android pop-ups overlayed on the content.

3) An addition to the features supported by the FeatureEngagementTracker
for media download IPH.

The feature is currently gated behind a finch flag which is disabled by
default.

BUG=715185

Review-Url: https://codereview.chromium.org/2943983003
Cr-Commit-Position: refs/heads/master@{#496361}
Committed:
https://chromium.googlesource.com/chromium/src/+/c5447db2943848192c0452c7b8e7...
==========

[commit-bot: I haz the power, 2017-08-22 17:53:35]

Committed patchset #19 (id:360001) as
https://chromium.googlesource.com/chromium/src/+/c5447db2943848192c0452c7b8e7...