Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2282153002:
android: Remove CleanupReference from CookiesFetcher (Closed)
Descriptionandroid: Remove CleanupReference from CookiesFetcher
Just have native side delete itself when done.
BUG=507405
Committed: https://crrev.com/6f9330835277c36481df6533d767ee4101eaf947
Cr-Commit-Position: refs/heads/master@{#415054}
Patch Set 1 #Patch Set 2 : fix double delete #
Total comments: 3
Messages
Total messages: 24 (14 generated)
boliu@chromium.org changed reviewers: + yfriedman@chromium.org
and actually finishing this!!
The CQ bit was checked by boliu@chromium.org to run a CQ dry run
The CQ bit was unchecked by boliu@chromium.org
On 2016/08/26 21:25:43, boliu wrote: > and actually finishing this!! actually, persist can be made static all the way too I think, which is probably safer
On 2016/08/26 21:27:02, boliu wrote: > On 2016/08/26 21:25:43, boliu wrote: > > and actually finishing this!! > > actually, persist can be made static all the way too I think, which is probably > safer oh, never mind, it needs to keep hold of the java Context, so this is about as good as it will get..
The CQ bit was checked by boliu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by boliu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
boliu@chromium.org changed reviewers: + dfalcantara@chromium.org
+dfalcantara for review since yaron is ooo
https://chromiumcodereview.appspot.com/2282153002/diff/20001/chrome/browser/a... File chrome/browser/android/cookies/cookies_fetcher.cc (right): https://chromiumcodereview.appspot.com/2282153002/diff/20001/chrome/browser/a... chrome/browser/android/cookies/cookies_fetcher.cc:66: base::Bind(&CookiesFetcher::OnCookiesFetchFinished, base::Owned(this))); What destroys the object on this path? It seems to run through OnCookiesFetchedFinished() and not try to delete itself.
https://chromiumcodereview.appspot.com/2282153002/diff/20001/chrome/browser/a... File chrome/browser/android/cookies/cookies_fetcher.cc (right): https://chromiumcodereview.appspot.com/2282153002/diff/20001/chrome/browser/a... chrome/browser/android/cookies/cookies_fetcher.cc:66: base::Bind(&CookiesFetcher::OnCookiesFetchFinished, base::Owned(this))); On 2016/08/29 18:07:53, dfalcantara wrote: > What destroys the object on this path? It seems to run through > OnCookiesFetchedFinished() and not try to delete itself. It's owned and so deleted by the callback. Look up base::Owned :p
lgtm :P https://chromiumcodereview.appspot.com/2282153002/diff/20001/chrome/browser/a... File chrome/browser/android/cookies/cookies_fetcher.cc (right): https://chromiumcodereview.appspot.com/2282153002/diff/20001/chrome/browser/a... chrome/browser/android/cookies/cookies_fetcher.cc:66: base::Bind(&CookiesFetcher::OnCookiesFetchFinished, base::Owned(this))); On 2016/08/29 18:53:45, boliu wrote: > On 2016/08/29 18:07:53, dfalcantara wrote: > > What destroys the object on this path? It seems to run through > > OnCookiesFetchedFinished() and not try to delete itself. > > It's owned and so deleted by the callback. Look up base::Owned :p Sorry, I don't do many native code reviews :P
The CQ bit was checked by boliu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== android: Remove CleanupReference from CookiesFetcher Just have native side delete itself when done. BUG=507405 ========== to ========== android: Remove CleanupReference from CookiesFetcher Just have native side delete itself when done. BUG=507405 Committed: https://crrev.com/6f9330835277c36481df6533d767ee4101eaf947 Cr-Commit-Position: refs/heads/master@{#415054} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/6f9330835277c36481df6533d767ee4101eaf947 Cr-Commit-Position: refs/heads/master@{#415054} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
