821859 - Use stylesheet for tab delay resizing instead of setting each tab.style

Status: RESOLVED WONTFIX

(Firefox :: Tabbed Browser, defect)

Description

[ithinc]

A step to push bug 649654.

Comment 1

[ithinc]

Attachment: patch

Comment 2

[ithinc]

Attachment: let spacer grows instead of calculating tab width

Comment 3

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

Comment on attachment 692686 [details] [diff] [review]
let spacer grows instead of calculating tab width

This is not my area of expertise - Frank or MattN may have opinions, though.

Comment 4

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

Comment on attachment 692430 [details] [diff] [review]
patch

I assume this patch is obsoleted by the latest one.

Comment 5

[Frank Yan (:fryn)]

Comment on attachment 692686 [details] [diff] [review]
let spacer grows instead of calculating tab width

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

Thanks for writing this patch. :)
The basic framework is good, but there is one unexpected problem that is keeping this patch from working:

::: browser/base/content/browser.css
@@ +51,5 @@
>                max-width 250ms ease-out,
>                opacity 50ms ease-out 180ms /* hide the tab for the last 20ms of the max-width transition */;
>  }
>  
> +.tabbrowser-tabs[dontresize] > .tabbrowser-tab:not([pinned])[fadein] {

Change the selector text to:
.tabbrowser-tabs[dontresize] > .tabbrowser-tab[fadein]:not([pinned])

Change the text of the selector above it to:
.tabbrowser-tab:not([fadein]):not([pinned])

See related comment in tabbrowser.xml for the reason.

::: browser/base/content/tabbrowser.xml
@@ +1669,5 @@
>              // Remove the tab's filter and progress listener.
>              const filter = this.mTabFilters[aTab._tPos];
>  #ifdef MOZ_E10S_COMPAT
>              // Bug 666801 - WebProgress support for e10s
> +#else

Another patch already fixed this trailing whitespace.

@@ +3050,5 @@
> +        "chrome://browser/content/browser.css"
> +      </field>
> +      <field name="_delayResizingRuleSelector" readonly="true">
> +        ".tabbrowser-tabs[dontresize] > .tabbrowser-tab:not([pinned])[fadein]"
> +      </field>

This is broken, because the CSS parser rearranges selector text in CSS files into a canonical format.
In this case, the parser prefers the following format:
.tabbrowser-tabs[dontresize] > .tabbrowser-tab[fadein]:not([pinned])
so the loop doesn't match anything and exits with the property value undefined.

Fix this by changing the field value to the above string.

@@ +3063,5 @@
> +              }
> +            }
> +          }
> +        ]]></getter>
> +      </property>

Please use the `for (let i of arrayLikeObject)` syntax here.
https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Statements/for...of

This should be a field, not a property.
Fields are compiled upon first access and avoid the overhead of creating a function and then destroying it.
http://enndeakin.wordpress.com/2011/10/04/xbl-performance-tips/

The field definition should look like this:

      <field name="_delayResizingRule" readonly="true"><![CDATA[
        let match;
        for (let sheet of document.styleSheets) {
          if (sheet.href == this._delayResizingRuleSheet) {
            for (let rule of sheet.cssRules) {
              if (rule.selectorText == this._delayResizingRuleSelector) {
                match = rule;
                break;
              }
            }
          }
        }
        match
      ]]></field>

I prefer no semicolon on the last line to emphasize that the entire block is an expression, just like other fields.

Comment 6

[Frank Yan (:fryn)]

I support this change, despite the initial overhead of finding the rule in an O(m*n) loop, because it has the potential to reduce the overhead of initiating the animation to a maximum of 2 style reflows, which O(1), instead of the current maximum of O(n) reflows, due to setting the style individually for each tab and the race condition between setting property values and animations.

I filed bug 827642 for the aforementioned CSS parser behavior.

Comment 7

[ithinc]

(In reply to Frank Yan (:fryn) from comment #5)
> Please use the `for (let i of arrayLikeObject)` syntax here.
> https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Statements/for.
> ..of

document.styleSheets and styleSheet.cssRules are not iterable at the moment. See bug 738196.

> This should be a field, not a property.
> ...

Thanks, good to know it. I was not sure a code block could be placed in a field.

Comment 8

[Frank Yan (:fryn)]

(In reply to Frank Yan (:fryn) from comment #6)
> I filed bug 827642 for the aforementioned CSS parser behavior.

Turns out that this behavior is intentional, since the stylesheet properties are not accessed as text but rather through serialized objects from its object model (CSSOM), just as HTML properties are accessed through the DOM.

(In reply to ithinc from comment #7)
> (In reply to Frank Yan (:fryn) from comment #5)
> > Please use the `for (let i of arrayLikeObject)` syntax here.
> > https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Statements/for.
> > ..of
> 
> document.styleSheets and styleSheet.cssRules are not iterable at the moment.
> See bug 738196.

That's unfortunate. Let's just use plain `for` loops then. I like the syntactical neatness of Array.forEach, but the functional overhead isn't worth it.

Comment 9

[Frank Yan (:fryn)]

Regarding this._delayResizingRuleSheet and this._delayResizingRuleSelector:
Since these are internal values that we only read once, it doesn't seem necessary to expose them as fields, since no methods use them. Let's have these defined only inside the _delayResizingRule definition.

Comment 10

[ithinc]

Attachment: updated to tips

I use an anonymous function call in the _delayResizingRule field. I think it looks better.

Comment 11

[ithinc]

Attachment: patch v4

Comment 12

[ithinc]

Attachment: patch v5

Comment 13

[ithinc]

Attachment: patch v6

Comment 14

[Frank Yan (:fryn)]

Comment on attachment 699168 [details] [diff] [review]
patch v6

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

::: browser/base/content/tabbrowser.xml
@@ +3095,5 @@
> +              }
> +            }
> +          }
> +        })()
> +      ]]></field>

I like the Array.slice workaround here. :)

It's not a big deal, but I slightly prefer the following:

<field name="_delayResizingRule" readonly="true"><![CDATA[
  const href = "chrome://browser/content/browser.css";
  const selector = ".tabbrowser-tabs[dontresize] > .tabbrowser-tab[fadein]:not([pinned])";

  // XXX: document.styleSheets is not iterable (see bug 738196)
  for (let sheet of Array.slice(document.styleSheets))
    if (sheet.href == href)
      for (let rule of Array.slice(sheet.cssRules))
        if (rule.selectorText == selector) { rule; break; }
]]></field>

@@ +3177,5 @@
>              this.removeAttribute("using-closing-tabs-spacer");
>              this._closingTabsSpacer.style.width = 0;
>            }
> +          this._closingTabsSpacer.style.minWidth = "";
> +          this.removeAttribute("dontresize");

Why are we modifying both width and min-width?
It seems possible to fold this down to only modifying min-width, which would I think it would be less prone to glitches and easier to maintain.
Am I overlooking something?

Comment 15

[ithinc]

(In reply to Frank Yan (:fryn) from comment #14)
> It's not a big deal, but I slightly prefer the following:
> 
> <field name="_delayResizingRule" readonly="true"><![CDATA[
>   const href = "chrome://browser/content/browser.css";
>   const selector = ".tabbrowser-tabs[dontresize] >
> .tabbrowser-tab[fadein]:not([pinned])";
> 
>   // XXX: document.styleSheets is not iterable (see bug 738196)
>   for (let sheet of Array.slice(document.styleSheets))
>     if (sheet.href == href)
>       for (let rule of Array.slice(sheet.cssRules))
>         if (rule.selectorText == selector) { rule; break; }
> ]]></field>

Is this a standard way? IMO it's hard to understand and easy to make a mistake. Anyway, I'll upload a new patch.

> Why are we modifying both width and min-width?
> It seems possible to fold this down to only modifying min-width, which would
> I think it would be less prone to glitches and easier to maintain.
> Am I overlooking something?

spacer.style.width is used in overflow mode before, So I plan to modify it in bug 649654. I supposed it doesn't impact the logic in this patch.

Comment 16

[Frank Yan (:fryn)]

Comment on attachment 699168 [details] [diff] [review]
patch v6

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

> Is this a standard way? IMO it's hard to understand and easy to make a
> mistake. Anyway, I'll upload a new patch.

XBL fields with multiple statements in them are rare, and XBL is not a standard,
so there is no standard way. Both ways have a bit of weirdness.
Like I said, it's not a big deal, and I won't reject a patch based on an
insignificant difference in opinion on code style.

> spacer.style.width is used in overflow mode before, So I plan to modify it
> in bug 649654. I supposed it doesn't impact the logic in this patch.

Sounds good to me.

Comment 17

[ithinc]

Attachment: patch for checkin

Dao, do you have any objection or comment here?

Comment 18

[Daniel Holbert [:dholbert]]

Just to be clear -- this is marked 'checkin-needed' -- is it OK to land, or are we waiting on a response to Comment 17?

Comment 19

[Frank Yan (:fryn)]

(In reply to Daniel Holbert [:dholbert] from comment #18)
> Just to be clear -- this is marked 'checkin-needed' -- is it OK to land, or
> are we waiting on a response to Comment 17?

I'd like to wait until bug 649654's patch is ready (since this bug's patch alone regresses a bit of the animation) and then land these together.
I can push both of them when that happens.

Of course, we'll heed any comments that Dão has, but the patch is low-risk enough that I'm comfortable with my review alone. We'll address any regressions if they occur.

Adding not-ready to whiteboard to reflect this.

Comment 20

[Frank Yan (:fryn)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b1b6fb261d36

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b1b6fb261d36

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Poking at chrome style sheets using the CSSOM causes the entire style sheet to no longer be shared between browser windows, which is a memory and performance problem.  This shouldn't be done using the CSSOM.

Comment 23

[Frank Yan (:fryn)]

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #22)
> Poking at chrome style sheets using the CSSOM causes the entire style sheet
> to no longer be shared between browser windows, which is a memory and
> performance problem.  This shouldn't be done using the CSSOM.

Ah, I didn't know that optimized style sheets that way.
Do we really need to optimize for having a large number of windows over a large number of tabs? It seems to me like setting tab.style on a bunch of tabs — O(n) for tabs for the property-setting loops — is worse (and more prone to error) than setting it once in the window — O(n) for windows, but O(1) for tabs? — and just letting the tab strip reflow all at once.
What are your thoughts on creation of a separate stylesheet at runtime that contains only this rule for resizing?

On a separate note: I'm considering backing all this stuff out because of other, unrelated problems that we're encountering that I do not have the time or priority to solve. I still would like answers to the above questions though.

Comment 24

[Frank Yan (:fryn)]

We're definitely backing this out (the backout patch is in bug 851436), but I still would like answers to my questions in comment 23, so I can better understand what to do or avoid for these types of things in the future.

Comment 25

[Frank Yan (:fryn)]

Backed out in bug 851436.

https://hg.mozilla.org/mozilla-central/rev/7fbba1340a23

Comment 26

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Frank Yan (:fryn) from comment #23)
> (In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from
> comment #22)
> > Poking at chrome style sheets using the CSSOM causes the entire style sheet
> > to no longer be shared between browser windows, which is a memory and
> > performance problem.  This shouldn't be done using the CSSOM.
> 
> Ah, I didn't know that optimized style sheets that way.
> Do we really need to optimize for having a large number of windows over a
> large number of tabs? It seems to me like setting tab.style on a bunch of
> tabs — O(n) for tabs for the property-setting loops — is worse (and more
> prone to error) than setting it once in the window — O(n) for windows, but
> O(1) for tabs? — and just letting the tab strip reflow all at once.

It's not clear to me exactly what configuration of things you're talking about.  But:
 * it's preferable to have a class in the markup and selectors that select on the class; however, you can't do that here because of the number you're generating
 * so if you wanted to do this using the approach in this patch, I'd recommend actually having an entirely separate style sheet just for this (or for this and any other things that you change dynamically per-window).

> What are your thoughts on creation of a separate stylesheet at runtime that
> contains only this rule for resizing?

That would be fine (as I said above), though I'm not sure if it seems worth it.


As far as "reflow all at once" -- separate sets on different elements' style attributes should still get coalesced into a single reflow, unless you're doing something to flush layout in between the sets, which would be really bad.