[webkit-reviews] review granted: [Bug 221636] [iOS][FCR] Add new picker for <select multiple> elements : [Attachment 419772] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Feb 11 09:49:20 PST 2021
Wenson Hsieh <wenson_hsieh at apple.com> has granted Aditya Keerthi
<akeerthi at apple.com>'s request for review:
Bug 221636: [iOS][FCR] Add new picker for <select multiple> elements
https://bugs.webkit.org/show_bug.cgi?id=221636
Attachment 419772: Patch
https://bugs.webkit.org/attachment.cgi?id=419772&action=review
--- Comment #3 from Wenson Hsieh <wenson_hsieh at apple.com> ---
Comment on attachment 419772
--> https://bugs.webkit.org/attachment.cgi?id=419772
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=419772&action=review
> Source/WebKit/UIProcess/ios/forms/WKFormSelectPicker.mm:733
> + RetainPtr<UIBarButtonItem> nextPreviousSpacer =
adoptNS([[UIBarButtonItem alloc]
initWithBarButtonSystemItem:UIBarButtonSystemItemFixedSpace target:nil
action:NULL]);
Nit - I think `auto` instead of `RetainPtr<UIBarButtonItem>` would be a bit
cleaner here, for the `nextPreviousSpacer` and `closeButton`.
> Source/WebKit/UIProcess/ios/forms/WKFormSelectPicker.mm:744
> + for (const OptionItem& option :
_contentView.focusedSelectElementOptions) {
Nit - `auto& option`?
> Source/WebKit/UIProcess/ios/forms/WKFormSelectPicker.mm:763
> + for (const OptionItem& option :
_contentView.focusedSelectElementOptions) {
Nit - `auto& option`?
> Source/WebKit/UIProcess/ios/forms/WKFormSelectPicker.mm:815
> + for (const OptionItem& option :
_contentView.focusedSelectElementOptions) {
Nit - auto&?
> Source/WebKit/UIProcess/ios/forms/WKFormSelectPicker.mm:903
> + for (const OptionItem& option :
_contentView.focusedSelectElementOptions) {
Nit - `auto& option`?
> Source/WebKit/UIProcess/ios/forms/WKFormSelectPicker.mm:919
> +- (OptionItem *)optionItemAtIndexPath:(NSIndexPath *)indexPath
Nit - this is fine as-is since all call sites use the `OptionItem` immediately
without risk of reentrant calls that might change the focused element options,
but I think it would be safer to instead return an `Optional<OptionItem>`
instead. While this does copy `OptionItem` an extra time, it would avoid any
risk of accidentally UAF-ing option items in the future.
> Source/WebKit/UIProcess/ios/forms/WKFormSelectPicker.mm:922
> + for (OptionItem& option : _contentView.focusedSelectElementOptions) {
Nit - `auto& option`?
> Source/WebKit/UIProcess/ios/forms/WKFormSelectPicker.mm:950
> + ASSERT(option);
Nit - I think this is okay as-is since there are no out-of-band IPC messages a
compromised web process could use to update focused element from underneath us,
but it might still be safer to add a null check here.
> Source/WebKit/UIProcess/ios/forms/WKFormSelectPicker.mm:958
> + if (option->disabled)
> + cell.userInteractionEnabled = NO;
> + else
> + cell.userInteractionEnabled = YES;
Nit - this might be cleaner as `cell.userInteractionEnabled =
!option->disabled;`
> Source/WebKit/UIProcess/ios/forms/WKFormSelectPicker.mm:979
> + ASSERT(option);
(Ditto)
> Source/WebKit/UIProcess/ios/forms/WKFormSelectPicker.mm:993
> + [_contentView accessoryTab:YES];
These should probably capture the content view (either weakly or strongly).
> Source/WebKit/UIProcess/ios/forms/WKFormSelectPicker.mm:1000
> + [_contentView accessoryTab:NO];
(Ditto)
> Source/WebKit/UIProcess/ios/forms/WKFormSelectPicker.mm:1007
> + [_contentView accessoryDone];
(Ditto)
> Source/WebKit/UIProcess/ios/forms/WKFormSelectPicker.mm:1095
> + NSInteger currentRow = -1;
> + NSInteger totalRows = -1;
Nit - can we instead start these at 0, and move the increment after the return
statement below?
> Source/WebKit/UIProcess/ios/forms/WKFormSelectPicker.mm:1097
> + for (const OptionItem& option : _view.focusedSelectElementOptions) {
Nit - `auto& option`?
More information about the webkit-reviews
mailing list