(Similar to #224517, but on macOS this time)
<rdar://problem/76664721>
Created attachment 426044 [details] Patch
Comment on attachment 426044 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=426044&action=review r=me > Source/WebCore/page/EventHandler.cpp:960 > + VisibleSelection newSelection { oldSelection }; NIT: Do we have a preference as to style of this vs something like `auto newSelection = oldSelection;`? > Source/WebCore/page/EventHandler.cpp:1009 > + auto isImageOverlayText = [&] (RefPtr<Node>&& node) { NIT: Does this need to capture `&`? NIT: It's a bit odd that this takes a `RefPtr<Node>&&` instead of just `RefPtr<Node>&` or something 🤔
Comment on attachment 426044 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=426044&action=review Thanks for the review! >> Source/WebCore/page/EventHandler.cpp:960 >> + VisibleSelection newSelection { oldSelection }; > > NIT: Do we have a preference as to style of this vs something like `auto newSelection = oldSelection;`? I don't think the style guidelines cover this, but I do agree that we generally use `auto/=` in new code, so I'll change it to that. >> Source/WebCore/page/EventHandler.cpp:1009 >> + auto isImageOverlayText = [&] (RefPtr<Node>&& node) { > > NIT: Does this need to capture `&`? > > NIT: It's a bit odd that this takes a `RefPtr<Node>&&` instead of just `RefPtr<Node>&` or something 🤔 Good point — removed the `&` capture, and changed to take `const RefPtr<Node>&`.
(In reply to Wenson Hsieh from comment #4) > Comment on attachment 426044 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=426044&action=review > > Thanks for the review! > > >> Source/WebCore/page/EventHandler.cpp:960 > >> + VisibleSelection newSelection { oldSelection }; > > > > NIT: Do we have a preference as to style of this vs something like `auto newSelection = oldSelection;`? > > I don't think the style guidelines cover this, but I do agree that we > generally use `auto/=` in new code, so I'll change it to that. > > >> Source/WebCore/page/EventHandler.cpp:1009 > >> + auto isImageOverlayText = [&] (RefPtr<Node>&& node) { > > > > NIT: Does this need to capture `&`? > > > > NIT: It's a bit odd that this takes a `RefPtr<Node>&&` instead of just `RefPtr<Node>&` or something 🤔 > > Good point — removed the `&` capture, and changed to take `const > RefPtr<Node>&`. I'm going to add a `HTMLElement::isImageOverlayText(const Node*)` overload and use that instead, as you suggested over Slack.
Created attachment 426050 [details] Address comments
Committed r275988 (236540@main): <https://commits.webkit.org/236540@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 426050 [details].