Closed
Bug 1089000
Opened 10 years ago
Closed 10 years ago
Remove broken code for detaching a tab to a new window
Categories
(Firefox :: Tours, defect)
Firefox
Tours
Tracking
()
People
(Reporter: tomasz, Assigned: MattN)
References
Details
Attachments
(1 file)
6.54 KB,
patch
|
Unfocused
:
review+
Sylvestre
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Updated•10 years ago
|
Assignee | ||
Comment 1•10 years ago
|
||
For follow-up bugs we make them block the original.
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?
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.
Comment 4•10 years ago
|
||
Hey tomasz - do you have some specific STR for us to reproduce this bug with?
Flags: needinfo?(tkolodziejski)
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.
Flags: needinfo?(tkolodziejski)
Updated•10 years ago
|
tracking-e10s:
--- → m7+
Updated•10 years ago
|
Flags: firefox-backlog+
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.
Component: General → Tours
Assignee | ||
Updated•10 years ago
|
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?
Flags: needinfo?(gijskruitbosch+bugs)
Comment 8•10 years ago
|
||
(In reply to Matthew N. [:MattN] from comment #7)
+1 from me, FWIW
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.
Flags: needinfo?(gijskruitbosch+bugs)
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.
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
Assignee | ||
Comment 11•10 years ago
|
||
I left the test since it's still valuable I think.
Attachment #8552599 -
Flags: review?(bmcbride)
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.
Attachment #8552599 -
Flags: review?(bmcbride) → review+
Assignee | ||
Comment 13•10 years ago
|
||
status-firefox38:
--- → fixed
Whiteboard: [fixed-in-fx-team]
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.
Comment 15•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 38
Assignee | ||
Comment 16•10 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.
Attachment #8552599 -
Flags: approval-mozilla-beta?
Attachment #8552599 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•10 years ago
|
status-firefox36:
--- → affected
status-firefox37:
--- → affected
Updated•10 years ago
|
Attachment #8552599 -
Flags: approval-mozilla-beta?
Attachment #8552599 -
Flags: approval-mozilla-beta+
Attachment #8552599 -
Flags: approval-mozilla-aurora?
Attachment #8552599 -
Flags: approval-mozilla-aurora+
Comment 17•10 years ago
|
||
Assignee | ||
Comment 18•10 years ago
|
||
You need to log in
before you can comment on or make changes to this bug.
Description
•