Closed Bug 1122830 Opened 5 years ago Closed 5 years ago

Remove pinned tab APIs from UITour

Categories

(Firefox :: Tours, defect)

defect
Not set
Points:
3

Tracking

()

RESOLVED FIXED
Firefox 38
Iteration:
38.1 - 26 Jan
Tracking Status
firefox36 --- fixed
firefox37 --- fixed
firefox38 --- fixed

People

(Reporter: MattN, Assigned: MattN)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Flags: qe-verify-
Flags: firefox-backlog+
/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
https://hg.mozilla.org/mozilla-central/rev/2719c6ce004e
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 38
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.
Attachment #8550649 - Flags: approval-mozilla-beta?
Attachment #8550649 - Flags: approval-mozilla-aurora?
Attachment #8550649 - Flags: approval-mozilla-beta?
Attachment #8550649 - Flags: approval-mozilla-beta+
Attachment #8550649 - Flags: approval-mozilla-aurora?
Attachment #8550649 - Flags: approval-mozilla-aurora+
(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
Attachment #8550649 - Attachment is obsolete: true
Attachment #8619171 - Flags: review+
You need to log in before you can comment on or make changes to this bug.