[Webkit-unassigned] [Bug 83865] [EFL] Add Web Inspector to WebKit-EFL

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=83865





--- Comment #3 from Seokju Kwon <seokju.kwon at samsung.com>  2012-04-23 00:59:03 PST ---
(In reply to comment #2)
> (From update of attachment 137052 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=137052&action=review
> 
> It's a good start, but there are some issues to take care first.
> 
> > Source/WebKit/ChangeLog:6
> > +        [EFL] Add Web Inspector to WebKit-EFL
> > +        https://bugs.webkit.org/show_bug.cgi?id=83865
> > +
> > +        Add EFL port implementation for Web Inspector
> 
> Please try to be more descriptive in the changelog entries.
> 
I will add additional comments in changelog entries.

> > Source/WebKit/PlatformEfl.cmake:291
> > +    FILE(COPY ${WEB_INSPECTOR_DATA} DESTINATION ${WEB_INSPECTOR_DIR})
> > +    FILE(COPY ${WEB_INSPECTOR_UGLIFYJS_DATA} DESTINATION ${WEB_INSPECTOR_UGLIFYJS_DIR})
> > +    FILE(COPY ${WEB_INSPECTOR_IMAGES_DATA} DESTINATION ${WEB_INSPECTOR_IMAGES_DIR})
> 
> This will be executed only during the build process; unless I'm mistaken, the inspector won't work properly unless these are actually installed.
> 
You're right. I will chanage it for these are actually installed.

> > Source/WebKit/efl/WebCoreSupport/InspectorClientEfl.cpp:34
> > +    InspectorFrontendClientEfl* inspectorFrontendClient = (InspectorFrontendClientEfl*)userData;
> 
> C++-style cast.
>
Ok, I will fix it.

> > Source/WebKit/efl/WebCoreSupport/InspectorClientEfl.cpp:36
> > +        inspectorFrontendClient->destroyInspectorWindow(true);
> 
> I'm not sure what 'true' means here; consider using an enum.

The argument of destoryInspectorWindow(bool notifyInspectorController) decides whether to notify inspectorController when destorying window. Other ports(Qt, GTK) are also using it like this.

> > Source/WebKit/efl/WebCoreSupport/InspectorClientEfl.cpp:41
> > +public:
> > +    virtual ~InspectorFrontendSettingsEfl() { }
> 
> No need to implement this.

Sure, I will remove it.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.