Some clients of WebKit may want to filter out icons based on link element's attributes. <rdar://problem/35442605>
Created attachment 336103 [details] Patch
Comment on attachment 336103 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=336103&action=review > Source/WebKit/UIProcess/API/Cocoa/_WKLinkIconParameters.mm:66 > + _attributes.get()[[(NSString *)attribute.key copy]] = [(NSString *)attribute.value copy]; This looks like a memory leak. Why are you copying the NSStrings? WTF::String::operator NSString* returns an NSString you could probably just use without copying.
(In reply to Alex Christensen from comment #2) > Comment on attachment 336103 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=336103&action=review > > > Source/WebKit/UIProcess/API/Cocoa/_WKLinkIconParameters.mm:66 > > + _attributes.get()[[(NSString *)attribute.key copy]] = [(NSString *)attribute.value copy]; > > This looks like a memory leak. Why are you copying the NSStrings? > WTF::String::operator NSString* returns an NSString you could probably just > use without copying. Presumably we want to be creating a different copy for each _WKLinkIconParameters. Why else are we making copies of linkIcon.url and linkIcon.mimeType? Either we need to make copies of keys & values here as well or copies of those values are also not needed.
Comment on attachment 336103 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=336103&action=review >>> Source/WebKit/UIProcess/API/Cocoa/_WKLinkIconParameters.mm:66 >>> + _attributes.get()[[(NSString *)attribute.key copy]] = [(NSString *)attribute.value copy]; >> >> This looks like a memory leak. Why are you copying the NSStrings? WTF::String::operator NSString* returns an NSString you could probably just use without copying. > > Presumably we want to be creating a different copy for each _WKLinkIconParameters. Why else are we making copies of linkIcon.url and linkIcon.mimeType? Either we need to make copies of keys & values here as well or copies of those values are also not needed. I'm not sure, but those copies are enclosed by adoptNS and these are not, making these memory leaks.
Brady says we don't need to make a copy of those either so let's just get rid of them all.
Created attachment 336157 [details] Don't copy strings
Comment on attachment 336157 [details] Don't copy strings Can we use a Vector instead of a HashMap?
(In reply to Darin Adler from comment #7) > Comment on attachment 336157 [details] > Don't copy strings > > Can we use a Vector instead of a HashMap? Like a pair of Strings? We can do that.
Created attachment 336162 [details] Use vector
Comment on attachment 336162 [details] Use vector View in context: https://bugs.webkit.org/attachment.cgi?id=336162&action=review > Source/WebCore/html/LinkIconCollector.cpp:111 > + attributes.uncheckedAppend(std::make_pair(attribute.localName(), attribute.value())); Can we use an initializer list instead of make_pair? > Source/WebCore/html/LinkIconCollector.cpp:114 > + icons.append({ url, iconType, linkElement.type(), iconSize, attributes }); WTFMove(attributes). Otherwise this does an unnecessary Vector copy, right? > Source/WebKit/UIProcess/API/Cocoa/_WKLinkIconParameters.mm:66 > + _attributes.get()[(NSString *)attributePair.first] = (NSString *)attributePair.second; Are the explicit (NSString *)s necessary?
(In reply to Alex Christensen from comment #10) > Comment on attachment 336162 [details] > Use vector > > View in context: > https://bugs.webkit.org/attachment.cgi?id=336162&action=review > > > Source/WebCore/html/LinkIconCollector.cpp:111 > > + attributes.uncheckedAppend(std::make_pair(attribute.localName(), attribute.value())); > > Can we use an initializer list instead of make_pair? Fixed. > > Source/WebCore/html/LinkIconCollector.cpp:114 > > + icons.append({ url, iconType, linkElement.type(), iconSize, attributes }); > > WTFMove(attributes). Otherwise this does an unnecessary Vector copy, right? Yup. Fixed. > > Source/WebKit/UIProcess/API/Cocoa/_WKLinkIconParameters.mm:66 > > + _attributes.get()[(NSString *)attributePair.first] = (NSString *)attributePair.second; > > Are the explicit (NSString *)s necessary? The key needs one but not the value so fixed that.
Committed r229784: <https://trac.webkit.org/changeset/229784>