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

Issue 844473002: Refactor handleMessage() functionality in fullscreen (Closed)

Created:
5 years, 11 months ago by wajahat
Modified:
5 years, 11 months ago
CC:
chromium-reviews
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Refactor handleMessage() functionality in fullscreen to static inner class to avoid HandlerLeak lint warnings The following HandlerLeak lint warnings are reported that are legitimate: (1) org/chromium/chrome/browser/fullscreen/ChromeFullscreenManager.java This Handler class should be static or leaks might occur (org.chromium.chrome.browser.fullscreen.ChromeFullscreenManager.2): HandlerLeak [warning] (2) org/chromium/chrome/browser/fullscreen/FullscreenHtmlApiHandler.java This Handler class should be static or leaks might occur (org.chromium.chrome.browser.fullscreen.FullscreenHtmlApiHandler.1): HandlerLeak [warning] The above warnings needs to fixed as the references to Handler class might prevent outer class from getting garbage collected. Making static inner class do not hold an implicit reference to the outer class and hence this problem can be avoided. The same is also done for other classes eg: TapGestureDetector. BUG=None. Committed: https://crrev.com/640ca0b5920679e4d0e8e8d53251e757d148e540 Cr-Commit-Position: refs/heads/master@{#311226}

Patch Set 1 #

Total comments: 4

Patch Set 2 : review comments incorporated #

Unified diffs Side-by-side diffs Delta from patch set Stats (+111 lines, -80 lines) Patch
M chrome/android/java/src/org/chromium/chrome/browser/fullscreen/ChromeFullscreenManager.java View 1 3 chunks +31 lines, -17 lines 0 comments Download
M chrome/android/java/src/org/chromium/chrome/browser/fullscreen/FullscreenHtmlApiHandler.java View 1 3 chunks +80 lines, -63 lines 0 comments Download

Messages

Total messages: 18 (4 generated)
wajahat
Ref: upstream fullscreen manager https://codereview.chromium.org/810853003 Pl. review. Thanks !
5 years, 11 months ago (2015-01-07 07:55:11 UTC) #2
Jaekyun Seok (inactive)
lgtm.
5 years, 11 months ago (2015-01-07 08:43:52 UTC) #3
wajahat
On 2015/01/07 08:43:52, Jaekyun Seok wrote: > lgtm. Thanks Jaekyun @Nyquist/Tedc could you pl. have ...
5 years, 11 months ago (2015-01-08 05:29:34 UTC) #4
Ted C
lgtm https://codereview.chromium.org/844473002/diff/1/chrome/android/java/src/org/chromium/chrome/browser/fullscreen/ChromeFullscreenManager.java File chrome/android/java/src/org/chromium/chrome/browser/fullscreen/ChromeFullscreenManager.java (right): https://codereview.chromium.org/844473002/diff/1/chrome/android/java/src/org/chromium/chrome/browser/fullscreen/ChromeFullscreenManager.java#newcode227 chrome/android/java/src/org/chromium/chrome/browser/fullscreen/ChromeFullscreenManager.java:227: public View getControlContainer() { this doesn't need to ...
5 years, 11 months ago (2015-01-09 19:53:36 UTC) #5
nyquist
https://codereview.chromium.org/844473002/diff/1/chrome/android/java/src/org/chromium/chrome/browser/fullscreen/ChromeFullscreenManager.java File chrome/android/java/src/org/chromium/chrome/browser/fullscreen/ChromeFullscreenManager.java (right): https://codereview.chromium.org/844473002/diff/1/chrome/android/java/src/org/chromium/chrome/browser/fullscreen/ChromeFullscreenManager.java#newcode165 chrome/android/java/src/org/chromium/chrome/browser/fullscreen/ChromeFullscreenManager.java:165: ChromeFullscreenManager chromeFullscreenManager = mChromeFullscreenManager.get(); I wonder, are there any ...
5 years, 11 months ago (2015-01-10 01:27:05 UTC) #6
wajahat
Review comments incorporated, PTAL! Thanks. https://codereview.chromium.org/844473002/diff/1/chrome/android/java/src/org/chromium/chrome/browser/fullscreen/ChromeFullscreenManager.java File chrome/android/java/src/org/chromium/chrome/browser/fullscreen/ChromeFullscreenManager.java (right): https://codereview.chromium.org/844473002/diff/1/chrome/android/java/src/org/chromium/chrome/browser/fullscreen/ChromeFullscreenManager.java#newcode165 chrome/android/java/src/org/chromium/chrome/browser/fullscreen/ChromeFullscreenManager.java:165: ChromeFullscreenManager chromeFullscreenManager = mChromeFullscreenManager.get(); ...
5 years, 11 months ago (2015-01-12 08:06:46 UTC) #7
nyquist
lgtm
5 years, 11 months ago (2015-01-12 22:42:30 UTC) #8
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/844473002/20001
5 years, 11 months ago (2015-01-13 05:22:43 UTC) #10
commit-bot: I haz the power
Try jobs failed on following builders: ios_dbg_simulator on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator/builds/48351)
5 years, 11 months ago (2015-01-13 05:28:45 UTC) #12
wajahat
On 2015/01/13 05:28:45, I haz the power (commit-bot) wrote: > Try jobs failed on following ...
5 years, 11 months ago (2015-01-13 06:04:56 UTC) #13
Jaekyun Seok (inactive)
If so, you need to try again by clicking the commit button.
5 years, 11 months ago (2015-01-13 06:09:45 UTC) #14
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/844473002/20001
5 years, 11 months ago (2015-01-13 06:14:59 UTC) #16
commit-bot: I haz the power
Committed patchset #2 (id:20001)
5 years, 11 months ago (2015-01-13 06:23:52 UTC) #17
commit-bot: I haz the power
5 years, 11 months ago (2015-01-13 06:24:33 UTC) #18
Message was sent while issue was closed.
Patchset 2 (id:??) landed as
https://crrev.com/640ca0b5920679e4d0e8e8d53251e757d148e540
Cr-Commit-Position: refs/heads/master@{#311226}

Powered by Google App Engine
This is Rietveld 408576698