1262923 - Create schema for the history API

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect)

Description

[Bob Silverberg [:bsilverberg]]

Create a schema file for the history 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/history
[2] https://src.chromium.org/viewvc/chrome/trunk/src/chrome/common/extensions/api/history.json

Comment 1

[Bob Silverberg [:bsilverberg]]

Attachment: MozReview Request: Bug 1262923 - Create schema for the history API, r?aswan

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

Comment 2

[Andrew Swan [:aswan]]

Comment on attachment 8743385 [details]
MozReview Request: Bug 1262923 - Create schema for the history API, r?aswan

https://reviewboard.mozilla.org/r/47731/#review44877

::: browser/components/extensions/ext-history.js:10
(Diff revision 1)
> +extensions.registerSchemaAPI("history", "history", (extension, context) => {
> +  return {
> +    history: {
> +      search: function(query) {
> +        // FIXME: Implement history.search
> +        // This is just here so a test can be written that calls a history API method

Lets not do this.  If this lands as-is and somebody tries to use this method they are going to be very surprised.
You can have an automated test check for the presence of chrome.history / browser.history but until we have live code for individual methods, I don't think there's any point testing for them.

::: browser/components/extensions/schemas/history.json:98
(Diff revision 1)
> +      }
> +    ],
> +    "functions": [
> +      {
> +        "name": "search",
> +        "unsupported": false,

apropos the above comment this should be true.

::: browser/components/extensions/test/mochitest/mochitest.ini:3
(Diff revision 1)
> +[DEFAULT]
> +
> +[test_ext_history.html]

This test works as a mochitest, but it seems likely that all the substantive tests will have to be browser_* tests?  If that's the case, I would think it would be ideal to just have all the tests there rather than have this one lonely mochitest all by itself here...

Comment 3

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8743385 [details]
MozReview Request: Bug 1262923 - Create schema for the history API, r?aswan

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

Comment 4

[Bob Silverberg [:bsilverberg]]

https://reviewboard.mozilla.org/r/47731/#review44877

> Lets not do this.  If this lands as-is and somebody tries to use this method they are going to be very surprised.
> You can have an automated test check for the presence of chrome.history / browser.history but until we have live code for individual methods, I don't think there's any point testing for them.

Yeah, I wasn't crazy about this either, and my intent was to land the implementation ASAP, but you're right, it's not a good idea. I'll just check for the existence of `browser.history` as you suggest.

> This test works as a mochitest, but it seems likely that all the substantive tests will have to be browser_* tests?  If that's the case, I would think it would be ideal to just have all the tests there rather than have this one lonely mochitest all by itself here...

When I started working on `history.search()` I realized that I would need a browser test, so yeah, this makes sense.

Comment 5

[Andrew Swan [:aswan]]

Comment on attachment 8743385 [details]
MozReview Request: Bug 1262923 - Create schema for the history API, r?aswan

https://reviewboard.mozilla.org/r/47731/#review44941

Comment 6

[Bob Silverberg [:bsilverberg]]

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

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/a07ea7fb572f

Comment 8

[Carsten Book [:Tomcat]]

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