Bug 145455

Summary: Use modern for-loops in WebCore/page.
Product: WebKit Reporter: Hunseop Jeong <hs85.jeong>
Component: WebCore Misc.Assignee: Hunseop Jeong <hs85.jeong>
Status: RESOLVED FIXED    
Severity: Normal CC: cmarcelo, commit-queue, darin, dino, dstockwell, jamesr, luiz, mkwst, simon.fraser, tonikitoo
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

Description Hunseop Jeong 2015-05-28 22:19:23 PDT
Use modern for-loops in WebCore/page.
Comment 1 Hunseop Jeong 2015-05-29 00:17:24 PDT
Created attachment 253897 [details]
Patch
Comment 2 Darin Adler 2015-05-29 09:09:47 PDT
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->".
Comment 3 Hunseop Jeong 2015-05-29 23:49:10 PDT
Created attachment 253951 [details]
Patch
Comment 4 Hunseop Jeong 2015-05-29 23:50:01 PDT
(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.
Comment 5 Hunseop Jeong 2015-06-02 18:32:18 PDT
@Darin,
Could you give me the cq+?
Comment 6 WebKit Commit Bot 2015-06-03 13:53:08 PDT
Comment on attachment 253951 [details]
Patch

Clearing flags on attachment: 253951

Committed r185167: <http://trac.webkit.org/changeset/185167>
Comment 7 WebKit Commit Bot 2015-06-03 13:53:13 PDT
All reviewed patches have been landed.  Closing bug.