[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 bugzilla-request-daemon at macosforge.org
Sun Feb 4 18:40:40 PST 2007

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

Attachment 12925: Patch v3

------- 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];


+	 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,

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]

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

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



More information about the webkit-reviews mailing list