Stephan Sokolow Reporter
	Description • 15 years ago

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.0.6) Gecko/2009020905 Gentoo Firefox/3.0.6 Ubiquity/0.1.5
Build Identifier: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.0.6) Gecko/2009020905 Gentoo Firefox/3.0.6 Ubiquity/0.1.5

The Keyhole-style back/forward/history construct is present in the toolbar on all major platforms except Linux.

While desktop integration is all well and good, identity should also be maintained and Firefox doesn't really feel like firefox with such a distinctive GUI element missing.

It doesn't help that addons.mozilla.org lies about the workaround (the "XP on Vista" and "Vista on XP" themes) being Linux-incompatible. (They work perfectly well)

Reproducible: Always

	Michael Monreal [:micmon]
	Comment 1 • 14 years ago

I have been against the keyhole on linux for a long time now but seeing Steven's new (Fx4) mockups and hearing what people think about those vs native icons convinced me that at least having the choice to have the keyhole would be nice.

I would go one step further and make the keyhole default for all mozilla.org builds. Keeping GTK icon support should not be a big issue, so maybe distributions should decide if they ship a "mozilla identity" or "GTK integration" version of Firefox. Users could always switch to the other option.

	Paul [pwd]
	Comment 4 • 11 years ago

I'm assuming that as a result of the patch for this bug, the clipping on the download-finished icon will also be fixed? Or does that need a separate bug filed?

	Jared Wein [:jaws] (please needinfo? me)
	Comment 5 • 11 years ago

(In reply to Paul [sabret00the] from comment #4)
> I'm assuming that as a result of the patch for this bug, the clipping on the
> download-finished icon will also be fixed? Or does that need a separate bug
> filed?

This is already filed as bug 941737 :)

	Jared Wein [:jaws] (please needinfo? me)
	Comment 6 • 10 years ago

Brandon, I've got this patch mostly done but the clip-path for the back button isn't exact, and I know that you're planning on making some changes to the back button size. Could you help out here with fixing the clip-path?

	Brandon Cheng
	Comment 7 • 10 years ago

Definitely. Things are slowing down for me in terms of school work for next week. I plan to be knocking out those two bugs I'm assigned to in that time.

	Brandon Cheng
	Comment 8 • 10 years ago

I assume you'd like the adjusted clip-path after bug 893661 is patched, right? In that case, we're waiting on the assets.

	Jared Wein [:jaws] (please needinfo? me)
	Comment 9 • 10 years ago

(In reply to Brandon Cheng from comment #8)
> I assume you'd like the adjusted clip-path after bug 893661 is patched,
> right? In that case, we're waiting on the assets.

Yes, that's correct. I'll see what I can do to get the assets sooner.

	Mike de Boer [:mikedeboer] Assignee
	Comment 10 • 10 years ago

This patch also work in RTL mode and doesn't hijack the forward button's hover effect.

	Dão Gottwald [:dao]
	Comment 11 • 10 years ago

Comment on attachment 8389213 [details] [diff] [review]
Patch v1: Keyhole back/ forward button for Linux

We shouldn't hard-code non-native background and border colors for the url bar and search bar.

	Mike de Boer [:mikedeboer] Assignee
	Comment 12 • 10 years ago

(In reply to Dão Gottwald [:dao] from comment #11)
> We shouldn't hard-code non-native background and border colors for the url
> bar and search bar.

What about using -moz-field for the urlbar background and ThreeDShadow for the border?

	Dão Gottwald [:dao]
	Comment 13 • 10 years ago

(In reply to Mike de Boer [:mikedeboer] from comment #12)
> (In reply to Dão Gottwald [:dao] from comment #11)
> > We shouldn't hard-code non-native background and border colors for the url
> > bar and search bar.
> 
> What about using -moz-field for the urlbar background and ThreeDShadow for
> the border?

These are indeed the native colors that are supposed to be used for text fields. textbox.css uses them too.

	Mike de Boer [:mikedeboer] Assignee
	Comment 14 • 10 years ago

Native colors win.

	Dão Gottwald [:dao]
	Comment 15 • 10 years ago

Comment on attachment 8389266 [details] [diff] [review]
Patch v1.1: Keyhole back/ forward button for Linux

What about the focused state? -moz-appearance: textfield automatically gave us a native glow or colored border and you don't seem to have implemented an replacement.

	Jared Wein [:jaws] (please needinfo? me)
	Comment 16 • 10 years ago

Comment on attachment 8389266 [details] [diff] [review]
Patch v1.1: Keyhole back/ forward button for Linux

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

::: browser/base/content/browser.xul
@@ +1168,1 @@
>      <svg:clipPath id="windows-keyhole-forward-clip-path" clipPathUnits="objectBoundingBox">

we should rename this mask so it is obvious that it is used for linux and windows.

::: browser/themes/linux/browser.css
@@ +14,5 @@
>  %filter substitution
>  
>  %define forwardTransitionLength 150ms
>  %define conditionalForwardWithUrlbar window:not([chromehidden~="toolbar"]) #urlbar-container
> +%define conditionalForwardWithUrlbarWidth 27 

nit, trailing whitespace

	Jared Wein [:jaws] (please needinfo? me)
	Comment 18 • 10 years ago

With the patch applied, the identity block is too close to the back button when the forward button is hidden. This doesn't look bad when there is a notification showing (like the Remember Password notification).

	Jared Wein [:jaws] (please needinfo? me)
	Comment 19 • 10 years ago

Comment on attachment 8389838 [details] [diff] [review]
Patch v1.2: Keyhole back/ forward button for Linux

r- for the issue in the previous comment. looks good otherwise.

	Mike de Boer [:mikedeboer] Assignee
	Comment 21 • 10 years ago

Now also mirroring the identity-box CSS from the Windows theme.

	Jared Wein [:jaws] (please needinfo? me)
	Comment 22 • 10 years ago

Comment on attachment 8390192 [details] [diff] [review]
Patch v1.4: Keyhole back/ forward button for Linux

I tested it on my Ubuntu VM and it now looks better. Ship it!

	Mike de Boer [:mikedeboer] Assignee
	Comment 23 • 10 years ago

Landed as https://hg.mozilla.org/integration/fx-team/rev/15516aebd58d

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 24 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/15516aebd58d

	Mike de Boer [:mikedeboer] Assignee
	Comment 25 • 10 years ago

This caused two Talos regressions:

Regression: Fx-Team-Non-PGO - SVG, Opacity Row Major - Ubuntu HW 12.04 - 67.9% increase
---------------------------------------------------------------------------------------
   Previous: avg 157.792 stddev 36.136 of 12 runs up to revision 02be46ac2b55
   New     : avg 265.000 stddev 1.279 of 12 runs since revision 15516aebd58d
   Change  : +107.208 (67.9% / z=2.967)
   Graph   : http://mzl.la/1fC5A4G

and

Regression: Fx-Team-Non-PGO - Tab Animation Test - Ubuntu HW 12.04 - 5.75% increase
-----------------------------------------------------------------------------------
   Previous: avg 4.631 stddev 0.023 of 12 runs up to revision 02be46ac2b55
   New     : avg 4.897 stddev 0.029 of 12 runs since revision 15516aebd58d
   Change  : +0.266 (5.75% / z=11.482)
   Graph   : http://mzl.la/1fC5wSy

The SVG test regresses, because we're now using SVG clipping masks in the toolbar for the unified back/ forward button.

The TART test regresses, because of the same reason, actually.

I think both regression are expected & acceptable, thus I feel comfortable at this point to request uplift.

	Mike de Boer [:mikedeboer] Assignee
	Comment 26 • 10 years ago

Comment on attachment 8390192 [details] [diff] [review]
Patch v1.4: Keyhole back/ forward button for Linux

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Australis (but really? WAY BACK! - bug 682534 and bug 674744)
User impact if declined: All platforms have the Firefox-trademark-keyhole, except the Linux flavors. It's time to not let them feel left out anymore. This patch does regress the 'TART' and 'SVG, Opacity Row Major' Talos tests with 5.75% and 67.9% respectively, but they are expected as explained above.
Testing completed (on m-c, etc.): landed on m-c.
Risk to taking this patch (and alternatives if risky): minor, no known regressions apart from Talos.
String or IDL/UUID changes made by this patch: n/a

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 27 • 10 years ago

(In reply to Mike de Boer [:mikedeboer] from comment #25)
> The SVG test regresses, because we're now using SVG clipping masks in the
> toolbar for the unified back/ forward button.

Why does that regress an SVG test?

> The TART test regresses, because of the same reason, actually.

Why is it acceptable to regress tab animation performance here? How does TART performance on Linux compare to the other platforms, and does the keyhole have the same impact there?

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 28 • 10 years ago

Comment on attachment 8390192 [details] [diff] [review]
Patch v1.4: Keyhole back/ forward button for Linux

Doesn't seem like a viable uplift candidate until we sort out the performance regressions at the very least. I'm not convinced we should be spending time on this now.

	Mike de Boer [:mikedeboer] Assignee
	Comment 29 • 10 years ago

Agreed, I won't be spending time on Talos regressions right now.

	Dão Gottwald [:dao]
	Comment 30 • 10 years ago

Should this be backed out?

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 31 • 10 years ago

Sounds reasonable.

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

(In reply to :Gavin Sharp (email gavin@gavinsharp.com) from comment #27)
> (In reply to Mike de Boer [:mikedeboer] from comment #25)
> > The SVG test regresses, because we're now using SVG clipping masks in the
> > toolbar for the unified back/ forward button.
> 
> Why does that regress an SVG test?

It shouldn't, I wouldn't have thought. No idea why this happened, primarily because I thought this was a core/layout/gfx perf test, which shouldn't be affected by a change in the UI such as this one.

> > The TART test regresses, because of the same reason, actually.
> 
> Why is it acceptable to regress tab animation performance here? How does
> TART performance on Linux compare to the other platforms, and does the
> keyhole have the same impact there?

We don't know because there was no TART test at the time, because we didn't have TART, did we? It's a known issue with TART that it is affected by UI updates that happen when you switch tabs (id est updating the bookmarks star, back button state, identity indicator panel in the URL bar, etc. causes reflows, which make the animation frame rate drop, which regresses TART). I wouldn't have thought that this is any worse than the drag that's already being caused by the same stuff on Windows or Mac. In any case, that seems very hard to figure out.

As compared to other platforms, Linux is comfortably smoother (although I admittedly don't know how our talos reference platforms compare perf-wise and to what extent that skews the numbers):

http://graphs.mozilla.org/graph.html#tests=[[293,132,33],[293,132,37],[293,132,25]]&sel=1394699082554.1133,1394757890047.4229&displayrange=7&datatype=running

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 33 • 10 years ago

(In reply to :Gijs Kruitbosch from comment #32)
> We don't know because there was no TART test at the time, because we didn't
> have TART, did we? It's a known issue with TART that it is affected by UI
> updates that happen when you switch tabs (id est updating the bookmarks
> star, back button state, identity indicator panel in the URL bar, etc.
> causes reflows, which make the animation frame rate drop, which regresses
> TART). I wouldn't have thought that this is any worse than the drag that's
> already being caused by the same stuff on Windows or Mac. In any case, that
> seems very hard to figure out.

What about disabling "the same stuff" on Windows/Mac and pushing to talos?

In any case, leading-questions aside, my main point is just that regressing Talos tests requires more justification and thought than what was put into comment 25, particularly for changes of that magnitude, and especially if the win you're trading off isn't significant. We don't have a lot of time at the moment to do that investigation, given other Australis commitments, so let's back this out and revisit when we do.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 34 • 10 years ago

Backed out on fx-team:
https://hg.mozilla.org/integration/fx-team/rev/46355c54bd1f

	Avi Halachmi (:avih)
	Comment 36 • 10 years ago

(In reply to :Gavin Sharp (email gavin@gavinsharp.com) from comment #27)
> Why does that regress an SVG test?

I added a note at https://wiki.mozilla.org/Buildbot/Talos/Tests#tsvg-opacity .

In a nutshell: all the page load tests measure from before the location is changed until onload. Therefore chrome perf from location change is measured as well. SVG opacity specifically is rather quick, so the changes in percentage is bigger than with other page load tests.

(In reply to Mike de Boer [:mikedeboer] from comment #25)
> This caused two Talos regressions:
> 
> Regression: Fx-Team-Non-PGO - SVG, Opacity Row Major - Ubuntu HW 12.04 - 67.9% increase
> ---------------------------------------------------------------------------------------
>    Previous: avg 157.792 stddev 36.136 of 12 runs up to revision 02be46ac2b55
>    New     : avg 265.000 stddev 1.279 of 12 runs since revision 15516aebd58d
>    Change  : +107.208 (67.9% / z=2.967)
>    Graph   : http://mzl.la/1fC5A4G

Specifically for this regression, if we ignore the percentage, we see 110ms regression, which, if indeed is a result of this patch, is probably from the chrome perf on location change, and IMO too much to land as is.

	Paul [pwd]
	Comment 37 • 10 years ago

With Firefox about to undergo the massive promotion and scrutiny that comes with the launch of Australis we need to have a unified product. The keyhole is a part of the Firefox brand identity. And given that there's no movement on fixing regressions that bring a platform up to par with the rest, it's absurd that this was backed out in the first place. There's not even evidence to say that this is platform specific given the changes, just that Linux never got its keyhole before such effects were measurable. This is a case of branding versus performance, so I think it's a call for Product? But the argument for performance hasn't been made properly, thus at this point branding wins by default.

	Avi Halachmi (:avih)
	Comment 38 • 10 years ago

OK, let's split the discussion into the two regressions:

1. TART: this is "only" 6%, and at least from the graph linked from comment 32, Linux is better than other platforms to begin with, so unless someone can visually notice the regression from this patch with tab animation, I'd say that's not a blocker. But we do need someone with Linux to play for few mins with Firefox with and without the patch, and confirm that there's no noticeable regression in tab animation. Volunteers?

2. SVG Opacity. This is a tougher one. I'm looking at this graph: http://graphs.mozilla.org/graph.html#tests=[[225,132,33]]&sel=1387893593024,1395669593024&displayrange=90&datatype=running (m-i looks very similar).

Before Jan-16 it was around 130. Between Jan-16 and Feb-1 they were around 260. On Feb-1 they returned to ~130, and on Feb-19 they became multi-modal with buckets (in decreasing probability) at around 135, 175, 265 and few around 300 (with average around 160).

This patch apparently made the 260 bucket the dominant one, and it returned to the multi-modal behavior once it got backed out.

It's a hard call without understanding why the results became multi-modal to begin with and why did the patch make them more deterministic, however, considering the importance of keeping the brand identity, I'd say this also needs to be assessed by a human. I.e. load few pages with and without the patch and see if there's a noticeable difference.


We should remember that numbers are just numbers. The important factor is whether or not users will suffer from performance. So while the numbers help us notice things which we otherwise might not have, it's us which call the shots based on common sense, while taking the numbers into account as well.

So first, someone with native (not VM) linux should compare the behavior visually with and without the patch and report here. Then, assuming no major differences was observed wrt performance, I'd say this is not a blocker.

Regardless, since this patch affected 2 tests, it might be worth another review, maybe there are some low hanging fruits here.

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

Build with patch:

http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-linux/1394762989/firefox-30.0a1.en-US.linux-i686.tar.bz2

Build with patch backed out:

http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-linux/1395031757/firefox-30.0a1.en-US.linux-i686.tar.bz2

	Avi Halachmi (:avih)
	Comment 40 • 10 years ago

Still at SVG Opacity, looking a year back, it seems that except for (not continuous) few months, this test has been multimodal for a long time. This increases the chance that this patch didn't necessarily regressed explicitly, but somehow made the test more deterministic on one of the (higher) buckets.

http://graphs.mozilla.org/graph.html#tests=[[225,132,33]]&sel=none&displayrange=365&datatype=running

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

I just tested those two builds on a Linux Mint machine we had here in the office. I didn't notice a discernible performance difference in tab animation, or general browser performance.

	Avi Halachmi (:avih)
	Comment 42 • 10 years ago

Thanks. OK, let's land this, while keeping an eye on performance other than SVG opacity.

	Chris Coulson
	Comment 43 • 10 years ago

I've compared both of these builds (well, actually, I tested the equivalent 64-bit builds) on my Ubuntu machine. I didn't notice any difference in performance between the 2 builds when opening / closing / switching between tabs and navigating within sites.

	Jared Wein [:jaws] (please needinfo? me)
	Comment 44 • 10 years ago

Something I wanted to see was how much difference the presence of an unused SVG clip path would make on the SVG Opacity talos test.

I pushed three csets to the try server so we can get data on this:
Baseline: https://tbpl.mozilla.org/?tree=Try&rev=ddedc49c62ad
With keyhole: https://tbpl.mozilla.org/?tree=Try&rev=0316e12a38c9
With keyhole + unused clip path: https://tbpl.mozilla.org/?tree=Try&rev=a1df4d0cfbb3

After I get the results I'm planning on re-landing the keyhole patch on fx-team.

	Jared Wein [:jaws] (please needinfo? me)
	Comment 45 • 10 years ago

The try results above show that adding an unused SVG clip path show a similar performance hit for SVG Opacity.

Landed on fx-team, https://hg.mozilla.org/integration/fx-team/rev/d8846585295f

	Andreas Tolfsen ❲:ato❳
	Comment 46 • 10 years ago

Having performed the same test as Chris Coulson above, I came to the same conclusions on my 64-bit Ubuntu machine in the London office.

	Paul [pwd]
	Comment 47 • 10 years ago

I tested on a i386 machine with ten tabs open and my results match those of Chris and :ato. It's fantastic news, as it's looking like we'll be able to launch complete Australis across all platforms at once.

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 48 • 10 years ago

I had to back this out in https://hg.mozilla.org/integration/fx-team/rev/d8846585295f for apparently breaking Windows XP debug mochitests.

Not sure how or why, really, but retriggers of tests point to this commit as the cause.

Failures look like this: https://tbpl.mozilla.org/php/getParsedLog.php?id=36633724&tree=Fx-Team

	Paul [pwd]
	Comment 49 • 10 years ago

Did the mochitests fail on the original landing on this patch too or is it just this time around?

	Mike de Boer [:mikedeboer] Assignee
	Comment 50 • 10 years ago

(In reply to Paul [sabret00the] from comment #49)
> Did the mochitests fail on the original landing on this patch too or is it
> just this time around?

Just this time around. :/

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

(In reply to Paul [sabret00the] from comment #49)
> Did the mochitests fail on the original landing on this patch too or is it
> just this time around?

This time around. As discussed on IRC yesterday, this patch is affected by a recent change to the SVG paths (bug 893661), and therefore it now needs some more testing / updating. Please let Jared and Mike take care of it, they're more than capable. Thanks!

	Jared Wein [:jaws] (please needinfo? me)
	Comment 52 • 10 years ago

Mike, can you pick this back up and check that this displays correctly?

	Mike de Boer [:mikedeboer] Assignee
	Comment 53 • 10 years ago

Absolutely!

	Mike de Boer [:mikedeboer] Assignee
	Comment 54 • 10 years ago

Unbitrotted patch that works with the updated clip-paths. I don't have a working public key yet, so I can't push to try or anything. Jared, could you do the honors?

	Jared Wein [:jaws] (please needinfo? me)
	Comment 55 • 10 years ago

https://tbpl.mozilla.org/?tree=Try&rev=288142c1911b

	Brandon Cheng
	Comment 56 • 10 years ago

(In reply to Mike de Boer [:mikedeboer] from comment #54)
> Created attachment 8397119 [details] [diff] [review]
> Patch v1.5: Keyhole back/ forward button for Linux
> 
> Unbitrotted patch that works with the updated clip-paths.

I'm actually a bit curious to know. Does calculating an arc path instead of a cubic one perform better?

	Jared Wein [:jaws] (please needinfo? me)
	Comment 57 • 10 years ago

Comment on attachment 8397119 [details] [diff] [review]
Patch v1.5: Keyhole back/ forward button for Linux

This is breaking the combined bookmarks/bookmarks-menu button. See attached screenshot.

	Jared Wein [:jaws] (please needinfo? me)
	Comment 59 • 10 years ago

Comment on attachment 8397119 [details] [diff] [review]
Patch v1.5: Keyhole back/ forward button for Linux

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

Setting margin-top: 3px; margin-bottom: 3px; on .toolbarbutton-menubutton-dropmarker fixes the issue for me. With that fixed you've got r+.

	Mike de Boer [:mikedeboer] Assignee
	Comment 60 • 10 years ago

I had to update the notifications test, because test #15 places a normal button in the notification are, which has standard GTK styling and dimensions.
Due to the clip-path, this button gets clipped off, which makes it necessary to synthesize the mouse click a little bit downward. To be safe, I adjusted the x-offset with the same amount.

	Mike de Boer [:mikedeboer] Assignee
	Comment 61 • 10 years ago

Margin fix. Carrying over r=jaws.

	Jared Wein [:jaws] (please needinfo? me)
	Comment 62 • 10 years ago

Comment on attachment 8397849 [details] [diff] [review]
Patch 2: update test

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

::: browser/base/content/test/general/browser_popupNotification.js
@@ +180,5 @@
>  var tests = [
>    { // Test #0
>      run: function () {
>        this.notifyObj = new basicNotification();
> +      dump("0-1: " + this.notifyObj.id + "\n");

please remove these dumps

@@ +534,5 @@
>        this.notifyObj.anchorID = this.box.id = "nested-box";
>        this.notifyObj.addOptions({dismissed: true});
>        this.notification = showNotification(this.notifyObj);
>  
> +      EventUtils.synthesizeMouse(button, 4, 4, {});

Can you add a comment about needing to move the point due to the clip path?

	Mike de Boer [:mikedeboer] Assignee
	Comment 63 • 10 years ago

Pushed both patches with comments addressed:

https://hg.mozilla.org/integration/fx-team/rev/7baa06f4b49c
https://hg.mozilla.org/integration/fx-team/rev/7fe4e2d84d76

Thanks Jared!

	Mike de Boer [:mikedeboer] Assignee
	Comment 64 • 10 years ago

I pushed to Try before, forgot to mention that here:

https://tbpl.mozilla.org/?tree=Try&rev=1973904f2e84

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 65 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/7baa06f4b49c
https://hg.mozilla.org/mozilla-central/rev/7fe4e2d84d76

	Lukas Blakk [:lsblakk] use ?needinfo
	Comment 66 • 10 years ago

uplift nominations for aurora/beta?

	Mike de Boer [:mikedeboer] Assignee
	Comment 67 • 10 years ago

Comment on attachment 8397928 [details] [diff] [review]
Patch v1.6: Keyhole back/ forward button for Linux

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Australis
User impact if declined:

The 'keyhole' conditional back/ forward button is one of Firefox' most identifying UI elements, which is already present on Windows and OSX.
With Australis, the keyhole fits even better in the 'curvy' design.
This set of patches implements the keyhole on Linux, so these users also get this experience.

Testing completed (on m-c, etc.):

This has landed on m-c. Bug 989701 needs to be fixed first, before I push any patch to aurora or beta.

Risk to taking this patch (and alternatives if risky):

atm, no major regressions, apart from bug 989701, have been reported and the performance regressions have been analyzed and accepted. If anything blocking comes up on aurora or beta, the patches can be backed out quite easily (few to no dependencies).

String or IDL/UUID changes made by this patch: n/a

	Dão Gottwald [:dao]
	Comment 68 • 10 years ago

(In reply to Mike de Boer [:mikedeboer] from comment #67)
> performance regressions have been analyzed and accepted.

What about bug 990084?

	Mike de Boer [:mikedeboer] Assignee
	Comment 69 • 10 years ago

(In reply to Dão Gottwald [:dao] from comment #68)
> What about bug 990084?

Well, ehm, I didn't count that one yet... :/ So, I will hold off with uplifting any patches until the tresize regression is explained/ resolved.

	Sylvestre Ledru [:Sylvestre]
	Comment 70 • 10 years ago

Comment on attachment 8397928 [details] [diff] [review]
Patch v1.6: Keyhole back/ forward button for Linux

Refuse the uplift for now (cf comment #69). Don't hesitate to resubmit when the performance regression are fixed.

	Mike de Boer [:mikedeboer] Assignee
	Comment 71 • 10 years ago

Pushed to Aurora to get a clearer picture of the regressions and I wanted to know if the tresize regression was truly caused by this bug.

baseline Try push: https://tbpl.mozilla.org/?tree=Try&rev=fcf193449da4
keyhole Try push: https://tbpl.mozilla.org/?tree=Try&rev=32c9583cea1c

http://compare-talos.mattn.ca/?oldRevs=fcf193449da4&newRev=32c9583cea1c&server=graphs.mozilla.org&submit=true

The table shows that the newly introduced keyhole does indeed regress all the things. We're OK with TART (bug 990163) and SVG Opacity regressing as they are (see comment 38), but tresize seems too much. We need more investigation here. This work will be tracked in bug 990084.

	Mike de Boer [:mikedeboer] Assignee
	Comment 72 • 10 years ago

Comment on attachment 8397928 [details] [diff] [review]
Patch v1.6: Keyhole back/ forward button for Linux

Please see the approval request in comment 67.

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

Aurora:

https://hg.mozilla.org/releases/mozilla-aurora/rev/5e17be6d9e03
https://hg.mozilla.org/releases/mozilla-aurora/rev/45ed5edb396b


Beta:

https://hg.mozilla.org/releases/mozilla-beta/rev/871c60982cac
https://hg.mozilla.org/releases/mozilla-beta/rev/ff6f90421768

	Camelia Badau [:cbadau], Desktop QA
	Comment 74 • 10 years ago

Mozilla/5.0 (X11; Linux i686; rv:29.0) Gecko/20100101 Firefox/29.0
Mozilla/5.0 (X11; Linux i686; rv:30.0) Gecko/20100101 Firefox/30.0
Mozilla/5.0 (X11; Linux i686; rv:31.0) Gecko/20100101 Firefox/31.0

Verified as fixed on Ubuntu 13.10 x86 using Firefox 29.0b8 and the 04/15 Firefox 30.0a2 and 31.0a1.

	Joel Maher ( :jmaher ) (UTC -8)
	Comment 75 • 10 years ago

when this landed on beta, we see a 12-13% regression on windows 8 for the tart test, I verified by backing out the patch:
https://tbpl.mozilla.org/?tree=Try&rev=be431ebd29a0