Bug 175028 – Use references rather than pointers for register/unregister functions, and more

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

[Darin Adler]

Use references rather than pointers for register/unregister functions, and more

[Darin Adler]

Created attachment 316861 [details]
Patch

[Darin Adler]

Current patch compiles on Mac, but likely not on iOS, GTK, WPE, Windows.

[Darin Adler]

Created attachment 319727 [details]
Patch

[Darin Adler]

Created attachment 319838 [details]
Patch

[Darin Adler]

Created attachment 319840 [details]
Patch

[Darin Adler]

Created attachment 319841 [details]
Patch

[Darin Adler]

Created attachment 319843 [details]
Patch

[Darin Adler]

Created attachment 319854 [details]
Patch

[Darin Adler]

Need help getting this to compile on iOS. The EWS log does not include the compile error I missed.

[Darin Adler]

Created attachment 320401 [details]
Patch

[Darin Adler]

Created attachment 359623 [details]
Patch

[Darin Adler]

Looks like this compiles, passes all tests, so ready to review and land.

[Daniel Bates]

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

[Darin Adler]

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".

[Darin Adler]

Committed r240237: <https://trac.webkit.org/changeset/240237>

[Radar WebKit Bug Importer]

<rdar://problem/47431526>

[David Kilzer (:ddkilzer)]

(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>

[Darin Adler]

(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.