[Webkit-unassigned] [Bug 115513] Improving PageSerializer.cpp to retrieve the subresources specified in @media rules in css

[bugzilla-daemon at webkit.org]

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


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #200308|review?                     |review-
               Flag|                            |




--- Comment #9 from Darin Adler <darin at apple.com>  2013-05-02 09:40:32 PST ---
(From update of attachment 200308)
View in context: https://bugs.webkit.org/attachment.cgi?id=200308&action=review

Thanks for tackling this!

> Source/WebCore/ChangeLog:8
> +        No new tests (OOPS!).

We need to add a test demonstrating that this did not work before and now does work after the fact. Often, the hardest part of making a good bug fix is making the test.

We can’t check in a change with this “OOPS” line in the patch. The change log either needs to include a test (preferred) or to explain why there is no test. Not contain the OOPS.

> Source/WebCore/page/PageSerializer.cpp:282
> +            for (int i = 0; i < mediaRule->length(); ++i) {

This won’t compile because it’s comparing an int with an unsigned. We’ll need to correct the type to make it compile.

> Source/WebCore/page/PageSerializer.cpp:284
> +                if (mediaRule->item(i)->isStyleRule())
> +                    retrieveResourcesForRule(static_cast<CSSStyleRule*>(mediaRule->item(i))->styleRule(), document);

This is a slow way to iterate the style rules. It’s doing multiple function calls and levels of virtual dispatch each time we call item(i). At the very least we need to put medaRule->item(i) into a local variable, but it would be even better to eventually make this code use our internal style data structures rather than the DOM wrappers.

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