[webkit-reviews] review granted: [Bug 11839] Webarchive fails to save CSS files in @import statements : [Attachment 12925] Patch v3

[bugzilla-request-daemon at macosforge.org]

Darin Adler <darin at apple.com> has granted Darin Adler <darin at apple.com>'s
request for review:
Bug 11839: Webarchive fails to save CSS files in @import statements
http://bugs.webkit.org/show_bug.cgi?id=11839

Attachment 12925: Patch v3
http://bugs.webkit.org/attachment.cgi?id=12925&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
+    NSMutableSet *URLList = [NSMutableSet setWithCapacity:[URLs count]];
+    [URLList addObjectsFromArray:URLs];

+[NSMutableSet setWithArray:] will do the same thing in one line.

+    NSMutableArray *sheetList = [NSMutableArray arrayWithCapacity:1];
+    [sheetList addObject:self];

+[NSMutableArray arrayWithObject:] will do the same thing in one line.

+    NSMutableArray *baseURLList = [NSMutableArray arrayWithCapacity:1];
+    [baseURLList addObject:URL];

Ditto.

+	 DOMCSSStyleSheet *sheet = [sheetList objectAtIndex:0];
+	 [sheetList removeObjectAtIndex:0];

It's unnecessarily inefficient to keep removing the first object from an array.
This will be slow if the array is large. I think it might scale better if these
style sheets were in a set rather than an array since the order doesn't matter.
We could store the base URLs in a dictionary. For common cases this would be
slower, but it would have better worst-case behavior.

+		     NSURL *importedStylesheetURL = [[NSURL
URLWithString:importedStylesheetURLString relativeToURL:baseURL] absoluteURL];

We can get slightly better efficiency here by using initWithString: instead of
URLWithString: and manually releasing the URL. Our rule of thumb is that we try
not to rely on autorelease except when it's required because of a return value.


+    [URLs setArray:[URLList allObjects]];

Shouldn't this be addObjectsFromArray: instead? Should we be sorting the list?

Another idea is to write the logic of this entire function in C++, with
conversion on entry and exit. Something like this:

    void addSubresourceURLs(CSSStyleSheet*, const KURL& baseURL,
Vector<KURL>&);

That would be more forward looking, since we hope to some day have this web
archiving code be cross-platform.

+	 NSURL *stylesheetURL = [[self _URLsFromSelectors:@selector(href), nil]
objectAtIndex:0];

Why is it OK to look only at the first URL?

+	 NSMutableArray *URLs = [NSMutableArray arrayWithCapacity:1];
+	 [URLs addObject:stylesheetURL];

Again.

r=me