Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2348723002:
[Extensions] Remove UIThreadExtensionFunction::DelegateForTests (Closed)
Description[Extensions] Remove UIThreadExtensionFunction::DelegateForTests
The ExtensionFunction test delegate basically just served as a way of
getting the results from the function, but we already have that by simply
setting the response callback in the function. Remove the
DelegateForTests class and update uses of it to use the callback.
Also add a response() field on ExtensionFunction for use in determining
if the function succeeded or failed. Previously, code would rely on
whether or not results_ were set, but this was wrong for a few reasons:
- The absence of results doesn't always indicate failure
- The presence of results doesn't always indicate success
- In practice, we always send back an empty list value (even if
results weren't previously set).
Update code to use the response() value instead.
BUG=648276
TBR=benwells@chromium.org (apps browsertest change)
Committed: https://crrev.com/187edaa9952ac1c26f40b034cdf99cf3aae7e91f
Cr-Commit-Position: refs/heads/master@{#419560}
Patch Set 1 : fix #Patch Set 2 : polish #
Total comments: 4
Patch Set 3 : lazyboy's #Messages
Total messages: 48 (39 generated)
The CQ bit was checked by rdevlin.cronin@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_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by rdevlin.cronin@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_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by rdevlin.cronin@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: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) 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 rdevlin.cronin@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.
Description was changed from ========== Remove testdelegate BUG= ========== to ========== [Extensions] Remove UIThreadExtensionFunction::DelegateForTests The ExtensionFunction test delegate basically just served as a way of getting the results from the function, but we already have that by simply setting the response callback in the function. Remove the DelegateForTests class and update uses of it to use the callback. Also add a response() field on ExtensionFunction for use in determining if the function succeeded or failed. Previously, code would rely on whether or not results_ were set, but this was wrong for a few reasons: - The presence of results doesn't always indicate failure - The absence doesn't always indicate success - In practice, we always send back an empty list value (even if results weren't previously set). Update code to use the response() value instead. BUG= ==========
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
Description was changed from ========== [Extensions] Remove UIThreadExtensionFunction::DelegateForTests The ExtensionFunction test delegate basically just served as a way of getting the results from the function, but we already have that by simply setting the response callback in the function. Remove the DelegateForTests class and update uses of it to use the callback. Also add a response() field on ExtensionFunction for use in determining if the function succeeded or failed. Previously, code would rely on whether or not results_ were set, but this was wrong for a few reasons: - The presence of results doesn't always indicate failure - The absence doesn't always indicate success - In practice, we always send back an empty list value (even if results weren't previously set). Update code to use the response() value instead. BUG= ========== to ========== [Extensions] Remove UIThreadExtensionFunction::DelegateForTests The ExtensionFunction test delegate basically just served as a way of getting the results from the function, but we already have that by simply setting the response callback in the function. Remove the DelegateForTests class and update uses of it to use the callback. Also add a response() field on ExtensionFunction for use in determining if the function succeeded or failed. Previously, code would rely on whether or not results_ were set, but this was wrong for a few reasons: - The presence of results doesn't always indicate failure - The absence doesn't always indicate success - In practice, we always send back an empty list value (even if results weren't previously set). Update code to use the response() value instead. BUG=634140 ==========
Description was changed from ========== [Extensions] Remove UIThreadExtensionFunction::DelegateForTests The ExtensionFunction test delegate basically just served as a way of getting the results from the function, but we already have that by simply setting the response callback in the function. Remove the DelegateForTests class and update uses of it to use the callback. Also add a response() field on ExtensionFunction for use in determining if the function succeeded or failed. Previously, code would rely on whether or not results_ were set, but this was wrong for a few reasons: - The presence of results doesn't always indicate failure - The absence doesn't always indicate success - In practice, we always send back an empty list value (even if results weren't previously set). Update code to use the response() value instead. BUG=634140 ========== to ========== [Extensions] Remove UIThreadExtensionFunction::DelegateForTests The ExtensionFunction test delegate basically just served as a way of getting the results from the function, but we already have that by simply setting the response callback in the function. Remove the DelegateForTests class and update uses of it to use the callback. Also add a response() field on ExtensionFunction for use in determining if the function succeeded or failed. Previously, code would rely on whether or not results_ were set, but this was wrong for a few reasons: - The presence of results doesn't always indicate failure - The absence doesn't always indicate success - In practice, we always send back an empty list value (even if results weren't previously set). Update code to use the response() value instead. BUG=648276 ==========
rdevlin.cronin@chromium.org changed reviewers: + lazyboy@chromium.org
lazyboy@, can you take a look?
lgtm with couple of nits. I think you need to reverse the words "presence" and "absence" in your CL description? https://codereview.chromium.org/2348723002/diff/120001/extensions/browser/ext... File extensions/browser/extension_function.cc (right): https://codereview.chromium.org/2348723002/diff/120001/extensions/browser/ext... extensions/browser/extension_function.cc:457: response_ = base::MakeUnique<ResponseType>(response); I'd name this response_type_ (and the getter). Because skimming through the code, I was reading that this field holds the entire response (i.e. what |results_| currently holds). https://codereview.chromium.org/2348723002/diff/120001/extensions/browser/ext... File extensions/browser/extension_function.h (right): https://codereview.chromium.org/2348723002/diff/120001/extensions/browser/ext... extensions/browser/extension_function.h:480: // The response of the function, if one has been set. The response type of the function, if response has been set.
The CQ bit was checked by rdevlin.cronin@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.
https://codereview.chromium.org/2348723002/diff/120001/extensions/browser/ext... File extensions/browser/extension_function.cc (right): https://codereview.chromium.org/2348723002/diff/120001/extensions/browser/ext... extensions/browser/extension_function.cc:457: response_ = base::MakeUnique<ResponseType>(response); On 2016/09/19 18:08:56, lazyboy wrote: > I'd name this response_type_ (and the getter). Because skimming through the > code, I was reading that this field holds the entire response (i.e. what > |results_| currently holds). SG, done. https://codereview.chromium.org/2348723002/diff/120001/extensions/browser/ext... File extensions/browser/extension_function.h (right): https://codereview.chromium.org/2348723002/diff/120001/extensions/browser/ext... extensions/browser/extension_function.h:480: // The response of the function, if one has been set. On 2016/09/19 18:08:56, lazyboy wrote: > The response type of the function, if response has been set. Done.
The CQ bit was checked by rdevlin.cronin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lazyboy@chromium.org Link to the patchset: https://codereview.chromium.org/2348723002/#ps140001 (title: "lazyboy's")
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Description was changed from ========== [Extensions] Remove UIThreadExtensionFunction::DelegateForTests The ExtensionFunction test delegate basically just served as a way of getting the results from the function, but we already have that by simply setting the response callback in the function. Remove the DelegateForTests class and update uses of it to use the callback. Also add a response() field on ExtensionFunction for use in determining if the function succeeded or failed. Previously, code would rely on whether or not results_ were set, but this was wrong for a few reasons: - The presence of results doesn't always indicate failure - The absence doesn't always indicate success - In practice, we always send back an empty list value (even if results weren't previously set). Update code to use the response() value instead. BUG=648276 ========== to ========== [Extensions] Remove UIThreadExtensionFunction::DelegateForTests The ExtensionFunction test delegate basically just served as a way of getting the results from the function, but we already have that by simply setting the response callback in the function. Remove the DelegateForTests class and update uses of it to use the callback. Also add a response() field on ExtensionFunction for use in determining if the function succeeded or failed. Previously, code would rely on whether or not results_ were set, but this was wrong for a few reasons: - The presence of results doesn't always indicate failure - The absence doesn't always indicate success - In practice, we always send back an empty list value (even if results weren't previously set). Update code to use the response() value instead. BUG=648276 TBR=benwells@chromium.org (apps browsertest change) ==========
The CQ bit was checked by rdevlin.cronin@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 rdevlin.cronin@chromium.org
Description was changed from ========== [Extensions] Remove UIThreadExtensionFunction::DelegateForTests The ExtensionFunction test delegate basically just served as a way of getting the results from the function, but we already have that by simply setting the response callback in the function. Remove the DelegateForTests class and update uses of it to use the callback. Also add a response() field on ExtensionFunction for use in determining if the function succeeded or failed. Previously, code would rely on whether or not results_ were set, but this was wrong for a few reasons: - The presence of results doesn't always indicate failure - The absence doesn't always indicate success - In practice, we always send back an empty list value (even if results weren't previously set). Update code to use the response() value instead. BUG=648276 TBR=benwells@chromium.org (apps browsertest change) ========== to ========== [Extensions] Remove UIThreadExtensionFunction::DelegateForTests The ExtensionFunction test delegate basically just served as a way of getting the results from the function, but we already have that by simply setting the response callback in the function. Remove the DelegateForTests class and update uses of it to use the callback. Also add a response() field on ExtensionFunction for use in determining if the function succeeded or failed. Previously, code would rely on whether or not results_ were set, but this was wrong for a few reasons: - The absence of results doesn't always indicate failure - The presence of results doesn't always indicate success - In practice, we always send back an empty list value (even if results weren't previously set). Update code to use the response() value instead. BUG=648276 TBR=benwells@chromium.org (apps browsertest change) ==========
The CQ bit was checked by rdevlin.cronin@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.
Description was changed from ========== [Extensions] Remove UIThreadExtensionFunction::DelegateForTests The ExtensionFunction test delegate basically just served as a way of getting the results from the function, but we already have that by simply setting the response callback in the function. Remove the DelegateForTests class and update uses of it to use the callback. Also add a response() field on ExtensionFunction for use in determining if the function succeeded or failed. Previously, code would rely on whether or not results_ were set, but this was wrong for a few reasons: - The absence of results doesn't always indicate failure - The presence of results doesn't always indicate success - In practice, we always send back an empty list value (even if results weren't previously set). Update code to use the response() value instead. BUG=648276 TBR=benwells@chromium.org (apps browsertest change) ========== to ========== [Extensions] Remove UIThreadExtensionFunction::DelegateForTests The ExtensionFunction test delegate basically just served as a way of getting the results from the function, but we already have that by simply setting the response callback in the function. Remove the DelegateForTests class and update uses of it to use the callback. Also add a response() field on ExtensionFunction for use in determining if the function succeeded or failed. Previously, code would rely on whether or not results_ were set, but this was wrong for a few reasons: - The absence of results doesn't always indicate failure - The presence of results doesn't always indicate success - In practice, we always send back an empty list value (even if results weren't previously set). Update code to use the response() value instead. BUG=648276 TBR=benwells@chromium.org (apps browsertest change) ==========
Message was sent while issue was closed.
Committed patchset #3 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== [Extensions] Remove UIThreadExtensionFunction::DelegateForTests The ExtensionFunction test delegate basically just served as a way of getting the results from the function, but we already have that by simply setting the response callback in the function. Remove the DelegateForTests class and update uses of it to use the callback. Also add a response() field on ExtensionFunction for use in determining if the function succeeded or failed. Previously, code would rely on whether or not results_ were set, but this was wrong for a few reasons: - The absence of results doesn't always indicate failure - The presence of results doesn't always indicate success - In practice, we always send back an empty list value (even if results weren't previously set). Update code to use the response() value instead. BUG=648276 TBR=benwells@chromium.org (apps browsertest change) ========== to ========== [Extensions] Remove UIThreadExtensionFunction::DelegateForTests The ExtensionFunction test delegate basically just served as a way of getting the results from the function, but we already have that by simply setting the response callback in the function. Remove the DelegateForTests class and update uses of it to use the callback. Also add a response() field on ExtensionFunction for use in determining if the function succeeded or failed. Previously, code would rely on whether or not results_ were set, but this was wrong for a few reasons: - The absence of results doesn't always indicate failure - The presence of results doesn't always indicate success - In practice, we always send back an empty list value (even if results weren't previously set). Update code to use the response() value instead. BUG=648276 TBR=benwells@chromium.org (apps browsertest change) Committed: https://crrev.com/187edaa9952ac1c26f40b034cdf99cf3aae7e91f Cr-Commit-Position: refs/heads/master@{#419560} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/187edaa9952ac1c26f40b034cdf99cf3aae7e91f Cr-Commit-Position: refs/heads/master@{#419560} |
