Closed Bug 1419132 Opened 3 years ago Closed 2 years ago

deprecate windowTypes for windows.get, windows.getCurrent and windows.getLastFocused

Categories

(WebExtensions :: Frontend, enhancement, P5)

58 Branch
enhancement

Tracking

(firefox62 fixed)

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: bsilverberg, Assigned: Oriol, Mentored)

Details

(Keywords: dev-doc-complete, Whiteboard: [good first bug][windows])

Attachments

(1 file)

As discussed in bug 1415913, the windowTypes option of the getInfo object for the above API methods doesn't really make sense, and is not currently implemented. We should mark it as deprecated in the schema.
Mentor: bob.silverberg
I think this is supposed to address the "windows" API not the "tabs" API.
(In reply to MartijnJoosstens from comment #1)
> I think this is supposed to address the "windows" API not the "tabs" API.

Thanks for catching this.
Summary: deprecate windowTypes for tabs.get, tabs.getCurrent and tabs.getLastFocused → deprecate windowTypes for windows.get, windows.getCurrent and windows.getLastFocused
Comment on attachment 8974839 [details]
Bug 1419132 - Deprecate windowTypes for windows.get, windows.getCurrent and windows.getLastFocused

https://reviewboard.mozilla.org/r/243212/#review249054

I think the comments in bug 1419132 was to mark it deprecated in schema.  Removing it creates potential incompatibility with chrome in a new unexpected way.
Attachment #8974839 - Flags: review?(mixedpuppy) → review-
Ah, I misunderstood. Then what about bug 1404014, should android also accept windowTypes even if it's ignored?
I dont think new apis on android are incredibly important right now, unless there is some indication otherwise.  Overall though, given a window api on android, it should probably be compatible with desktop.
Comment on attachment 8974839 [details]
Bug 1419132 - Deprecate windowTypes for windows.get, windows.getCurrent and windows.getLastFocused

https://reviewboard.mozilla.org/r/243212/#review249274

Looks good, I just want to understand the last chunk.

::: browser/components/extensions/schemas/windows.json:114
(Diff revision 2)
> +            "items": {
> +              "$ref": "WindowType"
> +            },
> +            "optional": true,
> +            "deprecated": true,
> +            "description": "This property is tolerated for compatibility with Chrome, but its value is ignored."

Lets change this comment to something like: <code>windowTypes</code> is deprecated and ignored on Firefox.

::: browser/components/extensions/schemas/windows.json:145
(Diff revision 2)
>            },
>            {
> -            "type": "object",
> +            "$ref": "GetInfo",
>              "name": "getInfo",
>              "optional": true,
> -            "description": "",
> +            "description": ""

We should add comments on this (and the others), unsure of a good one.  Maybe something like: Specifies properties used to filter windows returned.

::: browser/components/extensions/schemas/windows.json:215
(Diff revision 2)
>          "description": "Gets all windows.",
>          "async": "callback",
>          "parameters": [
>            {
>              "type": "object",
> +            "$import": "GetInfo",

huh, I've not used import.  Why the difference for this api?
Attachment #8974839 - Flags: review?(mixedpuppy)
(In reply to Shane Caraveo (:mixedpuppy) from comment #8)
> huh, I've not used import.  Why the difference for this api?

I just thought it was a nice way to avoid repeating things all over the place. Do you prefer not to use it?
(In reply to Oriol Brufau [:Oriol] from comment #9)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #8)
> > huh, I've not used import.  Why the difference for this api?
> 
> I just thought it was a nice way to avoid repeating things all over the
> place. Do you prefer not to use it?

Why not "$ref": "GetInfo"?  Are we preserving use of windowTypes here?
(In reply to Shane Caraveo (:mixedpuppy) from comment #11)
> Why not "$ref": "GetInfo"?  Are we preserving use of windowTypes here?

Yes, my understanding is that windowTypes should be preserved in getAll but not in get, getCurrent nor getLastFocused.
Did I get it wrong?
Comment on attachment 8974839 [details]
Bug 1419132 - Deprecate windowTypes for windows.get, windows.getCurrent and windows.getLastFocused

https://reviewboard.mozilla.org/r/243212/#review249334
Attachment #8974839 - Flags: review?(mixedpuppy) → review+
Assignee: nobody → oriol-bugzilla
Status: NEW → ASSIGNED
OS: Linux → All
Hardware: x86_64 → All
Pushed by csabou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3adff617b271
Deprecate windowTypes for windows.get, windows.getCurrent and windows.getLastFocused r=mixedpuppy
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/3adff617b271
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Can you please add some STRs to this bug(and test extension if possible) or mark it as "qe-verify-" ?
Flags: needinfo?(bob.silverberg)
Victor, I think Shane is the one who worked on this. Transferring my needinfo to him.
Flags: needinfo?(bob.silverberg) → needinfo?(mixedpuppy)
Flags: qe-verify-
Flags: needinfo?(mixedpuppy)
Flags: in-testsuite+
Product: Toolkit → WebExtensions
Note added to the page for windowType and also to the pages for each of the  methods that take a parameter of windowType. This includes: get(), getAll(), getCurrent(), and getLastFocused().
(In reply to Irene Smith from comment #18)
> Note added to the page for windowType and also to the pages for each of the 
> methods that take a parameter of windowType. This includes: get(), getAll(),
> getCurrent(), and getLastFocused().

I don't think windows.WindowType nor windowType in getAll() are deprecated.

It's just windowType property of getInfo parameter for get(), getCurrent() and getLastFocused().
(In reply to Oriol Brufau [:Oriol] from comment #19)
> (In reply to Irene Smith from comment #18)
> > Note added to the page for windowType and also to the pages for each of the 
> > methods that take a parameter of windowType. This includes: get(), getAll(),
> > getCurrent(), and getLastFocused().
> 
> I don't think windows.WindowType nor windowType in getAll() are deprecated.
> 
> It's just windowType property of getInfo parameter for get(), getCurrent()
> and getLastFocused().

I misunderstood the intention of the issue. I will delete the pull request for browser compatibility changes and remove the note from the windowType and getAll pages.
(In reply to Oriol Brufau [:Oriol] from comment #19)
> 
> I don't think windows.WindowType nor windowType in getAll() are deprecated.
> 
> It's just windowType property of getInfo parameter for get(), getCurrent()
> and getLastFocused().

Can you confirm that this would be an appropriate warning?

Deprecated. The use of the windowTypes component of the getInfo parameter has been deprecated as of Firefox 62. If supplied, the value is ignored.
Yes, that's correct. But not sure if specifying Firefox 62 may make people believe that windowType worked before that version, it was already ignored before becoming deprecated.
(In reply to Oriol Brufau [:Oriol] from comment #22)
> Yes, that's correct. But not sure if specifying Firefox 62 may make people
> believe that windowType worked before that version, it was already ignored
> before becoming deprecated.

I see your point. This is what I ended up with:

If supplied, the windowTypes component of getInfo is ignored. The use of windowTypes has been deprecated as of Firefox 62.
Chris, not sure if there has been a confusion, I don't think get(), getCurrent() and getLastFocused() are deprecated, it's just their windowTypes parameter.
Flags: needinfo?(cmills)
(In reply to Oriol Brufau [:Oriol] from comment #24)
> Chris, not sure if there has been a confusion, I don't think get(),
> getCurrent() and getLastFocused() are deprecated, it's just their
> windowTypes parameter.

Sorry about that; I've removed the problem deprecated banners again.
Flags: needinfo?(cmills)
You need to log in before you can comment on or make changes to this bug.