Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(159)

Issue 2812753003: 🏠 Reader Mode info bar implementation (Closed)

Created:
3 years, 8 months ago by mdjones
Modified:
3 years, 7 months ago
Reviewers:
Peter Kasting, gone, jwd
CC:
chromium-reviews, agrieve+watch_chromium.org, dfalcantara+watch_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

[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

Patch Set 1 #

Patch Set 2 : add missing include #

Total comments: 11

Patch Set 3 : address comments #

Patch Set 4 : add equals method #

Patch Set 5 : rebase past metrics separation #

Total comments: 2

Patch Set 6 : address comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+192 lines, -0 lines) Patch
A chrome/android/java/src/org/chromium/chrome/browser/infobar/ReaderModeInfoBar.java View 1 2 1 chunk +84 lines, -0 lines 0 comments Download
M chrome/android/java_sources.gni View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/BUILD.gn View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/android/chrome_jni_registrar.cc View 1 2 3 4 2 chunks +2 lines, -0 lines 0 comments Download
M chrome/browser/ui/BUILD.gn View 1 2 3 4 1 chunk +2 lines, -0 lines 0 comments Download
A chrome/browser/ui/android/infobars/reader_mode_infobar.h View 1 chunk +37 lines, -0 lines 0 comments Download
A chrome/browser/ui/android/infobars/reader_mode_infobar.cc View 1 2 3 4 5 1 chunk +63 lines, -0 lines 0 comments Download
M components/infobars/core/infobar_delegate.h View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M tools/metrics/histograms/enums.xml View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download

Dependent Patchsets:

Messages

Total messages: 20 (8 generated)
mdjones
ptal
3 years, 8 months ago (2017-04-11 01:30:24 UTC) #3
gone
Why was it crashing before? https://codereview.chromium.org/2812753003/diff/20001/chrome/android/java/src/org/chromium/chrome/browser/infobar/ReaderModeInfoBar.java File chrome/android/java/src/org/chromium/chrome/browser/infobar/ReaderModeInfoBar.java (right): https://codereview.chromium.org/2812753003/diff/20001/chrome/android/java/src/org/chromium/chrome/browser/infobar/ReaderModeInfoBar.java#newcode45 chrome/android/java/src/org/chromium/chrome/browser/infobar/ReaderModeInfoBar.java:45: getContext().getResources().getDimension(R.dimen.overlay_panel_text_size)); This is neither ...
3 years, 8 months ago (2017-04-11 17:51:44 UTC) #4
mdjones
It was crashing because of some logic in the ReaderModeManager that assumes the feature is ...
3 years, 8 months ago (2017-04-11 20:38:57 UTC) #5
gone
lgtm https://codereview.chromium.org/2812753003/diff/20001/chrome/browser/android/chrome_jni_registrar.cc File chrome/browser/android/chrome_jni_registrar.cc (right): https://codereview.chromium.org/2812753003/diff/20001/chrome/browser/android/chrome_jni_registrar.cc#newcode387 chrome/browser/android/chrome_jni_registrar.cc:387: {"ReaderModeInfoBar", RegisterReaderModeInfoBar}, On 2017/04/11 20:38:57, mdjones wrote: > ...
3 years, 8 months ago (2017-04-11 21:52:07 UTC) #6
mdjones
+jwd for histograms
3 years, 7 months ago (2017-05-08 20:08:10 UTC) #8
mdjones
+pkasting for infobar_delegate.h
3 years, 7 months ago (2017-05-08 20:21:53 UTC) #10
jwd
metrics LGTM And for the future, in case you didn't read the OWNERS, you don't ...
3 years, 7 months ago (2017-05-08 22:44:39 UTC) #11
mdjones
On 2017/05/08 22:44:39, jwd wrote: > metrics LGTM > > And for the future, in ...
3 years, 7 months ago (2017-05-08 23:22:47 UTC) #12
Peter Kasting
LGTM https://codereview.chromium.org/2812753003/diff/80001/chrome/browser/ui/android/infobars/reader_mode_infobar.cc File chrome/browser/ui/android/infobars/reader_mode_infobar.cc (right): https://codereview.chromium.org/2812753003/diff/80001/chrome/browser/ui/android/infobars/reader_mode_infobar.cc#newcode26 chrome/browser/ui/android/infobars/reader_mode_infobar.cc:26: return InfoBarDelegate::InfoBarIdentifier ::READER_MODE_INFOBAR_ANDROID; Nit:Extra space
3 years, 7 months ago (2017-05-08 23:26:52 UTC) #13
mdjones
https://codereview.chromium.org/2812753003/diff/80001/chrome/browser/ui/android/infobars/reader_mode_infobar.cc File chrome/browser/ui/android/infobars/reader_mode_infobar.cc (right): https://codereview.chromium.org/2812753003/diff/80001/chrome/browser/ui/android/infobars/reader_mode_infobar.cc#newcode26 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: ...
3 years, 7 months ago (2017-05-09 15:37:56 UTC) #14
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2812753003/100001
3 years, 7 months ago (2017-05-09 15:40:24 UTC) #17
commit-bot: I haz the power
3 years, 7 months ago (2017-05-09 18:51:06 UTC) #20
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/14bd822963d10989b62db8bd675e...

Powered by Google App Engine
This is Rietveld 408576698