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

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 2883643002: Clean up ReaderMode OverlayPanel classes (Closed)

Created:
3 years, 7 months ago by mdjones
Modified:
3 years, 5 months ago
CC:
chromium-reviews, mlamouri+watch-content_chromium.org, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, mdjones+watch_chromium.org, donnd+watch_chromium.org, jam, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, darin-cc_chromium.org, agrieve+watch_chromium.org, darin (slow to review)
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Clean up ReaderMode OverlayPanel classes Since Reader Mode is now implemented as an infobar, all of the classes related to the OverlayPanel implementation can be removed. This change cleans up those classes as well as some functionality that became obsolete in related classes. BUG=710014 Review-Url: https://codereview.chromium.org/2883643002 Cr-Commit-Position: refs/heads/master@{#479800} Committed: https://chromium.googlesource.com/chromium/src/+/4ab35aa42b13b7ef865e414c3072da90d12f2634

Patch Set 1 #

Patch Set 2 : more #

Patch Set 3 : rebase #

Patch Set 4 : rebase #

Total comments: 2

Patch Set 5 : rebase to main reader patch #

Patch Set 6 : remove extra enum #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+18 lines, -1138 lines) Patch
M chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java View 1 2 3 4 5 2 chunks +1 line, -10 lines 0 comments Download
D chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/readermode/OWNERS View 1 chunk +0 lines, -1 line 0 comments Download
D chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/readermode/ReaderModeBarControl.java View 1 chunk +0 lines, -66 lines 0 comments Download
D chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/readermode/ReaderModePanel.java View 1 chunk +0 lines, -373 lines 0 comments Download
D chrome/android/java/src/org/chromium/chrome/browser/compositor/scene_layer/ReaderModeSceneLayer.java View 1 chunk +0 lines, -144 lines 0 comments Download
M chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/DomDistillerUIUtils.java View 4 3 chunks +8 lines, -20 lines 2 comments Download
M chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/ReaderModeManager.java View 1 2 3 4 10 chunks +9 lines, -76 lines 0 comments Download
D chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/ReaderModeManagerDelegate.java View 1 chunk +0 lines, -55 lines 0 comments Download
M chrome/android/java_sources.gni View 1 2 3 4 3 chunks +0 lines, -4 lines 0 comments Download
M chrome/browser/BUILD.gn View 1 2 3 4 3 chunks +0 lines, -5 lines 0 comments Download
M chrome/browser/android/chrome_jni_registrar.cc View 1 2 3 4 2 chunks +0 lines, -2 lines 0 comments Download
D chrome/browser/android/compositor/layer/reader_mode_layer.h View 1 chunk +0 lines, -48 lines 0 comments Download
D chrome/browser/android/compositor/layer/reader_mode_layer.cc View 1 chunk +0 lines, -52 lines 0 comments Download
D chrome/browser/android/compositor/scene_layer/reader_mode_scene_layer.h View 1 chunk +0 lines, -89 lines 0 comments Download
D chrome/browser/android/compositor/scene_layer/reader_mode_scene_layer.cc View 1 chunk +0 lines, -160 lines 0 comments Download
M chrome/browser/android/dom_distiller/distiller_ui_handle_android.h View 1 chunk +0 lines, -2 lines 0 comments Download
M chrome/browser/android/dom_distiller/distiller_ui_handle_android.cc View 1 chunk +0 lines, -6 lines 0 comments Download
M components/dom_distiller/content/browser/distiller_javascript_service_impl.h View 1 chunk +0 lines, -3 lines 0 comments Download
M components/dom_distiller/content/browser/distiller_javascript_service_impl.cc View 1 chunk +0 lines, -9 lines 0 comments Download
M components/dom_distiller/content/browser/distiller_ui_handle.h View 1 chunk +0 lines, -3 lines 0 comments Download
M components/dom_distiller/content/common/distiller_javascript_service.mojom View 1 chunk +0 lines, -4 lines 0 comments Download
M components/dom_distiller/content/renderer/distiller_native_javascript.cc View 4 1 chunk +0 lines, -6 lines 0 comments Download

Depends on Patchset:

Issue 2878543003 Patch 190001

Messages

Total messages: 25 (10 generated)
mdjones
ptal
3 years, 7 months ago (2017-05-22 21:30:00 UTC) #2
wychen
lgtm https://codereview.chromium.org/2883643002/diff/60001/chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java File chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java (right): https://codereview.chromium.org/2883643002/diff/60001/chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java#newcode79 chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:79: TAB_NAVIGATION Is this still used?
3 years, 7 months ago (2017-05-22 23:12:36 UTC) #3
Theresa
lgtm \o/
3 years, 7 months ago (2017-05-24 20:44:22 UTC) #4
mdjones
https://codereview.chromium.org/2883643002/diff/60001/chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java File chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java (right): https://codereview.chromium.org/2883643002/diff/60001/chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java#newcode79 chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:79: TAB_NAVIGATION On 2017/05/22 23:12:36, wychen wrote: > Is this ...
3 years, 6 months ago (2017-06-13 23:03:32 UTC) #5
mdjones
+dtrainor for owners on chrome/browser/android/* +dcheng for security on *.mojom
3 years, 6 months ago (2017-06-13 23:11:22 UTC) #7
dcheng
ipc lgtm
3 years, 6 months ago (2017-06-13 23:30:12 UTC) #8
David Trainor- moved to gerrit
chrome/browser/android lgtm!
3 years, 6 months ago (2017-06-14 19:25:06 UTC) #9
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/2883643002/100001
3 years, 6 months ago (2017-06-15 15:28:54 UTC) #12
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/479669)
3 years, 6 months ago (2017-06-15 16:28:47 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/2883643002/100001
3 years, 6 months ago (2017-06-15 16:31:35 UTC) #16
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/479777)
3 years, 6 months ago (2017-06-15 17:29:56 UTC) #18
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/2883643002/100001
3 years, 6 months ago (2017-06-15 18:50:21 UTC) #20
commit-bot: I haz the power
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/4ab35aa42b13b7ef865e414c3072da90d12f2634
3 years, 6 months ago (2017-06-15 20:00:47 UTC) #23
wychen
https://codereview.chromium.org/2883643002/diff/100001/chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/DomDistillerUIUtils.java File chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/DomDistillerUIUtils.java (right): https://codereview.chromium.org/2883643002/diff/100001/chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/DomDistillerUIUtils.java#newcode23 chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/DomDistillerUIUtils.java:23: private static ReaderModeManager sManagerManager; Do we still need this?
3 years, 5 months ago (2017-06-28 21:32:37 UTC) #24
mdjones
3 years, 5 months ago (2017-06-29 15:38:35 UTC) #25
Message was sent while issue was closed. 
https://codereview.chromium.org/2883643002/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/DomDistillerUIUtils.java
(right):

https://codereview.chromium.org/2883643002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/DomDistillerUIUtils.java:23:
private static ReaderModeManager sManagerManager;
On 2017/06/28 21:32:37, wychen wrote:
> Do we still need this?

Guess not, kept it around for the settings util but we don't access it there.

Powered by Google App Engine
This is Rietveld 408576698