Closed Bug 1238310 Opened 8 years ago Closed 8 years ago

Implement browser.tabs zoom APIs

Categories

(WebExtensions :: Untriaged, defect, P4)

defect

Tracking

(firefox49 fixed)

RESOLVED FIXED
mozilla49
Iteration:
48.3 - Apr 25
Tracking Status
firefox49 --- fixed

People

(Reporter: kmag, Assigned: kmag)

References

Details

(Whiteboard: [tabs] triaged)

Attachments

(5 files)

This includes:

    getZoom()
    setZoom()
    getZoomSettings()
    setZoomSettings()
    onZoomChange
Priority: -- → P4
Whiteboard: [tabs] → [tabs] triaged
Assignee: nobody → kmaglione+bmo
Iteration: --- → 48.3 - Apr 18
Attachment #8743115 - Flags: feedback?(aswan)
Attachment #8743113 - Flags: feedback?(gkrizsanits)
Comment on attachment 8743113 [details]
MozReview Request: Bug 1238310: Part 3 - Implement the base browser.tabs zoom API. r?aswan f?gabor

https://reviewboard.mozilla.org/r/47575/#review44543

Looks good but looks like the schema includes stuff from Chrome that we don't support (e.g., modes other than automatic) which is probably worth adding to https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs#Chrome_incompatibilities
Attachment #8743113 - Flags: review?(aswan) → review+
Comment on attachment 8743114 [details]
MozReview Request: Bug 1238310: Part 4 - Refactor tab listener code. r?aswan

https://reviewboard.mozilla.org/r/47577/#review44549

Overall, this seems like a nice step toward clarity and a more logical structure.
One nit, the tabListener can never be disabled so if an extension stops listening for tab events (either through its own logic or by being disabled or uninstalled) the tabListener handlers will still be doing some small amount of unnecessary work.  But I think this is a pattern that we follow in lots of other places so probably not worth worrying about.

::: browser/components/extensions/ext-tabs.js:202
(Diff revision 1)
> +  },
> +
> +  handleWindowClose(window) {
> +    for (let tab of window.gBrowser.tabs) {
> +      if (this.adoptedTabs.has(tab)) {
> +        this.emitDetached(tab, this.adoptedTabs.get(tab));

It looks like the original implementation of tabs.onDetached wasn't listening for these events.  So with this change it seems like we'll trigger onDetached in cases that we weren't previously.  The new behavior seems logical but I'm missing enough of the bigger picture to evaluate which one is really right.  This also seems like the only place where you're using adoptedTabs, since it doesn't appear to correspond to anything in the original code, I'm not following exactly what its for.
Attachment #8743114 - Flags: review?(aswan)
https://reviewboard.mozilla.org/r/47577/#review44549

Yeah, once we add the listeners, they're never removed. I could implement refcounting, but I don't think it's worth it, given the extremely low overhead, and the low likelihood of event listeners being added and then permanently removed.

> It looks like the original implementation of tabs.onDetached wasn't listening for these events.  So with this change it seems like we'll trigger onDetached in cases that we weren't previously.  The new behavior seems logical but I'm missing enough of the bigger picture to evaluate which one is really right.  This also seems like the only place where you're using adoptedTabs, since it doesn't appear to correspond to anything in the original code, I'm not following exactly what its for.

I don't think that this ever happens in practice. I initially wrote this patch to deal with the corner case of a window being closed as a result of its last tab being moved to another window, but it turned out to be unnecessary in practice. I still think that it makes sense to keep the logic, though, just in case that changes.
Comment on attachment 8743114 [details]
MozReview Request: Bug 1238310: Part 4 - Refactor tab listener code. r?aswan

https://reviewboard.mozilla.org/r/47577/#review44565

::: browser/components/extensions/ext-tabs.js:202
(Diff revision 1)
> +  },
> +
> +  handleWindowClose(window) {
> +    for (let tab of window.gBrowser.tabs) {
> +      if (this.adoptedTabs.has(tab)) {
> +        this.emitDetached(tab, this.adoptedTabs.get(tab));

Having the logic there to cover all the bases seems prudent.  Is it worthy of a test?
Attachment #8743114 - Flags: review+
Attachment #8743111 - Flags: review?(adw) → review+
Comment on attachment 8743111 [details]
MozReview Request: Bug 1238310: Part 1 - Allow setting zoom for background tabs. r?adw

https://reviewboard.mozilla.org/r/47571/#review44921
Comment on attachment 8743112 [details]
MozReview Request: Bug 1238310: Part 2 - Return a promise from FullZoom.reset that resolves on completion. r?adw

https://reviewboard.mozilla.org/r/47573/#review44919

Hmm, this makes _getGlobalValue always async since promises resolve asyncly, but I guess that's OK.  The big thing around sync vs. async is switching tabs: when onLocationChange is called with aIsTabSwitch=true, we don't want the zoom level to start out at one value and then suddenly jump to a different value after it's fetched from the cps database.  _getGlobalValue isn't involved in then though.  I guess _getGlobalValue's calling its callback syncly was just an opportunistic thing.

::: browser/base/content/browser-fullZoom.js:285
(Diff revision 1)
>    },
>  
>    /**
>     * Sets the zoom level of the page in the given browser to the global zoom
>     * level.
>     */

Please add a @return comment.
Attachment #8743112 - Flags: review?(adw) → review+
https://reviewboard.mozilla.org/r/47573/#review44919

Yeah, that was what I figured. But the original code was implemented before we had native promises, and promise microtasks execute before we return to the main event loop, so I don't think it should have the same issues.

Thanks
Comment on attachment 8743115 [details]
MozReview Request: Bug 1238310: Part 5 - Implement the browser.tabs.onZoomChange event. r?gabor f?aswan

The browser bits are over my head but the bits in ext-tabs.js and related tests look sound to me.
Attachment #8743115 - Flags: feedback?(aswan) → feedback+
Keywords: leave-open
Comment on attachment 8743115 [details]
MozReview Request: Bug 1238310: Part 5 - Implement the browser.tabs.onZoomChange event. r?gabor f?aswan

https://reviewboard.mozilla.org/r/47579/#review45489

I'm not quite sure why you need the event to bubble but I'm not too concerned about it either.
Attachment #8743115 - Flags: review?(gkrizsanits) → review+
Comment on attachment 8743113 [details]
MozReview Request: Bug 1238310: Part 3 - Implement the base browser.tabs zoom API. r?aswan f?gabor

https://reviewboard.mozilla.org/r/47575/#review45491

I don't know much about ZoomManager, but after reading some code, this seems reasonable to me.
Attachment #8743113 - Flags: review+
Thanks
Keywords: leave-open
https://hg.mozilla.org/integration/fx-team/rev/2e75a8ab94a9331d0f84a4a6edb13a7843e904e8
Bug 1238310: Part 5 - Implement the browser.tabs.onZoomChange event. r=gabor f=aswan
Comment on attachment 8743113 [details]
MozReview Request: Bug 1238310: Part 3 - Implement the base browser.tabs zoom API. r?aswan f?gabor

I wanted to clear the feedback? from reviewboard but it just added an r+... anyway clearing it manually now.
Attachment #8743113 - Flags: feedback?(gkrizsanits)
https://hg.mozilla.org/mozilla-central/rev/2e75a8ab94a9
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Andy or Kris, are you planning to add release notes for Web Extensions changes in MDN for 49 and other versions? We plan to do that for a general "developer" link from the release notes but if you want a separate mention of Web Extensions we could also do that.
Flags: needinfo?(kmaglione+bmo)
Flags: needinfo?(amckay)
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #23)
> Andy or Kris, are you planning to add release notes for Web Extensions
> changes in MDN for 49 and other versions? We plan to do that for a general
> "developer" link from the release notes but if you want a separate mention
> of Web Extensions we could also do that.

I don't think its worth listing every bug, but rather big or important things in the release notes, so I was going to evaluate it on a bug by bug basis. Where would the "developer" link go to, MDN?
Flags: needinfo?(kmaglione+bmo)
Flags: needinfo?(amckay)
Depends on: 1313863
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.