IPH - Added triggers for Download page

IPH - Added triggers for Download page

This CL adds in-product-help to download a page for viewing offline later.
The IPH would be shown after a successful page navigation after the user
has seen an offline dino.

Backend interaction:
1 - On every successful page load - Check ShouldTriggerHelpUI(IPH_DownloadPage)
2 - On every show of dino page - Send event user_has_seen_dino
3 - On every of download of page - Send event download_page_started
4 - On dismiss of IPH - Send dismissed() to tracker

BUG=710646
Review-Url: https://codereview.chromium.org/2814843006
Cr-Commit-Position: refs/heads/master@{#467091}
Committed: https://chromium.googlesource.com/chromium/src/+/758540e55198236edc0a417b7dd69e60a3ceedac

[shaktisahu, 2017-04-12 23:20:44]

Description was changed from

==========
IPH - Download page

BUG=710646
==========

to

==========
IPH - Added triggers for Download page

This CL adds in-product-help to download a page for viewing offline later.

BUG=710646
==========

[shaktisahu, 2017-04-12 23:22:11]

Description was changed from

==========
IPH - Added triggers for Download page

This CL adds in-product-help to download a page for viewing offline later.

BUG=710646
==========

to

==========
IPH - Added triggers for Download page

This CL adds in-product-help to download a page for viewing offline later.
The IPH would be shown after a successful page navigation after the user
has seen an offline dino.

BUG=710646
==========

[shaktisahu, 2017-04-12 23:37:55]

Patchset #2 (id:20001) has been deleted

[shaktisahu, 2017-04-12 23:38:10]

Patchset #1 (id:1) has been deleted

[shaktisahu, 2017-04-12 23:38:51]

shaktisahu@chromium.org changed reviewers:
+ dtrainor@chromium.org, nyquist@chromium.org

[shaktisahu, 2017-04-12 23:38:51]

PTAL.

[shaktisahu, 2017-04-12 23:39:16]

Description was changed from

==========
IPH - Added triggers for Download page

This CL adds in-product-help to download a page for viewing offline later.
The IPH would be shown after a successful page navigation after the user
has seen an offline dino.

BUG=710646
==========

to

==========
IPH - Added triggers for Download page

This CL adds in-product-help to download a page for viewing offline later.
The IPH would be shown after a successful page navigation after the user
has seen an offline dino.

BUG=710646
==========

[David Trainor- moved to gerrit, 2017-04-13 03:58:25]

lgtm % nit.  Are there other criteria we care about for whether or not we want
to show this?  e.g. do we want this showing up for custom tabs?  Should this
only show for the chrome tabbed activity?  I think ToolbarManager might be used
for all types of page viewing?

https://codereview.chromium.org/2814843006/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java
(right):

https://codereview.chromium.org/2814843006/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java:493:
private void displayFeatureEngagementTextBubbleIfNecessary(Tab tab, int
errorCode) {
Should we dismiss the bubble if the page navigates again?

[David Trainor- moved to gerrit, 2017-04-13 03:58:39]

On 2017/04/13 03:58:25, David Trainor-ping if over 24h wrote:
> lgtm % nit.  Are there other criteria we care about for whether or not we want
> to show this?  e.g. do we want this showing up for custom tabs?  Should this
> only show for the chrome tabbed activity?  I think ToolbarManager might be
used
> for all types of page viewing?
> 
>
https://codereview.chromium.org/2814843006/diff/40001/chrome/android/java/src...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java
> (right):
> 
>
https://codereview.chromium.org/2814843006/diff/40001/chrome/android/java/src...
>
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java:493:
> private void displayFeatureEngagementTextBubbleIfNecessary(Tab tab, int
> errorCode) {
> Should we dismiss the bubble if the page navigates again?

Oh also, make sure we're not in overview mode?

[David Trainor- moved to gerrit, 2017-04-13 04:09:15]

On 2017/04/13 03:58:39, David Trainor-ping if over 24h wrote:
> On 2017/04/13 03:58:25, David Trainor-ping if over 24h wrote:
> > lgtm % nit.  Are there other criteria we care about for whether or not we
want
> > to show this?  e.g. do we want this showing up for custom tabs?  Should this
> > only show for the chrome tabbed activity?  I think ToolbarManager might be
> used
> > for all types of page viewing?
> > 
> >
>
https://codereview.chromium.org/2814843006/diff/40001/chrome/android/java/src...
> > File
> >
>
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/2814843006/diff/40001/chrome/android/java/src...
> >
>
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java:493:
> > private void displayFeatureEngagementTextBubbleIfNecessary(Tab tab, int
> > errorCode) {
> > Should we dismiss the bubble if the page navigates again?
> 
> Oh also, make sure we're not in overview mode?

Finally, when https://codereview.chromium.org/2779543005/ lands, you should be
able to set the highlight on the menu through the AppMenuAdapter.  One note
about that, be sure to clear the highlight when the popup is dismissed.  Once
the menu is showing clearing the highlight does nothing.  You might have an
issue where you're clearing it too early.  If that's the case maybe post? 
You'll have to investigate.

[David Trainor- moved to gerrit, 2017-04-13 07:33:34]

On 2017/04/13 04:09:15, David Trainor-ping if over 24h wrote:
> On 2017/04/13 03:58:39, David Trainor-ping if over 24h wrote:
> > On 2017/04/13 03:58:25, David Trainor-ping if over 24h wrote:
> > > lgtm % nit.  Are there other criteria we care about for whether or not we
> want
> > > to show this?  e.g. do we want this showing up for custom tabs?  Should
this
> > > only show for the chrome tabbed activity?  I think ToolbarManager might be
> > used
> > > for all types of page viewing?
> > > 
> > >
> >
>
https://codereview.chromium.org/2814843006/diff/40001/chrome/android/java/src...
> > > File
> > >
> >
>
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2814843006/diff/40001/chrome/android/java/src...
> > >
> >
>
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java:493:
> > > private void displayFeatureEngagementTextBubbleIfNecessary(Tab tab, int
> > > errorCode) {
> > > Should we dismiss the bubble if the page navigates again?
> > 
> > Oh also, make sure we're not in overview mode?
> 
> Finally, when https://codereview.chromium.org/2779543005/ lands, you should be
> able to set the highlight on the menu through the AppMenuAdapter.  One note
> about that, be sure to clear the highlight when the popup is dismissed.  Once
> the menu is showing clearing the highlight does nothing.  You might have an
> issue where you're clearing it too early.  If that's the case maybe post? 
> You'll have to investigate.

More thoughts.. we probably don't want this on tablets.  The download page
button is actually in the toolbar. We could do something else for that trigger
if we want to.

[shaktisahu, 2017-04-13 23:00:42]

Made some more changes :
1- Added menu highlight
2- Do this for only ChromeTabbedActivity
3- Exclude tablets
4- Exclude if in overview mode
5- Added a delay of 500ms, or else DownloadUtils.isAllowedToDownloadPage cannot
find if it is already an offline page correctly
6- Dismiss the bubble on another navigation e.g. back button press.

[shaktisahu, 2017-04-13 23:28:42]

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

[commit-bot: I haz the power, 2017-04-13 23:29:11]

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

[nyquist, 2017-04-13 23:48:57]

https://codereview.chromium.org/2814843006/diff/60001/components/feature_enga...
File
components/feature_engagement_tracker/public/android/java/src/org/chromium/components/feature_engagement_tracker/FeatureConstants.java
(right):

https://codereview.chromium.org/2814843006/diff/60001/components/feature_enga...
components/feature_engagement_tracker/public/android/java/src/org/chromium/components/feature_engagement_tracker/FeatureConstants.java:14:
public static final String USER_HAS_SEEN_DINO_EVENT = "user_has_seen_dino";
Could you put this in EventConstants.java? (same for native if they are needed
there)

[David Trainor- moved to gerrit, 2017-04-13 23:50:49]

https://codereview.chromium.org/2814843006/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java
(right):

https://codereview.chromium.org/2814843006/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java:506:
if (!(activity instanceof ChromeTabbedActivity)
Is there a way to detect this without ChromeTabbedActivity?  If not I'm ok with
it I guess.

https://codereview.chromium.org/2814843006/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java:518:
return;
Do you need this if we're returning on the next if block?

https://codereview.chromium.org/2814843006/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java:538:
mToolbar.setMenuButtonHighlight(false);
You need to call through the
AppMenuHandler#setHighlightId(R.id.download_page_id)?

[commit-bot: I haz the power, 2017-04-13 23:53:02]

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

[commit-bot: I haz the power, 2017-04-13 23:53:04]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[shaktisahu, 2017-04-14 03:21:39]

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

[commit-bot: I haz the power, 2017-04-14 03:21:48]

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

[shaktisahu, 2017-04-14 03:57:51]

Patchset #5 (id:120001) has been deleted

[shaktisahu, 2017-04-14 04:01:06]

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

[commit-bot: I haz the power, 2017-04-14 04:01:18]

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

[shaktisahu, 2017-04-14 04:19:34]

Addressed the menu item highlight timing issue.
PTAL

https://codereview.chromium.org/2814843006/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java
(right):

https://codereview.chromium.org/2814843006/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java:506:
if (!(activity instanceof ChromeTabbedActivity)
On 2017/04/13 23:50:49, David Trainor-ping if over 24h wrote:
> Is there a way to detect this without ChromeTabbedActivity?  If not I'm ok
with
> it I guess.

Hmm, didn't find any so far..

https://codereview.chromium.org/2814843006/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java:518:
return;
On 2017/04/13 23:50:49, David Trainor-ping if over 24h wrote:
> Do you need this if we're returning on the next if block?

Done.

https://codereview.chromium.org/2814843006/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java:538:
mToolbar.setMenuButtonHighlight(false);
On 2017/04/13 23:50:49, David Trainor-ping if over 24h wrote:
> You need to call through the
> AppMenuHandler#setHighlightId(R.id.download_page_id)?

Done.

Added a 20ms delay before clearing the menu highlight. This works nicely in both
:
1- when touched outside
2- when touched on the menu anchor (> delay between tapping on anchor and menu
showing up)

[commit-bot: I haz the power, 2017-04-14 05:01:03]

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

[commit-bot: I haz the power, 2017-04-14 05:01:04]

Dry run: This issue passed the CQ dry run.

[David Trainor- moved to gerrit, 2017-04-17 23:03:41]

https://codereview.chromium.org/2814843006/diff/160001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java
(right):

https://codereview.chromium.org/2814843006/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java:121:
private static final int CLEAR_MENU_ITEM_HIGHLIGHT_DELAY_MS = 20;
Do we have to delay here?  Should we just post the task?

https://codereview.chromium.org/2814843006/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java:509:
if (!(activity instanceof ChromeTabbedActivity)
Add a TODO and a bug to key this off of whether or not the menu item will be
there.

https://codereview.chromium.org/2814843006/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java:551:
}, INITIAL_DELAY_IPH_DOWNLOAD_PAGE_MS);
Why do we delay the show call again?

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

https://codereview.chromium.org/2814843006/diff/160001/chrome/android/java/st...
chrome/android/java/strings/android_chrome_strings.grd:2928: <message
name="IDS_IPH_DOWNLOAD_PAGE_FOR_OFFLINE_USAGE_TEXT" desc="The in-product-help
message after a successful navigation.">
Should this description say something about using pages offline?

https://codereview.chromium.org/2814843006/diff/160001/components/feature_eng...
File
components/feature_engagement_tracker/public/android/java/src/org/chromium/components/feature_engagement_tracker/EventConstants.java
(right):

https://codereview.chromium.org/2814843006/diff/160001/components/feature_eng...
components/feature_engagement_tracker/public/android/java/src/org/chromium/components/feature_engagement_tracker/EventConstants.java:8:
* EventConstants contains the String name of all in-product help events.
Should these be mirrored in native as well?  I can add that if you want, I need
them there.

[shaktisahu, 2017-04-18 21:48:33]

Patchset #7 (id:180001) has been deleted

[shaktisahu, 2017-04-18 21:48:48]

Patchset #7 (id:200001) has been deleted

[nyquist, 2017-04-18 22:51:50]

Interaction with //components/feature_engagement_tracker lgtm

[shaktisahu, 2017-04-19 02:39:11]

dtrainor@ - Can you please take another look?
I removed the timing delays.
Added an inset to the bubble to match with the slides.
Also improved the condition when the "user has seen offline dino" event is
fired.

https://codereview.chromium.org/2814843006/diff/160001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java
(right):

https://codereview.chromium.org/2814843006/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java:121:
private static final int CLEAR_MENU_ITEM_HIGHLIGHT_DELAY_MS = 20;
On 2017/04/17 23:03:41, David Trainor-ping if over 24h wrote:
> Do we have to delay here?  Should we just post the task?

No, just posting the task seems sufficient.

https://codereview.chromium.org/2814843006/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java:509:
if (!(activity instanceof ChromeTabbedActivity)
On 2017/04/17 23:03:41, David Trainor-ping if over 24h wrote:
> Add a TODO and a bug to key this off of whether or not the menu item will be
> there.

Done. Added below.

https://codereview.chromium.org/2814843006/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java:551:
}, INITIAL_DELAY_IPH_DOWNLOAD_PAGE_MS);
On 2017/04/17 23:03:41, David Trainor-ping if over 24h wrote:
> Why do we delay the show call again?

Removed this delay. Moved trigger part of this function and the preconditions to
onPageLoadFinished() instead.

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

https://codereview.chromium.org/2814843006/diff/160001/chrome/android/java/st...
chrome/android/java/strings/android_chrome_strings.grd:2928: <message
name="IDS_IPH_DOWNLOAD_PAGE_FOR_OFFLINE_USAGE_TEXT" desc="The in-product-help
message after a successful navigation.">
On 2017/04/17 23:03:41, David Trainor-ping if over 24h wrote:
> Should this description say something about using pages offline?

Done.

https://codereview.chromium.org/2814843006/diff/160001/components/feature_eng...
File
components/feature_engagement_tracker/public/android/java/src/org/chromium/components/feature_engagement_tracker/EventConstants.java
(right):

https://codereview.chromium.org/2814843006/diff/160001/components/feature_eng...
components/feature_engagement_tracker/public/android/java/src/org/chromium/components/feature_engagement_tracker/EventConstants.java:8:
* EventConstants contains the String name of all in-product help events.
On 2017/04/17 23:03:41, David Trainor-ping if over 24h wrote:
> Should these be mirrored in native as well?  I can add that if you want, I
need
> them there.

Ok. I didn't think they are needed in native. Yea, go ahead if you are adding
them there.

[nyquist, 2017-04-24 20:24:20]

https://codereview.chromium.org/2814843006/diff/260001/components/feature_eng...
File
components/feature_engagement_tracker/public/android/java/src/org/chromium/components/feature_engagement_tracker/EventConstants.java
(right):

https://codereview.chromium.org/2814843006/diff/260001/components/feature_eng...
components/feature_engagement_tracker/public/android/java/src/org/chromium/components/feature_engagement_tracker/EventConstants.java:11:
public static final String USER_HAS_SEEN_DINO = "user_has_seen_dino";
Can you add a JavaDoc for all these?

[David Trainor- moved to gerrit, 2017-04-25 03:41:09]

lgtm % nits.

https://codereview.chromium.org/2814843006/diff/260001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java
(right):

https://codereview.chromium.org/2814843006/diff/260001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java:120:
private static final float TEXT_BUBBLE_Y_INSET_DP = 14.f;
Remove?

https://codereview.chromium.org/2814843006/diff/260001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java:358:
notifyFeatureEngagementTracker(tab);
Should this be more descriptive?  It's doing something very specific.  Notify
just seems like general notify.

https://codereview.chromium.org/2814843006/diff/260001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java:362:
displayFeatureEngagementTextBubbleIfNecessary(tab);
Should this be more descriptive that we're doing it on page load? 
handlePageLoadedEngagementTrackers(tab)?

https://codereview.chromium.org/2814843006/diff/260001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java:526:
if (!DownloadUtils.isAllowedToDownloadPage(tab)) return;
Move this up to the above if block (that covers other reasons to early-out) and
move the comment above that?

https://codereview.chromium.org/2814843006/diff/260001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java:528:
return;
Does this fit on one line? :D

https://codereview.chromium.org/2814843006/diff/260001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java:561:
|| !bridge.isShowingDownloadButtonInErrorPage(tab.getWebContents())) {
Don't feel strongly either way, but maybe this should be inside
NetErrorTabHelper::OnSetIsShowingDownloadButtonInErrorPage?

[David Trainor- moved to gerrit, 2017-04-25 03:44:04]

https://codereview.chromium.org/2814843006/diff/260001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java
(right):

https://codereview.chromium.org/2814843006/diff/260001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java:561:
|| !bridge.isShowingDownloadButtonInErrorPage(tab.getWebContents())) {
On 2017/04/25 03:41:09, David Trainor-ping if over 24h wrote:
> Don't feel strongly either way, but maybe this should be inside
> NetErrorTabHelper::OnSetIsShowingDownloadButtonInErrorPage?

Ah maybe it won't work if it's in net?  Hrm.

[shaktisahu, 2017-04-25 19:05:14]

Description was changed from

==========
IPH - Added triggers for Download page

This CL adds in-product-help to download a page for viewing offline later.
The IPH would be shown after a successful page navigation after the user
has seen an offline dino.

BUG=710646
==========

to

==========
IPH - Added triggers for Download page

This CL adds in-product-help to download a page for viewing offline later.
The IPH would be shown after a successful page navigation after the user
has seen an offline dino.

Backend interaction:
1 - On every successful page load - Check ShouldTriggerHelpUI(IPH_DownloadPage)
2 - On every show of dino page - Send event user_has_seen_dino
3 - On every of download of page - Send event download_page_started
4 - On dismiss of IPH - Send dismissed() to tracker

BUG=710646
==========

[shaktisahu, 2017-04-25 19:13:54]

Patchset #10 (id:280001) has been deleted

[shaktisahu, 2017-04-25 19:14:24]

The CQ bit was checked by shaktisahu@chromium.org

[shaktisahu, 2017-04-25 19:14:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
nyquist@chromium.org, dtrainor@chromium.org
Link to the patchset: https://codereview.chromium.org/2814843006/#ps300001
(title: "nits")

[commit-bot: I haz the power, 2017-04-25 19:15:21]

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-04-25 20:20:58]

CQ is committing da patch.

Bot data: {"patchset_id": 300001, "attempt_start_ts": 1493147664977460,
"parent_rev": "5cb52246012a13494269d5316af065201a8725b4", "commit_rev":
"758540e55198236edc0a417b7dd69e60a3ceedac"}

[commit-bot: I haz the power, 2017-04-25 20:22:15]

Description was changed from

==========
IPH - Added triggers for Download page

This CL adds in-product-help to download a page for viewing offline later.
The IPH would be shown after a successful page navigation after the user
has seen an offline dino.

Backend interaction:
1 - On every successful page load - Check ShouldTriggerHelpUI(IPH_DownloadPage)
2 - On every show of dino page - Send event user_has_seen_dino
3 - On every of download of page - Send event download_page_started
4 - On dismiss of IPH - Send dismissed() to tracker

BUG=710646
==========

to

==========
IPH - Added triggers for Download page

This CL adds in-product-help to download a page for viewing offline later.
The IPH would be shown after a successful page navigation after the user
has seen an offline dino.

Backend interaction:
1 - On every successful page load - Check ShouldTriggerHelpUI(IPH_DownloadPage)
2 - On every show of dino page - Send event user_has_seen_dino
3 - On every of download of page - Send event download_page_started
4 - On dismiss of IPH - Send dismissed() to tracker

BUG=710646

Review-Url: https://codereview.chromium.org/2814843006
Cr-Commit-Position: refs/heads/master@{#467091}
Committed:
https://chromium.googlesource.com/chromium/src/+/758540e55198236edc0a417b7dd6...
==========

[commit-bot: I haz the power, 2017-04-25 20:22:17]

Committed patchset #10 (id:300001) as
https://chromium.googlesource.com/chromium/src/+/758540e55198236edc0a417b7dd6...

[shaktisahu, 2017-04-25 21:47:58]

https://codereview.chromium.org/2814843006/diff/260001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java
(right):

https://codereview.chromium.org/2814843006/diff/260001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java:120:
private static final float TEXT_BUBBLE_Y_INSET_DP = 14.f;
On 2017/04/25 03:41:09, David Trainor-ping if over 24h wrote:
> Remove?

Done.

https://codereview.chromium.org/2814843006/diff/260001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java:358:
notifyFeatureEngagementTracker(tab);
On 2017/04/25 03:41:09, David Trainor-ping if over 24h wrote:
> Should this be more descriptive?  It's doing something very specific.  Notify
> just seems like general notify.

Done.

https://codereview.chromium.org/2814843006/diff/260001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java:362:
displayFeatureEngagementTextBubbleIfNecessary(tab);
On 2017/04/25 03:41:09, David Trainor-ping if over 24h wrote:
> Should this be more descriptive that we're doing it on page load? 
> handlePageLoadedEngagementTrackers(tab)?

Done.

https://codereview.chromium.org/2814843006/diff/260001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java:526:
if (!DownloadUtils.isAllowedToDownloadPage(tab)) return;
On 2017/04/25 03:41:09, David Trainor-ping if over 24h wrote:
> Move this up to the above if block (that covers other reasons to early-out)
and
> move the comment above that?

Done.

https://codereview.chromium.org/2814843006/diff/260001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java:528:
return;
On 2017/04/25 03:41:09, David Trainor-ping if over 24h wrote:
> Does this fit on one line? :D

Done.