Hook up Reader Mode InfoBar

Hook up Reader Mode InfoBar

This change replaces the OverlayPanel UI for Reader Mode in favor of
the InfoBar UI. Some notable changes associated with the UI switch:

- Navigations to distilled content are not retained in history. This
  means a backward navigation will skip distilled content and bring
  the user to the original page.
- The UI no longer animates or sits on top of the original web page.
- Since Reader Mode is now an infobar, it interacts nicely with other
  infobars that may appear.
- All operations in the overflow menu use the base page URL.
- The "Appearance" overflow item is once again available.
- Code complexity has been considerably reduced.

BUG=710014, 624009
Review-Url: https://codereview.chromium.org/2878543003
Cr-Commit-Position: refs/heads/master@{#479229}
Committed: https://chromium.googlesource.com/chromium/src/+/75af3a8ba9bd8724e83d99db0113bc8836b472ae

[mdjones, 2017-05-11 01:04:22]

mdjones@chromium.org changed reviewers:
+ twellington@chromium.org

[mdjones, 2017-05-11 01:04:23]

ptal

[Theresa, 2017-05-11 22:31:19]

http://www.reactiongifs.us/wp-content/uploads/2013/06/cheering_minions.gif

https://codereview.chromium.org/2878543003/diff/30001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/readermode/ReaderModePanel.java
(right):

https://codereview.chromium.org/2878543003/diff/30001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/readermode/ReaderModePanel.java:37:
public class ReaderModePanel extends OverlayPanel {
Can we delete this class (or remove the extension) eventually? Or now? It looks
like the one instantiation in LayoutManagerDocument was removed.

https://codereview.chromium.org/2878543003/diff/30001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/ReaderModeManager.java
(right):

https://codereview.chromium.org/2878543003/diff/30001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/ReaderModeManager.java:277:
if (reason != StateChangeReason.FLING && reason != StateChangeReason.SWIPE
StateChangeReason doesn't really apply anymore right?

https://codereview.chromium.org/2878543003/diff/30001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/ReaderModeManager.java:299:
// TODO(mdjones): Remove this method and dependencies.
InfoBar handles automatically dismissing itself?

https://codereview.chromium.org/2878543003/diff/30001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/ReaderModeManager.java:344:
public void didFinishNavigation(String url, boolean isInMainFrame, boolean
isErrorPage,
When is this called vs onDidFinishNavigation?

https://codereview.chromium.org/2878543003/diff/30001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/ReaderModeManagerDelegate.java
(right):

https://codereview.chromium.org/2878543003/diff/30001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/ReaderModeManagerDelegate.java:24:
void onPanelShown();
onInfobarShown()? Or not needed?

https://codereview.chromium.org/2878543003/diff/30001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/ReaderModeManagerDelegate.java:27:
* Notify the manager that the panel has completely closed.
s/panel/infobar?

https://codereview.chromium.org/2878543003/diff/30001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/dom_distiller/ReaderModeManagerTest.java
(left):

https://codereview.chromium.org/2878543003/diff/30001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/dom_distiller/ReaderModeManagerTest.java:36:
* Tests logic in the ReaderModeManager.
This class was mostly testing overlay panel behavior?

[mdjones, 2017-05-12 19:13:10]

https://codereview.chromium.org/2878543003/diff/30001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/readermode/ReaderModePanel.java
(right):

https://codereview.chromium.org/2878543003/diff/30001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/readermode/ReaderModePanel.java:37:
public class ReaderModePanel extends OverlayPanel {
On 2017/05/11 22:31:18, Theresa wrote:
> Can we delete this class (or remove the extension) eventually? Or now? It
looks
> like the one instantiation in LayoutManagerDocument was removed.

I was going to do a followup CL because there are maybe 8 classes that need to
be deleted.

https://codereview.chromium.org/2878543003/diff/30001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/ReaderModeManager.java
(right):

https://codereview.chromium.org/2878543003/diff/30001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/ReaderModeManager.java:277:
if (reason != StateChangeReason.FLING && reason != StateChangeReason.SWIPE
On 2017/05/11 22:31:19, Theresa wrote:
> StateChangeReason doesn't really apply anymore right?

Done.

https://codereview.chromium.org/2878543003/diff/30001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/ReaderModeManager.java:299:
// TODO(mdjones): Remove this method and dependencies.
On 2017/05/11 22:31:19, Theresa wrote:
> InfoBar handles automatically dismissing itself?

In most cases where we want it, yes. That's the nice thing about being an
infobar, most of the work is already done.

https://codereview.chromium.org/2878543003/diff/30001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/ReaderModeManager.java:344:
public void didFinishNavigation(String url, boolean isInMainFrame, boolean
isErrorPage,
On 2017/05/11 22:31:19, Theresa wrote:
> When is this called vs onDidFinishNavigation?

Not a big difference, I'll move those functions into this observer.

https://codereview.chromium.org/2878543003/diff/30001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/ReaderModeManagerDelegate.java
(right):

https://codereview.chromium.org/2878543003/diff/30001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/ReaderModeManagerDelegate.java:24:
void onPanelShown();
On 2017/05/11 22:31:19, Theresa wrote:
> onInfobarShown()? Or not needed?

This is one of the classes that is going to disappear.

https://codereview.chromium.org/2878543003/diff/30001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/ReaderModeManagerDelegate.java:27:
* Notify the manager that the panel has completely closed.
On 2017/05/11 22:31:19, Theresa wrote:
> s/panel/infobar?

Cleanup CL to come.

https://codereview.chromium.org/2878543003/diff/30001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/dom_distiller/ReaderModeManagerTest.java
(left):

https://codereview.chromium.org/2878543003/diff/30001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/dom_distiller/ReaderModeManagerTest.java:36:
* Tests logic in the ReaderModeManager.
On 2017/05/11 22:31:18, Theresa wrote:
> This class was mostly testing overlay panel behavior?

Exclusively. It was testing all the interactions that infobars just get for
free. We don't need to have any custom handling any more, so this class is
unneeded.

[Theresa, 2017-05-12 20:52:51]

lgtm

[mdjones, 2017-05-12 21:37:57]

mdjones@chromium.org changed reviewers:
+ wychen@chromium.org

[mdjones, 2017-05-12 21:37:58]

+wychen ptal

[wychen, 2017-05-12 23:22:38]

Awesome! Could you rebase it? I'd like to give it a try.

[mdjones, 2017-05-15 16:36:51]

On 2017/05/12 23:22:38, wychen wrote:
> Awesome! Could you rebase it? I'd like to give it a try.

Done.

[wychen, 2017-05-16 00:19:03]

https://codereview.chromium.org/2878543003/diff/70001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/readermode/ReaderModePanel.java
(right):

https://codereview.chromium.org/2878543003/diff/70001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/readermode/ReaderModePanel.java:37:
public class ReaderModePanel extends OverlayPanel {
Is ReaderModePanel needed anymore?

https://codereview.chromium.org/2878543003/diff/70001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/2878543003/diff/70001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:13534: <histogram
name="DomDistiller.Time.SwipeToPaint" units="ms">
DomDistiller.Time.ViewingReaderModePanel would be obsolete after this, right?
We'd need a signal telling us how much time the user has spent in the distilled
page.

[mdjones, 2017-05-16 23:29:35]

https://codereview.chromium.org/2878543003/diff/70001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/readermode/ReaderModePanel.java
(right):

https://codereview.chromium.org/2878543003/diff/70001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/readermode/ReaderModePanel.java:37:
public class ReaderModePanel extends OverlayPanel {
On 2017/05/16 00:19:03, wychen wrote:
> Is ReaderModePanel needed anymore?

Cleanup CL here: https://codereview.chromium.org/2883643002/

https://codereview.chromium.org/2878543003/diff/70001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/2878543003/diff/70001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:13534: <histogram
name="DomDistiller.Time.SwipeToPaint" units="ms">
On 2017/05/16 00:19:03, wychen wrote:
> DomDistiller.Time.ViewingReaderModePanel would be obsolete after this, right?
> We'd need a signal telling us how much time the user has spent in the
distilled
> page.

Added the new duration metric and cleaned up a bit more.

[wychen, 2017-05-17 07:24:12]

https://codereview.chromium.org/2878543003/diff/90001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/ReaderModeManager.java
(right):

https://codereview.chromium.org/2878543003/diff/90001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/ReaderModeManager.java:202:
if (!mTabStatusMap.containsKey(tab.getId())) {
Is it safe to reuse tabInfo? If we forget to reset some fields, it could be
polluted by the previous item.

Or are you deliberately preserving some fields? If so, comments would be
helpful.

https://codereview.chromium.org/2878543003/diff/90001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/ReaderModeTabInfo.java
(right):

https://codereview.chromium.org/2878543003/diff/90001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/ReaderModeTabInfo.java:58:
mViewStartTimeMs = System.currentTimeMillis();
Monotonic time like elapsedRealtime() should be better.

[mdjones, 2017-05-17 16:51:53]

https://codereview.chromium.org/2878543003/diff/90001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/ReaderModeManager.java
(right):

https://codereview.chromium.org/2878543003/diff/90001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/ReaderModeManager.java:202:
if (!mTabStatusMap.containsKey(tab.getId())) {
On 2017/05/17 07:24:12, wychen wrote:
> Is it safe to reuse tabInfo? If we forget to reset some fields, it could be
> polluted by the previous item.
> 
> Or are you deliberately preserving some fields? If so, comments would be
> helpful.

Deliberately preserving some fields. Turns out we don't have to delete tab state
at all. Added doc.

[mdjones, 2017-05-17 16:52:15]

https://codereview.chromium.org/2878543003/diff/90001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/ReaderModeTabInfo.java
(right):

https://codereview.chromium.org/2878543003/diff/90001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/ReaderModeTabInfo.java:58:
mViewStartTimeMs = System.currentTimeMillis();
On 2017/05/17 07:24:12, wychen wrote:
> Monotonic time like elapsedRealtime() should be better.

Done.

[wychen, 2017-05-18 15:16:55]

lgtm

[wychen, 2017-05-18 15:26:22]

https://codereview.chromium.org/2878543003/diff/110001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/ReaderModeManager.java
(right):

https://codereview.chromium.org/2878543003/diff/110001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/ReaderModeManager.java:63:
/** The ChromeActivity that this panel exists in. */
Do we still call it panel?

https://codereview.chromium.org/2878543003/diff/110001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/ReaderModeManager.java:115:
// If the reader panel was dismissed, stop here.
s/panel/infobar/?

[mdjones, 2017-05-18 16:45:55]

https://codereview.chromium.org/2878543003/diff/110001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/ReaderModeManager.java
(right):

https://codereview.chromium.org/2878543003/diff/110001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/ReaderModeManager.java:63:
/** The ChromeActivity that this panel exists in. */
On 2017/05/18 15:26:22, wychen wrote:
> Do we still call it panel?

I renamed panel to infobar in places that will exist after cleanup.

https://codereview.chromium.org/2878543003/diff/110001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/ReaderModeManager.java:115:
// If the reader panel was dismissed, stop here.
On 2017/05/18 15:26:21, wychen wrote:
> s/panel/infobar/?

Done.

[mdjones, 2017-05-18 16:46:37]

mdjones@chromium.org changed reviewers:
+ jwd@chromium.org

[mdjones, 2017-05-18 16:46:38]

+jwd for histograms.xml

[jwd, 2017-05-19 19:48:03]

lgtm

[mdjones, 2017-05-19 20:28:47]

mdjones@chromium.org changed reviewers:
+ tedchoc@chromium.org

[mdjones, 2017-05-19 20:28:47]

+tedchoc ptal

[Ted C, 2017-05-19 23:19:07]

https://codereview.chromium.org/2878543003/diff/150001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/ReaderModeInfoBar.java
(right):

https://codereview.chromium.org/2878543003/diff/150001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/infobar/ReaderModeInfoBar.java:60:
@Override
per some offline discussion, infobars "should" delete themselves on navigations

https://cs.chromium.org/chromium/src/components/infobars/core/infobar_delegat...

[mdjones, 2017-05-22 21:11:03]

https://codereview.chromium.org/2878543003/diff/150001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/ReaderModeInfoBar.java
(right):

https://codereview.chromium.org/2878543003/diff/150001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/infobar/ReaderModeInfoBar.java:60:
@Override
On 2017/05/19 23:19:07, Ted C wrote:
> per some offline discussion, infobars "should" delete themselves on
navigations
> 
>
https://cs.chromium.org/chromium/src/components/infobars/core/infobar_delegat...

Done.

[Ted C, 2017-05-22 21:14:12]

lgtm

[mdjones, 2017-06-07 00:22:49]

Description was changed from

==========
Hook up Reader Mode InfoBar

This change replaces the OverlayPanel UI for Reader Mode in favor of
the InfoBar UI. Some notable changes associated with the UI switch:

- Navigations to distilled content are not retained in history. This
  means a backward navigation will skip distilled content and bring
  the user to the original page.
- The UI no longer animates or sits on top of the original web page.
- Since Reader Mode is now an infobar, it interacts nicely with other
  infobars that may appear.
- All operations in the overflow menu use the base page URL.
- The "Appearance" overflow item is once again available.
- Code complexity has been considerably reduced.

BUG=710014
==========

to

==========
Hook up Reader Mode InfoBar

This change replaces the OverlayPanel UI for Reader Mode in favor of
the InfoBar UI. Some notable changes associated with the UI switch:

- Navigations to distilled content are not retained in history. This
  means a backward navigation will skip distilled content and bring
  the user to the original page.
- The UI no longer animates or sits on top of the original web page.
- Since Reader Mode is now an infobar, it interacts nicely with other
  infobars that may appear.
- All operations in the overflow menu use the base page URL.
- The "Appearance" overflow item is once again available.
- Code complexity has been considerably reduced.

BUG=710014, 624009
==========

[mdjones, 2017-06-13 20:45:16]

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

[commit-bot: I haz the power, 2017-06-13 20:45:47]

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-13 21:58:02]

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

[commit-bot: I haz the power, 2017-06-13 21:58:03]

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

[mdjones, 2017-06-13 22:44:56]

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

[commit-bot: I haz the power, 2017-06-13 22:45:22]

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-14 00:20:00]

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

[commit-bot: I haz the power, 2017-06-14 00:20:01]

Dry run: This issue passed the CQ dry run.

[mdjones, 2017-06-14 00:34:02]

The CQ bit was checked by mdjones@chromium.org

[mdjones, 2017-06-14 00:34:03]

The patchset sent to the CQ was uploaded after l-g-t-m from
twellington@chromium.org, wychen@chromium.org, jwd@chromium.org,
tedchoc@chromium.org
Link to the patchset: https://codereview.chromium.org/2878543003/#ps190001
(title: "fix tests")

[commit-bot: I haz the power, 2017-06-14 00:34:45]

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-14 00:39:11]

CQ is committing da patch.

Bot data: {"patchset_id": 190001, "attempt_start_ts": 1497400442827630,
"parent_rev": "05227511e9c093b76f6f6ec991f582bbd6aa6b8b", "commit_rev":
"75af3a8ba9bd8724e83d99db0113bc8836b472ae"}

[commit-bot: I haz the power, 2017-06-14 00:39:34]

Description was changed from

==========
Hook up Reader Mode InfoBar

This change replaces the OverlayPanel UI for Reader Mode in favor of
the InfoBar UI. Some notable changes associated with the UI switch:

- Navigations to distilled content are not retained in history. This
  means a backward navigation will skip distilled content and bring
  the user to the original page.
- The UI no longer animates or sits on top of the original web page.
- Since Reader Mode is now an infobar, it interacts nicely with other
  infobars that may appear.
- All operations in the overflow menu use the base page URL.
- The "Appearance" overflow item is once again available.
- Code complexity has been considerably reduced.

BUG=710014, 624009
==========

to

==========
Hook up Reader Mode InfoBar

This change replaces the OverlayPanel UI for Reader Mode in favor of
the InfoBar UI. Some notable changes associated with the UI switch:

- Navigations to distilled content are not retained in history. This
  means a backward navigation will skip distilled content and bring
  the user to the original page.
- The UI no longer animates or sits on top of the original web page.
- Since Reader Mode is now an infobar, it interacts nicely with other
  infobars that may appear.
- All operations in the overflow menu use the base page URL.
- The "Appearance" overflow item is once again available.
- Code complexity has been considerably reduced.

BUG=710014, 624009

Review-Url: https://codereview.chromium.org/2878543003
Cr-Commit-Position: refs/heads/master@{#479229}
Committed:
https://chromium.googlesource.com/chromium/src/+/75af3a8ba9bd8724e83d99db0113...
==========

[commit-bot: I haz the power, 2017-06-14 00:39:35]

Committed patchset #11 (id:190001) as
https://chromium.googlesource.com/chromium/src/+/75af3a8ba9bd8724e83d99db0113...