Bug 104329 – [EFL] Refactor duplicate code into EflInspectorUtilities

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Seokju Kwon]

Remove duplicated work to get the resource path for inspector.

[Seokju Kwon]

Created attachment 178362 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 178362 [details]
Patch

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

> Source/WebKit/efl/WebCoreSupport/InspectorClientEfl.cpp:143
> +    return ASCIILiteral("file://") + getInspectorResourcePath();

In concatenation case, it looks that "file://" + getInspectorResourcePath() is enough, doesn't it ?

> Source/WebKit2/UIProcess/efl/WebInspectorProxyEfl.cpp:165
> +    return ASCIILiteral("file://") + WebCore::getInspectorResourcePath();

ditto.

[Ryuan Choi]

Comment on attachment 178362 [details]
Patch

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

> Source/WebCore/platform/efl/FileSystemEfl.cpp:121
> +String getInspectorResourcePath()
> +{

Can we guard this function in ENABLE(INSPECTOR).

[Seokju Kwon]

(In reply to comment #2)
> (From update of attachment 178362 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=178362&action=review
> 
> > Source/WebKit/efl/WebCoreSupport/InspectorClientEfl.cpp:143
> > +    return ASCIILiteral("file://") + getInspectorResourcePath();
> 
> In concatenation case, it looks that "file://" + getInspectorResourcePath() is enough, doesn't it ?
> 
> > Source/WebKit2/UIProcess/efl/WebInspectorProxyEfl.cpp:165
> > +    return ASCIILiteral("file://") + WebCore::getInspectorResourcePath();
> 
> ditto.

I will remove ASCIILiteral().

[Seokju Kwon]

(In reply to comment #3)
> (From update of attachment 178362 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=178362&action=review
> 
> > Source/WebCore/platform/efl/FileSystemEfl.cpp:121
> > +String getInspectorResourcePath()
> > +{
> 
> Can we guard this function in ENABLE(INSPECTOR).

I'll move it.

[Seokju Kwon]

Created attachment 178544 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 178544 [details]
Patch

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

Almost looks good to me.

> Source/WebCore/platform/FileSystem.h:233
> +String getInspectorResourcePath();

I think *inspectorResourcePath* is better according to webkit coding style guideline

Precede setters with the word "set". Use bare words for getters. Setter and getter names should match the names of the variables being set/gotten.
Right:
void setCount(size_t); // sets m_count
size_t count(); // returns m_count

Wrong:
void setCount(size_t); // sets m_theCount
size_t getCount();

[Gyuyoung Kim]

Comment on attachment 178544 [details]
Patch

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

>> Source/WebCore/platform/FileSystem.h:233
>> +String getInspectorResourcePath();
> 
> I think *inspectorResourcePath* is better according to webkit coding style guideline
> 
> Precede setters with the word "set". Use bare words for getters. Setter and getter names should match the names of the variables being set/gotten.
> Right:
> void setCount(size_t); // sets m_count
> size_t count(); // returns m_count
> 
> Wrong:
> void setCount(size_t); // sets m_theCount
> size_t getCount();

As we talked on irc, we need to find proper place for this utility function.

[Seokju Kwon]

Created attachment 179029 [details]
Patch

[Seokju Kwon]

(In reply to comment #8)
> (From update of attachment 178544 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=178544&action=review
> 
> >> Source/WebCore/platform/FileSystem.h:233
> >> +String getInspectorResourcePath();
> > 
> > I think *inspectorResourcePath* is better according to webkit coding style guideline
> > 
> > Precede setters with the word "set". Use bare words for getters. Setter and getter names should match the names of the variables being set/gotten.
> > Right:
> > void setCount(size_t); // sets m_count
> > size_t count(); // returns m_count
> > 
> > Wrong:
> > void setCount(size_t); // sets m_theCount
> > size_t getCount();
> 
> As we talked on irc, we need to find proper place for this utility function.

All done.
- Change function name.
- Move duplicate work into EflInspectorUtilities.

Please leave the comment, if you has a proper place for it.
If not, I will move it into EflInspectorUtilities for consistency.

[Ryuan Choi]

Comment on attachment 179029 [details]
Patch

Good

[WebKit Review Bot]

Comment on attachment 179029 [details]
Patch

Clearing flags on attachment: 179029

Committed r137531: <http://trac.webkit.org/changeset/137531>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.