Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2398793002:
[Extensions] Convert some ChromeSyncExtensionFunctions (Closed)
|
Created:
4 years, 2 months ago by Devlin Modified:
4 years, 2 months ago Reviewers:
asargent_no_longer_on_chrome CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Extensions] Convert some ChromeSyncExtensionFunctions
SyncExtensionFunctions, AsyncExtensionFunctions,
ChromeSyncExtensionFunctions, and ChromeAsyncExtensionFunctions are
deprecated.
Remove ChromeSyncExtensionFunctions from:
- tabs
BUG=634140
Committed: https://crrev.com/6458eb87e45739312f88a36942062c009070f1d6
Cr-Commit-Position: refs/heads/master@{#423733}
Patch Set 1 #Patch Set 2 : compile #
Total comments: 17
Patch Set 3 : Antony's #
Messages
Total messages: 23 (16 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_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_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: This issue passed the CQ dry run.
rdevlin.cronin@chromium.org changed reviewers: + asargent@chromium.org
Antony, can you take a look?
lgtm https://codereview.chromium.org/2398793002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/tabs/tabs_api.cc (right): https://codereview.chromium.org/2398793002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_api.cc:1047: return RespondNow(Error(kUnknownErrorDoNotUse)); are you sure you wanted to introduce a new usage of this apparently-forbidden error code? https://codereview.chromium.org/2398793002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_api.cc:1258: &opener_contents, NULL)) might as well s/NULL/nullptr/ while you're touching these lines? https://codereview.chromium.org/2398793002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_api.cc:1386: // can determine its success (as we return errors below). If a tree falls in a forest and no one is around to hear it, does it make a sound? https://codereview.chromium.org/2398793002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_api.cc:1574: NULL, &contents, NULL, error)) { NULL->nullptr https://codereview.chromium.org/2398793002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_api.cc:1701: NULL, &contents, NULL, &error_)) { NULL->nullptr https://codereview.chromium.org/2398793002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_api.cc:1832: NULL, &contents, NULL, &error_)) { NULL->nullptr https://codereview.chromium.org/2398793002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_api.cc:1889: &browser, NULL, &contents, NULL, &error_) && NULL->nullptr https://codereview.chromium.org/2398793002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_api.cc:1930: NULL /* ignore int tab_index output */, &error_); NULL->nullptr
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/2398793002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/tabs/tabs_api.cc (right): https://codereview.chromium.org/2398793002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_api.cc:1047: return RespondNow(Error(kUnknownErrorDoNotUse)); On 2016/10/06 19:50:04, Antony Sargent wrote: > are you sure you wanted to introduce a new usage of this apparently-forbidden > error code? Thanks for asking (that means it worked)! :) // A string used in the case of an unknown error being detected. // DON'T USE THIS. It's only here during conversion to flag cases where errors // aren't already set. // TODO(devlin): Remove this if/when all functions are updated to return real // errors. static const char* kUnknownErrorDoNotUse; But that's exactly what this one is for. https://codereview.chromium.org/2398793002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_api.cc:1258: &opener_contents, NULL)) On 2016/10/06 19:50:04, Antony Sargent wrote: > might as well s/NULL/nullptr/ while you're touching these lines? Done. https://codereview.chromium.org/2398793002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_api.cc:1386: // can determine its success (as we return errors below). On 2016/10/06 19:50:04, Antony Sargent wrote: > If a tree falls in a forest and no one is around to hear it, does it make a > sound? > Don't quite follow. You mean if we set the last error and there's no callback, does it matter? It actually does, because we throw unchecked lastErrors (even if there's no callback). https://codereview.chromium.org/2398793002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_api.cc:1574: NULL, &contents, NULL, error)) { On 2016/10/06 19:50:04, Antony Sargent wrote: > NULL->nullptr Done. https://codereview.chromium.org/2398793002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_api.cc:1701: NULL, &contents, NULL, &error_)) { On 2016/10/06 19:50:04, Antony Sargent wrote: > NULL->nullptr Done. https://codereview.chromium.org/2398793002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_api.cc:1832: NULL, &contents, NULL, &error_)) { On 2016/10/06 19:50:04, Antony Sargent wrote: > NULL->nullptr Done. https://codereview.chromium.org/2398793002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_api.cc:1889: &browser, NULL, &contents, NULL, &error_) && On 2016/10/06 19:50:04, Antony Sargent wrote: > NULL->nullptr Done. https://codereview.chromium.org/2398793002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_api.cc:1930: NULL /* ignore int tab_index output */, &error_); On 2016/10/06 19:50:04, Antony Sargent wrote: > NULL->nullptr 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 asargent@chromium.org Link to the patchset: https://codereview.chromium.org/2398793002/#ps40001 (title: "Antony's")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2398793002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/tabs/tabs_api.cc (right): https://codereview.chromium.org/2398793002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_api.cc:1386: // can determine its success (as we return errors below). On 2016/10/06 23:21:17, Devlin wrote: > On 2016/10/06 19:50:04, Antony Sargent wrote: > > If a tree falls in a forest and no one is around to hear it, does it make a > > sound? > > > > Don't quite follow. You mean if we set the last error and there's no callback, > does it matter? It actually does, because we throw unchecked lastErrors (even > if there's no callback). This was an attempt at humor - your comment just reminded me of the classic philosophical question. (I.e. if an extension function invocation fails and there's no callback to notice the failure... sounds pretty similar to "if a tree falls and there's no one to hear it")
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [Extensions] Convert some ChromeSyncExtensionFunctions SyncExtensionFunctions, AsyncExtensionFunctions, ChromeSyncExtensionFunctions, and ChromeAsyncExtensionFunctions are deprecated. Remove ChromeSyncExtensionFunctions from: - tabs BUG=634140 ========== to ========== [Extensions] Convert some ChromeSyncExtensionFunctions SyncExtensionFunctions, AsyncExtensionFunctions, ChromeSyncExtensionFunctions, and ChromeAsyncExtensionFunctions are deprecated. Remove ChromeSyncExtensionFunctions from: - tabs BUG=634140 Committed: https://crrev.com/6458eb87e45739312f88a36942062c009070f1d6 Cr-Commit-Position: refs/heads/master@{#423733} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/6458eb87e45739312f88a36942062c009070f1d6 Cr-Commit-Position: refs/heads/master@{#423733} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
