Back out bug 666816 from Firefox 26

RESOLVED FIXED in Firefox 26

Status

()

Toolkit
Find Toolbar
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: quicksaver, Assigned: mikedeboer)

Tracking

26 Branch
mozilla26
x86_64
Windows 7
Points:
---

Firefox Tracking Flags

(firefox26+ fixed, firefox27 wontfix)

Details

Attachments

(1 attachment, 3 obsolete attachments)

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:23.0) Gecko/20100101 Firefox/23.0 (Beta/Release)
Build ID: 20130814063812

Steps to reproduce:

Setting the target milestone for bug 666816 as Firefox 26 is premature in my opinion. This is a big change to the find bar's mechanisms, involving a whole new module written practically anew, or in many cases from copy-paste as I've seen mentioned in that bug a few times. I've just filed bug 916534 for a specific problem I found, and that wasn't from browsing the web, it was simply by looking at the code, and at the very first method I looked at in it as well.

Clearly there have been some things that were overlooked, which is understandable for a big rewrite of this kind of component. That is why it should stay in Nightly for at least another release cycle in my opinion. The few days that remain in the current one are not enough to test even the basics of this implementation, and definitely not enough if there are further fixes to be made to it.
As you mentioned, this kind of problem can happen, we you change a lot of code. With this kind of logic we would always have to land changes at the begging of a train, which might make sense for stuff like Australis, but at least the findbar is quite self contained, so that we could fix potential problem even on Aurora.
Status: UNCONFIRMED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → WONTFIX
Considering the number of regressions caused by bug 666816, I see no point in letting it ride the 26 train.
Status: RESOLVED → REOPENED
tracking-firefox26: --- → ?
Component: Untriaged → Find Toolbar
Ever confirmed: true
Product: Firefox → Toolkit
Resolution: WONTFIX → ---
Summary: Target milestone firefox 26 is premature for bug 666816 → Back out bug 666816 from Firefox 26
(Assignee)

Comment 3

5 years ago
I agree. Tom, I know this is quite an endeavor considering the number of follow-up patches that have landed, but I'm afraid that can't be helped. Having it on Aurora has gained us considerable and valuable feedback that we wouldn't have had otherwise.
Please don't hesitate to let me know if I can help in any way (remember I backed out findbar-on-top from Aurora before)!
Assignee: nobody → evilpies
Status: REOPENED → ASSIGNED
(Assignee)

Comment 4

5 years ago
Taking this, will add a patch on Monday.
Assignee: evilpies → mdeboer
I agree that this turned out to be necessary, thanks Mike for doing this job!
status-firefox26: --- → affected
tracking-firefox26: ? → +
Mike, any progress here?
(Assignee)

Comment 7

5 years ago
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #6)
> Mike, any progress here?

This is on my list for today!
(Assignee)

Comment 8

5 years ago
Created attachment 815305 [details] [diff] [review]
Patch 1: Backout findbar e10s refactor

Humongous, folded patch.
Attachment #815305 - Flags: review?(evilpies)
(Assignee)

Comment 9

5 years ago
Created attachment 815306 [details] [diff] [review]
Patch 2: Backout moving findbar to the top

Carrying over r=dao. See bug 914180.
Attachment #815306 - Flags: review+
(Assignee)

Comment 10

5 years ago
Try push (opted for one platform to get quick results, will opt for more if necessary): https://tbpl.mozilla.org/?tree=Try&rev=8e3bc23130a8
All the find bar on top stuff is added back like that (CSS, transition/scroll effects, append it as first child...). Since that has been backed out from central (bug 914180), I think this isn't supposed to happen.
Comment on attachment 815305 [details] [diff] [review]
Patch 1: Backout findbar e10s refactor

Thanks for working through this. I tried this backout, but the changes to findbar at the top made it indeed harder. I asked Mike to diff the findbar.xml and there was no change. :0
Attachment #815305 - Flags: review?(evilpies) → review+
(Assignee)

Comment 14

5 years ago
While discussing this on IRC with Tom:

<evilpie> Can you try diffing findbar.xml against a version before my stuff landed?
<evilpie> the rest looked good, but that file is just too big
<mikedeboer> evilpie: compared findbar.xml, aurora version with patches applied, with http://hg.mozilla.org/releases/mozilla-aurora/file/9bbb8bade260/toolkit/content/widgets/findbar.xml
<mikedeboer> evilpie: no difference, files entirely the same.
<evilpie> :) cool
(Assignee)

Comment 15

5 years ago
=== Please only land the patches on Aurora, this is a very specific backout. ===
Keywords: checkin-needed
(Assignee)

Comment 16

5 years ago
Created attachment 815440 [details] [diff] [review]
Patch 1.1: Backout findbar e10s refactor

Updated commit message. Carrying over r=evilpies
Attachment #815305 - Attachment is obsolete: true
Attachment #815440 - Flags: review+
Needs approval.
Keywords: checkin-needed
I was speaking to Felipe about this yesterday, and given the lack of traction on the regressions, I think we should back this out on trunk as well. Can the existing patch be used for that as well?
(Assignee)

Comment 19

5 years ago
(In reply to Dão Gottwald [:dao] from comment #17)
> Needs approval.

Whoops, forgot about that, sorry. :S
(Assignee)

Comment 20

5 years ago
Comment on attachment 815440 [details] [diff] [review]
Patch 1.1: Backout findbar e10s refactor

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 666816
User impact if declined: Weird issues while using the findbar
Testing completed (on m-c, etc.): bug 666816 still on m-c, this is an aurora-only backout
Risk to taking this patch (and alternatives if risky): minor
String or IDL/UUID changes made by this patch: none.
Attachment #815440 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 21

5 years ago
Comment on attachment 815306 [details] [diff] [review]
Patch 2: Backout moving findbar to the top

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 666816
User impact if declined: Weird issues while using the findbar, back on top.
Testing completed (on m-c, etc.): bug 666816 still on m-c, this is an aurora-only backout
Risk to taking this patch (and alternatives if risky): minor
String or IDL/UUID changes made by this patch: none
Attachment #815306 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

5 years ago
Attachment #815306 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

5 years ago
Attachment #815440 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 22

5 years ago
Per IRC discussion with Gavin, I will roll up the two patches into one and request approval for that.
(Assignee)

Comment 23

5 years ago
Created attachment 815787 [details] [diff] [review]
Patch 1.2: Backout findbar e10s refactor

Cumulative patch, carrying over r=evilpies.
Attachment #815306 - Attachment is obsolete: true
Attachment #815440 - Attachment is obsolete: true
Attachment #815787 - Flags: review+
(Assignee)

Comment 24

5 years ago
Comment on attachment 815787 [details] [diff] [review]
Patch 1.2: Backout findbar e10s refactor

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 666816
User impact if declined: Weird behavior while using the findbar
Testing completed (on m-c, etc.): bug 666816 still on m-c, this is an aurora-only backout
Risk to taking this patch (and alternatives if risky): minor
String or IDL/UUID changes made by this patch: none
Attachment #815787 - Flags: approval-mozilla-aurora?
Attachment #815787 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Comment 25

5 years ago
=== Please only land the patch on Aurora, this is a very specific backout. ===
Keywords: checkin-needed
https://hg.mozilla.org/releases/mozilla-aurora/rev/50d134c566c4
status-firefox26: affected → fixed
status-firefox27: --- → wontfix
Keywords: checkin-needed
Target Milestone: --- → mozilla26
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.