Bug 92687 - StyleResolver::canShareStyleWithElement does not need to use getAttribute for classAttr in the non-SVG case
Summary: StyleResolver::canShareStyleWithElement does not need to use getAttribute for...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Eric Seidel (no email)
URL:
Keywords:
Depends on:
Blocks: 92258
  Show dependency treegraph
 
Reported: 2012-07-30 15:32 PDT by Eric Seidel (no email)
Modified: 2012-07-31 16:33 PDT (History)
10 users (show)

See Also:


Attachments
Patch (1.86 KB, patch)
2012-07-30 15:34 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Patch (2.32 KB, patch)
2012-07-30 17:06 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2012-07-30 15:32:31 PDT
StyleResolver::canShareStyleWithElement does not need to use getAttribute for classAttr
Comment 1 Eric Seidel (no email) 2012-07-30 15:34:52 PDT
Created attachment 155385 [details]
Patch
Comment 2 Eric Seidel (no email) 2012-07-30 17:06:14 PDT
Created attachment 155401 [details]
Patch
Comment 3 Antti Koivisto 2012-07-31 07:56:10 PDT
Comment on attachment 155401 [details]
Patch

Seems fine as is. However I suspect SVG elements rarely end up sharing style anyway. It would be good to collect some statistics. If this is indeed the case we could just do if (element->isSVGElement()) return false; in the beginning, simplifying and speeding up the code.
Comment 4 Andreas Kling 2012-07-31 07:57:45 PDT
(In reply to comment #3)
> (From update of attachment 155401 [details])
> Seems fine as is. However I suspect SVG elements rarely end up sharing style anyway. It would be good to collect some statistics. If this is indeed the case we could just do if (element->isSVGElement()) return false; in the beginning, simplifying and speeding up the code.

Right. And if it's a helpful optimization for SVG, we could consider splitting the function into SVG and HTML versions to only check the things that make sense for either element type.
Comment 5 Eric Seidel (no email) 2012-07-31 11:25:37 PDT
(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 155401 [details] [details])
> > Seems fine as is. However I suspect SVG elements rarely end up sharing style anyway. It would be good to collect some statistics. If this is indeed the case we could just do if (element->isSVGElement()) return false; in the beginning, simplifying and speeding up the code.
> 
> Right. And if it's a helpful optimization for SVG, we could consider splitting the function into SVG and HTML versions to only check the things that make sense for either element type.

I really like that idea.
Comment 6 Eric Seidel (no email) 2012-07-31 11:26:31 PDT
Is there something you'd like changed Antti, or can I just cq+ this?  I'm confused by your comment.  I'm happy to file bugs about future potential optimizations to this function if that's what you're looking for.
Comment 8 Eric Seidel (no email) 2012-07-31 14:17:17 PDT
For the record, this was the diff I used to collect my numbers:

diff --git a/Source/WebCore/css/StyleResolver.cpp b/Source/WebCore/css/StyleResolver.cpp
index 91f999a..55825f4 100644
--- a/Source/WebCore/css/StyleResolver.cpp
+++ b/Source/WebCore/css/StyleResolver.cpp
@@ -1740,10 +1740,13 @@ PassRefPtr<RenderStyle> StyleResolver::styleForElement(Element* element, RenderS
     initElement(element);
     initForStyleResolve(element, defaultParent);
     m_regionForStyling = regionForStyling;
+    printf("STYLE\n");
     if (sharingBehavior == AllowStyleSharing) {
         RenderStyle* sharedStyle = locateSharedStyle();
-        if (sharedStyle)
+        if (sharedStyle) {
+            printf("YES\n");
             return sharedStyle;
+        }
     }
 
     m_style = RenderStyle::create();
Comment 9 Eric Seidel (no email) 2012-07-31 14:17:57 PDT
Comment on attachment 155401 [details]
Patch

I believe I've answered everyone's questions.  Going to cq+ this.  If there are any issues, or you'd like follow-up bugs I'm happy to do more here.
Comment 10 WebKit Review Bot 2012-07-31 14:31:21 PDT
Comment on attachment 155401 [details]
Patch

Rejecting attachment 155401 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

ERROR: /mnt/git/webkit-commit-queue/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/13411123
Comment 11 Eric Seidel (no email) 2012-07-31 14:44:36 PDT
Comment on attachment 155401 [details]
Patch

Confused bot...
Comment 12 Antti Koivisto 2012-07-31 15:00:20 PDT
(In reply to comment #6)
> Is there something you'd like changed Antti, or can I just cq+ this?  I'm confused by your comment.  I'm happy to file bugs about future potential optimizations to this function if that's what you're looking for.

I just wanted to give you chance to consider alternatives before landing. As I said I think the patch is fine as it is.
Comment 13 WebKit Review Bot 2012-07-31 15:55:34 PDT
Comment on attachment 155401 [details]
Patch

Clearing flags on attachment: 155401

Committed r124260: <http://trac.webkit.org/changeset/124260>
Comment 14 WebKit Review Bot 2012-07-31 15:55:40 PDT
All reviewed patches have been landed.  Closing bug.
Comment 15 Eric Seidel (no email) 2012-07-31 16:33:25 PDT
(In reply to comment #12)
> (In reply to comment #6)
> > Is there something you'd like changed Antti, or can I just cq+ this?  I'm confused by your comment.  I'm happy to file bugs about future potential optimizations to this function if that's what you're looking for.
> 
> I just wanted to give you chance to consider alternatives before landing. As I said I think the patch is fine as it is.

Ah.  Thank you.  I agree with you that we should split canShareStyleWithElement into SVG and non-SVG variants in the future.