Bug 196021 – [iOS] Inline -_ensureFormAccessoryView into -formAccessoryView and have -_updateAccessory ensure we have a form accessory

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Daniel Bates]

I don't see much benefit to -formAccessoryView being implemented in terms of -_ensureFormAccessoryView just inline the latter into the former and remove a method. Also, every caller except -formAccessoryView calls -_ensureFormAccessoryView followed immediately by -_updateAccessory. So, let's have -_updateAccessory just ensure we have a form accessory view instead of each caller.

[Daniel Bates]

Created attachment 365374 [details]
Patch

[Wenson Hsieh]

Comment on attachment 365374 [details]
Patch

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

> Source/WebKit/ChangeLog:9
> +        Every caller of -_ensureFormAccessoryView, except -formAccessoryView, immediately the call with
> +        -_updateAccessory. Let's just have -_updateAccessory ensure we have a form accessory view and

Nit - "immediately the call with"

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:3784
> +    [self formAccessoryView]; // Creates one if needed.

Nit - we can also do something like:

    UIWebFormAccessory *accessoryView = [self formAccessoryView];

...and then just use the accessoryView variable below, instead of _formAccessoryView. I think we wouldn't need the comment if we did this.

[Daniel Bates]

Comment on attachment 365374 [details]
Patch

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

>> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:3784
>> +    [self formAccessoryView]; // Creates one if needed.
> 
> Nit - we can also do something like:
> 
>     UIWebFormAccessory *accessoryView = [self formAccessoryView];
> 
> ...and then just use the accessoryView variable below, instead of _formAccessoryView. I think we wouldn't need the comment if we did this.

Of course! What was I thinking!

[Daniel Bates]

Comment on attachment 365374 [details]
Patch

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

>>> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:3784
>>> +    [self formAccessoryView]; // Creates one if needed.
>> 
>> Nit - we can also do something like:
>> 
>>     UIWebFormAccessory *accessoryView = [self formAccessoryView];
>> 
>> ...and then just use the accessoryView variable below, instead of _formAccessoryView. I think we wouldn't need the comment if we did this.
> 
> Of course! What was I thinking!

I'm still going to keep the comment though, because it seems subtle and I absolutely can envision a person in the future coming through this code,  realizing we have _formAccessoryView and without thinking swap out the self.formAccessoryView with _formAccessoryView.

[Daniel Bates]

(In reply to Daniel Bates from comment #4)
> Comment on attachment 365374 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=365374&action=review
> 
> >>> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:3784
> >>> +    [self formAccessoryView]; // Creates one if needed.
> >> 
> >> Nit - we can also do something like:
> >> 
> >>     UIWebFormAccessory *accessoryView = [self formAccessoryView];
> >> 
> >> ...and then just use the accessoryView variable below, instead of _formAccessoryView. I think we wouldn't need the comment if we did this.
> > 
> > Of course! What was I thinking!
> 
> I'm still going to keep the comment though, because it seems subtle and I
> absolutely can envision a person in the future coming through this code, 
> realizing we have _formAccessoryView and without thinking swap out the
> self.formAccessoryView with _formAccessoryView.

Having a test would be better to catch this though :/ Leaving the comment in for now.

[Daniel Bates]

Created attachment 365573 [details]
To Land

[Daniel Bates]

Committed r243302: <https://trac.webkit.org/changeset/243302>

[Radar WebKit Bug Importer]

<rdar://problem/49115052>