916536 - Back out bug 666816 from Firefox 26

Status: RESOLVED FIXED

(Toolkit :: Find Toolbar, defect)

Description

[Luís Miguel [:quicksaver] (not reading bugmail)]

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.

Comment 1

[Tom S [:evilpie]]

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.

Comment 2

[Dão Gottwald [:dao]]

Considering the number of regressions caused by bug 666816, I see no point in letting it ride the 26 train.

Comment 3

[Mike de Boer [:mikedeboer]]

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)!

Comment 4

[Mike de Boer [:mikedeboer]]

Taking this, will add a patch on Monday.

Comment 5

[Tom S [:evilpie]]

I agree that this turned out to be necessary, thanks Mike for doing this job!

Comment 6

[:Gavin Sharp [email: [e-mail]]]

Mike, any progress here?

Comment 7

[Mike de Boer [:mikedeboer]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #6)
> Mike, any progress here?

This is on my list for today!

Comment 8

[Mike de Boer [:mikedeboer]]

Attachment: Patch 1: Backout findbar e10s refactor

Humongous, folded patch.

Comment 9

[Mike de Boer [:mikedeboer]]

Attachment: Patch 2: Backout moving findbar to the top

Carrying over r=dao. See bug 914180.

Comment 10

[Mike de Boer [:mikedeboer]]

Try push (opted for one platform to get quick results, will opt for more if necessary): https://tbpl.mozilla.org/?tree=Try&rev=8e3bc23130a8

Comment 11

[Luís Miguel [:quicksaver] (not reading bugmail)]

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 12

[Luís Miguel [:quicksaver] (not reading bugmail)]

Nvm :)

Comment 13

[Tom S [:evilpie]]

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

Comment 14

[Mike de Boer [:mikedeboer]]

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

Comment 15

[Mike de Boer [:mikedeboer]]

=== Please only land the patches on Aurora, this is a very specific backout. ===

Comment 16

[Mike de Boer [:mikedeboer]]

Attachment: Patch 1.1: Backout findbar e10s refactor

Updated commit message. Carrying over r=evilpies

Comment 17

[Dão Gottwald [:dao]]

Needs approval.

Comment 18

[:Gavin Sharp [email: [e-mail]]]

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?

Comment 19

[Mike de Boer [:mikedeboer]]

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

Whoops, forgot about that, sorry. :S

Comment 20

[Mike de Boer [:mikedeboer]]

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.

Comment 21

[Mike de Boer [:mikedeboer]]

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

Comment 22

[Mike de Boer [:mikedeboer]]

Per IRC discussion with Gavin, I will roll up the two patches into one and request approval for that.

Comment 23

[Mike de Boer [:mikedeboer]]

Attachment: Patch 1.2: Backout findbar e10s refactor

Cumulative patch, carrying over r=evilpies.

Comment 24

[Mike de Boer [:mikedeboer]]

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

Comment 25

[Mike de Boer [:mikedeboer]]

=== Please only land the patch on Aurora, this is a very specific backout. ===

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/50d134c566c4