Bug 145455 – Use modern for-loops in WebCore/page.

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

[Hunseop Jeong]

Use modern for-loops in WebCore/page.

[Hunseop Jeong]

Created attachment 253897 [details]
Patch

[Darin Adler]

Comment on attachment 253897 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=253897&action=review

> Source/WebCore/page/DOMWindow.cpp:263
> +        if (!set.contains(&window.get()))

Could use window.ptr() instead of &window.get().

> Source/WebCore/page/DOMWindow.cpp:302
> +        if (!set.contains(&window.get()))

Could use window.ptr() instead of &window.get().

> Source/WebCore/page/DeviceController.cpp:83
> +        listener->dispatchEvent(event);

The is indented incorrectly. Need to be in four spaces.

> Source/WebCore/page/DeviceController.cpp:98
> +        if (listener->document()
> +            && !listener->document()->activeDOMObjectsAreSuspended()
> +            && !listener->document()->activeDOMObjectsAreStopped())

I suggest putting listener->document() into a local variable and then putting this entire if statement on one line.

    auto document = listener->document();
    if (document && !document->activeDOMObjectsAreSuspended() && !document->activeDOMObjectsAreStopped())

That will fix the formatting problem we currently see here.

> Source/WebCore/page/DeviceController.cpp:99
> +        listener->dispatchEvent(getLastEvent());

This line of code needs to be indented; it doesn’t look like it’s guarded by the if!

> Source/WebCore/page/EventHandler.cpp:3869
> +    for (const auto& point : points) {

No need for const here.

> Source/WebCore/page/EventHandler.cpp:3876
> +    for (const auto& point : points) {

No need for const here.

> Source/WebCore/page/Frame.cpp:299
> +        if (Document* doc = frame->document())

Should rename this document instead of doc.

> Source/WebCore/page/Page.cpp:407
> +    for (auto map : viewModeMap) {
> +        if (text == map.name)
> +            return map.type;
>      }

Needs to be auto&, not auto. Individual view modes are not maps, they are modes, so the local variable should be named "mode", not "map".

> Source/WebCore/page/PerformanceUserTiming.cpp:168
> +    for (const auto& entry : performanceEntryMap.values())

No need for const here.

> Source/WebCore/page/WindowFeatures.cpp:243
> +    for (const auto& featureString : vector) {

No need for const here.

> Source/WebCore/page/animation/AnimationController.cpp:195
> +        Element* element = event.element.get();

Should be:

    auto& element = *event.element;

Then use "element." below instead of "element->".

[Hunseop Jeong]

Created attachment 253951 [details]
Patch

[Hunseop Jeong]

(In reply to comment #2)
> Comment on attachment 253897 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=253897&action=review
> 
> > Source/WebCore/page/DOMWindow.cpp:263
> > +        if (!set.contains(&window.get()))
> 
> Could use window.ptr() instead of &window.get().
> 
> > Source/WebCore/page/DOMWindow.cpp:302
> > +        if (!set.contains(&window.get()))
> 
> Could use window.ptr() instead of &window.get().
> 
> > Source/WebCore/page/DeviceController.cpp:83
> > +        listener->dispatchEvent(event);
> 
> The is indented incorrectly. Need to be in four spaces.
> 
> > Source/WebCore/page/DeviceController.cpp:98
> > +        if (listener->document()
> > +            && !listener->document()->activeDOMObjectsAreSuspended()
> > +            && !listener->document()->activeDOMObjectsAreStopped())
> 
> I suggest putting listener->document() into a local variable and then
> putting this entire if statement on one line.
> 
>     auto document = listener->document();
>     if (document && !document->activeDOMObjectsAreSuspended() &&
> !document->activeDOMObjectsAreStopped())
> 
> That will fix the formatting problem we currently see here.
> 
> > Source/WebCore/page/DeviceController.cpp:99
> > +        listener->dispatchEvent(getLastEvent());
> 
> This line of code needs to be indented; it doesn’t look like it’s guarded by
> the if!
> 
> > Source/WebCore/page/EventHandler.cpp:3869
> > +    for (const auto& point : points) {
> 
> No need for const here.
> 
> > Source/WebCore/page/EventHandler.cpp:3876
> > +    for (const auto& point : points) {
> 
> No need for const here.
> 
> > Source/WebCore/page/Frame.cpp:299
> > +        if (Document* doc = frame->document())
> 
> Should rename this document instead of doc.
> 
> > Source/WebCore/page/Page.cpp:407
> > +    for (auto map : viewModeMap) {
> > +        if (text == map.name)
> > +            return map.type;
> >      }
> 
> Needs to be auto&, not auto. Individual view modes are not maps, they are
> modes, so the local variable should be named "mode", not "map".
> 
> > Source/WebCore/page/PerformanceUserTiming.cpp:168
> > +    for (const auto& entry : performanceEntryMap.values())
> 
> No need for const here.
> 
> > Source/WebCore/page/WindowFeatures.cpp:243
> > +    for (const auto& featureString : vector) {
> 
> No need for const here.
> 
> > Source/WebCore/page/animation/AnimationController.cpp:195
> > +        Element* element = event.element.get();
> 
> Should be:
> 
>     auto& element = *event.element;
> 
> Then use "element." below instead of "element->".

All done. Thanks to your review.

[Hunseop Jeong]

@Darin,
Could you give me the cq+?

[WebKit Commit Bot]

Comment on attachment 253951 [details]
Patch

Clearing flags on attachment: 253951

Committed r185167: <http://trac.webkit.org/changeset/185167>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.