🏠 Reader Mode info bar implementation

[Home] Reader Mode info bar implementation

This change introduces the classes for the InfoBar implementation of
Reader Mode for Chrome Home. The code is not yet triggered and
requires hookup to the manager.

BUG=710014
Review-Url: https://codereview.chromium.org/2812753003
Cr-Commit-Position: refs/heads/master@{#470394}
Committed: https://chromium.googlesource.com/chromium/src/+/14bd822963d10989b62db8bd675e3743b5780fa5

[mdjones, 2017-04-11 01:29:44]

Description was changed from

==========
[Home] Reader Mode info bar implementation

This change introduces the classes for the InfoBar implementation of
Reader Mode for Chrome Home. The code is not yet triggered and
requires hookup to the manager.

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

to

==========
[Home] Reader Mode info bar implementation

This change introduces the classes for the InfoBar implementation of
Reader Mode for Chrome Home. The code is not yet triggered and
requires hookup to the manager.

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

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

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

[mdjones, 2017-04-11 01:30:24]

ptal

[gone, 2017-04-11 17:51:44]

Why was it crashing before?

https://codereview.chromium.org/2812753003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/ReaderModeInfoBar.java
(right):

https://codereview.chromium.org/2812753003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/ReaderModeInfoBar.java:45:
getContext().getResources().getDimension(R.dimen.overlay_panel_text_size));
This is neither 14sp or 16sp.

go/clankstyles

https://codereview.chromium.org/2812753003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/ReaderModeInfoBar.java:46:
prompt.setTextColor(Color.BLACK);
Don't use black.  Same link as above.

https://codereview.chromium.org/2812753003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/ReaderModeInfoBar.java:59:
prompt.getLayoutParams().height = LayoutParams.MATCH_PARENT;
You shouldn't be mucking about with the layout params, especially because you're
messing with linear layout parameters that mess with your 1.0f weight.  Why do
you think you need to do this?

https://codereview.chromium.org/2812753003/diff/20001/chrome/browser/android/...
File chrome/browser/android/chrome_jni_registrar.cc (right):

https://codereview.chromium.org/2812753003/diff/20001/chrome/browser/android/...
chrome/browser/android/chrome_jni_registrar.cc:387: {"ReaderModeInfoBar",
RegisterReaderModeInfoBar},
Is this more common than the static method version?  ReaderModeInfoBar:Register

https://codereview.chromium.org/2812753003/diff/20001/components/infobars/cor...
File components/infobars/core/infobar_delegate.h (right):

https://codereview.chromium.org/2812753003/diff/20001/components/infobars/cor...
components/infobars/core/infobar_delegate.h:149: READER_MODE_INFOBAR_ANDROID =
75,
histograms.xml needs to be updated.

[mdjones, 2017-04-11 20:38:57]

It was crashing because of some logic in the ReaderModeManager that assumes the
feature is not an InfoBar. This can be fixed by adding the compare method to
ReaderModeInfoBarDelegate, but in practice this shouldn't be a problem.

https://codereview.chromium.org/2812753003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/ReaderModeInfoBar.java
(right):

https://codereview.chromium.org/2812753003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/ReaderModeInfoBar.java:45:
getContext().getResources().getDimension(R.dimen.overlay_panel_text_size));
On 2017/04/11 17:51:43, overloaded wrote:
> This is neither 14sp or 16sp.
> 
> go/clankstyles

Changed to infobar_text_size.

https://codereview.chromium.org/2812753003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/ReaderModeInfoBar.java:46:
prompt.setTextColor(Color.BLACK);
On 2017/04/11 17:51:43, overloaded wrote:
> Don't use black.  Same link as above.

Done.

https://codereview.chromium.org/2812753003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/ReaderModeInfoBar.java:59:
prompt.getLayoutParams().height = LayoutParams.MATCH_PARENT;
On 2017/04/11 17:51:44, overloaded wrote:
> You shouldn't be mucking about with the layout params, especially because
you're
> messing with linear layout parameters that mess with your 1.0f weight.  Why do
> you think you need to do this?

Didn't realize it automatically filled the parent. Removed.

https://codereview.chromium.org/2812753003/diff/20001/chrome/browser/android/...
File chrome/browser/android/chrome_jni_registrar.cc (right):

https://codereview.chromium.org/2812753003/diff/20001/chrome/browser/android/...
chrome/browser/android/chrome_jni_registrar.cc:387: {"ReaderModeInfoBar",
RegisterReaderModeInfoBar},
On 2017/04/11 17:51:44, overloaded wrote:
> Is this more common than the static method version? 
ReaderModeInfoBar:Register

This file is a mixed bag; I've always done it this way. If you prefer the other,
I can change it.

https://codereview.chromium.org/2812753003/diff/20001/components/infobars/cor...
File components/infobars/core/infobar_delegate.h (right):

https://codereview.chromium.org/2812753003/diff/20001/components/infobars/cor...
components/infobars/core/infobar_delegate.h:149: READER_MODE_INFOBAR_ANDROID =
75,
On 2017/04/11 17:51:44, overloaded wrote:
> histograms.xml needs to be updated.

Done.

[gone, 2017-04-11 21:52:07]

lgtm

https://codereview.chromium.org/2812753003/diff/20001/chrome/browser/android/...
File chrome/browser/android/chrome_jni_registrar.cc (right):

https://codereview.chromium.org/2812753003/diff/20001/chrome/browser/android/...
chrome/browser/android/chrome_jni_registrar.cc:387: {"ReaderModeInfoBar",
RegisterReaderModeInfoBar},
On 2017/04/11 20:38:57, mdjones wrote:
> On 2017/04/11 17:51:44, overloaded wrote:
> > Is this more common than the static method version? 
> ReaderModeInfoBar:Register
> 
> This file is a mixed bag; I've always done it this way. If you prefer the
other,
> I can change it.

Acknowledged.

[mdjones, 2017-05-08 20:08:09]

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

[mdjones, 2017-05-08 20:08:10]

+jwd for histograms

[mdjones, 2017-05-08 20:21:53]

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

[mdjones, 2017-05-08 20:21:53]

+pkasting for infobar_delegate.h

[jwd, 2017-05-08 22:44:39]

metrics LGTM

And for the future, in case you didn't read the OWNERS, you don't need owners
approval for enums.xml changes.

[mdjones, 2017-05-08 23:22:47]

On 2017/05/08 22:44:39, jwd wrote:
> metrics LGTM
> 
> And for the future, in case you didn't read the OWNERS, you don't need owners
> approval for enums.xml changes.

Got it; I added you before I realized the rebase put my code in the enums.xml
file. I'll remember for next time.

[Peter Kasting, 2017-05-08 23:26:52]

LGTM

https://codereview.chromium.org/2812753003/diff/80001/chrome/browser/ui/andro...
File chrome/browser/ui/android/infobars/reader_mode_infobar.cc (right):

https://codereview.chromium.org/2812753003/diff/80001/chrome/browser/ui/andro...
chrome/browser/ui/android/infobars/reader_mode_infobar.cc:26: return
InfoBarDelegate::InfoBarIdentifier ::READER_MODE_INFOBAR_ANDROID;
Nit:Extra space

[mdjones, 2017-05-09 15:37:56]

https://codereview.chromium.org/2812753003/diff/80001/chrome/browser/ui/andro...
File chrome/browser/ui/android/infobars/reader_mode_infobar.cc (right):

https://codereview.chromium.org/2812753003/diff/80001/chrome/browser/ui/andro...
chrome/browser/ui/android/infobars/reader_mode_infobar.cc:26: return
InfoBarDelegate::InfoBarIdentifier ::READER_MODE_INFOBAR_ANDROID;
On 2017/05/08 23:26:51, Peter Kasting wrote:
> Nit:Extra space

Done.

[mdjones, 2017-05-09 15:39:49]

The CQ bit was checked by mdjones@chromium.org

[mdjones, 2017-05-09 15:39:49]

The patchset sent to the CQ was uploaded after l-g-t-m from
dfalcantara@chromium.org, pkasting@chromium.org, jwd@chromium.org
Link to the patchset: https://codereview.chromium.org/2812753003/#ps100001
(title: "address comments")

[commit-bot: I haz the power, 2017-05-09 15:40:24]

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-05-09 18:50:52]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1494344389188890,
"parent_rev": "28574ad090e3bdeb2a57b03f1da19aff92d65d7c", "commit_rev":
"14bd822963d10989b62db8bd675e3743b5780fa5"}

[commit-bot: I haz the power, 2017-05-09 18:51:03]

Description was changed from

==========
[Home] Reader Mode info bar implementation

This change introduces the classes for the InfoBar implementation of
Reader Mode for Chrome Home. The code is not yet triggered and
requires hookup to the manager.

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

to

==========
[Home] Reader Mode info bar implementation

This change introduces the classes for the InfoBar implementation of
Reader Mode for Chrome Home. The code is not yet triggered and
requires hookup to the manager.

BUG=710014

Review-Url: https://codereview.chromium.org/2812753003
Cr-Commit-Position: refs/heads/master@{#470394}
Committed:
https://chromium.googlesource.com/chromium/src/+/14bd822963d10989b62db8bd675e...
==========

[commit-bot: I haz the power, 2017-05-09 18:51:06]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/14bd822963d10989b62db8bd675e...