This includes: getZoom() setZoom() getZoomSettings() setZoomSettings() onZoomChange

Review commit: https://reviewboard.mozilla.org/r/47571/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/47571/

Review commit: https://reviewboard.mozilla.org/r/47573/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/47573/

Review commit: https://reviewboard.mozilla.org/r/47575/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/47575/

Review commit: https://reviewboard.mozilla.org/r/47577/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/47577/

Review commit: https://reviewboard.mozilla.org/r/47579/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/47579/

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

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.

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?

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.

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.

https://hg.mozilla.org/integration/fx-team/rev/517ccf702ca23a525440b0617aae4dd9a88559f5 Bug 1238310: Part 1 - Allow setting zoom for background tabs. r=adw https://hg.mozilla.org/integration/fx-team/rev/660fd42c40243a9011595b694347ea42af0da06d Bug 1238310: Part 2 - Return a promise from FullZoom.reset that resolves on completion. r=adw https://hg.mozilla.org/integration/fx-team/rev/620da4d591b8d8fb80a8bf19714445d898d20554 Bug 1238310: Part 3 - Implement the base browser.tabs zoom API. r=aswan https://hg.mozilla.org/integration/fx-team/rev/f47b31f50a9a82984640ff6e17a86239808c8e15 Bug 1238310: Part 4 - Refactor tab listener code. r=aswan

https://hg.mozilla.org/integration/fx-team/rev/67e03d012ea92d93c128bd8b20bbea7fd6f048a2 Bug 1238310: Follow-up: Fix some timing issues in tests. r=bustage

https://hg.mozilla.org/mozilla-central/rev/517ccf702ca2 https://hg.mozilla.org/mozilla-central/rev/660fd42c4024 https://hg.mozilla.org/mozilla-central/rev/620da4d591b8 https://hg.mozilla.org/mozilla-central/rev/f47b31f50a9a https://hg.mozilla.org/mozilla-central/rev/67e03d012ea9

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.

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.

Thanks

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.

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.

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