Closed Bug 1262923 Opened 8 years ago Closed 8 years ago

Create schema for the history API

Categories

(WebExtensions :: Untriaged, defect)

defect
Not set
normal

Tracking

(firefox48 fixed)

RESOLVED FIXED
mozilla48
Iteration:
48.3 - Apr 25
Tracking Status
firefox48 --- fixed

People

(Reporter: bsilverberg, Assigned: bsilverberg)

References

Details

(Whiteboard: [history]triaged)

Attachments

(1 file)

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
Assignee: nobody → bob.silverberg
Status: NEW → ASSIGNED
Iteration: --- → 48.3 - Apr 25
Whiteboard: [history]triaged
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...
Attachment #8743385 - Flags: review?(aswan)
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/
Attachment #8743385 - Flags: review?(aswan)
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 on attachment 8743385 [details]
MozReview Request: Bug 1262923 - Create schema for the history API, r?aswan

https://reviewboard.mozilla.org/r/47731/#review44941
Attachment #8743385 - Flags: review?(aswan) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a07ea7fb572f
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.