Move processes API to dev channel

Move processes API to dev channel

BUG=259490
TBR=sky@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=217559

[pals, 2013-08-07 13:25:19]

+isherman for histograms.xml
+nasko, mpcomplete for the rest

PTAL, Thanks

[pals, 2013-08-07 13:31:54]

third_party/webpagereplay/perftracker/extension uses experimental.processes api.
We should probably update this extension.

[nasko, 2013-08-07 15:41:59]

Thanks for making this change!
The overall comment is that we don't need a new permission, since the API is
supposed to require only "tabs" permission. 

Including creis@ for his view, but I think this API shouldn't be accessible for
apps, since they can use methods in this API to kill other apps or get
information about them.

https://codereview.chromium.org/22584002/diff/1/chrome/common/extensions/api/...
File chrome/common/extensions/api/_api_features.json (right):

https://codereview.chromium.org/22584002/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/_api_features.json:390: "dependencies":
["permission:processes"],
We don't need another permission for this API. It's designed to require tabs
permission.

https://codereview.chromium.org/22584002/diff/1/chrome/common/extensions/api/...
File chrome/common/extensions/api/_permission_features.json (right):

https://codereview.chromium.org/22584002/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/_permission_features.json:480: "extension_types":
["extension", "packaged_app", "platform_app"]
I'm not sure this API should be accessible to apps, since they are supposed to
be isolated from the browser. Using this API, one app can kill another app's
process.

https://codereview.chromium.org/22584002/diff/1/chrome/common/extensions/docs...
File
chrome/common/extensions/docs/examples/api/processes/process_monitor/manifest.json
(right):

https://codereview.chromium.org/22584002/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/examples/api/processes/process_monitor/manifest.json:6:
"processes", "tabs"
No need for new permissions, tabs should cover both.

https://codereview.chromium.org/22584002/diff/1/chrome/common/extensions/docs...
File
chrome/common/extensions/docs/examples/api/processes/process_monitor/popup.js
(right):

https://codereview.chromium.org/22584002/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/examples/api/processes/process_monitor/popup.js:80:
document.addEventListener('DOMContentLoaded', init);
nit: No white space at the end of the line.

https://codereview.chromium.org/22584002/diff/1/chrome/common/extensions/docs...
File
chrome/common/extensions/docs/examples/api/processes/show_tabs/manifest.json
(right):

https://codereview.chromium.org/22584002/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/examples/api/processes/show_tabs/manifest.json:6:
"processes", "tabs", "chrome://favicon/*"
No need for new permission here.

https://codereview.chromium.org/22584002/diff/1/chrome/test/data/extensions/a...
File chrome/test/data/extensions/api_test/processes/api/manifest.json (right):

https://codereview.chromium.org/22584002/diff/1/chrome/test/data/extensions/a...
chrome/test/data/extensions/api_test/processes/api/manifest.json:9:
"permissions": ["processes", "tabs"]
No need for new permission.

https://codereview.chromium.org/22584002/diff/1/chrome/test/data/extensions/a...
File chrome/test/data/extensions/api_test/processes/onupdated/manifest.json
(right):

https://codereview.chromium.org/22584002/diff/1/chrome/test/data/extensions/a...
chrome/test/data/extensions/api_test/processes/onupdated/manifest.json:9:
"permissions": [ "tabs", "processes" ]
No need for new permission.

[Charlie Reis, 2013-08-07 17:04:04]

https://codereview.chromium.org/22584002/diff/1/chrome/common/extensions/api/...
File chrome/common/extensions/api/_permission_features.json (right):

https://codereview.chromium.org/22584002/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/_permission_features.json:480: "extension_types":
["extension", "packaged_app", "platform_app"]
On 2013/08/07 15:42:00, nasko wrote:
> I'm not sure this API should be accessible to apps, since they are supposed to
> be isolated from the browser. Using this API, one app can kill another app's
> process.

Agreed.  It should only be accessible to extensions.

[Matt Perry, 2013-08-07 22:26:35]

On 2013/08/07 15:41:59, nasko wrote:
> Thanks for making this change!
> The overall comment is that we don't need a new permission, since the API is
> supposed to require only "tabs" permission. 

All APIs should require a permission. Whether that permission is associated with
a visible warning message to the user is a different story. Assuming we're
equating this with "tabs", this permission should have the same warning message
as "tabs".

> 
> Including creis@ for his view, but I think this API shouldn't be accessible
for
> apps, since they can use methods in this API to kill other apps or get
> information about them.
> 
>
https://codereview.chromium.org/22584002/diff/1/chrome/common/extensions/api/...
> File chrome/common/extensions/api/_api_features.json (right):
> 
>
https://codereview.chromium.org/22584002/diff/1/chrome/common/extensions/api/...
> chrome/common/extensions/api/_api_features.json:390: "dependencies":
> ["permission:processes"],
> We don't need another permission for this API. It's designed to require tabs
> permission.
> 
>
https://codereview.chromium.org/22584002/diff/1/chrome/common/extensions/api/...
> File chrome/common/extensions/api/_permission_features.json (right):
> 
>
https://codereview.chromium.org/22584002/diff/1/chrome/common/extensions/api/...
> chrome/common/extensions/api/_permission_features.json:480: "extension_types":
> ["extension", "packaged_app", "platform_app"]
> I'm not sure this API should be accessible to apps, since they are supposed to
> be isolated from the browser. Using this API, one app can kill another app's
> process.
> 
>
https://codereview.chromium.org/22584002/diff/1/chrome/common/extensions/docs...
> File
>
chrome/common/extensions/docs/examples/api/processes/process_monitor/manifest.json
> (right):
> 
>
https://codereview.chromium.org/22584002/diff/1/chrome/common/extensions/docs...
>
chrome/common/extensions/docs/examples/api/processes/process_monitor/manifest.json:6:
> "processes", "tabs"
> No need for new permissions, tabs should cover both.
> 
>
https://codereview.chromium.org/22584002/diff/1/chrome/common/extensions/docs...
> File
> chrome/common/extensions/docs/examples/api/processes/process_monitor/popup.js
> (right):
> 
>
https://codereview.chromium.org/22584002/diff/1/chrome/common/extensions/docs...
>
chrome/common/extensions/docs/examples/api/processes/process_monitor/popup.js:80:
> document.addEventListener('DOMContentLoaded', init);
> nit: No white space at the end of the line.
> 
>
https://codereview.chromium.org/22584002/diff/1/chrome/common/extensions/docs...
> File
> chrome/common/extensions/docs/examples/api/processes/show_tabs/manifest.json
> (right):
> 
>
https://codereview.chromium.org/22584002/diff/1/chrome/common/extensions/docs...
>
chrome/common/extensions/docs/examples/api/processes/show_tabs/manifest.json:6:
> "processes", "tabs", "chrome://favicon/*"
> No need for new permission here.
> 
>
https://codereview.chromium.org/22584002/diff/1/chrome/test/data/extensions/a...
> File chrome/test/data/extensions/api_test/processes/api/manifest.json (right):
> 
>
https://codereview.chromium.org/22584002/diff/1/chrome/test/data/extensions/a...
> chrome/test/data/extensions/api_test/processes/api/manifest.json:9:
> "permissions": ["processes", "tabs"]
> No need for new permission.
> 
>
https://codereview.chromium.org/22584002/diff/1/chrome/test/data/extensions/a...
> File chrome/test/data/extensions/api_test/processes/onupdated/manifest.json
> (right):
> 
>
https://codereview.chromium.org/22584002/diff/1/chrome/test/data/extensions/a...
> chrome/test/data/extensions/api_test/processes/onupdated/manifest.json:9:
> "permissions": [ "tabs", "processes" ]
> No need for new permission.

[nasko, 2013-08-07 22:33:22]

On 2013/08/07 22:26:35, Matt Perry wrote:
> On 2013/08/07 15:41:59, nasko wrote:
> > Thanks for making this change!
> > The overall comment is that we don't need a new permission, since the API is
> > supposed to require only "tabs" permission. 
> 
> All APIs should require a permission. Whether that permission is associated
with
> a visible warning message to the user is a different story. Assuming we're
> equating this with "tabs", this permission should have the same warning
message
> as "tabs".

The goal was not to introduce new permissions and since extension having "tabs"
permission already was almost as powerful, we designed it this way.
If having a separate permission a requirement now, then I'd rather have
meaningful explanation.

[Matt Perry, 2013-08-07 22:36:20]

On 2013/08/07 22:33:22, nasko wrote:
> On 2013/08/07 22:26:35, Matt Perry wrote:
> > On 2013/08/07 15:41:59, nasko wrote:
> > > Thanks for making this change!
> > > The overall comment is that we don't need a new permission, since the API
is
> > > supposed to require only "tabs" permission. 
> > 
> > All APIs should require a permission. Whether that permission is associated
> with
> > a visible warning message to the user is a different story. Assuming we're
> > equating this with "tabs", this permission should have the same warning
> message
> > as "tabs".
> 
> The goal was not to introduce new permissions and since extension having
"tabs"
> permission already was almost as powerful, we designed it this way.
> If having a separate permission a requirement now, then I'd rather have
> meaningful explanation.

Extensions requires a permission entry for every API. For one, this helps us
keep track of which APIs are in use in the wild. (We can just inspect the
manifest file and see what APIs an extension uses.)

Keep in mind that a new permission isn't the same as a new warning message. An
extension that asks for "processes" and "tabs" could easily have the same
warnings in the install prompt as one that asks for just "tabs".

[Matt Perry, 2013-08-07 22:37:37]

One additional thing we need to do is contact any extension developers that are
using this API, and let them know of the upcoming change. Nasko, you seem to
know a bit about that - can you contact the extension developers?

https://codereview.chromium.org/22584002/diff/1/tools/metrics/histograms/hist...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/22584002/diff/1/tools/metrics/histograms/hist...
tools/metrics/histograms/histograms.xml:16842: +  <int value="37"
label="DELETED_EXPERIMENTAL_PROCESSES_GETPROCESSINFO"/>
Rename the existing entry rather than deleting it and adding a new one.

[Matt Perry, 2013-08-07 22:40:02]

Also, add a new APIPermission and an entry to chrome_api_permissions.cc similar
to the "tabs" entry here:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte...

[nasko, 2013-08-07 22:57:31]

On 2013/08/07 22:37:37, Matt Perry wrote:
> One additional thing we need to do is contact any extension developers that
are
> using this API, and let them know of the upcoming change. Nasko, you seem to
> know a bit about that - can you contact the extension developers?

I will ping the one developer I know about, but can't cover them all. Does dev
channel restriction mean it won't work on canary? Or are those considered the
same?

[Matt Perry, 2013-08-07 23:01:45]

On 2013/08/07 22:57:31, nasko wrote:
> On 2013/08/07 22:37:37, Matt Perry wrote:
> > One additional thing we need to do is contact any extension developers that
> are
> > using this API, and let them know of the upcoming change. Nasko, you seem to
> > know a bit about that - can you contact the extension developers?
> 
> I will ping the one developer I know about, but can't cover them all. Does dev
> channel restriction mean it won't work on canary? Or are those considered the
> same?

Channel restrictions work on that channel and anything more cutting edge. So Dev
== Dev+canary.

[pals, 2013-08-08 06:30:51]

Fixed the review comments. Please review.

-Added APIPermission same as "tabs"
-Processes APIs can only be used in extensions

https://codereview.chromium.org/22584002/diff/1/chrome/common/extensions/api/...
File chrome/common/extensions/api/_permission_features.json (right):

https://codereview.chromium.org/22584002/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/_permission_features.json:480: "extension_types":
["extension", "packaged_app", "platform_app"]
On 2013/08/07 17:04:04, creis wrote:
> On 2013/08/07 15:42:00, nasko wrote:
> > I'm not sure this API should be accessible to apps, since they are supposed
to
> > be isolated from the browser. Using this API, one app can kill another app's
> > process.
> 
> Agreed.  It should only be accessible to extensions.

Done.

https://codereview.chromium.org/22584002/diff/1/tools/metrics/histograms/hist...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/22584002/diff/1/tools/metrics/histograms/hist...
tools/metrics/histograms/histograms.xml:16842: +  <int value="37"
label="DELETED_EXPERIMENTAL_PROCESSES_GETPROCESSINFO"/>
On 2013/08/07 22:37:37, Matt Perry wrote:
> Rename the existing entry rather than deleting it and adding a new one.

Done.

[Ilya Sherman, 2013-08-08 07:59:32]

histograms.xml lgtm

[nasko, 2013-08-08 15:36:09]

lgtm with a nit.

https://codereview.chromium.org/22584002/diff/19001/chrome/common/extensions/...
File
chrome/common/extensions/docs/examples/api/processes/process_monitor/popup.js
(right):

https://codereview.chromium.org/22584002/diff/19001/chrome/common/extensions/...
chrome/common/extensions/docs/examples/api/processes/process_monitor/popup.js:80:
document.addEventListener('DOMContentLoaded', init);
nit: The whitespace is still around.

[Matt Perry, 2013-08-08 20:32:14]

lgtm

[commit-bot: I haz the power, 2013-08-12 05:17:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sanjoy.pal@samsung.com/22584002/19001

[commit-bot: I haz the power, 2013-08-12 05:18:02]

Failed to apply patch for
chrome/browser/extensions/activity_log/api_name_constants.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  can't find file to patch at input line 6
  Perhaps you used the wrong -p or --strip option?
  The text leading up to this was:
  --------------------------
  |Index: chrome/browser/extensions/activity_log/api_name_constants.h
  |diff --git a/chrome/browser/extensions/activity_log/api_name_constants.h
b/chrome/browser/extensions/activity_log/api_name_constants.h
  |index
d745b667956e61691f946eadae5cde1c88721c2b..352b62fe0be46fce6d5b206f8c8209f161f2a6b2
100644
  |--- a/chrome/browser/extensions/activity_log/api_name_constants.h
  |+++ b/chrome/browser/extensions/activity_log/api_name_constants.h
  --------------------------
  No file to patch.  Skipping patch.
  1 out of 1 hunk ignored

Patch:       chrome/browser/extensions/activity_log/api_name_constants.h
Index: chrome/browser/extensions/activity_log/api_name_constants.h
diff --git a/chrome/browser/extensions/activity_log/api_name_constants.h
b/chrome/browser/extensions/activity_log/api_name_constants.h
index
d745b667956e61691f946eadae5cde1c88721c2b..352b62fe0be46fce6d5b206f8c8209f161f2a6b2
100644
--- a/chrome/browser/extensions/activity_log/api_name_constants.h
+++ b/chrome/browser/extensions/activity_log/api_name_constants.h
@@ -70,13 +70,13 @@ const char* const kNames[] = {
     "experimental.identity.launchWebAuthFlow", "infobars.show",
     "removed.experimental.mediaGalleries.assembleMediaFile",
     "removed.experimental.mediaGalleries.extractEmbeddedThumbnails",
-    "experimental.processes.getProcessIdForTab",
-    "experimental.processes.getProcessInfo",
-    "experimental.processes.onCreated", "experimental.processes.onExited",
-    "experimental.processes.onUnresponsive",
-    "experimental.processes.onUpdated",
-    "experimental.processes.onUpdatedWithMemory",
-    "experimental.processes.terminate",
+    "processes.getProcessIdForTab",
+    "processes.getProcessInfo",
+    "processes.onCreated", "processes.onExited",
+    "processes.onUnresponsive",
+    "processes.onUpdated",
+    "processes.onUpdatedWithMemory",
+    "processes.terminate",
     "experimental.record.captureURLs", "experimental.record.replayURLs",
     "experimental.speechInput.isRecording", "experimental.speechInput.onError",
     "experimental.speechInput.onResult", "experimental.speechInput.onSoundEnd",

[pals, 2013-08-12 05:22:35]

Hi nasko,

Please see my comment below. please let me know if its OK to commit this CL.
Thank you

https://codereview.chromium.org/22584002/diff/19001/chrome/common/extensions/...
File
chrome/common/extensions/docs/examples/api/processes/process_monitor/popup.js
(right):

https://codereview.chromium.org/22584002/diff/19001/chrome/common/extensions/...
chrome/common/extensions/docs/examples/api/processes/process_monitor/popup.js:80:
document.addEventListener('DOMContentLoaded', init);
On 2013/08/08 15:36:09, nasko wrote:
> nit: The whitespace is still around.
I am not sure, how the above line is showing this diff as I haven't touched this
line. I guess its because the statement is outside any block and it is at the
end of file as the git diff is showing "\ No newline at the end of file"

[commit-bot: I haz the power, 2013-08-12 06:51:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sanjoy.pal@samsung.com/22584002/35001

[commit-bot: I haz the power, 2013-08-12 09:54:29]

Retried try job too often on mac_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-08-12 11:28:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sanjoy.pal@samsung.com/22584002/61001

[commit-bot: I haz the power, 2013-08-12 19:59:26]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-08-13 05:35:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sanjoy.pal@samsung.com/22584002/61001

[commit-bot: I haz the power, 2013-08-13 05:35:45]

Failed to apply patch for
chrome/browser/extensions/extension_function_histogram_value.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/extensions/extension_function_histogram_value.h
  Hunk #4 FAILED at 590.
  1 out of 4 hunks FAILED -- saving rejects to file
chrome/browser/extensions/extension_function_histogram_value.h.rej

Patch:       chrome/browser/extensions/extension_function_histogram_value.h
Index: chrome/browser/extensions/extension_function_histogram_value.h
diff --git a/chrome/browser/extensions/extension_function_histogram_value.h
b/chrome/browser/extensions/extension_function_histogram_value.h
index
1defcb838be3db551ddd51a0e934de79283c5ecd..171b2664d1050f9bb3634db05db36d2714381e85
100644
--- a/chrome/browser/extensions/extension_function_histogram_value.h
+++ b/chrome/browser/extensions/extension_function_histogram_value.h
@@ -96,7 +96,7 @@ enum HistogramValue {
   AUTOTESTPRIVATE_RESTART,
   USB_CLAIMINTERFACE,
   MEDIAPLAYERPRIVATE_SETWINDOWHEIGHT,
-  EXPERIMENTAL_PROCESSES_GETPROCESSINFO,
+  DELETED_EXPERIMENTAL_PROCESSES_GETPROCESSINFO,
   HISTORY_GETVISITS,
   SOCKET_BIND,
   TABS_MOVE,
@@ -161,7 +161,7 @@ enum HistogramValue {
   WINDOWS_CREATE,
   DEVELOPERPRIVATE_LOADUNPACKED,
   DELETED_DOWNLOADS_SETDESTINATION,
-  EXPERIMENTAL_PROCESSES_GETPROCESSIDFORTAB,
+  DELETED_EXPERIMENTAL_PROCESSES_GETPROCESSIDFORTAB,
   BOOKMARKS_GETCHILDREN,
   BROWSERACTION_GETTITLE,
   TERMINALPRIVATE_OPENTERMINALPROCESS,
@@ -354,7 +354,7 @@ enum HistogramValue {
   EXPERIMENTAL_APP_NOTIFY,
   METRICSPRIVATE_RECORDLONGTIME,
   SOCKET_READ,
-  EXPERIMENTAL_PROCESSES_TERMINATE,
+  DELETED_EXPERIMENTAL_PROCESSES_TERMINATE,
   METRICSPRIVATE_RECORDTIME,
   BOOKMARKMANAGERPRIVATE_GETSTRINGS,
   USB_ISOCHRONOUSTRANSFER,
@@ -590,6 +590,9 @@ enum HistogramValue {
   LOGPRIVATE_GETHISTORICAL,
   EXPERIMENTAL_INPUT_VIRTUALKEYBOARD_MOVECURSOR,
   METRICSPRIVATE_GETVARIATIONPARAMS,
+  PROCESSES_GETPROCESSINFO,
+  PROCESSES_GETPROCESSIDFORTAB,
+  PROCESSES_TERMINATE,
   ENUM_BOUNDARY // Last entry: Add new entries above.
 };

[commit-bot: I haz the power, 2013-08-14 13:22:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sanjoy.pal@samsung.com/22584002/86001

[commit-bot: I haz the power, 2013-08-14 15:30:19]

Change committed as 217559