|
|
Descriptionhistory_api -> UIThreadExtensionFunction.
This removes direct usage of AsyncExtensionFunction::error_/results_.
BUG=648275
Test=None, compile.
Review-Url: https://codereview.chromium.org/2669423004
Cr-Commit-Position: refs/heads/master@{#451358}
Committed: https://chromium.googlesource.com/chromium/src/+/df7686e843360e2ae174bda45648708b59653830
Patch Set 1 #
Total comments: 6
Patch Set 2 : remove extra PostTasks #Patch Set 3 : sync #
Messages
Total messages: 29 (17 generated)
lazyboy@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
https://codereview.chromium.org/2669423004/diff/1/chrome/browser/extensions/a... File chrome/browser/extensions/api/history/history_api.cc (right): https://codereview.chromium.org/2669423004/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/history/history_api.cc:261: void HistoryFunctionWithCallback::SendAsyncResponse( Do we have to SendAsyncResponse in order to prevent accidentally calling back synchronously? It looks like most of these are chained from callbacks - are some of those (possibly) called synchronously?
https://codereview.chromium.org/2669423004/diff/1/chrome/browser/extensions/a... File chrome/browser/extensions/api/history/history_api.cc (right): https://codereview.chromium.org/2669423004/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/history/history_api.cc:261: void HistoryFunctionWithCallback::SendAsyncResponse( On 2017/02/03 22:30:53, Devlin wrote: > Do we have to SendAsyncResponse in order to prevent accidentally calling back > synchronously? It looks like most of these are chained from callbacks - are > some of those (possibly) called synchronously? HistoryService apis run on its own TaskRunner, e.g.: https://cs.chromium.org/chromium/src/components/history/core/browser/history_... All of these apis QueryURL/QueryHistory/AddPage/DeleteURL/ExpireHistoryBetween will call PostTask, so the callbacks shouldn't ever be called synchronously. I discovered one early out in AddPage from HistoryAddUrlFunction: https://cs.chromium.org/chromium/src/components/history/core/browser/history_... However, since we don't wait for the task to finish before responding, that shouldn't be an issue either.
https://codereview.chromium.org/2669423004/diff/1/chrome/browser/extensions/a... File chrome/browser/extensions/api/history/history_api.cc (right): https://codereview.chromium.org/2669423004/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/history/history_api.cc:261: void HistoryFunctionWithCallback::SendAsyncResponse( On 2017/02/03 23:49:06, lazyboy wrote: > On 2017/02/03 22:30:53, Devlin wrote: > > Do we have to SendAsyncResponse in order to prevent accidentally calling back > > synchronously? It looks like most of these are chained from callbacks - are > > some of those (possibly) called synchronously? > > HistoryService apis run on its own TaskRunner, e.g.: > https://cs.chromium.org/chromium/src/components/history/core/browser/history_... > > All of these apis > QueryURL/QueryHistory/AddPage/DeleteURL/ExpireHistoryBetween > will call PostTask, so the callbacks shouldn't ever be called synchronously. > > I discovered one early out in AddPage from HistoryAddUrlFunction: > https://cs.chromium.org/chromium/src/components/history/core/browser/history_... > However, since we don't wait for the task to finish before responding, that > shouldn't be an issue either. So if that's the case (that all these methods on history service run async), do we need SendAsyncResponse at all? Or could we just use Respond() in each place? https://codereview.chromium.org/2669423004/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/history/history_api.cc:305: SendAsyncResponse(ArgumentList(GetVisits::Results::Create(visit_item_vec))); e.g. this becomes just: Respond(ArgumentList(...)); Release(); // Balanced in Run().
The CQ bit was checked by lazyboy@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...
Sorry about the delay, lost track of this CL. Please take another look. https://codereview.chromium.org/2669423004/diff/1/chrome/browser/extensions/a... File chrome/browser/extensions/api/history/history_api.cc (right): https://codereview.chromium.org/2669423004/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/history/history_api.cc:261: void HistoryFunctionWithCallback::SendAsyncResponse( On 2017/02/06 16:09:26, Devlin wrote: > On 2017/02/03 23:49:06, lazyboy wrote: > > On 2017/02/03 22:30:53, Devlin wrote: > > > Do we have to SendAsyncResponse in order to prevent accidentally calling > back > > > synchronously? It looks like most of these are chained from callbacks - are > > > some of those (possibly) called synchronously? > > > > HistoryService apis run on its own TaskRunner, e.g.: > > > https://cs.chromium.org/chromium/src/components/history/core/browser/history_... > > > > All of these apis > > QueryURL/QueryHistory/AddPage/DeleteURL/ExpireHistoryBetween > > will call PostTask, so the callbacks shouldn't ever be called synchronously. > > > > I discovered one early out in AddPage from HistoryAddUrlFunction: > > > https://cs.chromium.org/chromium/src/components/history/core/browser/history_... > > However, since we don't wait for the task to finish before responding, that > > shouldn't be an issue either. > > So if that's the case (that all these methods on history service run async), do > we need SendAsyncResponse at all? Or could we just use Respond() in each place? Done. https://codereview.chromium.org/2669423004/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/history/history_api.cc:305: SendAsyncResponse(ArgumentList(GetVisits::Results::Create(visit_item_vec))); On 2017/02/06 16:09:26, Devlin wrote: > e.g. this becomes just: > Respond(ArgumentList(...)); > Release(); // Balanced in Run(). Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm!
The CQ bit was checked by lazyboy@chromium.org
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
Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by lazyboy@chromium.org
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
Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
The CQ bit was checked by lazyboy@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.
The CQ bit was checked by lazyboy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rdevlin.cronin@chromium.org Link to the patchset: https://codereview.chromium.org/2669423004/#ps40001 (title: "sync")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 40001, "attempt_start_ts": 1487357835399940, "parent_rev": "d9d34e59b180cb66b27b3bc28a61ce1ef6728db3", "commit_rev": "df7686e843360e2ae174bda45648708b59653830"}
Message was sent while issue was closed.
Description was changed from ========== history_api -> UIThreadExtensionFunction. This removes direct usage of AsyncExtensionFunction::error_/results_. BUG=648275 Test=None, compile. ========== to ========== history_api -> UIThreadExtensionFunction. This removes direct usage of AsyncExtensionFunction::error_/results_. BUG=648275 Test=None, compile. Review-Url: https://codereview.chromium.org/2669423004 Cr-Commit-Position: refs/heads/master@{#451358} Committed: https://chromium.googlesource.com/chromium/src/+/df7686e843360e2ae174bda45648... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/df7686e843360e2ae174bda45648... |