Tomasz Kołodziejski [:tomasz] Reporter
	Description • 10 years ago

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.

	Matthew N. [:MattN] Assignee
	Comment 1 • 10 years ago

For follow-up bugs we make them block the original.

	Tomasz Kołodziejski [:tomasz] Reporter
	Comment 2 • 10 years ago

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?

	Matthew N. [:MattN] Assignee
	Comment 3 • 10 years ago

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

	Mike Conley (:mconley) (:⚙️)
	Comment 4 • 10 years ago

Hey tomasz - do you have some specific STR for us to reproduce this bug with?

	Tomasz Kołodziejski [:tomasz] Reporter
	Comment 5 • 10 years ago

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.

	Matthew N. [:MattN] Assignee
	Comment 6 • 10 years ago

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.

	Matthew N. [:MattN] Assignee
	Comment 7 • 10 years ago

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?

	Blair McBride [:Unfocused] (UNAVAILABLE)
	Comment 8 • 10 years ago

(In reply to Matthew N. [:MattN] from comment #7)

+1 from me, FWIW

	:Gijs (he/him)
	Comment 9 • 10 years ago

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

	Matthew N. [:MattN] Assignee
	Comment 10 • 10 years ago

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

	Matthew N. [:MattN] Assignee
	Comment 11 • 10 years ago

I left the test since it's still valuable I think.

	Blair McBride [:Unfocused] (UNAVAILABLE)
	Comment 12 • 10 years ago

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.

	Matthew N. [:MattN] Assignee
	Comment 13 • 10 years ago

https://hg.mozilla.org/integration/fx-team/rev/98594b674c67

	Matthew N. [:MattN] Assignee
	Comment 14 • 10 years ago

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

	Carsten Book [:Tomcat]
	Comment 15 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/98594b674c67

	Matthew N. [:MattN] Assignee
	Comment 16 • 9 years ago

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.

	Ryan VanderMeulen [:RyanVM]
	Comment 17 • 9 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/2967a5af97cb

	Matthew N. [:MattN] Assignee
	Comment 18 • 9 years ago

https://hg.mozilla.org/releases/mozilla-beta/rev/fe2ce463edba