...
<rdar://problem/72399337>
Created attachment 419772 [details] Patch
Comment on attachment 419772 [details] 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`?
Created attachment 420010 [details] Patch for landing
Committed r272753: <https://commits.webkit.org/r272753> All reviewed patches have been landed. Closing bug and clearing flags on attachment 420010 [details].