[Webkit-unassigned] [Bug 50758] Defer loading print stylesheets

[bugzilla-daemon at webkit.org]

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





--- Comment #15 from Alexey Proskuryakov <ap at webkit.org>  2010-12-13 09:25:01 PST ---
(From update of attachment 76378)
View in context: https://bugs.webkit.org/attachment.cgi?id=76378&action=review

I've been reviewing the previous version (without build fixes). Looks good, and I'm especially happy about test coverage.

Please fix minor comments. I'm not quite sure what to do about the two bigger ones (possibility of printing problems, and breaking preloading for complex media). Is this something to discuss with Hyatt, or are you sure about this being fine?

> WebCore/ChangeLog:11
> +        - Use this mechanism to load print stylesheets using very low priority so they get loaded after everything else.

I'm slightly worried that this may increase problem rate when actually printing. It will be more likely that a page will not be printable yet when it looks complete on screen. The user would press Cmd+P, and observe broken layout.

> WebCore/ChangeLog:20
> +        * WebCore.xcodeproj/project.pbxproj:

Please modify other project files.

> WebCore/css/MediaQueryEvaluator.h:77
> -    bool eval(const MediaList*, CSSStyleSelector* = 0) const;
> +    bool eval(PassRefPtr<MediaList>, CSSStyleSelector* = 0) const;

This change is wrong. PassRefPtr should be used when passing ownership - but MediaQueryEvaluator doesn't take ownership of the MediaList.

> WebCore/html/HTMLLinkElement.cpp:227
> +    // FIXME: We shouldn't load unnecessary stylesheets at all.

Please remove this FIXME. There is no such thing as "unnecessary stylesheet".

> WebCore/html/HTMLLinkElement.cpp:261
> +        // Load print stylesheets with low priority so they don't affect normal page loading.
> +        ResourceLoadPriority priority = mediaIsScreen ? ResourceLoadPriorityUnresolved : ResourceLoadPriorityVeryLow;
> +        m_cachedSheet = document()->cachedResourceLoader()->requestCSSStyleSheet(m_url, charset, priority);

s/print/non-screen/?

> WebCore/html/parser/HTMLPreloadScanner.cpp:92
> +    bool linkMediaAttributeMatches(const String& attributeValue)

This function doesn't use class members, so it should be static. Also, the name implies matching something in class state, which is misleading. I suggest something like "linkMediaAttributeMatchesRenderingIntent" or linkMediaAttributeIsScreen".

> WebCore/html/parser/HTMLPreloadScanner.cpp:100
> +        // Only preload screen media stylesheets. Used this way, the evaluator evaluates to false for any 
> +        // rules containing complex queries (full evaluation is possible but it requires frame and style selector which
> +        // may be problematic here). Cases where picking these for preloading would help are probably rare.

Will this prevent preloading for screen size and resolution specific stylesheets? How bad is that for embedded devices?

> WebKitTools/ChangeLog:5
> +        Add setSerializeHTTPLoads function to allow testing resource load order on OS X.

Please consider also implementing this at least for WebKit2.

> WebKitTools/DumpRenderTree/mac/LayoutTestControllerMac.mm:1004
> +    [WebView _setLoadResourcesSerially:serialize forHost:@"127.0.0.1"];

What is the use case for passing a host here? We'll want at least localhost, too.

> LayoutTests/ChangeLog:1
> +2010-12-13  Antti Koivisto  <antti at apple.com>

Duplicated ChangeLog in LayoutTests.

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