Closed Bug 1089000 Opened 5 years ago Closed 5 years ago

Remove broken code for detaching a tab to a new window

Categories

(Firefox :: Tours, defect)

defect
Not set
Points:
2

Tracking

()

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

People

(Reporter: tomasz, Assigned: MattN)

References

Details

Attachments

(1 file)

After detaching works for e10s we should fix 970321 which will be guarded by e10s if in patch 1073238 making the first step of e10sifying UITour.
Depends on: 1073238, 1012784, 970321
For follow-up bugs we make them block the original.
No longer depends on: 1073238, 970321, 1012784
That's pretty unnatural for me because they depend on the original bug (they cannot be solved before the original bug because they are follow-ups; But on the other hand we might want to keep the original bug opened as long as the follow-ups are not finished and that's probably the reason.)

But two of those bugs (bug 970321 and bug 1012784) I added as related so they should be in depends on rather than blocks, right?
(In reply to Tomasz Kołodziejski [:tomasz] from comment #2)
> But two of those bugs (bug 970321 and bug 1012784) I added as related so
> they should be in depends on rather than blocks, right?

Well, I see this as a follow-up to keep those fixes working in E10S. A way of looking at it is that follow-up blocks block the original since they are required in order to fully fix it.
Hey tomasz - do you have some specific STR for us to reproduce this bug with?
Flags: needinfo?(tkolodziejski)
All the information should be in bug 970321.

When fixing e10s tour I just explicitly if'd e10s (http://hg.mozilla.org/mozilla-central/file/a255a234946e/browser/modules/UITour.jsm#l245) because detaching was not working yet (so I couldn't test it) and also the code was a little bit tricky. And so this follow-up.
Flags: needinfo?(tkolodziejski)
Flags: firefox-backlog+
Blocks: 1115345
Note that I think the current code is broken even outside E10S since the line
> this.onPageEvent(this._queuedEvents.shift());
should pass 2 arguments to onPageEvent.
Component: General → Tours
No longer blocks: 1012784
Depends on: 1012784
Gijs, do you have any objection to just removing the code? It doesn't seem to handle dragging from one window to another (only detaching from a tab to a *new* window) and it's currently broken due to a regression (see comment 6) while the test still passes (possibly from the other fix in the patch which was fixing this case).

I think tours have since changed to use Page Visibility to redo UI annotations and IIRC I saw that event firing during a detach. That would mean that we should be fine simply discarding the events during the detach instead of queueing them (at least for UI annotations which I think would be the most common case for the original bug).

Thoughts?
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Matthew N. [:MattN] from comment #7)

+1 from me, FWIW
(In reply to Matthew N. [:MattN] from comment #7)
> Gijs, do you have any objection to just removing the code? It doesn't seem
> to handle dragging from one window to another (only detaching from a tab to
> a *new* window) and it's currently broken due to a regression (see comment
> 6) while the test still passes (possibly from the other fix in the patch
> which was fixing this case).
> 
> I think tours have since changed to use Page Visibility to redo UI
> annotations and IIRC I saw that event firing during a detach. That would
> mean that we should be fine simply discarding the events during the detach
> instead of queueing them (at least for UI annotations which I think would be
> the most common case for the original bug).
> 
> Thoughts?

It's not clear to me from my memory of how this worked (which is almost entirely gone) and your description in your second paragraph what the repercussions would be of removing the code (are you saying the tours will now "just cope" with being dragged about?), but if it's already broken and doing more bad than good, removing sounds fine to me.
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to :Gijs Kruitbosch from comment #9)
> Are you saying the tours will now "just cope" with being dragged about?

Yes, they should as long as we get visibilitychange during a detach.

Although I would like to mentor this, I'm just going to get it out of the way while other changes are landing so we can concentrate the changes in one release. I already wrote the patch as part of bug 1110602 then realized I should split it into it's own bug for discussion.
Assignee: nobody → MattN+bmo
Status: NEW → ASSIGNED
Iteration: --- → 38.1 - 26 Jan
Points: --- → 2
Flags: qe-verify-
Summary: [e10s] UITour: Fix detaching tab in e10s → Remove broken code for detaching a tab to a new window
I left the test since it's still valuable I think.
Attachment #8552599 - Flags: review?(bmcbride)
Comment on attachment 8552599 [details] [diff] [review]
v.1 Remove the broken code that wouldn't work in e10s anyways

Review of attachment 8552599 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/tabbrowser.xml
@@ -2655,5 @@
>            <![CDATA[
>              if (this.tabs.length == 1)
>                return null;
>  
> -            let event = new CustomEvent("TabBecomingWindow", {

Thankfully, I can't find any add-ons using this.
Attachment #8552599 - Flags: review?(bmcbride) → review+
(In reply to Blair McBride [:Unfocused] (I don't read bugmail - needinfo? me!) from comment #12)
> Thankfully, I can't find any add-ons using this.

Yeah, I checked add-ons MXR on the weekend.
https://hg.mozilla.org/mozilla-central/rev/98594b674c67
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 38
Comment on attachment 8552599 [details] [diff] [review]
v.1 Remove the broken code that wouldn't work in e10s anyways

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 broken code being deleted to simplify things and unbreak e10s.
[Describe test coverage new/current, TreeHerder]: Existing extensive UITour tests pass.
[Risks and why]: Very low risk isolated to UITour. Code broke months ago and hurts more than helps. It's also dealing with an edge-case of a tab detaching to a window while running a tour so wouldn't affect very few people if something broke.
[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 #8552599 - Flags: approval-mozilla-beta?
Attachment #8552599 - Flags: approval-mozilla-aurora?
Attachment #8552599 - Flags: approval-mozilla-beta?
Attachment #8552599 - Flags: approval-mozilla-beta+
Attachment #8552599 - Flags: approval-mozilla-aurora?
Attachment #8552599 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.