1122830 - Remove pinned tab APIs from UITour

Status: RESOLVED FIXED

(Firefox :: Tours, defect)

Description

[Matthew N. [:MattN]]

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.

Comment 1

[Matthew N. [:MattN]]

Attachment: MozReview Request: bz://1122830/MattN

Comment 2

[Matthew N. [:MattN]]

/r/2641 - Bug 1122830 - Remove pinned tab APIs from UITour. r=Unfocused

Pull down this commit:

hg pull review -r d27149aaed561912b7c6351f5abbdbaa339e0fa2

Comment 3

[Matthew N. [:MattN]]

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 4

[Blair McBride [:Unfocused] (UNAVAILABLE)]

https://reviewboard.mozilla.org/r/2641/#review1943

Ship It!

Comment 5

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8550649 [details]
MozReview Request: bz://1122830/MattN

https://reviewboard.mozilla.org/r/2639/#review1945

Ship It!

Comment 6

[Matthew N. [:MattN]]

TODO: remove from lib

Comment 7

[Matthew N. [:MattN]]

https://hg.mozilla.org/integration/fx-team/rev/2719c6ce004e

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2719c6ce004e

Comment 9

[Matthew N. [:MattN]]

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.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

(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

Comment 11

[Matthew N. [:MattN]]

https://hg.mozilla.org/releases/mozilla-beta/rev/32f28bbf1fb4

Comment 13

[Matthew N. [:MattN]]

Attachment: MozReview Request: Bug 1122830 - Remove pinned tab APIs from UITour. r=Unfocused