Closed
Bug 1419132
Opened 7 years ago
Closed 7 years ago
deprecate windowTypes for windows.get, windows.getCurrent and windows.getLastFocused
Categories
(WebExtensions :: Frontend, enhancement, P5)
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.
Reporter | ||
Updated•7 years ago
|
Mentor: bob.silverberg
I think this is supposed to address the "windows" API not the "tabs" API.
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.
Summary: deprecate windowTypes for tabs.get, tabs.getCurrent and tabs.getLastFocused → deprecate windowTypes for windows.get, windows.getCurrent and windows.getLastFocused
Comment hidden (mozreview-request) |
Comment 4•7 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.
Attachment #8974839 -
Flags: review?(mixedpuppy) → review-
Assignee | ||
Comment 5•7 years ago
|
||
Ah, I misunderstood. Then what about bug 1404014, should android also accept windowTypes even if it's ignored?
Comment 6•7 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.
Comment hidden (mozreview-request) |
Comment 8•7 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?
Attachment #8974839 -
Flags: review?(mixedpuppy)
Assignee | ||
Comment 9•7 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?
Comment hidden (mozreview-request) |
Comment 11•7 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?
Assignee | ||
Comment 12•7 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?
Comment 13•7 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
Attachment #8974839 -
Flags: review?(mixedpuppy) → review+
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → oriol-bugzilla
Status: NEW → ASSIGNED
Keywords: checkin-needed,
dev-doc-needed
OS: Linux → All
Hardware: x86_64 → All
Comment 14•7 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
Keywords: checkin-needed
![]() |
||
Comment 15•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox62:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Comment 16•7 years ago
|
||
Can you please add some STRs to this bug(and test extension if possible) or mark it as "qe-verify-" ?
Flags: needinfo?(bob.silverberg)
Reporter | ||
Comment 17•7 years ago
|
||
Victor, I think Shane is the one who worked on this. Transferring my needinfo to him.
Flags: needinfo?(bob.silverberg) → needinfo?(mixedpuppy)
Updated•7 years ago
|
Flags: qe-verify-
Flags: needinfo?(mixedpuppy)
Flags: in-testsuite+
Updated•7 years ago
|
Product: Toolkit → WebExtensions
Comment 18•7 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().
Assignee | ||
Comment 19•7 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().
Comment 20•7 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.
Comment 21•7 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.
Assignee | ||
Comment 22•7 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.
Comment 23•7 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.
Updated•7 years ago
|
Keywords: dev-doc-needed → dev-doc-complete
Assignee | ||
Comment 24•7 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.
Flags: needinfo?(cmills)
Comment 25•7 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.
Flags: needinfo?(cmills)
You need to log in
before you can comment on or make changes to this bug.
Description
•