Closed
Bug 477948
Opened 16 years ago
Closed 11 years ago
"Keyhole" back/forward present on every major platform except Linux
Categories
(Firefox :: Theme, defect)
Tracking
()
VERIFIED
FIXED
Firefox 31
People
(Reporter: from_bugzilla3, Assigned: mikedeboer)
References
Details
(Whiteboard: [Australis:P?])
Attachments
(3 files, 9 obsolete files)
109.68 KB,
image/png
|
Details | |
3.31 KB,
patch
|
jaws
:
review+
|
Details | Diff | Splinter Review |
16.16 KB,
patch
|
mikedeboer
:
review+
Sylvestre
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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
Updated•16 years ago
|
Component: Toolbars → Theme
QA Contact: toolbars → theme
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.
Updated•11 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment 3•11 years ago
|
||
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?
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 :)
Comment 6•11 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?
Attachment #8334677 -
Attachment is obsolete: true
Flags: needinfo?(bcheng.gt)
Comment 7•11 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.
Flags: needinfo?(bcheng.gt)
Comment 8•11 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.
Comment 9•11 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.
Assignee | ||
Comment 10•11 years ago
|
||
This patch also work in RTL mode and doesn't hijack the forward button's hover effect.
Assignee: nobody → mdeboer
Attachment #8361991 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8389213 -
Flags: review?(jaws)
Comment 11•11 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.
Attachment #8389213 -
Flags: review-
Assignee | ||
Comment 12•11 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?
Flags: needinfo?(dao)
Comment 13•11 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.
Flags: needinfo?(dao)
Assignee | ||
Comment 14•11 years ago
|
||
Native colors win.
Attachment #8389213 -
Attachment is obsolete: true
Attachment #8389213 -
Flags: review?(jaws)
Attachment #8389266 -
Flags: review?(jaws)
Comment 15•11 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.
Comment 16•11 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
Assignee | ||
Comment 17•11 years ago
|
||
Attachment #8389838 -
Flags: review?(jaws)
Assignee | ||
Updated•11 years ago
|
Attachment #8389266 -
Attachment is obsolete: true
Attachment #8389266 -
Flags: review?(jaws)
Comment 18•11 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).
Comment 19•11 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.
Attachment #8389838 -
Flags: review?(jaws) → review-
Assignee | ||
Comment 20•11 years ago
|
||
Attachment #8389838 -
Attachment is obsolete: true
Attachment #8389920 -
Attachment is obsolete: true
Attachment #8390169 -
Flags: review?(jaws)
Assignee | ||
Updated•11 years ago
|
Attachment #8390169 -
Flags: review?(jaws)
Assignee | ||
Comment 21•11 years ago
|
||
Now also mirroring the identity-box CSS from the Windows theme.
Attachment #8390169 -
Attachment is obsolete: true
Attachment #8390192 -
Flags: review?(jaws)
Comment 22•11 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!
Attachment #8390192 -
Flags: review?(jaws) → review+
Assignee | ||
Comment 23•11 years ago
|
||
status-firefox29:
--- → affected
status-firefox30:
--- → affected
Whiteboard: [Australis:P?][fixed-in-fx-team]
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P?][fixed-in-fx-team] → [Australis:P?]
Target Milestone: --- → Firefox 30
Assignee | ||
Comment 25•11 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.
Assignee | ||
Comment 26•11 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
Attachment #8390192 -
Flags: approval-mozilla-aurora?
Comment 27•11 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?
Comment 28•11 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.
Attachment #8390192 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
Assignee | ||
Comment 29•11 years ago
|
||
Agreed, I won't be spending time on Talos regressions right now.
Comment 32•11 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
Comment 33•11 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.
Comment 34•11 years ago
|
||
Backed out on fx-team:
https://hg.mozilla.org/integration/fx-team/rev/46355c54bd1f
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: Firefox 30 → ---
Comment 36•11 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.
Comment 37•11 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.
Comment 38•11 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.
Comment 39•11 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
Comment 40•11 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
Comment 41•11 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.
Comment 42•11 years ago
|
||
Thanks. OK, let's land this, while keeping an eye on performance other than SVG opacity.
Comment 43•11 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.
Comment 44•11 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.
Comment 45•11 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
status-firefox31:
--- → affected
Whiteboard: [Australis:P?] → [Australis:P?][fixed-in-fx-team]
Comment 46•11 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.
Comment 47•11 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.
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
Flags: needinfo?(jaws)
Assignee | ||
Updated•11 years ago
|
Whiteboard: [Australis:P?][fixed-in-fx-team] → [Australis:P?]
Comment 49•11 years ago
|
||
Did the mochitests fail on the original landing on this patch too or is it just this time around?
Assignee | ||
Comment 50•11 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. :/
Comment 51•11 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!
Comment 52•11 years ago
|
||
Mike, can you pick this back up and check that this displays correctly?
Flags: needinfo?(jaws) → needinfo?(mdeboer)
Assignee | ||
Updated•11 years ago
|
Status: REOPENED → ASSIGNED
Assignee | ||
Comment 54•11 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?
Attachment #8390192 -
Attachment is obsolete: true
Attachment #8397119 -
Flags: review?(jaws)
Comment 55•11 years ago
|
||
Comment 56•11 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?
Comment 57•11 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.
Attachment #8397119 -
Flags: review?(jaws) → review-
Comment 58•11 years ago
|
||
Comment 59•11 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+.
Attachment #8397119 -
Flags: review- → review+
Assignee | ||
Comment 60•11 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.
Attachment #8397849 -
Flags: review?(jaws)
Assignee | ||
Comment 61•11 years ago
|
||
Margin fix. Carrying over r=jaws.
Attachment #8397119 -
Attachment is obsolete: true
Attachment #8397928 -
Flags: review+
Comment 62•11 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?
Attachment #8397849 -
Flags: review?(jaws) → review+
Assignee | ||
Comment 63•11 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!
Whiteboard: [Australis:P?] → [Australis:P?][fixed-in-fx-team]
Assignee | ||
Comment 64•11 years ago
|
||
I pushed to Try before, forgot to mention that here:
https://tbpl.mozilla.org/?tree=Try&rev=1973904f2e84
https://hg.mozilla.org/mozilla-central/rev/7baa06f4b49c
https://hg.mozilla.org/mozilla-central/rev/7fe4e2d84d76
Status: ASSIGNED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P?][fixed-in-fx-team] → [Australis:P?]
Target Milestone: --- → Firefox 31
Updated•11 years ago
|
Comment 66•11 years ago
|
||
uplift nominations for aurora/beta?
Updated•11 years ago
|
Flags: needinfo?(mdeboer)
Assignee | ||
Comment 67•11 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
Attachment #8397928 -
Flags: approval-mozilla-beta?
Attachment #8397928 -
Flags: approval-mozilla-aurora?
Flags: needinfo?(mdeboer)
Comment 68•11 years ago
|
||
(In reply to Mike de Boer [:mikedeboer] from comment #67)
> performance regressions have been analyzed and accepted.
What about bug 990084?
Assignee | ||
Comment 69•11 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.
Comment 70•11 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.
Attachment #8397928 -
Flags: approval-mozilla-beta?
Attachment #8397928 -
Flags: approval-mozilla-beta-
Attachment #8397928 -
Flags: approval-mozilla-aurora?
Attachment #8397928 -
Flags: approval-mozilla-aurora-
Assignee | ||
Comment 71•11 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.
Updated•11 years ago
|
Hardware: x86 → All
Assignee | ||
Comment 72•11 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.
Attachment #8397928 -
Flags: approval-mozilla-beta?
Attachment #8397928 -
Flags: approval-mozilla-beta-
Attachment #8397928 -
Flags: approval-mozilla-aurora?
Attachment #8397928 -
Flags: approval-mozilla-aurora-
Updated•11 years ago
|
Attachment #8397928 -
Flags: approval-mozilla-beta?
Attachment #8397928 -
Flags: approval-mozilla-beta+
Attachment #8397928 -
Flags: approval-mozilla-aurora?
Attachment #8397928 -
Flags: approval-mozilla-aurora+
Comment 73•11 years ago
|
||
Comment 74•11 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.
Status: RESOLVED → VERIFIED
Keywords: verifyme
Comment 75•11 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
You need to log in
before you can comment on or make changes to this bug.
Description
•