1261963 - createData should be optional for browser.windows.create

Status: RESOLVED FIXED

(WebExtensions :: Frontend, defect, P3)

Description

[Bob Silverberg [:bsilverberg]]

According to the docs [1][2] and also according to Chrome's behaviour, `createData` should be optional when calling `browser.windows.create.

This requires a change to the schema [3] and a test should probably be added to verify this change.

[1] https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/windows/create
[2] https://developer.chrome.com/extensions/windows#method-create
[3] https://dxr.mozilla.org/mozilla-central/source/browser/components/extensions/schemas/windows.json#284

Comment 1

[Bob Silverberg [:bsilverberg]]

I'm not sure that this should be a blocker, but on the other hand it's pretty simple and ideally we wouldn't have to introduce changes to a schema post 1.0.

Comment 2

[Bob Silverberg [:bsilverberg]]

Without bug 1262250 this becomes extra work, and this isn't critical, so I'm marking it as blocked.

Comment 3

[Bob Silverberg [:bsilverberg]]

Andy suggested I not work on bug 1262250, so I'm going to see what I can do to make this work without schema changes.

Comment 4

[Bob Silverberg [:bsilverberg]]

Attachment: Bug 1261963 - createData should be optional for browser.windows.create,

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

Comment 5

[Kris Maglione [:kmag]]

Comment on attachment 8739100 [details]
Bug 1261963 - createData should be optional for browser.windows.create,

https://reviewboard.mozilla.org/r/45043/#review41589

I'd rather handle this with bug 1262250.

Comment 6

[Andreas Wagner [:TheOne] [use NI]]

Should this buf (In reply to Kris Maglione [:kmag] from comment #5)
> Comment on attachment 8739100 [details]
> MozReview Request: Bug 1261963 - createData should be optional for
> browser.windows.create, r?kmag
> 
> https://reviewboard.mozilla.org/r/45043/#review41589
> 
> I'd rather handle this with bug 1262250.

Should this bug be closed then?

Comment 7

[Kris Maglione [:kmag]]

No, it's still a bug.

Comment 8

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8739100 [details]
Bug 1261963 - createData should be optional for browser.windows.create,

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

Comment 9

[Andrew Swan [:aswan]]

Comment on attachment 8739100 [details]
Bug 1261963 - createData should be optional for browser.windows.create,

https://reviewboard.mozilla.org/r/45043/#review79170

::: browser/components/extensions/test/browser/browser_ext_windows_create.js:26
(Diff revision 2)
>            browser.test.sendMessage("check-window", expected);
>          });
>        }
>  
>        function createWindow(params, expected, keep = false) {
>          return browser.windows.create(params).then(window => {

I'd like to see this actually call window.create() with no args (rather than with an arg of null).  I think the simplest way to do that is to make params an array and use the spread operator here.

Comment 10

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8739100 [details]
Bug 1261963 - createData should be optional for browser.windows.create,

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

Comment 11

[Andrew Swan [:aswan]]

Comment on attachment 8739100 [details]
Bug 1261963 - createData should be optional for browser.windows.create,

https://reviewboard.mozilla.org/r/45043/#review82310

Comment 12

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8739100 [details]
Bug 1261963 - createData should be optional for browser.windows.create,

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

Comment 13

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8739100 [details]
Bug 1261963 - createData should be optional for browser.windows.create,

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

Comment 14

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8739100 [details]
Bug 1261963 - createData should be optional for browser.windows.create,

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

Comment 15

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8739100 [details]
Bug 1261963 - createData should be optional for browser.windows.create,

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

Comment 16

[Bob Silverberg [:bsilverberg]]

I finally figured out why this test was failing on try on Windows. On Windows, the test was starting with the browser window maximized, and creating a window via browser.windows.create() (on Windows anyway) creates a maximized window if the current window is maximized. The test expects the window created to be normal, not maximized, so I simply changed the test to update the current window to state: "normal" at the beginning of the test and that seems to have solved the issue.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bb6c0cdfbea7
createData should be optional for browser.windows.create, r=aswan

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 19

[Bob Silverberg [:bsilverberg]]

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

Comment 20

[Bob Silverberg [:bsilverberg]]

Attachment: Patch for uplift to ESR.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: This is a very simple fix that will enhance the browser.windows.create API. 
User impact if declined: Web extensions will not be able to call browser.windows.create with zero arguments.
Fix Landed on Version: 55
Risk to taking this patch (and alternatives if risky): This is a one-line change to a json schema file, and is covered by an automated test, so risk is extremely low. A try run has been started at https://treeherder.mozilla.org/#/jobs?repo=try&revision=e8fdcc3cb9e3f3c2985849e071ff5a335609964d.
String or UUID changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 21

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8934203 [details] [diff] [review]
Patch for uplift to ESR.

Low risk fix, with automated tests, got a second opinion from Andy, ESR52+

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/d0c98f5b6c12