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)

63 Branch
enhancement

Tracking

(firefox63 fixed, firefox64 fixed)

RESOLVED FIXED
mozilla64
Tracking Status
firefox63 --- fixed
firefox64 --- fixed

People

(Reporter: Oriol, Assigned: Oriol)

References

Details

(Keywords: dev-doc-complete)

Attachments

(1 file)

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)
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)
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)
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)
(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.
compatibility first.
Priority: -- → P5
(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)
Lets make sure the doc notes on mdn discuss the performance aspect with this.
Keywords: dev-doc-needed
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+
per mconca we want to uplift this to avoid a later api change.  Oriol, can you make the request?
Flags: needinfo?(oriol-bugzilla)
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
Pushed by rvandermeulen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7be15d7f2e61
Add 'populate' parameter in 'tabs.highlight'. r=mixedpuppy
Keywords: checkin-needed
QA Contact: ddurst
https://hg.mozilla.org/mozilla-central/rev/7be15d7f2e61
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
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?
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)
I didn't set the priority.
Flags: needinfo?(oriol-bugzilla) → needinfo?(mixedpuppy)
@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)
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 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+
Flags: qe-verify-
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)
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)
(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.
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.
You need to log in before you can comment on or make changes to this bug.