Bug 183768

Summary: Expose content attributes on _WKLinkIconParameters
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: WebKit APIAssignee: Ryosuke Niwa <rniwa>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, beidson, cdumez, darin, dbates, esprehn+autocc, ews-watchlist, gyuyoung.kim, japhet, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Don't copy strings
none
Use vector achristensen: review+

Description Ryosuke Niwa 2018-03-19 22:30:41 PDT
Some clients of WebKit may want to filter out icons based on link element's attributes.

<rdar://problem/35442605>
Comment 1 Ryosuke Niwa 2018-03-19 22:37:47 PDT
Created attachment 336103 [details]
Patch
Comment 2 Alex Christensen 2018-03-20 10:51:41 PDT
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.
Comment 3 Ryosuke Niwa 2018-03-20 12:51:29 PDT
(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 4 Alex Christensen 2018-03-20 12:53:25 PDT
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.
Comment 5 Ryosuke Niwa 2018-03-20 12:58:15 PDT
Brady says we don't need to make a copy of those either so let's just get rid of them all.
Comment 6 Ryosuke Niwa 2018-03-20 15:03:02 PDT
Created attachment 336157 [details]
Don't copy strings
Comment 7 Darin Adler 2018-03-20 15:19:36 PDT
Comment on attachment 336157 [details]
Don't copy strings

Can we use a Vector instead of a HashMap?
Comment 8 Ryosuke Niwa 2018-03-20 15:45:30 PDT
(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.
Comment 9 Ryosuke Niwa 2018-03-20 16:00:58 PDT
Created attachment 336162 [details]
Use vector
Comment 10 Alex Christensen 2018-03-20 17:52:00 PDT
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?
Comment 11 Ryosuke Niwa 2018-03-20 19:09:26 PDT
(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.
Comment 12 Ryosuke Niwa 2018-03-20 19:52:22 PDT
Committed r229784: <https://trac.webkit.org/changeset/229784>