Closed Bug 875305 Opened 11 years ago Closed 11 years ago

Bug 833388 added potentially-slow rules to html.css

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
firefox22 + fixed
firefox23 + fixed
firefox24 + fixed

People

(Reporter: bzbarsky, Assigned: rillian)

References

Details

(Keywords: regression)

Attachments

(2 files, 4 obsolete files)

>+video > div {
>+video > div p {

These should be using classes on the right-hand-most thing if at all possible, imo.  It's possible the Bloom filter saves the day, but I'd rather we didn't depend on that.
Asking in IRC to know if Jesse is able to take this on, but not tracking unless we can get an assignee to work on this.
If we can't get a forward fix, what is the risk of backing out bug 833388 and bringing it back at a later time with a re-working of these rules?
bz are you able to assess the question in comment 2?
Flags: needinfo?(bzbarsky)
So those rules were added specifically for WEBVTT text. Right now they are only temporary. We've opened a bug https://bugzilla.mozilla.org/show_bug.cgi?id=876505 to get the CSS styling for WEBVTT up to date with the spec.

I don't know how important it would be to update the CSS here as it will be changing very soon in accordance with the spec. This temporary CSS won't be seeing much use either as WEBVTT is still behind a pref. Maybe it would be better to just ensure that the CSS that lands in the other bug uses fast rules.
> So those rules were added specifically for WEBVTT text. Right now they are only
> temporary.

My point is that these rules affect the performance of styling _all_ web content, in all builds that have that change.  Including builds that have webvtt disabled.  I don't think that's acceptable.

> what is the risk of backing out bug 833388

I think the risk of backing out the html.css changes from that bug is zero (since they don't actually _match_ anything unless webvtt is enabled, and it's not enabled) and that this is in fact what we should do ASAP for at least 22 and 23.
Flags: needinfo?(bzbarsky)
If you need an assignee to take on this
Assignee: nobody → rick.eyre
Blocks: 833382
Attached patch use classes instead (obsolete) — Splinter Review
Switch to using unprefixed class selectors instead, as recommended by https://developer.mozilla.org/en-US/docs/Web/Guide/CSS/Writing_efficient_CSS

Can we use a leading underscore for class names to reduce the chance of collision?
Assignee: rick.eyre → giles
or a leading dash, I guess.

There's no point is setting moz-caption-box on the anonymous div, since bug 833382 changes the css properties to use something inside that div as the caption box itself. Rick, does this look reasonable to you?
Looks great to me. It works when applied to bug833382. Much simpler solution then I had in mind too!
Attached patch use classes instead (obsolete) — Splinter Review
Updated patch. There's no need to set the class on the top-level caption div. This is just a container for the caption box div, which must be positioned inside the video frame.

The patch from bug 833382 will need to be updated to set the proper class names on the elements it creates.
Attachment #754870 - Attachment is obsolete: true
Attachment #754911 - Flags: review?(bzbarsky)
Comment on attachment 754911 [details] [diff] [review]
use classes instead

No, this is not ok either, since it will match things on web pages.

However, |video > .moz-caption-box| and |video > div .moz-caption-text| would probably be ok.  Still detectable by web pages, but no worse than stuff in forms.css.

> Can we use a leading underscore for class names to reduce the chance of
> collision?

You can use any Unicode character you want to in class names in CSS.  In HTML you can use any Unicode character except ASCII spaces.
Attachment #754911 - Flags: review?(bzbarsky) → review-
Comment on attachment 754870 [details] [diff] [review]
use classes instead

Rick prefers this patch. Sorry for the confusion.
Attachment #754870 - Flags: review?(bzbarsky)
Comment on attachment 754870 [details] [diff] [review]
use classes instead

bugzilla lag.
Attachment #754870 - Flags: review?(bzbarsky)
Updated patch.

- Keep 'video >' parent so we can't affect page content
- Drop the 'moz-' prefix from the caption names since they're restricted to video children.
- Restore the class on the top-level anonymous div. per reyre's comments on irc.
Attachment #754911 - Attachment is obsolete: true
Attachment #754935 - Flags: review?(bzbarsky)
Comment on attachment 754935 [details] [diff] [review]
use classes on the rightmost selector

>+    nsRefPtr<nsGenericHTMLElement> div = do_QueryObject(mCaptionDiv);
>+    if (div) {

That QI would result in a non-null pointer for any element on the RHS, since nsGenericHTMLElement does not have its own IID.  So please don't to that: it's wrong.

Instead, just static_cast mCaptionDiv to nsGenericHTMLElement*, since you know it is in fact one.

r=me with that and assuming that you don't need the caption-text class on anything yet.

Please don't forget to land this on the relevant branches!
Attachment #754935 - Flags: review?(bzbarsky) → review+
Fix nits:

- static_cast instead of QueryObject so we can call SetClassName.
- caption-text isn't used by code in m-c. bug 833382 will use it.

Carrying forward earlier r+
Attachment #754935 - Attachment is obsolete: true
Keywords: checkin-needed
OS: Mac OS X → All
Hardware: x86 → All
Fix commit message git->hg noise.
Attachment #754997 - Attachment is obsolete: true
Can we backout bug 833388 for FF22 and take this forward fix for FF23? What's the risk of the forward fix here?

Let's try to get a branch fix in this week.
Comment on attachment 755168 [details] [diff] [review]
apply classes to rightmost selector

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 833388
User impact if declined: Users may see slower page loads.
Testing completed (on m-c, etc.): Landed on m-i, green on try.
Risk to taking this patch (and alternatives if risky): Low. The CSS rules are for code which isn't present in Aurora.
String or IDL/UUID changes made by this patch: none.

Patch applied to aurora and pushed to try as https://tbpl.mozilla.org/?tree=Try&rev=d5302ee47a16
Attachment #755168 - Flags: approval-mozilla-aurora?
Alex, that makes sense to me. I'll prepare a patch just removing the css rules for Beta. That should be safer than backing out the code change as well.
Whiteboard: [leave open]
[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 833388
User impact if declined: Users may experience slower page rendering.
Testing completed (on m-c, etc.): Pushed to try as https://tbpl.mozilla.org/?tree=Try&rev=fa8c6cb6af08
Risk to taking this patch (and alternatives if risky): Risk it minimal; the patch just removes unused CSS rules from the default stylesheet.
String or IDL/UUID changes made by this patch: none.
Attachment #755658 - Flags: review?(bzbarsky)
Attachment #755658 - Flags: approval-mozilla-beta?
Comment on attachment 755658 [details] [diff] [review]
remove caption CSS from beta

r=me
Attachment #755658 - Flags: review?(bzbarsky) → review+
(In reply to Ralph Giles (:rillian) from comment #23)
> Testing completed (on m-c, etc.): Pushed to try as
> https://tbpl.mozilla.org/?tree=Try&rev=fa8c6cb6af08

Would you mind reviewing the failures and ensuring that none of them are related to this patch?
Attachment #755168 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(In reply to Alex Keybl [:akeybl] from comment #26)

> > https://tbpl.mozilla.org/?tree=Try&rev=fa8c6cb6af08
> 
> Would you mind reviewing the failures and ensuring that none of them are
> related to this patch?

I've starred a number of them on the beta push. The Android red is I think just broken; changes to html.css can't make the configure script go missing and those aren't running on beta commits in tbpl.

The rest aren't obviously related, but I didn't dig too deeply. I can look more closely, but probably not before Tuesday. I've respun some of those to measure the repeatability.
Please let us know how things turned out - we may end up landing blindly to mozilla-beta and wait for results if your investigation goes past Tuesday (our b4 go-to-build).
FWIW I don't really think any of these failures could be caused by the patch as the CSS that was removed was only being used by WEBVTT which is currently preffed off and the pieces that use it haven't landed in the tree yet.
Comment on attachment 755658 [details] [diff] [review]
remove caption CSS from beta

Let's land and backout if necessary, since we suspect this will build/test cleanly.
Attachment #755658 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
This should be resolved on all branches now.
Whiteboard: [leave open]
Status: NEW → RESOLVED
Closed: 11 years ago
OS: All → Mac OS X
Hardware: All → x86
Resolution: --- → FIXED
Thank you stale cache interaction, and dbaron for noticing.

I've reset the status flags, but don't have permission to change the tracking flags back to '+' for firefox 22,23,24. Could someone please reset those?
OS: Mac OS X → All
Hardware: x86 → All
You need to log in before you can comment on or make changes to this bug.