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

Issue 594103002: [aw] Unregister delegates from AwContents.destroy() (Closed)

Created:
6 years, 3 months ago by Ignacio Solla
Modified:
6 years, 2 months ago
CC:
chromium-reviews, darin-cc_chromium.org, jam, android-webview-reviews_chromium.org, benm (inactive)
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Project:
chromium
Visibility:
Public.

Description

[aw] Unregister delegates from AwContents.destroy() AwContents are deleted asynchronously, so to stop receiving callbacks we unregister delegates / observers / listeners from AwContents.destroy() and ContentViewCore.destroy(). Receiving callbacks after AwContents.destroy() is called is problematic because Java refs have already been set to null. BUG=415666 Committed: https://crrev.com/90bdfe3249589f571fd8cdb7159fb1c678052233 Cr-Commit-Position: refs/heads/master@{#296977}

Patch Set 1 #

Total comments: 4

Patch Set 2 : Address review comments. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+31 lines, -8 lines) Patch
M android_webview/java/src/org/chromium/android_webview/AwContents.java View 1 1 chunk +2 lines, -0 lines 0 comments Download
M android_webview/native/aw_contents.cc View 1 1 chunk +4 lines, -0 lines 0 comments Download
M content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java View 1 4 chunks +25 lines, -8 lines 0 comments Download

Messages

Total messages: 18 (5 generated)
Ignacio Solla
I believe that this should stop callbacks from chromium, but please let me know if ...
6 years, 3 months ago (2014-09-23 11:32:26 UTC) #2
mkosiba (inactive)
lgtm
6 years, 3 months ago (2014-09-23 15:56:33 UTC) #3
boliu
aw lgtm https://codereview.chromium.org/594103002/diff/1/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/594103002/diff/1/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java#newcode831 content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:831: // currently a real Null Object. That's ...
6 years, 3 months ago (2014-09-23 17:20:18 UTC) #4
Ignacio Solla
https://codereview.chromium.org/594103002/diff/1/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/594103002/diff/1/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java#newcode831 content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:831: // currently a real Null Object. On 2014/09/23 17:20:17, ...
6 years, 3 months ago (2014-09-23 17:30:15 UTC) #5
Ignacio Solla
benm@chromium.org: are you happy reviewing the changes in content/ or should I find somebody else? ...
6 years, 3 months ago (2014-09-23 17:31:11 UTC) #7
boliu
On 2014/09/23 17:31:11, Ignacio Solla wrote: > mailto:benm@chromium.org: are you happy reviewing the changes in ...
6 years, 3 months ago (2014-09-23 17:31:57 UTC) #8
Ignacio Solla
tedchoc@chromium.org: Please review changes in content/
6 years, 3 months ago (2014-09-23 17:45:17 UTC) #11
Ted C
https://codereview.chromium.org/594103002/diff/1/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/594103002/diff/1/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java#newcode831 content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:831: // currently a real Null Object. On 2014/09/23 17:30:15, ...
6 years, 2 months ago (2014-09-26 16:54:08 UTC) #12
Ignacio Solla
https://codereview.chromium.org/594103002/diff/1/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/594103002/diff/1/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java#newcode831 content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:831: // currently a real Null Object. On 2014/09/26 16:54:08, ...
6 years, 2 months ago (2014-09-26 17:02:49 UTC) #13
Ted C
lgtm
6 years, 2 months ago (2014-09-26 17:08:13 UTC) #14
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/594103002/20001
6 years, 2 months ago (2014-09-26 17:12:10 UTC) #16
commit-bot: I haz the power
Committed patchset #2 (id:20001) as 02f149ceb65d1dcb90dac293228a1409005007cd
6 years, 2 months ago (2014-09-26 18:01:53 UTC) #17
commit-bot: I haz the power
6 years, 2 months ago (2014-09-26 18:02:23 UTC) #18
Message was sent while issue was closed.
Patchset 2 (id:??) landed as
https://crrev.com/90bdfe3249589f571fd8cdb7159fb1c678052233
Cr-Commit-Position: refs/heads/master@{#296977}

Powered by Google App Engine
This is Rietveld 408576698