|
|
DescriptionAdd deprecation message in chrome.pushMessaging API
Deprecation messages are added in:
* getChannelId function - displayed to console
* documentation of chrome.pushMessaging/GCM for Chrome V1
BUG=403952
Committed: https://crrev.com/0dcab3d52efddd480376816797a8257a371995b9
Cr-Commit-Position: refs/heads/master@{#293539}
Patch Set 1 #
Total comments: 12
Patch Set 2 : Updates based on code review feedback #Patch Set 3 : Applying update setting deprecation to Chrome 38, removal to Chrome 40. #
Total comments: 4
Patch Set 4 : Final updates to the documentation in idl #Patch Set 5 : Updating the API removal milestone #
Messages
Total messages: 24 (7 generated)
Ben, Mike, please take a look. I want both of you to confirm that wording makes sense. Would there be a way to add a message in event handler (without adding a lot of code there?)
https://codereview.chromium.org/476883003/diff/1/chrome/browser/extensions/ap... File chrome/browser/extensions/api/push_messaging/push_messaging_api.cc (right): https://codereview.chromium.org/476883003/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:100: WriteToConsole(content::CONSOLE_MESSAGE_LEVEL_WARNING, kDeprecationMessage); How often do apps call this method? Because if it's being called often - for whatever reason - it could get really spammy. https://codereview.chromium.org/476883003/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:101: Also: we should add an install warning for when the permission is requested. I can't remember where this is all done, yoz? https://codereview.chromium.org/476883003/diff/1/chrome/common/extensions/api... File chrome/common/extensions/api/push_messaging.idl (right): https://codereview.chromium.org/476883003/diff/1/chrome/common/extensions/api... chrome/common/extensions/api/push_messaging.idl:7: // Switch to <a href=gcm.html>chrome.gcm API</a> to take advantage of We actually have a [deprecated] annotation that generates code like: https://developer.chrome.com/extensions/tabs#property-Tab-selected I'm not sure if it works on namespaces, but it probably should. If not you could also apply it to getChannelId and onMessage for good measure: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte... https://codereview.chromium.org/476883003/diff/1/chrome/common/extensions/doc... File chrome/common/extensions/docs/templates/articles/cloudMessagingV1.html (right): https://codereview.chromium.org/476883003/diff/1/chrome/common/extensions/doc... chrome/common/extensions/docs/templates/articles/cloudMessagingV1.html:5: Google Cloud Messaging for Chrome V1 is deprecated. You might want to say "Since Chrome XX". Also if I'm to nit, deprecated means "don't use it" but you're talking about actually deleting it. Maybe the language should be suitably stronger.
https://codereview.chromium.org/476883003/diff/1/chrome/browser/extensions/ap... File chrome/browser/extensions/api/push_messaging/push_messaging_api.cc (right): https://codereview.chromium.org/476883003/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:101: On 2014/08/14 22:16:08, kalman wrote: > Also: we should add an install warning for when the permission is requested. I > can't remember where this is all done, yoz? chrome_api_permissions? I'm not sure I follow "install warning"... "when requested"
https://codereview.chromium.org/476883003/diff/1/chrome/browser/extensions/ap... File chrome/browser/extensions/api/push_messaging/push_messaging_api.cc (right): https://codereview.chromium.org/476883003/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:101: On 2014/08/15 01:40:13, Yoyo Zhou wrote: > On 2014/08/14 22:16:08, kalman wrote: > > Also: we should add an install warning for when the permission is requested. I > > can't remember where this is all done, yoz? > > chrome_api_permissions? > > I'm not sure I follow "install warning"... "when requested" I mean the same warning that shows up if you request a permission that doesn't exist, or append to InstallWarnings from a ManifestHandler. Is there an idiomatic way to append arbitrary data to that?
https://codereview.chromium.org/476883003/diff/1/chrome/browser/extensions/ap... File chrome/browser/extensions/api/push_messaging/push_messaging_api.cc (right): https://codereview.chromium.org/476883003/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:101: On 2014/08/15 15:54:17, kalman wrote: > On 2014/08/15 01:40:13, Yoyo Zhou wrote: > > On 2014/08/14 22:16:08, kalman wrote: > > > Also: we should add an install warning for when the permission is requested. > I > > > can't remember where this is all done, yoz? > > > > chrome_api_permissions? > > > > I'm not sure I follow "install warning"... "when requested" > > I mean the same warning that shows up if you request a permission that doesn't > exist, or append to InstallWarnings from a ManifestHandler. Is there an > idiomatic way to append arbitrary data to that? I dunno, I would just try calling extension->AddInstallWarning. Given that we have the error console, though, I don't know if adding the install warning will help much additionally.
Ben, I applied comments and looked into how to get the install time message done. For now there is no easy way without making some more changes in feature availability or permission parser. We could go easy route and single out pushMessaging, or implement long term solution, which would allow us to mark a given API deprecated, giving specific availability result. (think: IS_AVAILABLE_BUT_DEPRECATED) As for writing to console, I am seeing it used specifically for deprecation, so I didn't really change it. It would be best however if we could issue a message based on [deprecated] tag, as that would sweep all of the deprecated features. (probably a separate feature altogether). Let me know what you think. https://codereview.chromium.org/476883003/diff/1/chrome/browser/extensions/ap... File chrome/browser/extensions/api/push_messaging/push_messaging_api.cc (right): https://codereview.chromium.org/476883003/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:100: WriteToConsole(content::CONSOLE_MESSAGE_LEVEL_WARNING, kDeprecationMessage); On 2014/08/14 22:16:08, kalman wrote: > How often do apps call this method? Because if it's being called often - for > whatever reason - it could get really spammy. I am seeing it used very rarely (2 other APIs). app_window_api.cc:184 is actual deprecation message alarms_api.cc:114 Not sure for what reason. https://codereview.chromium.org/476883003/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:101: On 2014/08/15 19:25:31, Yoyo Zhou wrote: > On 2014/08/15 15:54:17, kalman wrote: > > On 2014/08/15 01:40:13, Yoyo Zhou wrote: > > > On 2014/08/14 22:16:08, kalman wrote: > > > > Also: we should add an install warning for when the permission is > requested. > > I > > > > can't remember where this is all done, yoz? > > > > > > chrome_api_permissions? > > > > > > I'm not sure I follow "install warning"... "when requested" > > > > I mean the same warning that shows up if you request a permission that doesn't > > exist, or append to InstallWarnings from a ManifestHandler. Is there an > > idiomatic way to append arbitrary data to that? > > I dunno, I would just try calling extension->AddInstallWarning. > > Given that we have the error console, though, I don't know if adding the install > warning will help much additionally. We could add a check for deprecated permissions in ParseHelper in permissions_parser.cc Feature::Availability availability = feature->IsAvailableToExtension(extension); 149: if (!availability.is_available()) { //stuff here (issues warning and removes permission) continue; } if (!availability.is_deprecated()) { extension->AddInstallWarning( InstallWarning(availability.message(), feature->name())); } Of course that would require extension to Feature::Availability and adding a bit to the manifest. Ben do you think it is reasonable to do it now? Or can we spent some time and do it right? I am sure it would be useful if we want to deprecate stuff in future. https://codereview.chromium.org/476883003/diff/1/chrome/common/extensions/api... File chrome/common/extensions/api/push_messaging.idl (right): https://codereview.chromium.org/476883003/diff/1/chrome/common/extensions/api... chrome/common/extensions/api/push_messaging.idl:7: // Switch to <a href=gcm.html>chrome.gcm API</a> to take advantage of On 2014/08/14 22:16:08, kalman wrote: > We actually have a [deprecated] annotation that generates code like: > > https://developer.chrome.com/extensions/tabs#property-Tab-selected > > I'm not sure if it works on namespaces, but it probably should. If not you could > also apply it to getChannelId and onMessage for good measure: > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte... Done. https://codereview.chromium.org/476883003/diff/1/chrome/common/extensions/doc... File chrome/common/extensions/docs/templates/articles/cloudMessagingV1.html (right): https://codereview.chromium.org/476883003/diff/1/chrome/common/extensions/doc... chrome/common/extensions/docs/templates/articles/cloudMessagingV1.html:5: Google Cloud Messaging for Chrome V1 is deprecated. On 2014/08/14 22:16:08, kalman wrote: > You might want to say "Since Chrome XX". Also if I'm to nit, deprecated means > "don't use it" but you're talking about actually deleting it. Maybe the language > should be suitably stronger. Done.
What you have is fine, so lgtm if you want to submit, but if you're keen to add the deprecated annotation to the features then that'd be awesome, and it would even let us generate the deprecated stuff in the server - avoid needing to add it all over the place. https://codereview.chromium.org/476883003/diff/1/chrome/browser/extensions/ap... File chrome/browser/extensions/api/push_messaging/push_messaging_api.cc (right): https://codereview.chromium.org/476883003/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:101: On 2014/08/20 22:46:46, fgorski wrote: > On 2014/08/15 19:25:31, Yoyo Zhou wrote: > > On 2014/08/15 15:54:17, kalman wrote: > > > On 2014/08/15 01:40:13, Yoyo Zhou wrote: > > > > On 2014/08/14 22:16:08, kalman wrote: > > > > > Also: we should add an install warning for when the permission is > > requested. > > > I > > > > > can't remember where this is all done, yoz? > > > > > > > > chrome_api_permissions? > > > > > > > > I'm not sure I follow "install warning"... "when requested" > > > > > > I mean the same warning that shows up if you request a permission that > doesn't > > > exist, or append to InstallWarnings from a ManifestHandler. Is there an > > > idiomatic way to append arbitrary data to that? > > > > I dunno, I would just try calling extension->AddInstallWarning. > > > > Given that we have the error console, though, I don't know if adding the > install > > warning will help much additionally. > > We could add a check for deprecated permissions in ParseHelper in > permissions_parser.cc > Feature::Availability availability = > feature->IsAvailableToExtension(extension); > 149: > if (!availability.is_available()) { > //stuff here (issues warning and removes permission) > continue; > } > > if (!availability.is_deprecated()) { > extension->AddInstallWarning( > InstallWarning(availability.message(), feature->name())); > } > > Of course that would require extension to Feature::Availability and adding a bit > to the manifest. > > Ben do you think it is reasonable to do it now? Or can we spent some time and do > it right? I am sure it would be useful if we want to deprecate stuff in future. I've wanted this actually, a feature file annotation to say an entire API is deprecated (would be useful for the socket API as well, for example). What you suggest sounds perfect. https://codereview.chromium.org/476883003/diff/40001/chrome/common/extensions... File chrome/common/extensions/api/push_messaging.idl (right): https://codereview.chromium.org/476883003/diff/40001/chrome/common/extensions... chrome/common/extensions/api/push_messaging.idl:5: // The <code>chrome.pushMessaging</code> API is currently deprecated since No "currently". https://codereview.chromium.org/476883003/diff/40001/chrome/common/extensions... chrome/common/extensions/api/push_messaging.idl:36: [deprecated="Use $(ref:gcm) API"] Nit: seems like it should read either "the gcm API" or just "gcm", so "Use $(ref:gcm chrome.gcm)" or "$Use the $(ref:gcm chrome.gcm API)".
I will not be adding the feature annotation in this CL, but I can look into it separately if time allows next week. https://codereview.chromium.org/476883003/diff/40001/chrome/common/extensions... File chrome/common/extensions/api/push_messaging.idl (right): https://codereview.chromium.org/476883003/diff/40001/chrome/common/extensions... chrome/common/extensions/api/push_messaging.idl:5: // The <code>chrome.pushMessaging</code> API is currently deprecated since On 2014/08/21 00:36:33, kalman wrote: > No "currently". Done. https://codereview.chromium.org/476883003/diff/40001/chrome/common/extensions... chrome/common/extensions/api/push_messaging.idl:36: [deprecated="Use $(ref:gcm) API"] On 2014/08/21 00:36:33, kalman wrote: > Nit: seems like it should read either "the gcm API" or just "gcm", so "Use > $(ref:gcm chrome.gcm)" or "$Use the $(ref:gcm chrome.gcm API)". Done.
The CQ bit was checked by fgorski@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/fgorski@chromium.org/476883003/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_clang_dbg_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
The CQ bit was checked by fgorski@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/fgorski@chromium.org/476883003/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by fgorski@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/fgorski@chromium.org/476883003/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by fgorski@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/fgorski@chromium.org/476883003/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as 8b7a0e188a0d57e6214c8c463bbe61da3fabb361
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/0dcab3d52efddd480376816797a8257a371995b9 Cr-Commit-Position: refs/heads/master@{#293539} |