Closed Bug 1122830 Opened 5 years ago Closed 5 years ago
Remove pinned tab APIs from UITour
39 bytes, text/x-review-board-request
The pinned tab APIs are getting in the way of cleaning up and simplifying teardown of the tours since it allows a tour loaded in a background tab to add annotations while the pinned tab is selected. This behaviour doesn't have tests. We only have the trivial stuff tested in test_pinnedTab so it makes it hard to touch this code. I think both dolske and Unfocused(?) agreed that we should remove the code until we actually need it and then we can design it for those specific needs.
/r/2641 - Bug 1122830 - Remove pinned tab APIs from UITour. r=Unfocused Pull down this commit: hg pull review -r d27149aaed561912b7c6351f5abbdbaa339e0fa2
https://reviewboard.mozilla.org/r/2639/#review1763 ::: browser/components/uitour/UITour.jsm (Diff revision 1) > - let selectedTab = window.gBrowser.selectedTab; > - let pinnedTab = this.pinnedTabs.get(window); > - if (pinnedTab && pinnedTab.tab == selectedTab) > - break; > - let originTabs = this.originTabs.get(window); > - if (originTabs && originTabs.has(selectedTab)) > - break; These were the main problems as they were `break`ing before the teardownTour call later in the `case`. It's simpler to always teardown when the selected tab changes (except for the tab detaching case which we may also be able to remove since Tomasz couldn't reproduce the original problem without the code below and I notice that the code is probably broken since the onPageEvent call is missing an argument. I commented in https://bugzilla.mozilla.org/show_bug.cgi?id=1089000#c6 about this.
Comment on attachment 8550649 [details] MozReview Request: bz://1122830/MattN https://reviewboard.mozilla.org/r/2639/#review1945 Ship It!
Attachment #8550649 - Flags: review?(bmcbride) → review+
TODO: remove from lib
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 38
5 years ago
Comment on attachment 8550649 [details] MozReview Request: bz://1122830/MattN Approval Request Comment [Feature/regressing bug #]: Makes uplifting for bug 1118874 dependencies easier and makes UITour consistent with Nightly where developers are testing. [User impact if declined]: None. This is unused code being cleaned up. [Describe test coverage new/current, TreeHerder]: deleted tests. Existing extensive UITour tests pass. [Risks and why]: Very low risk isolated to UITour. Code was never used. [String/UUID change made/needed]: None RyanVM/others: Note that I will do uplifts myself since there are many UITour patches to uplift in the correct order.
(In reply to Matthew N. [:MattN] from comment #9) > RyanVM/others: Note that I will do uplifts myself since there are many UITour > patches to uplift in the correct order. I rarely go through comments on uplifts (doesn't exactly scale well). Please throw something on the whiteboard or ping me directly in the future. Anyway, the Aurora uplifts were trivial, but I'll leave beta for you. https://hg.mozilla.org/releases/mozilla-aurora/rev/f9d325184938
You need to log in before you can comment on or make changes to this bug.