Bob Silverberg [:bsilverberg] Assignee
	Description • 8 years ago

Create a schema file for the management API. The Chrome docs for the API can be found at [1] and the Chrome schema can be found at [2].

[1] https://developer.chrome.com/extensions/management
[2] https://src.chromium.org/viewvc/chrome/trunk/src/chrome/common/extensions/api/management.json

	Bob Silverberg [:bsilverberg] Assignee
	Comment 1 • 8 years ago

Review commit: https://reviewboard.mozilla.org/r/63190/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63190/

	Bob Silverberg [:bsilverberg] Assignee
	Comment 2 • 8 years ago

https://reviewboard.mozilla.org/r/63190/#review60034

::: browser/components/extensions/schemas/management.json:1
(Diff revision 1)
> +// Copyright (c) 2012 The Chromium Authors. All rights reserved.

There are some things in the schema file that do not apply, some things that will never apply, and some that are likely in question. I've done my best to craft the schema with that in mind, but let me know if you think any of these should be handled differently.

	Andrew Swan [:aswan]
	Comment 3 • 8 years ago

https://reviewboard.mozilla.org/r/63190/#review60048

::: browser/components/extensions/schemas/management.json:23
(Diff revision 1)
> +    ]
> +  },
> +  {
> +    "namespace":"management",
> +    "description": "The <code>browser.management</code> API provides ways to manage the list of extensions that are installed and running.",
> +    "permissions": ["management"],

This needs to be added to individual methods since there are a few that don't need it (e.g. getSelf() )

::: browser/components/extensions/schemas/management.json:40
(Diff revision 1)
> +      {
> +        "id": "LaunchType",
> +        "type": "string",
> +        "enum": ["OPEN_AS_REGULAR_TAB", "OPEN_AS_PINNED_TAB", "OPEN_AS_WINDOW", "OPEN_FULL_SCREEN"],
> +        "description": "These are all possible app launch types. This type is not relevant to Firefox."
> +      },

Why have it in the schema if it is not relevant?

::: browser/components/extensions/schemas/management.json:107
(Diff revision 1)
> +          "isApp": {
> +            "description": "True if this is an app. Will always return false as Firefox does not support apps.",
> +            "type": "boolean",
> +            "deprecated": "Please use $(ref:management.ExtensionInfo.type)."
> +          },

If this is both deprecated and unsupported, lets just omit it

::: browser/components/extensions/schemas/management.json:116
(Diff revision 1)
> +          "appLaunchUrl": {
> +            "description": "The launch url (only present for apps, therefore will never be present).",
> +            "type": "string",
> +            "optional": true
> +          },

lets leave it out

::: browser/components/extensions/schemas/management.json:131
(Diff revision 1)
> +          "offlineEnabled": {
> +            "description": "Whether the extension declares that it supports offline.",
> +            "type": "boolean"
> +          },

I believe this is another app-specific thing?  If so lets leave it out

::: browser/components/extensions/schemas/management.json:167
(Diff revision 1)
> +          "launchType": {
> +            "description": "The app launch type (only present for apps, therefore will never be present).",
> +            "$ref": "LaunchType",
> +            "optional": true
> +          },
> +          "availableLaunchTypes": {
> +            "description": "The currently available launch types (only present for apps, therefore will never be present)",
> +            "type": "array",
> +            "optional": true,
> +            "items": {
> +              "$ref": "LaunchType"
> +            }
> +          }

lets leave these out

::: browser/components/extensions/schemas/management.json:184
(Diff revision 1)
> +      {
> +        "name": "getAll",
> +        "type": "function",
> +        "unsupported": true,
> +        "description": "Returns a list of information about installed extensions.",
> +        "async": "callback",
> +        "parameters": [
> +          {
> +            "name": "callback",
> +            "type": "function",
> +            "optional": true,
> +            "parameters": [
> +              {
> +                "type": "array",
> +                "name": "result",
> +                "items": {
> +                  "$ref": "ExtensionInfo"
> +                }
> +              }
> +            ]
> +          }
> +        ]
> +      },

I think we should leave this out until we have a good reason to add it (which I don't think we have yet, I realize you have some unaswered comments about this on a separate bug...)

::: browser/components/extensions/schemas/management.json:387
(Diff revision 1)
> +      {
> +        "name": "launchApp",
> +        "type": "function",
> +        "unsupported": true,
> +        "description": "Launches an application. Will never be supported on Firefox.",
> +        "async": "callback",
> +        "parameters": [
> +          {
> +            "name": "id",
> +            "type": "string",
> +            "description": "The extension id of the application."
> +          },
> +          {
> +            "name": "callback",
> +            "type": "function",
> +            "optional": true,
> +            "parameters": []
> +          }
> +        ]
> +      },
> +      {
> +        "name": "createAppShortcut",
> +        "type": "function",
> +        "unsupported": true,
> +        "description": "Display options to create shortcuts for an app. Will never be supported on Firefox.",
> +        "async": "callback",
> +        "parameters": [
> +          {
> +            "name": "id",
> +            "type": "string",
> +            "description": "This should be the id from an app item of $(ref:management.ExtensionInfo)."
> +          },
> +          {
> +            "name": "callback",
> +            "type": "function",
> +            "optional": true,
> +            "parameters": []
> +          }
> +        ]
> +      },
> +      {
> +        "name": "setLaunchType",
> +        "type": "function",
> +        "unsupported": true,
> +        "description": "Set the launch type of an app. Will never be supported on Firefox.",
> +        "async": "callback",
> +        "parameters": [
> +          {
> +            "name": "id",
> +            "type": "string",
> +            "description": "This should be the id from an app item of $(ref:management.ExtensionInfo)."
> +          },
> +          {
> +            "name": "launchType",
> +            "$ref": "LaunchType",
> +            "description": "The target launch type. Always check and make sure this launch type is in $(ref:ExtensionInfo.availableLaunchTypes), because the available launch types vary on different platforms and configurations."
> +          },
> +          {
> +            "name": "callback",
> +            "type": "function",
> +            "optional": true,
> +            "parameters": []
> +          }
> +        ]
> +      },
> +      {
> +        "name": "generateAppForLink",
> +        "type": "function",
> +        "unsupported": true,
> +        "description": "Generate an app for a URL.  Will never be supported on Firefox.",
> +        "async": "callback",
> +        "parameters": [
> +          {
> +            "name": "url",
> +            "type": "string",
> +            "description": "The URL of a web page. The scheme of the URL can only be \"http\" or \"https\"."
> +          },
> +          {
> +            "name": "title",
> +            "type": "string",
> +            "description": "The title of the generated app."
> +          },
> +          {
> +            "name": "callback",
> +            "type": "function",
> +            "optional": true,
> +            "parameters":  [
> +              {
> +                "name": "result",
> +                "$ref": "ExtensionInfo"
> +              }
> +            ]
> +          }

Leave these out

	Bob Silverberg [:bsilverberg] Assignee
	Comment 4 • 8 years ago

This will need to be updated after bug 1285063 lands as the location to register new APIs and schemas is changing. We can continue with the review, but I'll hold off landing this until that lands first, and then make the final changes for the new method of registration.

	Bob Silverberg [:bsilverberg] Assignee
	Comment 5 • 8 years ago

Comment on attachment 8769234 [details]
Bug 1285537 - Create schema for the management API,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63190/diff/1-2/

	Bob Silverberg [:bsilverberg] Assignee
	Comment 6 • 8 years ago

https://reviewboard.mozilla.org/r/63190/#review60048

> I think we should leave this out until we have a good reason to add it (which I don't think we have yet, I realize you have some unaswered comments about this on a separate bug...)

I believe we decided we would implement this, after doing `getSelf` and `unistallSelf`.

	Andrew Swan [:aswan]
	Comment 7 • 8 years ago

Comment on attachment 8769234 [details]
Bug 1285537 - Create schema for the management API,

https://reviewboard.mozilla.org/r/63190/#review60408

::: browser/components/extensions/schemas/management.json:133
(Diff revision 2)
> +          "hostPermissions": {
> +            "description": "Returns a list of host based permissions.",
> +            "type": "array",
> +            "items" : {
> +              "type": "string"
> +            }

Assuming this is related to Chrome's administrative policies, I don't think we have any analogue for this and should omit it.

::: browser/components/extensions/schemas/management.json:183
(Diff revision 2)
> +        "async": "callback",
> +        "parameters": [
> +          {
> +            "name": "id",
> +            "type": "string",
> +            "description": "The ID from an item of $(ref:management.ExtensionInfo)."

This can have a pattern directive that matches
https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProvider.jsm#282

(or you could create a separate type for it in the schema and referenc it here)

	Bob Silverberg [:bsilverberg] Assignee
	Comment 8 • 8 years ago

Review commit: https://reviewboard.mozilla.org/r/63706/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63706/

	Bob Silverberg [:bsilverberg] Assignee
	Comment 9 • 8 years ago

Comment on attachment 8769234 [details]
Bug 1285537 - Create schema for the management API,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63190/diff/2-3/

	Bob Silverberg [:bsilverberg] Assignee
	Comment 10 • 8 years ago

https://reviewboard.mozilla.org/r/63190/#review60408

> Assuming this is related to Chrome's administrative policies, I don't think we have any analogue for this and should omit it.

`hostPermissions` are permissions that are specified for specific urls or patterns, as opposed to permissions for APIs. E.g., if the permissions specified for an extension are: `["management", "cookies", "*://example.org/", "https://foo.example.org/"]` then `extensionInfo.permissions` would be `["management", "cookies"]` and `extensionInfo.hostPermissions` would be `["*://example.org/", "https://foo.example.org/"]`.

	Andrew Swan [:aswan]
	Comment 11 • 8 years ago

https://reviewboard.mozilla.org/r/63706/#review60686

This is overkill, you can just put a pattern property into the schema.
For example: https://dxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/schemas/manifest.json#243-258

	Bob Silverberg [:bsilverberg] Assignee
	Comment 12 • 8 years ago

https://reviewboard.mozilla.org/r/63706/#review60686

Ah, I misinterpreted your comment on https://reviewboard.mozilla.org/r/63190/ where you said "or you could create a separate type for it in the schema and reference it here", to mean a generic type that could be used by any schema, so I looked at Schemas.jsm and figured a format made more sense than a type. I see now that you meant just define a type in the `management.json` file and use that. Now that I've done this, you don't see any value in keeping it as a format for use by other schemas? It seems like `extensionId` might be something common to other APIs.

	Andrew Swan [:aswan]
	Comment 13 • 8 years ago

https://reviewboard.mozilla.org/r/63706/#review60686

I do think it makes sense to have a type that can be used in other places.  Whether you put it into manifest.json right now or just put it in management.json and let somebody else move it to a shared location if they need it, I don't particularly care.  The first comment in this thread was just saying that this can be implemented with a pattern property, wherever it happens to be put.

	Bob Silverberg [:bsilverberg] Assignee
	Comment 14 • 8 years ago

Comment on attachment 8769234 [details]
Bug 1285537 - Create schema for the management API,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63190/diff/3-4/

	Bob Silverberg [:bsilverberg] Assignee
	Comment 15 • 8 years ago

Comment on attachment 8769234 [details]
Bug 1285537 - Create schema for the management API,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63190/diff/4-5/

	Andrew Swan [:aswan]
	Comment 16 • 8 years ago

Comment on attachment 8769234 [details]
Bug 1285537 - Create schema for the management API,

https://reviewboard.mozilla.org/r/63190/#review60822

Not sure what's going on here, I think you left out some manifest.json changes and left some old test_ext_schemas.js changes in that need to be removed.

	Andrew Swan [:aswan]
	Comment 17 • 8 years ago

Comment on attachment 8769234 [details]
Bug 1285537 - Create schema for the management API,

https://reviewboard.mozilla.org/r/63190/#review60824

Oops, reviewboard glitch on my end, it showed me a wacky diff but after submitting that last comment it reverted to somethign sane.

	Bob Silverberg [:bsilverberg] Assignee
	Comment 18 • 8 years ago

Not to self: This needs to be updated to be in line with the changes from bug 1285063 after that lands.

	Bob Silverberg [:bsilverberg] Assignee
	Comment 19 • 8 years ago

Comment on attachment 8769234 [details]
Bug 1285537 - Create schema for the management API,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63190/diff/5-6/

	Bob Silverberg [:bsilverberg] Assignee
	Comment 20 • 8 years ago

I updated this to move the API from browser to toolkit, but this still needs to wait for bug 1285063 to land and then will require a few changes.

	Bob Silverberg [:bsilverberg] Assignee
	Comment 21 • 8 years ago

Comment on attachment 8769234 [details]
Bug 1285537 - Create schema for the management API,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63190/diff/6-7/

	Bob Silverberg [:bsilverberg] Assignee
	Comment 22 • 8 years ago

Comment on attachment 8769234 [details]
Bug 1285537 - Create schema for the management API,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63190/diff/7-8/

	Bob Silverberg [:bsilverberg] Assignee
	Comment 23 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8dd62d88b961

	Bob Silverberg [:bsilverberg] Assignee
	Comment 24 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f501719a4711

	Bob Silverberg [:bsilverberg] Assignee
	Comment 25 • 8 years ago

After rebasing this on top of Matt's changes from bug 1285063, try seems to be miserable. I don't see how that can be because of the changes in this bug, so I think that it's worth trying to check this in after bug 1285063 lands.

Please ensure that bug 1285063 lands before checking this in.

	Carsten Book [:Tomcat]
	Comment 26 • 8 years ago

(In reply to Bob Silverberg [:bsilverberg] from comment #25)
> After rebasing this on top of Matt's changes from bug 1285063, try seems to
> be miserable. I don't see how that can be because of the changes in this
> bug, so I think that it's worth trying to check this in after bug 1285063
> lands.
> 
> Please ensure that bug 1285063 lands before checking this in.

seems bug 1285063 has no checkin-needed or so - so it need to wait till that landed by matt

	Bob Silverberg [:bsilverberg] Assignee
	Comment 28 • 8 years ago

The try run at https://treeherder.mozilla.org/#/jobs?repo=try&revision=fe2fe6d5a699 looks good, and bug 1285063 has landed, so this should be good to check in.

	Pulsebot
	Comment 29 • 8 years ago

Pushed by kwierso@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/f90a98f65eba
Create schema for the management API, r=aswan

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 30 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/f90a98f65eba