createData should be optional for browser.windows.create

RESOLVED FIXED in Firefox 55

Status

()

Toolkit
WebExtensions: Frontend
P3
normal
RESOLVED FIXED
a year ago
24 days ago

People

(Reporter: bsilverberg, Assigned: bsilverberg)

Tracking

(Blocks: 1 bug)

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [windows]triaged)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

a year ago
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
(Assignee)

Updated

a year ago
Whiteboard: [good first bug]
(Assignee)

Comment 1

a year ago
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.
Assignee: nobody → bob.silverberg
Status: NEW → ASSIGNED
Iteration: --- → 48.3 - Apr 18
Flags: blocking-webextensions?(amckay)
(Assignee)

Updated

a year ago
Flags: blocking-webextensions?(amckay) → blocking-webextensions+
(Assignee)

Comment 2

a year ago
Without bug 1262250 this becomes extra work, and this isn't critical, so I'm marking it as blocked.
Depends on: 1262250
(Assignee)

Comment 3

a year ago
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.
(Assignee)

Comment 4

a year ago
Created attachment 8739100 [details]
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/
Attachment #8739100 - Flags: review?(kmaglione+bmo)
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.
Attachment #8739100 - Flags: review?(kmaglione+bmo)

Updated

a year ago
Whiteboard: [good first bug] → [windows][good first bug]triaged
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?
Flags: needinfo?(kmaglione+bmo)
No, it's still a bug.
Flags: needinfo?(kmaglione+bmo)

Updated

10 months ago
Keywords: good-first-bug
Whiteboard: [windows][good first bug]triaged → [windows]triaged
Comment hidden (mozreview-request)

Comment 9

8 months ago
mozreview-review
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 hidden (mozreview-request)

Comment 11

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

https://reviewboard.mozilla.org/r/45043/#review82310
Attachment #8739100 - Flags: review?(aswan) → review+
Comment hidden (mozreview-request)

Updated

7 months ago
Component: WebExtensions: Untriaged → WebExtensions: Frontend
Flags: blocking-webextensions+
Priority: -- → P3
Comment hidden (mozreview-request)

Updated

4 months ago
Keywords: good-first-bug
Comment hidden (mozreview-request)
webextensions: --- → ?

Updated

2 months ago
webextensions: ? → ---
Comment hidden (mozreview-request)
(Assignee)

Comment 16

25 days ago
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

25 days ago
Pushed by bsilverberg@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bb6c0cdfbea7
createData should be optional for browser.windows.create, r=aswan

Comment 18

24 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/bb6c0cdfbea7
Status: ASSIGNED → RESOLVED
Last Resolved: 24 days ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.