Integrate new Reader Mode panel

Integrate new Reader Mode panel

This change starts using the new panel and removes deprecated Reader
Mode code.

The most significant file changed is the ReaderModeManager. Instead
of having a manager and panel combo for each tab, a single manager
object exists that manages the state of each tab in a small object.
The Reader Mode panel will only ever show in a single tab at a time,
so it is possible to share a single panel instance between them.

TBR=dtrainor@chromium.org
BUG=521773
Committed: https://crrev.com/f03e51c2f79e9ce2a32c855df38d0ba0b9335a4b
Cr-Commit-Position: refs/heads/master@{#359689}

[mdjones, 2015-11-11 05:22:29]

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

[mdjones, 2015-11-11 05:22:29]

The piece to pay attention to is the ReaderModeManager in this change.
Everything else is small or deletion. PTAL

[David Trainor- moved to gerrit, 2015-11-11 16:20:22]

https://chromiumcodereview.appspot.com/1436743002/diff/20001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/ReaderModeManager.java
(right):

https://chromiumcodereview.appspot.com/1436743002/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/ReaderModeManager.java:72:
private Map<Tab, ReaderModeTabInfo> mTabStatusMap;
Can you just map id -> ReaderModeTabInfo?  Would prefer that to holding extra
references to all of the Tabs.

https://chromiumcodereview.appspot.com/1436743002/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/ReaderModeManager.java:75:
private Tab mTab;
Do you need to hold this or can you query this on demand?

https://chromiumcodereview.appspot.com/1436743002/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/ReaderModeManager.java:188:
public void onDestroyed(Tab tab) {
if (mTab == tab) { // clean up mTab and associated stuffs? }

https://chromiumcodereview.appspot.com/1436743002/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/ReaderModeManager.java:225:
tabInfo.setWebContentsObserver(createWebContentsObserver(tab.getWebContents()));
Do you have to destroy the old observer?

https://chromiumcodereview.appspot.com/1436743002/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/ReaderModeManager.java:241:
// Temporarily hide the reader mode panel while a video is shown.
might not be a video.  could be HTML5 fullscreen API?

https://chromiumcodereview.appspot.com/1436743002/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/ReaderModeManager.java:313:
final Tab mCurrentTab = mTab;
Don't prefix with m.

Storing a strong reference to Tab here is a bit scary.  You'll have to make
*sure* you drop all references to the observer (and should probably add unit
tests!).

Either way, if you can get around storing the Tab that would be ideal :).  Some
potential ideas:
- store the tab id (if you make your map use tab id this will be easier)?
- expose a getWebContents method on WebContentsObserver that accesses the
WeakReference WebContents and use WebContents#getUrl()?

https://chromiumcodereview.appspot.com/1436743002/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/ReaderModeManager.java:415:
final Tab mCurrentTab = mTab;
Don't prefix with m for a local var.

[pedro (no code reviews), 2015-11-11 18:47:38]

Description was changed from

==========
Integrate new Reader Mode panel

This change starts using the new panel and removes deprecated Reader
Mode code.

The most significant file changed is the ReaderModeManager. Instead
of having a manager and panel combo for each tab, a single manager
object exists that manages the state of each tab in a small object.
The Reader Mode panel will only ever show in a single tab at a time,
so it is possible to share a single panel instance between them.

BUG=521773
==========

to

==========
Integrate new Reader Mode panel

This change starts using the new panel and removes deprecated Reader
Mode code.

The most significant file changed is the ReaderModeManager. Instead
of having a manager and panel combo for each tab, a single manager
object exists that manages the state of each tab in a small object.
The Reader Mode panel will only ever show in a single tab at a time,
so it is possible to share a single panel instance between them.

BUG=521773
==========

[pedro (no code reviews), 2015-11-11 18:47:38]

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

[aurimas (slooooooooow), 2015-11-11 19:00:39]

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

[aurimas (slooooooooow), 2015-11-11 19:00:39]

https://chromiumcodereview.appspot.com/1436743002/diff/20001/chrome/android/j...
File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java
(left):

https://chromiumcodereview.appspot.com/1436743002/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:750: if
(mReaderModeActivityDelegate != null) {
Does mReaderModeManager need to be destroyed?

https://chromiumcodereview.appspot.com/1436743002/diff/20001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/ReaderModeManager.java
(right):

https://chromiumcodereview.appspot.com/1436743002/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/ReaderModeManager.java:86:
private static class ReaderModeTabInfo {
I think it would be fine pulling this out into a separate class to make sure
this manager does not grow to the size of ContextualSearchPanelManager.

https://chromiumcodereview.appspot.com/1436743002/diff/20001/chrome/android/j...
File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right):

https://chromiumcodereview.appspot.com/1436743002/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:1367: public
void attachOverlayContentViewCore(ContentViewCore content, boolean visible) {
Can this be removed also now?

[mdjones, 2015-11-12 03:43:44]

I addressed most comments; added question for the one I didn't.

https://codereview.chromium.org/1436743002/diff/20001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java
(left):

https://codereview.chromium.org/1436743002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:750: if
(mReaderModeActivityDelegate != null) {
On 2015/11/11 19:00:39, aurimas wrote:
> Does mReaderModeManager need to be destroyed?

Originally I didn't think so because there is no native component for my
manager, but it makes sense to clear some of the objects I'm maintaining. Done.

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

https://codereview.chromium.org/1436743002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/ReaderModeManager.java:72:
private Map<Tab, ReaderModeTabInfo> mTabStatusMap;
On 2015/11/11 16:20:22, David Trainor wrote:
> Can you just map id -> ReaderModeTabInfo?  Would prefer that to holding extra
> references to all of the Tabs.

Done.

https://codereview.chromium.org/1436743002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/ReaderModeManager.java:75:
private Tab mTab;
On 2015/11/11 16:20:22, David Trainor wrote:
> Do you need to hold this or can you query this on demand?

Done.

https://codereview.chromium.org/1436743002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/ReaderModeManager.java:86:
private static class ReaderModeTabInfo {
On 2015/11/11 19:00:39, aurimas wrote:
> I think it would be fine pulling this out into a separate class to make sure
> this manager does not grow to the size of ContextualSearchPanelManager.

Done.

https://codereview.chromium.org/1436743002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/ReaderModeManager.java:188:
public void onDestroyed(Tab tab) {
On 2015/11/11 16:20:22, David Trainor wrote:
> if (mTab == tab) { // clean up mTab and associated stuffs? }

No longer storing a tab object in this class (using ID instead). All other clean
up should be handled in "removeTabState".

https://codereview.chromium.org/1436743002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/ReaderModeManager.java:225:
tabInfo.setWebContentsObserver(createWebContentsObserver(tab.getWebContents()));
On 2015/11/11 16:20:22, David Trainor wrote:
> Do you have to destroy the old observer?

Not necessarily; I think it makes sense to destroy the existing one though. From
what I understand, one event that triggers this is when web contents are
swapped. Is the swapped web contents guaranteed to not be destroyed immediately
or is it safe to remove the observer from one and put it on the other?

https://codereview.chromium.org/1436743002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/ReaderModeManager.java:241:
// Temporarily hide the reader mode panel while a video is shown.
On 2015/11/11 16:20:22, David Trainor wrote:
> might not be a video.  could be HTML5 fullscreen API?

Done.

https://codereview.chromium.org/1436743002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/ReaderModeManager.java:313:
final Tab mCurrentTab = mTab;
On 2015/11/11 16:20:22, David Trainor wrote:
> Don't prefix with m.
> 
> Storing a strong reference to Tab here is a bit scary.  You'll have to make
> *sure* you drop all references to the observer (and should probably add unit
> tests!).
> 
> Either way, if you can get around storing the Tab that would be ideal :). 
Some
> potential ideas:
> - store the tab id (if you make your map use tab id this will be easier)?
> - expose a getWebContents method on WebContentsObserver that accesses the
> WeakReference WebContents and use WebContents#getUrl()?

I chose to store the ID; unit tests are in the works.

https://codereview.chromium.org/1436743002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/ReaderModeManager.java:415:
final Tab mCurrentTab = mTab;
On 2015/11/11 16:20:22, David Trainor wrote:
> Don't prefix with m for a local var.

Done.

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

https://codereview.chromium.org/1436743002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:1367: public
void attachOverlayContentViewCore(ContentViewCore content, boolean visible) {
On 2015/11/11 19:00:39, aurimas wrote:
> Can this be removed also now?

Yes; done.

[pedro (no code reviews), 2015-11-12 07:17:15]

lgtm

First of all, I'm really happy that the OverlayPanel is finally being used! And
it feels great realizing how cleaner the codebase is becoming. Thanks for all
the hard work!

I looked mostly at the chrome/browser/compositor/* changes and have just a few
comments. I delegate to dtrainor@ to review the
integration with remaining parts of the system since he has more context about
them.

I suggested a couple of nits, but other than that the changes in
chrome/browser/compositor/* look good to me.

https://codereview.chromium.org/1436743002/diff/40001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java
(right):

https://codereview.chromium.org/1436743002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1273: if
(mReaderModeManager != null) mReaderModeManager.onOrientationChange();
Shouldn't this be handled by the panel manager? For now I'm okay with keeping
the way it is implemented, but things that apply to all panels should be handled
by the system automatically. Put a TODO here to handle this?

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

https://codereview.chromium.org/1436743002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/readermode/ReaderModePanel.java:143:
updateBasePageTargetY();
If this method isn't called in an OverlayPanel, then the maximized state will
look broken, given the panel Bar will be obstructed by the Toolbar. In other
words, all OverlayPanels should move the Toolbar away, so this actually belongs
to the OverlayPanel class. Either fix it now or put a TODO to address it later.

https://codereview.chromium.org/1436743002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/StaticLayout.java
(left):

https://codereview.chromium.org/1436743002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/StaticLayout.java:271:
if (mReaderModePanel == null) return;
I'm glad to see how the OverlayPanel is simplifying the codebase! :)

[mdjones, 2015-11-12 19:45:14]

https://codereview.chromium.org/1436743002/diff/40001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java
(right):

https://codereview.chromium.org/1436743002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1273: if
(mReaderModeManager != null) mReaderModeManager.onOrientationChange();
On 2015/11/12 07:17:15, pedrosimonetti wrote:
> Shouldn't this be handled by the panel manager? For now I'm okay with keeping
> the way it is implemented, but things that apply to all panels should be
handled
> by the system automatically. Put a TODO here to handle this?

Done.

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

https://codereview.chromium.org/1436743002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/readermode/ReaderModePanel.java:143:
updateBasePageTargetY();
On 2015/11/12 07:17:15, pedrosimonetti wrote:
> If this method isn't called in an OverlayPanel, then the maximized state will
> look broken, given the panel Bar will be obstructed by the Toolbar. In other
> words, all OverlayPanels should move the Toolbar away, so this actually
belongs
> to the OverlayPanel class. Either fix it now or put a TODO to address it
later.

Added TODO; the state of many of the variables in ContextualSearchPanelBase is
dependent on a number of functions being called at specific times. This API can
be improved before moving it to OverlayPanel.

https://codereview.chromium.org/1436743002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/StaticLayout.java
(left):

https://codereview.chromium.org/1436743002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/StaticLayout.java:271:
if (mReaderModePanel == null) return;
On 2015/11/12 07:17:15, pedrosimonetti wrote:
> I'm glad to see how the OverlayPanel is simplifying the codebase! :)

Thanks to this comment, I realized we don't need this override anymore!

[aurimas (slooooooooow), 2015-11-13 19:55:20]

lgtm

[mdjones, 2015-11-13 21:15:22]

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

[commit-bot: I haz the power, 2015-11-13 21:16:19]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1436743002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1436743002/100001

[mdjones, 2015-11-13 22:19:55]

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

[commit-bot: I haz the power, 2015-11-13 22:20:48]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1436743002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1436743002/120001

[mdjones, 2015-11-13 23:10:47]

Description was changed from

==========
Integrate new Reader Mode panel

This change starts using the new panel and removes deprecated Reader
Mode code.

The most significant file changed is the ReaderModeManager. Instead
of having a manager and panel combo for each tab, a single manager
object exists that manages the state of each tab in a small object.
The Reader Mode panel will only ever show in a single tab at a time,
so it is possible to share a single panel instance between them.

BUG=521773
==========

to

==========
Integrate new Reader Mode panel

This change starts using the new panel and removes deprecated Reader
Mode code.

The most significant file changed is the ReaderModeManager. Instead
of having a manager and panel combo for each tab, a single manager
object exists that manages the state of each tab in a small object.
The Reader Mode panel will only ever show in a single tab at a time,
so it is possible to share a single panel instance between them.

TBR=dtrainor@chromium.org
BUG=521773
==========

[mdjones, 2015-11-13 23:11:09]

The CQ bit was unchecked by mdjones@chromium.org

[mdjones, 2015-11-13 23:11:18]

The CQ bit was checked by mdjones@chromium.org

[mdjones, 2015-11-13 23:11:18]

The patchset sent to the CQ was uploaded after l-g-t-m from
pedrosimonetti@chromium.org, aurimas@chromium.org
Link to the patchset: https://codereview.chromium.org/1436743002/#ps120001
(title: "findbugs and low-end devices")

[commit-bot: I haz the power, 2015-11-13 23:12:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1436743002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1436743002/120001

[commit-bot: I haz the power, 2015-11-13 23:45:00]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-11-13 23:45:51]

Patchset 7 (id:??) landed as
https://crrev.com/f03e51c2f79e9ce2a32c855df38d0ba0b9335a4b
Cr-Commit-Position: refs/heads/master@{#359689}