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

Issue 15701011: Only wait for CancelAllRequests done if the post task succeeds (Closed)

Created:
7 years, 6 months ago by michaelbai
Modified:
7 years, 6 months ago
Reviewers:
Yaron, sky
CC:
chromium-reviews, browser-components-watch_chromium.org
Visibility:
Public.

Description

SQLiteCursor could be destroyed in Java object's finalize method, and UI message loop might already shutdown, we need to check if the post task succeeds, then wait for callback. BUG=239482 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=203953

Patch Set 1 #

Total comments: 2

Patch Set 2 : address comment #

Total comments: 2

Patch Set 3 : Remove wait event #

Patch Set 4 : fix unit test #

Total comments: 4

Patch Set 5 : Address comments and sync #

Patch Set 6 : sync again #

Unified diffs Side-by-side diffs Delta from patch set Stats (+43 lines, -55 lines) Patch
M chrome/browser/history/android/sqlite_cursor.h View 1 2 3 4 2 chunks +4 lines, -3 lines 0 comments Download
M chrome/browser/history/android/sqlite_cursor.cc View 1 2 3 4 3 chunks +19 lines, -29 lines 0 comments Download
M chrome/browser/history/android/sqlite_cursor_unittest.cc View 1 2 3 1 chunk +20 lines, -23 lines 0 comments Download

Messages

Total messages: 13 (0 generated)
michaelbai
7 years, 6 months ago (2013-05-30 17:58:16 UTC) #1
Yaron
lgtm Not sure this is the cause but it's a good fix regardless. https://codereview.chromium.org/15701011/diff/1/chrome/browser/history/android/sqlite_cursor.cc File ...
7 years, 6 months ago (2013-05-30 18:14:00 UTC) #2
michaelbai
https://codereview.chromium.org/15701011/diff/1/chrome/browser/history/android/sqlite_cursor.cc File chrome/browser/history/android/sqlite_cursor.cc (right): https://codereview.chromium.org/15701011/diff/1/chrome/browser/history/android/sqlite_cursor.cc#newcode161 chrome/browser/history/android/sqlite_cursor.cc:161: &event))) On 2013/05/30 18:14:00, Yaron wrote: > Please add ...
7 years, 6 months ago (2013-05-30 18:33:56 UTC) #3
sky
https://codereview.chromium.org/15701011/diff/3/chrome/browser/history/android/sqlite_cursor.cc File chrome/browser/history/android/sqlite_cursor.cc (right): https://codereview.chromium.org/15701011/diff/3/chrome/browser/history/android/sqlite_cursor.cc#newcode159 chrome/browser/history/android/sqlite_cursor.cc:159: if (BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, From PostTask: " even if the ...
7 years, 6 months ago (2013-05-30 19:33:05 UTC) #4
michaelbai
https://codereview.chromium.org/15701011/diff/3/chrome/browser/history/android/sqlite_cursor.cc File chrome/browser/history/android/sqlite_cursor.cc (right): https://codereview.chromium.org/15701011/diff/3/chrome/browser/history/android/sqlite_cursor.cc#newcode159 chrome/browser/history/android/sqlite_cursor.cc:159: if (BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, In the case of UI message ...
7 years, 6 months ago (2013-05-31 05:17:02 UTC) #5
michaelbai
PTAL
7 years, 6 months ago (2013-05-31 05:17:47 UTC) #6
michaelbai
Sky@, would you like take another look?
7 years, 6 months ago (2013-06-03 16:54:10 UTC) #7
sky
LGTM https://codereview.chromium.org/15701011/diff/16001/chrome/browser/history/android/sqlite_cursor.h File chrome/browser/history/android/sqlite_cursor.h (right): https://codereview.chromium.org/15701011/diff/16001/chrome/browser/history/android/sqlite_cursor.h#newcode139 chrome/browser/history/android/sqlite_cursor.h:139: void DestroyOnUIThread(); Add a comment. https://codereview.chromium.org/15701011/diff/16001/chrome/browser/history/android/sqlite_cursor.h#newcode141 chrome/browser/history/android/sqlite_cursor.h:141: nit: ...
7 years, 6 months ago (2013-06-03 17:48:25 UTC) #8
michaelbai
https://codereview.chromium.org/15701011/diff/16001/chrome/browser/history/android/sqlite_cursor.h File chrome/browser/history/android/sqlite_cursor.h (right): https://codereview.chromium.org/15701011/diff/16001/chrome/browser/history/android/sqlite_cursor.h#newcode139 chrome/browser/history/android/sqlite_cursor.h:139: void DestroyOnUIThread(); On 2013/06/03 17:48:25, sky wrote: > Add ...
7 years, 6 months ago (2013-06-03 22:25:37 UTC) #9
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/michaelbai@chromium.org/15701011/26001
7 years, 6 months ago (2013-06-03 22:26:34 UTC) #10
commit-bot: I haz the power
Retried try job too often on mac for step(s) compile http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac&number=57408
7 years, 6 months ago (2013-06-03 22:42:41 UTC) #11
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/michaelbai@chromium.org/15701011/40001
7 years, 6 months ago (2013-06-04 04:09:07 UTC) #12
commit-bot: I haz the power
7 years, 6 months ago (2013-06-04 13:06:57 UTC) #13
Message was sent while issue was closed.
Change committed as 203953

Powered by Google App Engine
This is Rietveld 408576698