Bob Silverberg [:bsilverberg] Reporter
	Description • 7 years ago

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.

	u482975
	Comment 1 • 7 years ago

I think this is supposed to address the "windows" API not the "tabs" API.

	Bob Silverberg [:bsilverberg] Reporter
	Comment 2 • 7 years ago

(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.

	Shane Caraveo (:mixedpuppy)
	Comment 4 • 6 years ago
mozreview-review

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.

	Oriol Brufau [:Oriol] Assignee
	Comment 5 • 6 years ago

Ah, I misunderstood. Then what about bug 1404014, should android also accept windowTypes even if it's ignored?

	Shane Caraveo (:mixedpuppy)
	Comment 6 • 6 years ago

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.

	Shane Caraveo (:mixedpuppy)
	Comment 8 • 6 years ago
mozreview-review

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?

	Oriol Brufau [:Oriol] Assignee
	Comment 9 • 6 years ago

(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?

	Shane Caraveo (:mixedpuppy)
	Comment 11 • 6 years ago

(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?

	Oriol Brufau [:Oriol] Assignee
	Comment 12 • 6 years ago

(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?

	Shane Caraveo (:mixedpuppy)
	Comment 13 • 6 years ago
mozreview-review

Comment on attachment 8974839 [details]
Bug 1419132 - Deprecate windowTypes for windows.get, windows.getCurrent and windows.getLastFocused

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

	Pulsebot
	Comment 14 • 6 years ago

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

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 15 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/3adff617b271

	Victor Carciu
	Comment 16 • 6 years ago

Can you please add some STRs to this bug(and test extension if possible) or mark it as "qe-verify-" ?

	Bob Silverberg [:bsilverberg] Reporter
	Comment 17 • 6 years ago

Victor, I think Shane is the one who worked on this. Transferring my needinfo to him.

	Irene Smith
	Comment 18 • 6 years ago

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().

	Oriol Brufau [:Oriol] Assignee
	Comment 19 • 6 years ago

(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().

	Irene Smith
	Comment 20 • 6 years ago

(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.

	Irene Smith
	Comment 21 • 6 years ago

(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.

	Oriol Brufau [:Oriol] Assignee
	Comment 22 • 6 years ago

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.

	Irene Smith
	Comment 23 • 6 years ago

(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.

	Oriol Brufau [:Oriol] Assignee
	Comment 24 • 6 years ago

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.

	Chris Mills [:cmills]
	Comment 25 • 6 years ago

(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.