[webkit-reviews] review denied: [Bug 115513] Improving PageSerializer.cpp to retrieve the subresources specified in @media rules in css : [Attachment 200308] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Santosh Mahto
<santoshbit2007 at gmail.com>'s request for review:
Bug 115513: Improving PageSerializer.cpp to retrieve the subresources specified
in @media rules in css
https://bugs.webkit.org/show_bug.cgi?id=115513

Attachment 200308: Patch
https://bugs.webkit.org/attachment.cgi?id=200308&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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))->styleR
ule(), 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.