Bug 183768 – Expose content attributes on _WKLinkIconParameters

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit API

[Ryosuke Niwa]

Some clients of WebKit may want to filter out icons based on link element's attributes.

<rdar://problem/35442605>

[Ryosuke Niwa]

Created attachment 336103 [details]
Patch

[Alex Christensen]

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.

[Ryosuke Niwa]

(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.

[Alex Christensen]

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.

[Ryosuke Niwa]

Brady says we don't need to make a copy of those either so let's just get rid of them all.

[Ryosuke Niwa]

Created attachment 336157 [details]
Don't copy strings

[Darin Adler]

Comment on attachment 336157 [details]
Don't copy strings

Can we use a Vector instead of a HashMap?

[Ryosuke Niwa]

(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.

[Ryosuke Niwa]

Created attachment 336162 [details]
Use vector

[Alex Christensen]

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?

[Ryosuke Niwa]

(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.

[Ryosuke Niwa]

Committed r229784: <https://trac.webkit.org/changeset/229784>