Bug 95444 – [EFL] Simplify FrameLoaderClientEfl by adding a private method.

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Jinwoo Song]

In the FrameLoaderClientEfl, the codes which checks that if current frame is main frame are frequently used.
This patch adds a private method and replaces those codes with the method to simplify.

[Jinwoo Song]

Created attachment 161441 [details]
patch

[Chris Dumez]

Comment on attachment 161441 [details]
patch

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

> Source/WebKit/efl/WebCoreSupport/FrameLoaderClientEfl.cpp:1057
> +bool FrameLoaderClientEfl::isMainFrame() const

Sounds like a good idea but the name is not explicit enough IMO. How about "isLoadingMainFrame()" ?

[Gyuyoung Kim]

Comment on attachment 161441 [details]
patch

From the refactoring view, looks good. But, this checking is used by ChromeClientEfl, DumpRenderTreeSupportEfl as well. IMO, it would be good if this patch covers them as well.

[Jinwoo Song]

(In reply to comment #2)
> (From update of attachment 161441 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=161441&action=review
> 
> > Source/WebKit/efl/WebCoreSupport/FrameLoaderClientEfl.cpp:1057
> > +bool FrameLoaderClientEfl::isMainFrame() const
> 
> Sounds like a good idea but the name is not explicit enough IMO. How about "isLoadingMainFrame()" ?

I agree to you and will change the name as your suggestion. Thanks for nice name.

[Jinwoo Song]

(In reply to comment #3)
> (From update of attachment 161441 [details])
> From the refactoring view, looks good. But, this checking is used by ChromeClientEfl, DumpRenderTreeSupportEfl as well. IMO, it would be good if this patch covers them as well.

Okay, I'll do refactoring for the ChromeClientEfl and DumpRenderTreeSupportEfl, too. But as the aspect is different and this patch cannot cover them, I'll cover them in another bug.

[Jinwoo Song]

Created attachment 161635 [details]
patch.

[Gyuyoung Kim]

Comment on attachment 161635 [details]
patch.

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

> Source/WebKit/efl/WebCoreSupport/FrameLoaderClientEfl.h:227
> +    bool isLoadingMainFrame() const;

Generally, function is placed before member variable.

[Jinwoo Song]

Created attachment 161638 [details]
patch.

[Jinwoo Song]

(In reply to comment #7)
> (From update of attachment 161635 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=161635&action=review
> 
> > Source/WebKit/efl/WebCoreSupport/FrameLoaderClientEfl.h:227
> > +    bool isLoadingMainFrame() const;
> 
> Generally, function is placed before member variable.

Done.

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 161638 [details]
patch.

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

> Source/WebKit/efl/WebCoreSupport/FrameLoaderClientEfl.cpp:1060
> +bool FrameLoaderClientEfl::isLoadingMainFrame() const
> +{
> +    return m_frame == ewk_view_frame_main_get(m_view);
> +}

This looks simple enough to be declared inline.

[Jinwoo Song]

Created attachment 161642 [details]
patch.

[Jinwoo Song]

(In reply to comment #10)
> (From update of attachment 161638 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=161638&action=review
> 
> > Source/WebKit/efl/WebCoreSupport/FrameLoaderClientEfl.cpp:1060
> > +bool FrameLoaderClientEfl::isLoadingMainFrame() const
> > +{
> > +    return m_frame == ewk_view_frame_main_get(m_view);
> > +}
> 
> This looks simple enough to be declared inline.
Done.

[Gyuyoung Kim]

Comment on attachment 161642 [details]
patch.

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

> Source/WebKit/efl/WebCoreSupport/FrameLoaderClientEfl.h:214
> +    bool isLoadingMainFrame() const { return m_frame == ewk_view_frame_main_get(m_view); }

Add a new line.

[Jinwoo Song]

Created attachment 161655 [details]
patch.

[Jinwoo Song]

(In reply to comment #13)
> (From update of attachment 161642 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=161642&action=review
> 
> > Source/WebKit/efl/WebCoreSupport/FrameLoaderClientEfl.h:214
> > +    bool isLoadingMainFrame() const { return m_frame == ewk_view_frame_main_get(m_view); }
> 
> Add a new line.
Done.

[WebKit Review Bot]

Comment on attachment 161655 [details]
patch.

Clearing flags on attachment: 161655

Committed r127244: <http://trac.webkit.org/changeset/127244>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.