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

RESOLVED WONTFIX

Status

()

Firefox
Tabbed Browser
RESOLVED WONTFIX
5 years ago
4 years ago

People

(Reporter: ithinc, Unassigned)

Tracking

Trunk
Firefox 21
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 5 obsolete attachments)

(Reporter)

Description

5 years ago
A step to push bug 649654.
(Reporter)

Comment 1

5 years ago
Created attachment 692430 [details] [diff] [review]
patch
Attachment #692430 - Flags: review?(dao)
(Reporter)

Updated

5 years ago
Blocks: 649654
(Reporter)

Comment 2

5 years ago
Created attachment 692686 [details] [diff] [review]
let spacer grows instead of calculating tab width
Attachment #692686 - Flags: feedback?(gavin.sharp)
Attachment #692686 - Flags: feedback?(dao)
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.
Attachment #692686 - Flags: feedback?(mnoorenberghe+bmo)
Attachment #692686 - Flags: feedback?(gavin.sharp)
Attachment #692686 - Flags: feedback?(fyan)
Comment on attachment 692430 [details] [diff] [review]
patch

I assume this patch is obsoleted by the latest one.
Attachment #692430 - Attachment is obsolete: true
Attachment #692430 - Flags: review?(dao)

Comment 5

5 years ago
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.
Attachment #692686 - Flags: review-
Attachment #692686 - Flags: feedback?(mnoorenberghe+bmo)
Attachment #692686 - Flags: feedback?(fyan)
Attachment #692686 - Flags: feedback?(dao)

Updated

5 years ago
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true

Updated

5 years ago
OS: Windows 7 → All
Hardware: x86_64 → All

Comment 6

5 years ago
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.
(Reporter)

Comment 7

5 years ago
(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

5 years ago
(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

5 years ago
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.
(Reporter)

Comment 10

5 years ago
Created attachment 699146 [details] [diff] [review]
updated to tips

I use an anonymous function call in the _delayResizingRule field. I think it looks better.
Attachment #692686 - Attachment is obsolete: true
Attachment #699146 - Flags: review?(fyan)
(Reporter)

Comment 11

5 years ago
Created attachment 699155 [details] [diff] [review]
patch v4
Attachment #699146 - Attachment is obsolete: true
Attachment #699146 - Flags: review?(fyan)
Attachment #699155 - Flags: review?(fyan)
(Reporter)

Comment 12

5 years ago
Created attachment 699156 [details] [diff] [review]
patch v5
Attachment #699155 - Attachment is obsolete: true
Attachment #699155 - Flags: review?(fyan)
Attachment #699156 - Flags: review?(fyan)
(Reporter)

Comment 13

5 years ago
Created attachment 699168 [details] [diff] [review]
patch v6
Attachment #699156 - Attachment is obsolete: true
Attachment #699156 - Flags: review?(fyan)
Attachment #699168 - Flags: review?(fyan)

Comment 14

5 years ago
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?
(Reporter)

Comment 15

5 years ago
(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

5 years ago
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.
Attachment #699168 - Flags: review?(fyan) → review+
(Reporter)

Comment 17

5 years ago
Created attachment 699713 [details] [diff] [review]
patch for checkin

Dao, do you have any objection or comment here?
(Reporter)

Updated

5 years ago
Keywords: checkin-needed
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

5 years ago
(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.
Keywords: checkin-needed
Whiteboard: not-ready

Comment 20

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1b6fb261d36
Whiteboard: not-ready

Updated

5 years ago
Target Milestone: --- → Firefox 21
https://hg.mozilla.org/mozilla-central/rev/b1b6fb261d36
Assignee: nobody → ithinc
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Depends on: 851862
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

4 years ago
(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

4 years ago
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.
Flags: needinfo?(dbaron)

Comment 25

4 years ago
Backed out in bug 851436.

https://hg.mozilla.org/mozilla-central/rev/7fbba1340a23
Assignee: ithinc → nobody
Resolution: FIXED → WONTFIX

Updated

4 years ago
No longer blocks: 649654
(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.
Flags: needinfo?(dbaron)
You need to log in before you can comment on or make changes to this bug.