Use references rather than pointers for register/unregister functions, and more
Created attachment 316861 [details] Patch
Current patch compiles on Mac, but likely not on iOS, GTK, WPE, Windows.
Created attachment 319727 [details] Patch
Created attachment 319838 [details] Patch
Created attachment 319840 [details] Patch
Created attachment 319841 [details] Patch
Created attachment 319843 [details] Patch
Created attachment 319854 [details] Patch
Need help getting this to compile on iOS. The EWS log does not include the compile error I missed.
Created attachment 320401 [details] Patch
Created attachment 359623 [details] Patch
Looks like this compiles, passes all tests, so ready to review and land.
Comment on attachment 359623 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=359623&action=review I get the sense you were holding back from making more changes to keep this patch focused on the task at hand. I know I had difficulty keeping myself from commenting on other nits. I couldn’t help myself in the few comments I did make. Oops! Feel free to disregard. > Source/WebCore/dom/Document.h:1600 > + void buildAccessKeyMap(TreeScope& root); Parameter name doesn’t add much value to me :/ > Source/WebCore/dom/DocumentMarkerController.cpp:65 > + for (TextIterator markedText(&range); !markedText.atEnd(); markedText.advance()) { Ok as-is. Be a bit more idiomatic looking if we use assignment notation to invoke the constructor when initializing markedText. > Source/WebCore/dom/DocumentMarkerController.cpp:104 > + for (TextIterator markedText(&range); !markedText.atEnd(); markedText.advance()) { Ditto. > Source/WebCore/dom/DocumentMarkerController.cpp:117 > + for (TextIterator markedText(&range); !markedText.atEnd(); markedText.advance()) { Ditto > Source/WebCore/dom/DocumentMarkerController.cpp:128 > + for (TextIterator markedText(&range); !markedText.atEnd(); markedText.advance()) { Ditto > Source/WebCore/dom/DocumentMarkerController.cpp:138 > + for (TextIterator markedText(&range); !markedText.atEnd(); markedText.advance()) { Ditto
Comment on attachment 359623 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=359623&action=review >> Source/WebCore/dom/Document.h:1600 >> + void buildAccessKeyMap(TreeScope& root); > > Parameter name doesn’t add much value to me :/ Fixed. >> Source/WebCore/dom/DocumentMarkerController.cpp:65 >> + for (TextIterator markedText(&range); !markedText.atEnd(); markedText.advance()) { > > Ok as-is. Be a bit more idiomatic looking if we use assignment notation to invoke the constructor when initializing markedText. I think the semantics here are initialization and not assignment, so it’s probably not the right thing to do. If we did want to do it, we’d have to change the TextIterator constructor to no longer be marked "explicit".
Committed r240237: <https://trac.webkit.org/changeset/240237>
<rdar://problem/47431526>
(In reply to Darin Adler from comment #15) > Committed r240237: <https://trac.webkit.org/changeset/240237> Reverted the Xcode project changes here: Committed r240239: <https://trac.webkit.org/changeset/240239>
(In reply to David Kilzer (:ddkilzer) from comment #17) > Reverted the Xcode project changes here: > > Committed r240239: <https://trac.webkit.org/changeset/240239> Thanks David; I just let Xcode update the file and didn’t realize there was version skew.