Closed Bug 813786 Opened 12 years ago Closed 11 years ago

Australis tabs Windows lightweight theme support

Categories

(Firefox :: Theme, enhancement, P1)

All
Windows 7
enhancement

Tracking

()

RESOLVED FIXED
Firefox 28

People

(Reporter: MattN, Assigned: MattN)

References

()

Details

Attachments

(4 files, 7 obsolete files)

Designs:
http://cl.ly/image/3q1d3q0J1Z3e
http://cl.ly/image/3X3C2w2s3w2H

Borders are the main thing missing
* Since the selected tab has opacity < 1, the border between the TabsToolbar and the navigation toolbar needs to be covered up.
No longer depends on: australis-tabs-win
*Since the theme's image was included in the clipped tab curve, I needed to create a separate clip path for the end of the tab so that the clipped lwt image isn't flipped on the end.
* I created winstripeShared.inc to share define's between browser.css and browser-lightweightTheme.css.  It seemed cleaner than moving CSS to a Makefile.

Known issue:
* Console warning when a theme doesn't provide browser-lightweightTheme.css. I think we can live with this since our own themes won't be affected. Appending the stylesheet dynamically adds more complexity and getting the time right may he hard.
Attachment #683776 - Attachment is obsolete: true
Attachment #692502 - Flags: review?(dao)
Attachment #692502 - Flags: feedback?(mconley)
Status: NEW → ASSIGNED
Comment on attachment 692502 [details] [diff] [review]
v.1 Use background-attachment:fixed to cover up the border at the bottom of the selected tab

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

This looks good to me - just two minor points.

::: browser/base/content/browser-addons.js
@@ +419,5 @@
> +/*
> + * Listen for Lightweight Theme styling changes and update the browser's theme accordingly.
> + */
> +let LightweightThemeListener = {
> +  _modifiedStyles: {},

You've defined _modifiedStyles as an Object, but it looks like you're mostly using it like an Array down in updateStyleSheet. I suggest using an Array instead.

@@ +461,5 @@
> +  },
> +
> +  // nsIObserver
> +  observe: function (aSubject, aTopic, aData) {
> +    if (!this.styleSheet)

I know that we've only registered this observer to observe the "lightweight-theme-styling-update" topic, but isn't it common practice to assert that aTopic equals our desired topic in the observe function? "Just in case"?
Attachment #692502 - Flags: feedback?(mconley) → feedback+
Once this patch has r+, we can probably begin porting to pinstripe and gnomestripe.
Attachment #692502 - Attachment is obsolete: true
Attachment #692502 - Flags: review?(dao)
Attachment #698009 - Flags: review?(dao)
(Quoting Guillaume C. [:ge3k0s] from bug 738491 comment #85)
> Since the v5 has landed on UX the tabs are circa 1px too much into the tab
> strip (exact same problem we have seen before with add-on manager tab)

The problem was with the specificity of two rules so I added !important per bug 813802 comment 53.

Dão, could you make this bug a slightly higher priority than the platform-specific Australis tab-related bugs so that it will unblock porting to pinstripe and gnomestripe.

I'll push the updated version to UX for further testing.
Attachment #698009 - Attachment is obsolete: true
Attachment #698009 - Flags: review?(dao)
Attachment #700611 - Flags: review?(dao)
Attached image Bug? (obsolete) —
When i drag a tab, the tab background is still the same as before dragging.
Is this bug or is planned to be like that?
Can you post detailed before/after screenshots to make it clear what exactly you're fixing here?

What's the performance cost of setting the (usually large) lightweight theme header image as the selected tab's background image? Does it affect tab switching speed or any tab strip animations?
My point is, by dragging the tabs, the tab-background does not change with personas... background is fixed, and changes only after dropping tab...
(In reply to Dão Gottwald [:dao] from comment #6)
> Can you post detailed before/after screenshots to make it clear what exactly
> you're fixing here?

Not sure if you mean screenshots for v2.1 or for the bug as a whole?

Before v.2.1: https://bug738491.bugzilla.mozilla.org/attachment.cgi?id=698602
After v2.1:   http://grab.by/j710

> What's the performance cost of setting the (usually large) lightweight theme
> header image as the selected tab's background image? Does it affect tab
> switching speed or any tab strip animations?

Doing some quick tests using the procedure from bug 824845 comment 7 doesn't show a large change although there is quite a bit of variation between test runs.

Tab open:
Without LWT background-image on selected tab: Interval: 62.4 Paint: 11.9
   With LWT background-image on selected tab: Interval: 68.6 Paint: 10.9
(In reply to Dão Gottwald [:dao] from comment #6)
> What's the performance cost of setting the (usually large) lightweight theme
> header image as the selected tab's background image? Does it affect tab
> switching speed or any tab strip animations?

Is this to prevent the overlapping tab borders issue with Personas, or something more?
(In reply to Matthew N. [:MattN] from comment #8)
> (In reply to Dão Gottwald [:dao] from comment #6)
> > Can you post detailed before/after screenshots to make it clear what exactly
> > you're fixing here?
> 
> Not sure if you mean screenshots for v2.1 or for the bug as a whole?

The whole bug.
(In reply to Dão Gottwald [:dao] from comment #10)
> (In reply to Matthew N. [:MattN] from comment #8)
> > (In reply to Dão Gottwald [:dao] from comment #6)
> > > Can you post detailed before/after screenshots to make it clear what exactly
> > > you're fixing here?
> > 
> > Not sure if you mean screenshots for v2.1 or for the bug as a whole?
> 
> The whole bug.

To be more clear, I'd like to know whether this bug fixes major visual flaws or provides minor polish, to get an idea of the cost/benefit ratio.

(In reply to Matthew N. [:MattN] from comment #8)
> > What's the performance cost of setting the (usually large) lightweight theme
> > header image as the selected tab's background image? Does it affect tab
> > switching speed or any tab strip animations?
> 
> Doing some quick tests using the procedure from bug 824845 comment 7 doesn't
> show a large change although there is quite a bit of variation between test
> runs.
> 
> Tab open:
> Without LWT background-image on selected tab: Interval: 62.4 Paint: 11.9
>    With LWT background-image on selected tab: Interval: 68.6 Paint: 10.9

So that's a 10% perf regression and perceivably less smooth, right?
Flags: needinfo?(mnoorenberghe+bmo)
Flags: needinfo?(mnoorenberghe+bmo)
(In reply to Dão Gottwald [:dao] from comment #11)
> (In reply to Dão Gottwald [:dao] from comment #10)
> > (In reply to Matthew N. [:MattN] from comment #8)
> > > (In reply to Dão Gottwald [:dao] from comment #6)
> > > > Can you post detailed before/after screenshots to make it clear what exactly
> > > > you're fixing here?
> > > 
> > > Not sure if you mean screenshots for v2.1 or for the bug as a whole?
> > 
> > The whole bug.
> 
> To be more clear, I'd like to know whether this bug fixes major visual flaws
> or provides minor polish, to get an idea of the cost/benefit ratio.

Comment 0 describes the gist of the bug.  It's basically about having the borders on the tabs and between the navbar and tab bar to match the designs linked there. The screenshots use the same window coordinates so you can flip between the two to see the difference. I consider this bug a portion of the Australis tabs design.  Does that answer your question?

> (In reply to Matthew N. [:MattN] from comment #8)
> > > What's the performance cost of setting the (usually large) lightweight theme
> > > header image as the selected tab's background image? Does it affect tab
> > > switching speed or any tab strip animations?
> > 
> > Doing some quick tests using the procedure from bug 824845 comment 7 doesn't
> > show a large change although there is quite a bit of variation between test
> > runs.
> > 
> > Tab open:
> > Without LWT background-image on selected tab: Interval: 62.4 Paint: 11.9
> >    With LWT background-image on selected tab: Interval: 68.6 Paint: 10.9
> 
> So that's a 10% perf regression and perceivably less smooth, right?

I don't think users will notice a <10ms difference although I am on fast hardware.  Do you have a better solution for covering the border between the navbar and tab-strip for the selected tab?
(In reply to Matthew N. [:MattN] from comment #13)
> > > Tab open:
> > > Without LWT background-image on selected tab: Interval: 62.4 Paint: 11.9
> > >    With LWT background-image on selected tab: Interval: 68.6 Paint: 10.9

I just realized that these numbers were with a non-optimized build which is probably less meaningful than an opt. benchmark
Stephen, can you create a spec of how to use the lightweight theme's background colour or dominant colour on the selected tab to cover up the border between the tabstrip and navbar?
Flags: needinfo?(shorlander)
Attachment #700611 - Flags: review?(dao)
Priority: -- → P1
Alternative approach using dominant color.
Flags: needinfo?(shorlander)
Blocks: 858637
Attached patch v.3 Unbitrot and use @movingtab (obsolete) — Splinter Review
I un-bitrotted the patch to re-evaluate performance.

Results using procedure from bug 837885 (see the spreadsheet there for raw data):
					Tab open: Int.	Paint Close: Int./Paint
% Change with LWT patch without LWT applied	-6.76%	-4.79%	-2.30%	8.42%
% Change with LWT patch and LWT applied		0.36%	11.98%	4.17%	36.59%

This patch improves performance when a LWT is not applied and that seems to outweigh the cost when a LWT is applied. avih from the performance team thought that this was acceptable.

I will work on the Linux and OS X themes using the same approach.
Attachment #700611 - Attachment is obsolete: true
Attachment #701409 - Attachment is obsolete: true
Attachment #736175 - Flags: review?(mconley)
Attachment #736175 - Flags: review?(dao)
(In reply to Matthew N. [:MattN] from comment #17)
> This patch improves performance when a LWT is not applied and that seems to
> outweigh the cost when a LWT is applied.

You make this sound like a tradeoff, that we need to regress LWT relative to non-LWT in order to improve non-LWT. This doesn't make sense to me. First of all, if we can improve performance for when a LWT is not applied, we should do that regardless of what we do for when a LWT is applied. Why is this improvement even part of this bug? Secondly, we should still aim for the same performance for when a LWT is applied.
Comment on attachment 736175 [details] [diff] [review]
v.3 Unbitrot and use @movingtab

Have you tried the solid color approach? Getting the color applied to the tab background and toolbar gradient might require some hacks, but if we can get that to work in a somewhat reasonable way, that seems preferable.
Attachment #736175 - Flags: review?(dao) → review-
(In reply to Dão Gottwald [:dao] from comment #19)
> Comment on attachment 736175 [details] [diff] [review]
> v.3 Unbitrot and use @movingtab
> 
> Have you tried the solid color approach? Getting the color applied to the
> tab background and toolbar gradient might require some hacks, but if we can
> get that to work in a somewhat reasonable way, that seems preferable.

shorlander developed a mock-up using the dominant colour, and between the two, says he prefers this original approach.
Attachment #736175 - Flags: review- → review?(dao)
Comment on attachment 736175 [details] [diff] [review]
v.3 Unbitrot and use @movingtab

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

The code looks good, and (while slightly bitrotted), the patch does the job of making lw-themes look good with the Australis tabs.

I'm in the process of running some performance numbers on Blake's little netbook. I'll update this bug with the results.

::: browser/base/content/browser.xul
@@ +10,5 @@
>  <?xml-stylesheet href="chrome://browser/content/places/places.css" type="text/css"?>
>  <?xml-stylesheet href="chrome://browser/skin/devtools/common.css" type="text/css"?>
>  <?xml-stylesheet href="chrome://browser/skin/" type="text/css"?>
> +<?xml-stylesheet href="chrome://browser/skin/browser-lightweightTheme.css" type="text/css"?>
> +# TODO: Solve warnings for themes/platforms which don't provide this

Let's file a follow-up bug for this.

::: browser/themes/shared/tabs.inc.css
@@ +142,5 @@
>  .tab-background-start[selected=true]:-moz-lwtheme::before,
>  .tab-background-end[selected=true]:-moz-lwtheme::before,
>  .tab-background-middle[selected=true]:-moz-lwtheme {
>    background-color: transparent;
> +  /*background-image: @fgTabTexture@;*/

Remove these commented out properties.

@@ +148,3 @@
>  }
>  
> +

Drop the newline.
Comment on attachment 736175 [details] [diff] [review]
v.3 Unbitrot and use @movingtab

(In reply to Mike Conley (:mconley) from comment #20)
> shorlander developed a mock-up using the dominant colour, and between the
> two, says he prefers this original approach.

Sure, I'm not questioning that preference based on aesthetic considerations. But I also hear that the dominant color approach is aesthetically acceptable, and between one approach that degrades perf and one that doesn't, the latter is preferable for that reason.

(I'm curious to see your numbers, but unless they somehow render Matt's numbers straight-out obsolete, it's unclear to me how they'd change things.)
Attachment #736175 - Flags: review?(dao) → review-
Ok, here are my numbers:

https://docs.google.com/spreadsheet/ccc?key=0Av64yYvszN34dDdZX0FYWEd3MVpEVzdjYXlFcGpUQmc#gid=4

I used the same testing technique that we used to test performance of the curvy-tabs, on the same machine.

The structure of the table should be pretty self-explanatory. I've highlighted what I think are the most interesting numbers in green and red (where green means a positive performance change, and red a negative one).

Probably the most important thing about these numbers is that, despite the dramatic percentages showing improvement or regression, the change is *never* greater than 3ms. 2.80 ms is as close as it gets.

Avi, I just want to confirm here - as a representative from the perf team who's been working on tab performance - is there anything in that spreadsheet that is cause for concern?
Flags: needinfo?(avihpit)
(In reply to Mike Conley (:mconley) from comment #23)
> Avi, I just want to confirm here - as a representative from the perf team
> who's been working on tab performance - is there anything in that
> spreadsheet that is cause for concern?

Yeah, I'm concerned that I'm not needed anymore ;)

These are beautiful numbers!

If we look at intervals only, which are the ultimate goal, then they either improve or stay the same, or, on one case, regress in <6% which IMO is marginally noise level.

Basically, LWT (patch and patch+theme) doesn't regress performance in any meaningful way. If anything, the patch (without theme applied) actually improves our "historically" worse case: tab open animation, from average interval of 21.5ms, to 18.9ms).

Great work!
Flags: needinfo?(avihpit)
(In reply to Avi Halachmi (:avih) from comment #24)
> Basically, LWT (patch and patch+theme) doesn't regress performance in any
> meaningful way. If anything, the patch (without theme applied) actually
> improves our "historically" worse case: tab open animation, from average
> interval of 21.5ms, to 18.9ms).

This is a misleading way to put it. See comment 18. The patch improves the baseline perf (great, but I'm not sure why that's part of this patch) and then regresses perf for LWT. The goal is not to have LWT regress perf, because 1) it's a widely used feature and 2) we're not at a point where the animations are smooth enough that a 10% regression wouldn't matter.
(In reply to Dão Gottwald [:dao] from comment #25)
> This is a misleading way to put it. See comment 18. The patch improves the
> baseline perf (great, but I'm not sure why that's part of this patch) and
> then regresses perf for LWT. The goal is not to have LWT regress perf,
> because 1) it's a widely used feature and 2) we're not at a point where the
> animations are smooth enough that a 10% regression wouldn't matter.

Before I get into this discussion, it occurred to me that I don't fully understand the reason for this patch.

According to the bug title, I initially thought it adds LWT support on windows for Australis (==the UX branch) which I assumed got broken between m-c and UX, however, since the spreadsheet includes UX (without this patch) with LWT applied (D/10-13), then it implies that we could use LW themes on windows even without the patch.

So what problem does this patch address?
I think comment 13 explains the issue.
Apologies. I re-read all of the comments here, and I now see that the purpose of this bug is to fix tab border issues of LWT with Australis, and that these issues are probably caused by the difference between Australis and the previous theme.

(In reply to Dão Gottwald [:dao] from comment #25)
> This is a misleading way to put it. See comment 18. The patch improves the
> baseline perf (great, but I'm not sure why that's part of this patch) and
> then regresses perf for LWT. The goal is not to have LWT regress perf,
> because 1) it's a widely used feature and 2) we're not at a point where the
> animations are smooth enough that a 10% regression wouldn't matter.

I fully agree that general improvements should be applied regardless of LWT, if we can do that. However, I don't think it's clear what case should be used as a baseline for regression evaluation.

I'll focus on tab open average interval, since it's both the worse value overall and the one with highest variance among the cases.

Here are the cases I could consider as baseline:

1. UX with LW theme applied but without the patch. That's the closest logical case IMO, and also the only case which the patch specifically addresses. On this case, the patch regresses <6%, which, as I noted on comment 24, is marginally within the noise level of these measurements (from past experience, I assess the noise level of these measurements as +/- 5%).

2. UX with the patch but without LW theme applied. Here we see a 13% regression, and it looks even worse compared to the other cases (m-c, unpatched UX), where applying a LWT doesn't seem to regress at all. However, at least some of this "regression" could be attributed to the improvement which the patch brings to the no-LWT case - 10%.


I can't tell if the patch "tradeoff" (nice win without LWT, marginal regression with LTW compared to unpatched, bigger regression with LWT compared to patched but no LWT) is mandatory or not. It might be possible to improve the no-LTW case and still also improve the LWT case, or maybe not. It would certainly take time to try and answer this question by trying various approaches.

The perspective which I chose is #1 above. The patch makes a change, so we evaluate the same case before and after the patch. These numbers are 10% improvement without LWT, and marginal regression with LWT. To me this means the patch is good.

However, it's indeed possible that we have another potential improvement for the case of patched with LWT compared to patched w/o LWT. I think this could be resolved in another patch though, as this bug served its purpose already by fixing the problem without adding a meaningful regression.
(In reply to Avi Halachmi (:avih) from comment #28)
> I can't tell if the patch "tradeoff" (nice win without LWT, marginal
> regression with LTW compared to unpatched, bigger regression with LWT
> compared to patched but no LWT) is mandatory or not. It might be possible to
> improve the no-LTW case and still also improve the LWT case, or maybe not.

Maybe Matt can clarify what part of this patch improves the non-LWT case.

> It would certainly take time to try and answer this question by trying
> various approaches.

My understanding is that the overhead comes from the lightweight theme's header image being used as the selected tab's background image. This can be avoided by using a color instead of an image, which is exactly what we already do for non-LWT, so it should perform pretty much equally.
(In reply to Dão Gottwald [:dao] from comment #29)
> 
> > It would certainly take time to try and answer this question by trying
> > various approaches.
> 
> My understanding is that the overhead comes from the lightweight theme's
> header image being used as the selected tab's background image. This can be
> avoided by using a color instead of an image, which is exactly what we
> already do for non-LWT, so it should perform pretty much equally.

So it sounds like Dão is advocating Plan B (using the dominant colour from the lw-theme image). This would involve us calculating that colour, or (more performantly) getting the colour from AMO.

Of the two approaches, the UX team preferred the aesthetics of Plan A (using the background image on the selected tab).

Some facts:
1) A patch for Plan A is already ready for review, whereas a patch for Plan B will require some time to develop, as well as coordination with the AMO team in order to get AMO serving up dominant colours for lwthemes.
2) avih from the perf team has pointed out that the Plan A patch really doesn't introduce any notable regressions as far as he's concerned - and the fact that the patch improved an unrelated aspect is, frankly, unrelated.
3) During the last Australis weekly checkpoint meeting, we decided that we'd go with Plan A unless it introduced unacceptable performance regressions, which it apparently does not.

So I think what we should do is go with this patch for now, and file a follow-up bug to investigate its potential performance gain.
(In reply to Mike Conley (:mconley) from comment #30)
> Some facts:
> 1) A patch for Plan A is already ready for review, whereas a patch for Plan
> B will require some time to develop, as well as coordination with the AMO
> team in order to get AMO serving up dominant colours for lwthemes.

Lightweight themes come with a manually set dominant color that could be used right now and possibly be replaced by some calculated color in a separate bug.

> 2) avih from the perf team has pointed out that the Plan A patch really
> doesn't introduce any notable regressions as far as he's concerned - and the
> fact that the patch improved an unrelated aspect is, frankly, unrelated.

It shouldn't be related, but for a reason that's still unclear to me it's part this patch and therefore muddies the numbers. This means that the performance problem of using the header image in the selected tab's background is hidden behind an unrelated improvement. This kind of improvement/regression trading would be fine if our animations were in a good shape, but they aren't, so we need to try do better where we can rather than arbitrarily declaring victory.

> 3) During the last Australis weekly checkpoint meeting, we decided that we'd
> go with Plan A unless it introduced unacceptable performance regressions,
> which it apparently does not.

It should have become clear by now that we have different understandings of what a regression is. Here's a different way to put it that doesn't require an arbitrary baseline: Lightweight themes don't degrade the animation smoothness today. If applying a lightweight theme starts degrading the animation smoothness, that's a regression.
(In reply to Dão Gottwald [:dao] from comment #31)
> ... so we need to try do better where we can rather
> than arbitrarily declaring victory.

The problem for which this bug was opened is fixed. There's (arguably) no meaningful regression compared to pre-patched performance. It's not an arbitrary declaration.

> It should have become clear by now that we have different understandings of
> what a regression is. Here's a different way to put it that doesn't require
> an arbitrary baseline: Lightweight themes don't degrade the animation
> smoothness today. If applying a lightweight theme starts degrading the
> animation smoothness, that's a regression.

It wasn't an arbitrary base line either. It was the ultimate base line: performance before and after the patch. The patch fixes case A (LWT applied) without a regression. It also improves case B (no-LWT). The fact that cases A and B are comparable and now there's a gap which wasn't there before shouldn't be regarded as a regression IMO.

It's true that a possible potential improvement was exposed by this patch, which we might wish to explore, but a new bug would be the perfect place to do that.
(In reply to Dão Gottwald [:dao] from comment #31)
> Lightweight themes don't degrade the animation
> smoothness today. If applying a lightweight theme starts degrading the
> animation smoothness, that's a regression.

...that we might be willing to live with, given other factors (mitigation of the impact from other general performance improvements, improved visual appearance, simplicity of implementation, etc.).

Given the discussion here, it sounds like we've reached the point where the costs of debate outweigh the potential costs of error in deciding one way or the other.

Let's go with "option A" (the existing patch) to continue making progress here. We can revisit adjustments or changes in strategy if followup bugs, if needed.
(In reply to Avi Halachmi (:avih) from comment #32)
> (In reply to Dão Gottwald [:dao] from comment #31)
> > ... so we need to try do better where we can rather
> > than arbitrarily declaring victory.
> 
> The problem for which this bug was opened is fixed. There's (arguably) no
> meaningful regression compared to pre-patched performance. It's not an
> arbitrary declaration.

I've explained like four times now that the before/after-patch comparison is bogus, given that the patch does more than just fixing this bug. How and why it does this still hasn't been explained.

(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #33)
> (In reply to Dão Gottwald [:dao] from comment #31)
> > Lightweight themes don't degrade the animation
> > smoothness today. If applying a lightweight theme starts degrading the
> > animation smoothness, that's a regression.
> 
> ...that we might be willing to live with, given other factors (mitigation of
> the impact from other general performance improvements,

We're not at a point where improvements made the animations generally smooth. That's why bug 738491 blocks bug 593680 -- there was a hope that australis tabs improve performance. Merging improvements with regressions in one patch such that the regressions can land under the cover sabotages this goal.
(In reply to Dão Gottwald [:dao] from comment #29)
> (In reply to Avi Halachmi (:avih) from comment #28)
> > I can't tell if the patch "tradeoff" (nice win without LWT, marginal
> > regression with LTW compared to unpatched, bigger regression with LWT
> > compared to patched but no LWT) is mandatory or not. It might be possible to
> > improve the no-LTW case and still also improve the LWT case, or maybe not.
> 
> Maybe Matt can clarify what part of this patch improves the non-LWT case.

I don't know what is causing the perf win because I didn't make any perf-specific changes in this patch.  I suspect we're hitting a faster code path in layout/gfx, perhaps related to layers?

(In reply to Dão Gottwald [:dao] from comment #34)
> (In reply to Avi Halachmi (:avih) from comment #32)
> > (In reply to Dão Gottwald [:dao] from comment #31)
> > > ... so we need to try do better where we can rather
> > > than arbitrarily declaring victory.
> > 
> > The problem for which this bug was opened is fixed. There's (arguably) no
> > meaningful regression compared to pre-patched performance. It's not an
> > arbitrary declaration.
> 
> I've explained like four times now that the before/after-patch comparison is
> bogus, given that the patch does more than just fixing this bug. How and why
> it does this still hasn't been explained.

The patch only makes changes in order to match the UI spec.  There are no unrelated (perf) changes in the patch that I can see. If you see some unrelated change, let me know and I will explain what it's for.

> We're not at a point where improvements made the animations generally
> smooth. That's why bug 738491 blocks bug 593680 -- there was a hope that
> australis tabs improve performance.

That is a hope but it's not a goal/requirement especially since Australis adds complexity related to the curves and the respective clip-paths.
Attachment #736175 - Attachment is obsolete: true
Attachment #736175 - Flags: review?(mconley)
Attachment #738423 - Flags: review?(mconley)
Attachment #738423 - Flags: review?(dao)
(In reply to Matthew N. [:MattN] from comment #35)
> I don't know what is causing the perf win because I didn't make any
> perf-specific changes in this patch.  I suspect we're hitting a faster code
> path in layout/gfx, perhaps related to layers?

We should understand this in order to trust it. Taking a regression because it seems to come with an improvement that nobody understands is even worse than taking it with an unrelated but understood improvement.

I just re-read the patch but didn't see any change that should affect the layout structure for :not(:-moz-lwtheme) in any positive way.

> > We're not at a point where improvements made the animations generally
> > smooth. That's why bug 738491 blocks bug 593680 -- there was a hope that
> > australis tabs improve performance.
> 
> That is a hope but it's not a goal/requirement especially since Australis
> adds complexity related to the curves and the respective clip-paths.

It used to be an explicit Australis tabs goal until we ran into perf problems, but anyway... It remains an overall goal, which means that we need to look for problems and opportunities on all fronts. I think we've identified a problem here: Duplicating a lightweight theme's header image as the selected tab's background images imposes 10-13% tab animation performance regression. And we have a plan B that Stephen made a mockup for.
(In reply to Dão Gottwald [:dao] from comment #36)
> I think we've
> identified a problem here: Duplicating a lightweight theme's header image as
> the selected tab's background images imposes 10-13% tab animation
> performance regression. And we have a plan B that Stephen made a mockup for.

And I'll note that this was exactly the concern that made us talk about alternative approaches.
(In reply to Dão Gottwald [:dao] from comment #36)
> (In reply to Dão Gottwald [:dao] from comment #31)
> > Lightweight themes don't degrade the animation
> > smoothness today. If applying a lightweight theme starts degrading the
> > animation smoothness, that's a regression.
> 
> ...that we might be willing to live with, given other factors (mitigation of
> the impact from other general performance improvements, improved visual
> appearance, simplicity of implementation, etc.).
> 
> Given the discussion here, it sounds like we've reached the point where the
> costs of debate outweigh the potential costs of error in deciding one way or
> the other.
> 
> Let's go with "option A" (the existing patch) to continue making progress
> here. We can revisit adjustments or changes in strategy if followup bugs, if
> needed.

A decision has been made here. Let's heed it and move on - let's review this patch, land it, and then attack performance and alternative approaches in follow-up bugs as Gavin suggested.
The problem with the followup-bug plan is that Matt, mconley, shorlander and I discussed beforehand how and when to change strategy and when this would be needed. The cost of of duplicating the header image is now known to be significant, and according to the original plan this would make us pick the alternative approach. So, what kind of future insight would make us file a followup bug and change strategy when we wouldn't do it now?

(I've also lost trust that we can count on a real perf improvement, regardless of our different views on whether an improvement would make an unrelated regression acceptable.)
Looking at the measurements in comment 23: I think the only useful comparisons are between mozilla-central and UX + this patch. For an apples-to-apples comparison, a real user in either LWT state will have an equal-or-better experience compared to what they would get from m-c today. Specifically:

  * m-c vs UX+patch (both without a LWT)
  * m-c vs UX+patch (both with a LWT)

Both of these are looking good-to-adequate. By most measurements it's a clear improvement or in the noise. Only the one Tab Open Paint measurement is an outlier (10.10ms --> 12.85ms) -- discussion indicates this has no end-user impact, and our performance team chaperones [;-)] are aware of and ok with the data.

Comparison with the current UX branch isn't so useful, as it's just an incremental step not yet visually ready for shipping. If we'd broken more, I'm sure it could look even faster.

Comparison between with-LWT and without-LWT also isn't useful. This line of argument doesn't make sense to me... It implies that if I had a patch that simply made without-LWT 50% faster on m-c today, it would be rejected because it thusly makes with-LWT "50% slower" in relative terms -- even though it didn't change in absolute terms. Opposite sides of the same coin. We're already watching for any regressions compared to m-c (see above), so the relative with/without isn't interesting. It's also of such a small relative magnitude (unlike my hypothetical 50%!) that I'm not concerned with relative user perception.

So, this all reinforces gavin's decision in comment 33. We've got a patch with the preferred visual style and with acceptable performance. Let's get this wrapped up and landed, so we can get back to other major issues. I also do not believe that debating this further is likely to lead to any significant user benefit.

dao: Did you have any other concerns with the patch? So far the debate has centered around performance. With that settled, this just needs a review from either you or mconley before landing.
(In reply to Dão Gottwald [:dao] from comment #36)

> > > [...] there was a hope that australis tabs improve performance.
> > 
> > That is a hope but it's not a goal/requirement especially since Australis
> > adds complexity related to the curves and the respective clip-paths.
> 
> It used to be an explicit Australis tabs goal until we ran into perf
> problems, but anyway... It remains an overall goal, which means that we need
> to look for problems and opportunities on all fronts.

I also wanted to address this, specifically. I'm not sure where this came from, but the goals of Australis have never been about performance. Only updating/improving the visual style and customizability (while avoiding making things _worse_). I have a long list of front-end performance TODOs, and once Australis is out of the way we can focus on those anew. Turning projects into "fix the world" projects is a surefire way to never ship.

I fully expect that we'll be revisiting the tab strip implementation soon, specifically for performance (and, conversely, not for visual refreshing!). http://avih.github.io/blog/2013/04/14/tabstrip-project-intro/
Comment on attachment 738423 [details] [diff] [review]
v.3 Unbitrot and use @movingtab (address mconley's feedback)

I can't provide further feedback on this, as I don't understand the advertised across-the-board performance improvement that you say makes this patch acceptable by mitigating the added overhead for lightweight themes.

Regardless of the above, I'll note again, just for clarity, that pushing this patch through contradicts our agreed upon plan, which was to measure the cost of duplicating the header image rather than comparing with the current mozilla-central or some UX branch state.
Attachment #738423 - Flags: review?(dao)
Comment on attachment 738423 [details] [diff] [review]
v.3 Unbitrot and use @movingtab (address mconley's feedback)

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

The code looks fine, but I'm curious about the movingtab bit (see below). Withholding r+ until I hear more about it.

::: browser/base/content/browser-addons.js
@@ +448,5 @@
> +   */
> +  updateStyleSheet: function(headerImage) {
> +    if (!this.styleSheet)
> +      return;
> +    for (let i = 0; i < this.styleSheet.cssRules.length; i++) {

If I understand correctly, this whole hack will no longer be necessary once CSS variables are landed and reliable for use. We should file a follow-up bug to update this when that happens.

@@ +462,5 @@
> +  },
> +
> +  // nsIObserver
> +  observe: function (aSubject, aTopic, aData) {
> +    if (aTopic != "lightweight-theme-styling-update")

These two conditionals can probably be combined.

::: browser/themes/windows/browser-lightweightTheme.css
@@ +10,5 @@
> + * image to the background-image for each of the following rulesets.
> + */
> +
> +/* Lightweight theme on tabs */
> +#tabbrowser-tabs:not([movingtab]) > .tabbrowser-tab > .tab-stack > .tab-background > .tab-background-start[selected=true]:-moz-lwtheme::before,

Why not([movingtab])? This condition is causing the border of the nav-bar to appear at the bottom of the tab when I drag it.
(In reply to Mike Conley (:mconley) from comment #43)
> Why not([movingtab])? This condition is causing the border of the nav-bar to
> appear at the bottom of the tab when I drag it.

This was to avoid attachment 701409 [details]. The approach was suggested by Dao in TO and I believe shorlander was fine with it.
Comment on attachment 738423 [details] [diff] [review]
v.3 Unbitrot and use @movingtab (address mconley's feedback)

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

Ok, we got that cleared up; we don't want the background image set because then it'd be static while one dragged the tab. This was a compromise we consciously picked (it was Dao's idea in Toronto, according to Matt).

So yeah, I'm good with this - assuming the other little nits I picked up are fixed.
Attachment #738423 - Flags: review?(mconley) → review+
Blocks: 864562
Attachment #738423 - Attachment is obsolete: true
Whiteboard: [fixed-in-ux]
Depends on: 866282
Depends on: 871151
Depends on: 879588
Depends on: 885139
No longer blocks: 858637
Depends on: 858637
https://hg.mozilla.org/mozilla-central/rev/69523ed020c3
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-ux]
Target Milestone: --- → Firefox 28
Depends on: 940515
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: