Closed Bug 1253133 Opened 4 years ago Closed 4 years ago

Support changing size and position via browser.windows.update

Categories

(WebExtensions :: Untriaged, defect)

defect
Not set

Tracking

(firefox48 fixed)

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: kmag, Assigned: kmag)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Whiteboard: [windows][berlin][good first bug])

Attachments

(1 file)

No description provided.
Whiteboard: [windows][berlin] → [windows][berlin][good first bug]
Assignee: nobody → kmaglione+bmo
Attachment #8731772 - Flags: review?(aswan) → review+
Comment on attachment 8731772 [details]
MozReview Request: Bug 1253133: [webext] Support changing window geometry via windows.update. r?aswan

https://reviewboard.mozilla.org/r/40823/#review37533

Comments are all minor, looks good to me.

::: browser/components/extensions/ext-utils.js:670
(Diff revision 1)
>  
>      return "normal";
>    },
>  
> +  updateGeometry(window, options) {
> +    if (options.left !== null || options.top !== null) {

I know this was cut&pasted but I thought our standard was to prefer == and != to === and !== ?

::: browser/components/extensions/ext-windows.js:170
(Diff revision 1)
>            return WindowManager.convert(extension, window);
>          });
>        },
>  
>        update: function(windowId, updateInfo) {
> -        // TODO: When we support size/position updates:
> +        if (updateInfo.state !== null && updateInfo.state != "normal") {

The chrome docs actually say that geometry can't be supplied with sate "minimized", "maximized" or "fullscreen" but that seems to suggest that geometry could be supplied along with state "docked"?

::: browser/components/extensions/test/browser/browser_ext_windows_size.js:31
(Diff revision 1)
> +            browser.test.assertEq(expected[key], geom[key], `Expected '${key}' value`);
> +          }
> +
> +          return browser.windows.get(windowId);
> +        }).then(geom => {
> +          browser.test.log("Check API-reported window size");

This confused me for a moment, I think that putting this log right next to the call to browser.windows.get() might be clearer to a reader?  (Likewise the "Check actual window size" message above)
https://reviewboard.mozilla.org/r/40823/#review37533

> I know this was cut&pasted but I thought our standard was to prefer == and != to === and !== ?

Not in WebExtension code, and not in cases where there's a reason to use `===`. In this case, omitted values should be normalized to `null`, so `undefined` values signal an error, and shouldn't be treated the same as `null`.

> The chrome docs actually say that geometry can't be supplied with sate "minimized", "maximized" or "fullscreen" but that seems to suggest that geometry could be supplied along with state "docked"?

I think it's just badly documented. I can't see how geometry would make sense in combination with the "docked" state. In any case, we currently treat "docked" as an alias for "minimized", so it doesn't make sense to treat them differently here.

> This confused me for a moment, I think that putting this log right next to the call to browser.windows.get() might be clearer to a reader?  (Likewise the "Check actual window size" message above)

I guess. I added those logs so it would be easier to make sense of the test output just after them, but I guess it couldn't hurt to move them before we do the queries.
Depends on: 1258867
The tests for this are currently disabled on OS-X because, for some reason, we're winding up in fullscreen mode for some test runs, which prevents us from resizing even new windows.
https://hg.mozilla.org/integration/fx-team/rev/3ad9e7d55c6e393323c4afb57d4ce32e8d8a89cc
Bug 1253133: [webext] Support changing window geometry via windows.update. r=aswan
https://hg.mozilla.org/mozilla-central/rev/3ad9e7d55c6e
Status: NEW → RESOLVED
Closed: 4 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.