Closed
Bug 1489814
Opened 6 years ago
Closed 6 years ago
Add option to prevent tabs.highlight from populating the returned window
Categories
(WebExtensions :: Frontend, enhancement, P2)
Tracking
(firefox63 fixed, firefox64 fixed)
RESOLVED
FIXED
mozilla64
People
(Reporter: Oriol, Assigned: Oriol)
References
Details
(Keywords: dev-doc-complete)
Attachments
(1 file)
46 bytes,
text/x-phabricator-request
|
mixedpuppy
:
review+
pascalc
:
approval-mozilla-beta+
|
Details | Review |
tabs.highlight returns a promise fulfilled with the Window object populated with all tabs. Populating all tabs can be an expensive operation if there are hundreds or thousands of them, and I don't see why anyone would expect tabs.highlight to return a populated window. Doesn't seem useful at all, you can get that data with browser.windows.get(windowId, {populate: true}) instead. Seems a bug to me, and according to to Kris Maglione from bug 1481185 comment #5, > As a general rule, we actually try *not* to be bug-for-bug compatible unless > there's a particular reason we need to be. So I propose stopping stop populating it. Or, if compatibility really requires it, accept a `populate` property in highlightInfo that can be set to false to manually disable this behavior in order to improve performance.
Flags: needinfo?(mixedpuppy)
Comment 1•6 years ago
|
||
This isn't a bug-for-bug issue, it's an api compat issue. I'm not crazy about "populate". We only need compatibility for chrome.tabs.highlight. Returning a window object on this api seems rather silly. I would propose making browser.tabs.highlight simply return {windowId, tabIds: ...}. I'm not sure if we've done things like this elsewhere, but IMO the "highlight" API is just wrong in returning Window (and it's the only tab api that does that). I would like input on that from others on the team, so ni?aswan Another direction might be figuring some kind of optimization for Window.tabs. Do they really need to be retrieved before they are accessed?
Flags: needinfo?(mixedpuppy) → needinfo?(aswan)
Comment 2•6 years ago
|
||
I agree this is not a bug but Oriol's suggestion sounds like a reasonable way to offer a reasonably-performing option without breaking compatibility.
Flags: needinfo?(aswan)
Comment 3•6 years ago
|
||
ok, I just looked at the implementation. populate does make sense given it is already an option we use calling windowManager.convert. Go for it. ni? mconca just so he is a aware of the api improvement.
Flags: needinfo?(mconca)
Assignee | ||
Comment 4•6 years ago
|
||
(In reply to Shane Caraveo (:mixedpuppy) from comment #3) > ok, I just looked at the implementation. populate does make sense given it > is already an option we use calling windowManager.convert. > > Go for it. Defaulting to true or false? false would be much better (and consistent with window APIs), but true more compatible.
Comment 5•6 years ago
|
||
compatibility first.
Updated•6 years ago
|
Priority: -- → P5
Comment 6•6 years ago
|
||
(In reply to Shane Caraveo (:mixedpuppy) from comment #3) > ok, I just looked at the implementation. populate does make sense given it > is already an option we use calling windowManager.convert. > > Go for it. > > ni? mconca just so he is a aware of the api improvement. Approved. An optional 'populate' property to the highlightInfo makes sense. > compatibility first. This. The 'populate' property should default to true.
Severity: normal → enhancement
Flags: needinfo?(mconca)
Assignee | ||
Comment 7•6 years ago
|
||
Comment 8•6 years ago
|
||
Lets make sure the doc notes on mdn discuss the performance aspect with this.
Keywords: dev-doc-needed
Comment 9•6 years ago
|
||
Comment on attachment 9013080 [details] Bug 1489814 - Add 'populate' parameter in 'tabs.highlight'. r=mixedpuppy Shane Caraveo (:mixedpuppy) has approved the revision.
Attachment #9013080 -
Flags: review+
Comment 10•6 years ago
|
||
per mconca we want to uplift this to avoid a later api change. Oriol, can you make the request?
Flags: needinfo?(oriol-bugzilla)
Assignee | ||
Comment 11•6 years ago
|
||
I will do the request after this lands in nightly.
Assignee: nobody → oriol-bugzilla
Status: NEW → ASSIGNED
Flags: needinfo?(oriol-bugzilla)
Keywords: checkin-needed
QA Contact: ddurst
Summary: tabs.highlight shouldn't populate the returned window → Add option to prevent tabs.highlight from populating the returned window
Comment 12•6 years ago
|
||
Pushed by rvandermeulen@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/7be15d7f2e61 Add 'populate' parameter in 'tabs.highlight'. r=mixedpuppy
Keywords: checkin-needed
Updated•6 years ago
|
QA Contact: ddurst
Comment 13•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/7be15d7f2e61
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox64:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Assignee | ||
Comment 14•6 years ago
|
||
Comment on attachment 9013080 [details] Bug 1489814 - Add 'populate' parameter in 'tabs.highlight'. r=mixedpuppy [Beta/Release Uplift Approval Request] Feature/Bug causing the regression: Bug 1464862 User impact if declined: Later API change. The basic API was added in 63, not including this additional feature there may disincentivize authors from using it, avoiding the performance benefits. See comment 10. Is this code covered by automated tests?: Yes Has the fix been verified in Nightly?: Yes Needs manual test from QE?: No If yes, steps to reproduce: List of other uplifts needed: None Risk to taking this patch: Low Why is the change risky/not risky? (and alternatives if risky): Just allowing ad-on authors to decide whether the returned window should be populated or not (instead of always populating it). String changes made/needed:
Attachment #9013080 -
Flags: approval-mozilla-beta?
Comment 15•6 years ago
|
||
Oriol, this is a P5 bug, just landed on Nightly and we ship 63 in 2 weeks, is the priority correct for this bug?
Flags: needinfo?(oriol-bugzilla)
Assignee | ||
Comment 16•6 years ago
|
||
I didn't set the priority.
Flags: needinfo?(oriol-bugzilla) → needinfo?(mixedpuppy)
Comment 17•6 years ago
|
||
@pascalc, I originally set p5, however, this is a change on a new api we landed in 63. It's a relatively minor change with tests. We'd prefer to avoid api change a version later, but I understand it's late.
Flags: needinfo?(mixedpuppy) → needinfo?(pascalc)
Comment 18•6 years ago
|
||
setting as a p2, only because it's soo close to shipping. but still would be preferable to get in if possible.
Priority: P5 → P2
Comment 19•6 years ago
|
||
Comment on attachment 9013080 [details] Bug 1489814 - Add 'populate' parameter in 'tabs.highlight'. r=mixedpuppy Thanks for context Shane, the patch is minimal and has tests, uplift approved for 63 beta 14. Thanks
Flags: needinfo?(pascalc)
Attachment #9013080 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Updated•6 years ago
|
status-firefox63:
--- → affected
Comment 20•6 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/253f11fbc824
Updated•6 years ago
|
Flags: qe-verify-
Comment 21•6 years ago
|
||
This seems to have been documented already. Has the default value changed from true to false? https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/tabs/highlight
Flags: needinfo?(oriol-bugzilla)
Assignee | ||
Comment 22•6 years ago
|
||
I documented it in MDN. My https://github.com/mdn/browser-compat-data/pull/2987 was closed in favor of your https://github.com/mdn/browser-compat-data/pull/2816, please add it there. The default value has not changed, it's a new parameter.
Flags: needinfo?(oriol-bugzilla)
Comment 23•6 years ago
|
||
(In reply to Oriol Brufau [:Oriol] from comment #22) > I documented it in MDN. My > https://github.com/mdn/browser-compat-data/pull/2987 was closed in favor of > your https://github.com/mdn/browser-compat-data/pull/2816, please add it > there. The default value has not changed, it's a new parameter. I have updated the pull request.
Comment 24•5 years ago
|
||
Verifying that this documentation is complete; note added to 63 rel notes: https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/63#Tabs Aforementioned PR merged too.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in
before you can comment on or make changes to this bug.
Description
•