[webkit-reviews] review granted: [Bug 208472] [iOS] Ugly and misaligned form validation bubble : [Attachment 395526] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Wenson Hsieh
<wenson_hsieh at apple.com>'s request for review:
Bug 208472: [iOS] Ugly and misaligned form validation bubble
https://bugs.webkit.org/show_bug.cgi?id=208472

Attachment 395526: Patch

https://bugs.webkit.org/attachment.cgi?id=395526&action=review




--- Comment #13 from Darin Adler <darin at apple.com> ---
Comment on attachment 395526
  --> https://bugs.webkit.org/attachment.cgi?id=395526
Patch

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

Thanks. Looks good! I really appreciate you considering my suggestions.

> Source/WebCore/platform/ios/ValidationBubbleIOS.mm:41
> +// Add a bit of vertical and horizontal padding between the
> +// label and its parent view.

Would be even better if we somehow pointed out that these numbers were
carefully chosen to achieve a particular look.

> Source/WebCore/platform/ios/ValidationBubbleIOS.mm:47
> +// Avoid making the validation bubble too wide by enforcing a
> +// maximum width on the content size of the validation bubble
> +// view controller.

Great reasoning to have a constant, not clear where the 300 comes from, though.
Same question: Did we carefully choose this?

> Source/WebCore/platform/ios/ValidationBubbleIOS.mm:51
> +// Avoid making the validation bubble too tall by truncating
> +// the label to a maximum of 4 lines.

OK, now I am a broken record. I really want these to be "why" comments so the
new person coming along doesn’t just change them because they think they know
better.

> Source/WebCore/platform/ios/ValidationBubbleIOS.mm:57
> +static void *validationBubbleViewControllerLabelKey =
&validationBubbleViewControllerLabelKey;

Our coding style calls for void*, not "void *". Also, since the key is const
void* (not void*), we use this better version:

    static const void* const validationBubbleViewControllerLabelKey =
&validationBubbleViewControllerLabelKey;

> Source/WebCore/platform/ios/ValidationBubbleIOS.mm:59
> +inline UILabel
*webValidationBubbleViewControllerLabel(WebValidationBubbleViewController
*instance)

I would just say "static" here instead of "inline". The compiler should do a
good job of inlining if appropriate without our help. We only really need to
use "inline" if we are putting something into a header and need permission to
include it in more than one translation unit.

Also, static makes sure it’s private to a single translation unit, but inline
instead makes it safe to have copies in multiple translation units. We want the
former.

Making it private to a single translation unit also lets us take the risk of
using slightly shorter function names without as much fear of conflicts,
although unified sources bring that fear back. I’d love for these to have
shorter, but still clear, names. Since this is C++, the argument type
participates in overload resolution, so the name of the class does not need to
be included in the function name, particularly not the full name of the class.

> Source/WebCore/platform/ios/ValidationBubbleIOS.mm:64
> +inline CGRect
webValidationBubbleViewControllerLabelFrame(WebValidationBubbleViewController
*instance)

Ditto.

> Source/WebCore/platform/ios/ValidationBubbleIOS.mm:70
> +inline void invokeUIViewControllerSelector(id instance, SEL selector)

Ditto. Except that I would not change the name of this one. Unless …

You *could* change the name to callSuper or sendSuper or something like that
and change the argument type to WebValidationBubbleViewController *
specifically, instead of "id".

C++ function overloading can lead to much clearer function names with nearly
the same safety about getting the right function. If you were doing this for
more than one class, all the functions could still be named callSuper, and
overloading would choose the correct one.

> Source/WebCore/platform/ios/ValidationBubbleIOS.mm:72
>      objc_super superClass { instance, PAL::getUIViewControllerClass() };

The name "superClass" here is not really quite right. First of all, the term of
art is a single word "superclass". Second, an objc_super structure is not a
superclass. It’s a structure to indicate how to search for a method for a super
call, that can call to a superclass or to one of its superclasses. So I would
name this something different, like maybe just "super".

> Source/WebCore/platform/ios/ValidationBubbleIOS.mm:73
>      auto superclassFunction = reinterpret_cast<void(*)(objc_super*,
SEL)>(objc_msgSendSuper);

I don’t think superclassFunction is the perfect name for a casted copy of
objc_msgSendSuper. It’s the dispatcher, not an actual function from the
superclass, which is what superclassFunction sounds like to me. I think this
and the next line would read better as a one-liner. Our you could name this
msgSendSuper, or castedMsgSentSuper, or
msgSendSuperForNoArgumentsOrReturnValue.

> Source/WebCore/platform/ios/ValidationBubbleIOS.mm:82
>  static void
WebValidationBubbleViewController_dealloc(WebValidationBubbleViewController
*instance, SEL)
>  {
> -    [instance.label release];
> -    [instance setValue:nil forKey:@"_label"];
> +    objc_setAssociatedObject(instance,
validationBubbleViewControllerLabelKey, nil,
OBJC_ASSOCIATION_RETAIN_NONATOMIC);
>  
>      invokeUIViewControllerSelector(instance, @selector(dealloc));
>  }

This should not be needed; associated objects should already have their
lifetimes tied to the object they are associated with. Does something go wrong
if we don’t override dealloc?