Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2618493002:
Chrome app manifest support for action handlers. (Closed)
DescriptionChrome app manifest support for action handlers.
BUG=657139
Review-Url: https://codereview.chromium.org/2618493002
Cr-Commit-Position: refs/heads/master@{#448061}
Committed: https://chromium.googlesource.com/chromium/src/+/c903a542cee2a0a794fea8753864f81ed62512e7
Patch Set 1 : Initial upload #
Total comments: 3
Patch Set 2 : Address comments #
Total comments: 2
Patch Set 3 : Rebase #Patch Set 4 : Setup ManifestHandler/ManifestData for "action_handlers" #Patch Set 5 : Rebase #
Total comments: 14
Patch Set 6 : Address comments #
Total comments: 13
Patch Set 7 : Rebase #Patch Set 8 : Address comments #
Total comments: 8
Patch Set 9 : Rebase #Patch Set 10 : Address comments #Patch Set 11 : Make //components/version_info:version_info dep explicit #Dependent Patchsets: Messages
Total messages: 85 (68 generated)
The CQ bit was checked by jdufault@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 checked by jdufault@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.
Patchset #1 (id:1) has been deleted
The CQ bit was checked by jdufault@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.
Patchset #1 (id:20001) has been deleted
Description was changed from ========== notready: Chrome app manifest support for action handlers. BUG=657139 ========== to ========== notready: Chrome app manifest support for action handlers. BUG=657139 ==========
jdufault@chromium.org changed reviewers: + derat@chromium.org, mkearney@chromium.org, rdevlin.cronin@chromium.org
Description was changed from ========== notready: Chrome app manifest support for action handlers. BUG=657139 ========== to ========== Chrome app manifest support for action handlers. BUG=657139 ==========
rdevlin.cronin@: PTAL at extensions/* and chrome/common/extensions/api/* mkearney@: PTAL at chrome/common/extensions/docs/* derat@: PTAL at chrome/browser/chromeos/* Thanks!
lgtm for c/b/chromeos/ https://codereview.chromium.org/2618493002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/note_taking_helper.cc (right): https://codereview.chromium.org/2618493002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/note_taking_helper.cc:42: constexpr const char kNewNoteAction[] = "new_note"; nit: mind renaming this to kChromeNewNoteAction or something similar and adding a brief comment describing what it is?
https://codereview.chromium.org/2618493002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/note_taking_helper.cc (right): https://codereview.chromium.org/2618493002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/note_taking_helper.cc:42: constexpr const char kNewNoteAction[] = "new_note"; On 2017/01/06 21:01:45, Daniel Erat wrote: > nit: mind renaming this to kChromeNewNoteAction or something similar and adding > a brief comment describing what it is? oh, and i'd also recommend making it a public static member, as the android constants are, so the test can use it as well.
The CQ bit was checked by jdufault@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...
https://codereview.chromium.org/2618493002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/note_taking_helper.cc (right): https://codereview.chromium.org/2618493002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/note_taking_helper.cc:272: if (!extension->manifest()->GetList( This shouldn't be handled here. Instead, we should have an action_handler-related manifest handler that parses the data in the manifest and checks it for correctness. Then, things like this should call into that to get the appropriate data from the ManifestData. See examples in extensions/common/manifest_handlers.
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 jdufault@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/2618493002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/note_taking_helper.cc (right): https://codereview.chromium.org/2618493002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/note_taking_helper.cc:42: constexpr const char kNewNoteAction[] = "new_note"; On 2017/01/06 21:02:33, Daniel Erat wrote: > On 2017/01/06 21:01:45, Daniel Erat wrote: > > nit: mind renaming this to kChromeNewNoteAction or something similar and > adding > > a brief comment describing what it is? > > oh, and i'd also recommend making it a public static member, as the android > constants are, so the test can use it as well. Done. https://codereview.chromium.org/2618493002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/note_taking_helper.cc (right): https://codereview.chromium.org/2618493002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/note_taking_helper.cc:272: if (!extension->manifest()->GetList( On 2017/01/09 23:39:02, Devlin wrote: > This shouldn't be handled here. Instead, we should have an > action_handler-related manifest handler that parses the data in the manifest and > checks it for correctness. Then, things like this should call into that to get > the appropriate data from the ManifestData. See examples in > extensions/common/manifest_handlers. Done.
The CQ bit was checked by jdufault@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...
https://codereview.chromium.org/2618493002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/note_taking_helper.cc (right): https://codereview.chromium.org/2618493002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/note_taking_helper.cc:296: if (HasNewNoteActionHandler(extension.get())) we should probably skip whitelisted apps here to make sure they don't appear twice, right? https://codereview.chromium.org/2618493002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/note_taking_helper_unittest.cc (right): https://codereview.chromium.org/2618493002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/note_taking_helper_unittest.cc:365: const extensions::ExtensionId kInvalidNewNoteId = i think you aren't using this. did you mean to create one with a different handler? https://codereview.chromium.org/2618493002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/note_taking_helper_unittest.cc:390: EXPECT_EQ(GetAppString(kNewNoteId, kName, false), GetAppString(apps[0])); consider adding a test to check that whitelisted apps aren't listed twice https://codereview.chromium.org/2618493002/diff/120001/chrome/common/extensio... File chrome/common/extensions/api/_manifest_features.json (right): https://codereview.chromium.org/2618493002/diff/120001/chrome/common/extensio... chrome/common/extensions/api/_manifest_features.json:10: "action_handlers": { move this down to keep the list alphabetized https://codereview.chromium.org/2618493002/diff/120001/chrome/common/extensio... File chrome/common/extensions/docs/templates/articles/manifest/action_handlers.html (right): https://codereview.chromium.org/2618493002/diff/120001/chrome/common/extensio... chrome/common/extensions/docs/templates/articles/manifest/action_handlers.html:5: or intents that the application supports. An action is essentially an nit: either "declares user actions or intents that the application supports" or "declares which user actions or intents the application supports" (including both "which" and "that" sounds strange) https://codereview.chromium.org/2618493002/diff/120001/extensions/common/mani... File extensions/common/manifest_handlers/action_handlers_handler.cc (right): https://codereview.chromium.org/2618493002/diff/120001/extensions/common/mani... extensions/common/manifest_handlers/action_handlers_handler.cc:21: std::set<app_runtime::ActionType> ActionHandlersInfo::GetActionHandlers( i take it that this can't return a const reference since the extension may not have any handlers, right? could you avoid the copy by changing this to: bool ActionHandlersInfo::HasActionHandler(const Extension* extension, app_runtime::ActionType); ? that's all that you use it for right now, right? do you envision needing to be able to fetch the full list of handlers in the future?
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 jdufault@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_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #6 (id:140001) has been deleted
The CQ bit was checked by jdufault@chromium.org to run a CQ dry run
https://codereview.chromium.org/2618493002/diff/120001/chrome/common/extensio... File chrome/common/extensions/api/_manifest_features.json (right): https://codereview.chromium.org/2618493002/diff/120001/chrome/common/extensio... chrome/common/extensions/api/_manifest_features.json:10: "action_handlers": { Does this have to be in //chrome/common/extensions/api instead of //extension/common/api? Since the implementation is in //extensions we could make it available from there. Partly to decouple extensions APIs from Chrome, and also to make it easier to potentially use APIs like this on app_shell.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #6 (id:160001) has been deleted
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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 jdufault@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/2618493002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/note_taking_helper.cc (right): https://codereview.chromium.org/2618493002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/note_taking_helper.cc:296: if (HasNewNoteActionHandler(extension.get())) On 2017/01/12 23:55:03, Daniel Erat wrote: > we should probably skip whitelisted apps here to make sure they don't appear > twice, right? Done, nice catch. https://codereview.chromium.org/2618493002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/note_taking_helper_unittest.cc (right): https://codereview.chromium.org/2618493002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/note_taking_helper_unittest.cc:365: const extensions::ExtensionId kInvalidNewNoteId = On 2017/01/12 23:55:03, Daniel Erat wrote: > i think you aren't using this. did you mean to create one with a different > handler? Artifact from a previous patch before extension parsing was done via a manifest handler. Removed. https://codereview.chromium.org/2618493002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/note_taking_helper_unittest.cc:390: EXPECT_EQ(GetAppString(kNewNoteId, kName, false), GetAppString(apps[0])); On 2017/01/12 23:55:03, Daniel Erat wrote: > consider adding a test to check that whitelisted apps aren't listed twice Done. https://codereview.chromium.org/2618493002/diff/120001/chrome/common/extensio... File chrome/common/extensions/api/_manifest_features.json (right): https://codereview.chromium.org/2618493002/diff/120001/chrome/common/extensio... chrome/common/extensions/api/_manifest_features.json:10: "action_handlers": { On 2017/01/12 23:55:03, Daniel Erat wrote: > move this down to keep the list alphabetized Done. https://codereview.chromium.org/2618493002/diff/120001/chrome/common/extensio... chrome/common/extensions/api/_manifest_features.json:10: "action_handlers": { On 2017/01/13 22:17:07, michaelpg (NYC) wrote: > Does this have to be in //chrome/common/extensions/api instead of > //extension/common/api? Since the implementation is in //extensions we could > make it available from there. Partly to decouple extensions APIs from Chrome, > and also to make it easier to potentially use APIs like this on app_shell. Done. https://codereview.chromium.org/2618493002/diff/120001/chrome/common/extensio... File chrome/common/extensions/docs/templates/articles/manifest/action_handlers.html (right): https://codereview.chromium.org/2618493002/diff/120001/chrome/common/extensio... chrome/common/extensions/docs/templates/articles/manifest/action_handlers.html:5: or intents that the application supports. An action is essentially an On 2017/01/12 23:55:03, Daniel Erat wrote: > nit: either "declares user actions or intents that the application supports" or > "declares which user actions or intents the application supports" (including > both "which" and "that" sounds strange) Done. https://codereview.chromium.org/2618493002/diff/120001/extensions/common/mani... File extensions/common/manifest_handlers/action_handlers_handler.cc (right): https://codereview.chromium.org/2618493002/diff/120001/extensions/common/mani... extensions/common/manifest_handlers/action_handlers_handler.cc:21: std::set<app_runtime::ActionType> ActionHandlersInfo::GetActionHandlers( On 2017/01/12 23:55:03, Daniel Erat wrote: > i take it that this can't return a const reference since the extension may not > have any handlers, right? could you avoid the copy by changing this to: > > bool ActionHandlersInfo::HasActionHandler(const Extension* extension, > app_runtime::ActionType); > > ? that's all that you use it for right now, right? do you envision needing to be > able to fetch the full list of handlers in the future? Done. Tests need the full set of handlers, so I moved this code into the test file.
lgtm
Nice! A couple small nits. I'd also like to patch this in locally and see the documentation changes, but am remote for this week. Can this wait until next week to land? If not, I can figure something out to see how it looks. https://codereview.chromium.org/2618493002/diff/180001/extensions/common/api/... File extensions/common/api/_manifest_features.json (right): https://codereview.chromium.org/2618493002/diff/180001/extensions/common/api/... extensions/common/api/_manifest_features.json:11: "channel": "stable", Let's not launch this to stable just yet - it's usually good practice to have it in dev for awhile and let people fool around with it before we finalize everything and ship it. https://codereview.chromium.org/2618493002/diff/180001/extensions/common/api/... extensions/common/api/_manifest_features.json:13: }, should we restrict this to chromeos? https://codereview.chromium.org/2618493002/diff/180001/extensions/common/mani... File extensions/common/manifest_handlers/action_handlers_handler.cc (right): https://codereview.chromium.org/2618493002/diff/180001/extensions/common/mani... extensions/common/manifest_handlers/action_handlers_handler.cc:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. ditto https://codereview.chromium.org/2618493002/diff/180001/extensions/common/mani... extensions/common/manifest_handlers/action_handlers_handler.cc:38: auto info = base::MakeUnique<ActionHandlersInfo>(); nit: initialize this after line 44 https://codereview.chromium.org/2618493002/diff/180001/extensions/common/mani... File extensions/common/manifest_handlers/action_handlers_handler.h (right): https://codereview.chromium.org/2618493002/diff/180001/extensions/common/mani... extensions/common/manifest_handlers/action_handlers_handler.h:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. nit: no (c) https://codereview.chromium.org/2618493002/diff/180001/extensions/common/mani... File extensions/common/manifest_handlers/action_handlers_handler_unittest.cc (right): https://codereview.chromium.org/2618493002/diff/180001/extensions/common/mani... extensions/common/manifest_handlers/action_handlers_handler_unittest.cc:58: auto none = LoadAndExpectSuccess(CreateManifest("[]")); nit: It's not clear from the callsite what the return result here is, so let's not use auto
The CQ bit was checked by jdufault@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_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #8 (id:220001) has been deleted
The CQ bit was checked by jdufault@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_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 jdufault@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.
Patchset #8 (id:240001) has been deleted
On 2017/01/17 21:42:01, Devlin (slow) wrote: > Nice! A couple small nits. I'd also like to patch this in locally and see the > documentation changes, but am remote for this week. Can this wait until next > week to land? If not, I can figure something out to see how it looks. Sure, waiting until next week should be okay.
https://codereview.chromium.org/2618493002/diff/180001/extensions/common/api/... File extensions/common/api/_manifest_features.json (right): https://codereview.chromium.org/2618493002/diff/180001/extensions/common/api/... extensions/common/api/_manifest_features.json:11: "channel": "stable", On 2017/01/17 21:42:01, Devlin (slow) wrote: > Let's not launch this to stable just yet - it's usually good practice to have it > in dev for awhile and let people fool around with it before we finalize > everything and ship it. How long of an incubation period? The changes here seem pretty low-risk, and the user can already select custom Android apps, so it would be nice to have feature parity. Launching directly to stable seems reasonable to me since we will have the normal test cycle, but I'm not familiar with the typical extension API process. Also, changing this value to "dev" causes the unit tests to fail. Any idea why? https://codereview.chromium.org/2618493002/diff/180001/extensions/common/api/... extensions/common/api/_manifest_features.json:13: }, On 2017/01/17 21:42:01, Devlin (slow) wrote: > should we restrict this to chromeos? Done. https://codereview.chromium.org/2618493002/diff/180001/extensions/common/mani... File extensions/common/manifest_handlers/action_handlers_handler.cc (right): https://codereview.chromium.org/2618493002/diff/180001/extensions/common/mani... extensions/common/manifest_handlers/action_handlers_handler.cc:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. On 2017/01/17 21:42:01, Devlin (slow) wrote: > ditto Done. https://codereview.chromium.org/2618493002/diff/180001/extensions/common/mani... extensions/common/manifest_handlers/action_handlers_handler.cc:38: auto info = base::MakeUnique<ActionHandlersInfo>(); On 2017/01/17 21:42:01, Devlin (slow) wrote: > nit: initialize this after line 44 Done. https://codereview.chromium.org/2618493002/diff/180001/extensions/common/mani... File extensions/common/manifest_handlers/action_handlers_handler.h (right): https://codereview.chromium.org/2618493002/diff/180001/extensions/common/mani... extensions/common/manifest_handlers/action_handlers_handler.h:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. On 2017/01/17 21:42:01, Devlin (slow) wrote: > nit: no (c) Done. https://codereview.chromium.org/2618493002/diff/180001/extensions/common/mani... File extensions/common/manifest_handlers/action_handlers_handler_unittest.cc (right): https://codereview.chromium.org/2618493002/diff/180001/extensions/common/mani... extensions/common/manifest_handlers/action_handlers_handler_unittest.cc:58: auto none = LoadAndExpectSuccess(CreateManifest("[]")); On 2017/01/17 21:42:01, Devlin (slow) wrote: > nit: It's not clear from the callsite what the return result here is, so let's > not use auto Done.
ping rdelvin.cronin@
lgtm with a few nits and changing the channel to dev https://codereview.chromium.org/2618493002/diff/180001/extensions/common/api/... File extensions/common/api/_manifest_features.json (right): https://codereview.chromium.org/2618493002/diff/180001/extensions/common/api/... extensions/common/api/_manifest_features.json:11: "channel": "stable", On 2017/01/20 20:31:47, jdufault wrote: > On 2017/01/17 21:42:01, Devlin (slow) wrote: > > Let's not launch this to stable just yet - it's usually good practice to have > it > > in dev for awhile and let people fool around with it before we finalize > > everything and ship it. > > How long of an incubation period? The changes here seem pretty low-risk, and the > user can already select custom Android apps, so it would be nice to have feature > parity. Launching directly to stable seems reasonable to me since we will have > the normal test cycle, but I'm not familiar with the typical extension API > process. > > Also, changing this value to "dev" causes the unit tests to fail. Any idea why? Mostly, we want the consumers of this API to have a chance to try it out and see if it works, and be able to make API changes if it doesn't. Once we document that it's available on stable, we try very hard to never change the API in a non-backwards compatible way. Re why the unittest fails, you probably need to set the channel in the test. See also ScopedCurrentChannel (https://cs.chromium.org/chromium/src/extensions/common/features/feature_chann...). https://codereview.chromium.org/2618493002/diff/260001/chrome/browser/chromeo... File chrome/browser/chromeos/note_taking_helper_unittest.cc (right): https://codereview.chromium.org/2618493002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/note_taking_helper_unittest.cc:364: const extensions::ExtensionId kNewNoteId = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"; drive-by nit: prefer crx_file::id_util::GenerateId() (and pass in a seed to ensure the ids are different) https://codereview.chromium.org/2618493002/diff/260001/chrome/common/extensio... File chrome/common/extensions/docs/templates/articles/manifest/action_handlers.html (right): https://codereview.chromium.org/2618493002/diff/260001/chrome/common/extensio... chrome/common/extensions/docs/templates/articles/manifest/action_handlers.html:5: or intents the application supports. An action is essentially an alternative nit: maybe "...which user actions or intents the application supports; these can serve as alternate launch points for your application." ? https://codereview.chromium.org/2618493002/diff/260001/chrome/common/extensio... chrome/common/extensions/docs/templates/articles/manifest/action_handlers.html:6: launch point. Maybe call out "This API is only available on ChromeOS."? https://codereview.chromium.org/2618493002/diff/260001/chrome/common/extensio... chrome/common/extensions/docs/templates/articles/manifest/action_handlers.html:10: This list contains any of the <code>ActionType</code> values; see the nit: maybe "This list contains one or more of the <code>ActionType</code> values specified in the <code>ActionType</code> entry of <a href="../app_runtime#event-onLaunched">app.runtime.onLaunched</a>." ?
The CQ bit was checked by jdufault@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: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) 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 jdufault@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/2618493002/diff/260001/chrome/browser/chromeo... File chrome/browser/chromeos/note_taking_helper_unittest.cc (right): https://codereview.chromium.org/2618493002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/note_taking_helper_unittest.cc:364: const extensions::ExtensionId kNewNoteId = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"; On 2017/01/30 21:19:59, Devlin wrote: > drive-by nit: prefer crx_file::id_util::GenerateId() (and pass in a seed to > ensure the ids are different) Done. https://codereview.chromium.org/2618493002/diff/260001/chrome/common/extensio... File chrome/common/extensions/docs/templates/articles/manifest/action_handlers.html (right): https://codereview.chromium.org/2618493002/diff/260001/chrome/common/extensio... chrome/common/extensions/docs/templates/articles/manifest/action_handlers.html:5: or intents the application supports. An action is essentially an alternative On 2017/01/30 21:20:00, Devlin wrote: > nit: maybe > "...which user actions or intents the application supports; these can serve as > alternate launch points for your application." > ? Done. https://codereview.chromium.org/2618493002/diff/260001/chrome/common/extensio... chrome/common/extensions/docs/templates/articles/manifest/action_handlers.html:6: launch point. On 2017/01/30 21:19:59, Devlin wrote: > Maybe call out "This API is only available on ChromeOS."? Done. https://codereview.chromium.org/2618493002/diff/260001/chrome/common/extensio... chrome/common/extensions/docs/templates/articles/manifest/action_handlers.html:10: This list contains any of the <code>ActionType</code> values; see the On 2017/01/30 21:19:59, Devlin wrote: > nit: maybe > "This list contains one or more of the <code>ActionType</code> values specified > in the <code>ActionType</code> entry of <a > href="../app_runtime#event-onLaunched">app.runtime.onLaunched</a>." > ? Done.
The CQ bit was checked by jdufault@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from derat@chromium.org, rdevlin.cronin@chromium.org Link to the patchset: https://codereview.chromium.org/2618493002/#ps320001 (title: "Make //components/version_info:version_info dep explicit")
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": 320001, "attempt_start_ts": 1486153794545840,
"parent_rev": "295bbf67dae591db27bd7c0582063f54d53b0c07", "commit_rev":
"c903a542cee2a0a794fea8753864f81ed62512e7"}
Message was sent while issue was closed.
Description was changed from ========== Chrome app manifest support for action handlers. BUG=657139 ========== to ========== Chrome app manifest support for action handlers. BUG=657139 Review-Url: https://codereview.chromium.org/2618493002 Cr-Commit-Position: refs/heads/master@{#448061} Committed: https://chromium.googlesource.com/chromium/src/+/c903a542cee2a0a794fea8753864... ==========
Message was sent while issue was closed.
Committed patchset #11 (id:320001) as https://chromium.googlesource.com/chromium/src/+/c903a542cee2a0a794fea8753864... |
