Bug 150992 – Rework the way allowed argument classes are stored

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Anders Carlsson]

Rework the way allowed argument classes are stored

[Anders Carlsson]

Created attachment 265067 [details]
Patch

[Darin Adler]

Comment on attachment 265067 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=265067&action=review

> Source/WebKit2/Shared/API/Cocoa/WKRemoteObjectCoder.mm:384
> +        *static_cast<int*>(data) = [decodeObjectFromObjectStream(self, { [NSNumber class] }) intValue];

Seems a shame to have to create/destroy an HashSet with the NSNumber class in it so many times. Maybe have a global one to reuse or a code path that doesn’t require building the set?

> Source/WebKit2/Shared/API/Cocoa/WKRemoteObjectCoder.mm:424
> +    if (auto *allowedClasses = decoder->_allowedClasses) {

Should be auto* instead of auto *.

> Source/WebKit2/Shared/API/Cocoa/WKRemoteObjectCoder.mm:426
> +        for (Class cls = objectClass; cls; cls = class_getSuperclass(cls)) {

Would be nice to dodge “cls” by using some other name. Maybe superclass or baseClass or ancestralClass? Not completely accurate, but so much better than a letter cluster.

> Source/WebKit2/Shared/API/Cocoa/WKRemoteObjectCoder.mm:519
> +            const auto& allowedClasses = allowedArgumentClasses[i - 2];

Why not just auto&? Does const add much here?

> Source/WebKit2/Shared/API/Cocoa/_WKRemoteObjectInterface.mm:138
> +    unsigned superProtocolCount;

I might decide to coin the term superprotocol and treat it as a word the way we do superclass, thus omitting the capital P.

> Source/WebKit2/Shared/API/Cocoa/_WKRemoteObjectInterface.mm:206
> +    auto result = adoptNS([[NSMutableSet alloc] init]);
> +
> +    for (auto allowedClass : classesForSelectorArgument(self, selector, argumentIndex))
> +        [result addObject:allowedClass];
> +
> +    return result.autorelease();

It’d be tempting to make HashSet/NSSet translation functions, and thus write this as a one-liner, exposing the classesForSelectorArgument call better rather than having it be inside the for declaration.

> Source/WebKit2/Shared/API/Cocoa/_WKRemoteObjectInterface.mm:215
> +    HashSet<Class> allowedClasses;
> +    for (Class allowedClass in classes)
> +        allowedClasses.add(allowedClass);
> +
> +    classesForSelectorArgument(self, selector, argumentIndex) = WTF::move(allowedClasses);

It’d be tempting to make HashSet/NSSet translation functions, and thus write this as a one-liner without an explicit WTF::move.

> Source/WebKit2/Shared/API/Cocoa/_WKRemoteObjectInterface.mm:247
> -    auto it = _allowedArgumentClasses.find(selector);
> -    ASSERT(it != _allowedArgumentClasses.end());
> +    auto it = _methods.find(selector);
> +    ASSERT(it != _methods.end());
>  
> -    return it->value;
> +    return it->value.allowedArgumentClasses;

I like to do ASSERT(contains), return get; in cases like this rather than using find/value.

[Anders Carlsson]

(In reply to comment #2)
> Comment on attachment 265067 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=265067&action=review
> 
> > Source/WebKit2/Shared/API/Cocoa/WKRemoteObjectCoder.mm:384
> > +        *static_cast<int*>(data) = [decodeObjectFromObjectStream(self, { [NSNumber class] }) intValue];
> 
> Seems a shame to have to create/destroy an HashSet with the NSNumber class
> in it so many times. Maybe have a global one to reuse or a code path that
> doesn’t require building the set?

Agreed, I'll look at doing it in a follow-up patch.

> 
> > Source/WebKit2/Shared/API/Cocoa/WKRemoteObjectCoder.mm:426
> > +        for (Class cls = objectClass; cls; cls = class_getSuperclass(cls)) {
> 
> Would be nice to dodge “cls” by using some other name. Maybe superclass or
> baseClass or ancestralClass? Not completely accurate, but so much better
> than a letter cluster.

Unrolled one iteration of the loop so I could call it superclass.

> 
> > Source/WebKit2/Shared/API/Cocoa/WKRemoteObjectCoder.mm:519
> > +            const auto& allowedClasses = allowedArgumentClasses[i - 2];
> 
> Why not just auto&? Does const add much here?

Nope, removed.

> 
> > Source/WebKit2/Shared/API/Cocoa/_WKRemoteObjectInterface.mm:138
> > +    unsigned superProtocolCount;
> 
> I might decide to coin the term superprotocol and treat it as a word the way
> we do superclass, thus omitting the capital P.
> 

I renamed it to conformingProtocol count since superProtocol isn't really a correct name. A single protocol can conform to more than one other protocol.

> > Source/WebKit2/Shared/API/Cocoa/_WKRemoteObjectInterface.mm:206
> > +    auto result = adoptNS([[NSMutableSet alloc] init]);
> > +
> > +    for (auto allowedClass : classesForSelectorArgument(self, selector, argumentIndex))
> > +        [result addObject:allowedClass];
> > +
> > +    return result.autorelease();
> 
> It’d be tempting to make HashSet/NSSet translation functions, and thus write
> this as a one-liner, exposing the classesForSelectorArgument call better
> rather than having it be inside the for declaration.
> 
> > Source/WebKit2/Shared/API/Cocoa/_WKRemoteObjectInterface.mm:215
> > +    HashSet<Class> allowedClasses;
> > +    for (Class allowedClass in classes)
> > +        allowedClasses.add(allowedClass);
> > +
> > +    classesForSelectorArgument(self, selector, argumentIndex) = WTF::move(allowedClasses);
> 
> It’d be tempting to make HashSet/NSSet translation functions, and thus write
> this as a one-liner without an explicit WTF::move.

Thought about that but I couldn't come up with a good name :)

> 
> > Source/WebKit2/Shared/API/Cocoa/_WKRemoteObjectInterface.mm:247
> > -    auto it = _allowedArgumentClasses.find(selector);
> > -    ASSERT(it != _allowedArgumentClasses.end());
> > +    auto it = _methods.find(selector);
> > +    ASSERT(it != _methods.end());
> >  
> > -    return it->value;
> > +    return it->value.allowedArgumentClasses;
> 
> I like to do ASSERT(contains), return get; in cases like this rather than
> using find/value.

I also like ASSERT(contains) better than find + ASSERT(... != end()), aka the Adam Roben style.

I did change the ASSERT to contains, but I can't use get since I want to return a const reference.

[Anders Carlsson]

Committed r192162: <http://trac.webkit.org/changeset/192162>