[Webkit-unassigned] [Bug 85004] Cache parsed stylesheets

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=85004





--- Comment #8 from Darin Adler <darin at apple.com>  2012-04-27 10:29:24 PST ---
(From update of attachment 139068)
View in context: https://bugs.webkit.org/attachment.cgi?id=139068&action=review

I decided to do a quick pass of review on this patch even though it’s already been landed once.

> Source/WebCore/css/CSSStyleSheet.cpp:83
> +    // FIXME: This ignores the children of media and region rules.
> +    // Most rules are StyleRules.

I’m not sure this should say FIXME unless you actually think it needs to be fixed.

> Source/WebCore/css/StylePropertySet.cpp:1003
> +unsigned StylePropertySet::averageSizeInBytes()

Do we really want to pay function call overhead for this? Why not just put it inline in the header?

> Source/WebCore/html/HTMLLinkElement.cpp:305
> +        RefPtr<MediaQuerySet> media = MediaQuerySet::createAllowingDescriptionSyntax(m_media);
> +        restoredSheet->setMediaQueries(media.release());

I think this would read better without the local variable:

    restoredSheet->setMediaQueries(MediaQuerySet::createAllowingDescriptionSyntax(m_media));

> Source/WebCore/html/HTMLLinkElement.cpp:311
> +        m_sheet = CSSStyleSheet::create(restoredSheet, this);
> +        m_loading = false;
> +        sheetLoaded();
> +        notifyLoadedSheetAndAllCriticalSubresources(false);

This is the kind of code I find almost impossible to review. Are these the key lines of code to call to simulate a successfully loaded sheet? Is this now synchronous where the old code path was asynchronous? I don’t know how to tell.

> Source/WebCore/loader/cache/CachedCSSStyleSheet.h:56
> +        virtual void destroyDecodedData() OVERRIDE;

Can this be private or protected rather than public?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.