Bug 98991 – [EFL] Share resources installed for inspector

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Ryuan Choi]

Now, Webkit1/Efl and WebKit2/Efl install same resources related to inspector to different directory.

Patch will be added.

[Ryuan Choi]

Created attachment 168145 [details]
Patch

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 168145 [details]
Patch

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

> Source/PlatformEfl.cmake:12
> +        DESTINATION ${DATA_INSTALL_DIR}

Don't you need to install into WEB_INSPECTOR_INSTALL_DIR instead?

> Source/WebKit2/UIProcess/efl/WebInspectorProxyEfl.cpp:105
> +

Nit: extra empty line.

[Ryuan Choi]

Created attachment 168196 [details]
Patch

[Ryuan Choi]

(In reply to comment #2)
> (From update of attachment 168145 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=168145&action=review
> 
> > Source/PlatformEfl.cmake:12
> > +        DESTINATION ${DATA_INSTALL_DIR}
> 
> Don't you need to install into WEB_INSPECTOR_INSTALL_DIR instead?
> 
No.
It copy the directory.

> > Source/WebKit2/UIProcess/efl/WebInspectorProxyEfl.cpp:105
> > +
> 
> Nit: extra empty line.

Right, fixed.

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #4)
> (In reply to comment #2)
> > (From update of attachment 168145 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=168145&action=review
> > 
> > > Source/PlatformEfl.cmake:12
> > > +        DESTINATION ${DATA_INSTALL_DIR}
> > 
> > Don't you need to install into WEB_INSPECTOR_INSTALL_DIR instead?
> > 
> No.
> It copy the directory.

Hmm, then it looks fragile. I think you should use DATA_INSTALL_DIR in WEB_INSPECTOR_DIR instead of hardcoding `share' there, so that WEB_INSPECTOR_DIR and WEB_INSPECTOR_INSTALL_DIR only differ in their installation prefix.

[Ryuan Choi]

Created attachment 168336 [details]
Patch

[Ryuan Choi]

(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #2)
> > > (From update of attachment 168145 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=168145&action=review
> > > 
> > > > Source/PlatformEfl.cmake:12
> > > > +        DESTINATION ${DATA_INSTALL_DIR}
> > > 
> > > Don't you need to install into WEB_INSPECTOR_INSTALL_DIR instead?
> > > 
> > No.
> > It copy the directory.
> 
> Hmm, then it looks fragile. I think you should use DATA_INSTALL_DIR in WEB_INSPECTOR_DIR instead of hardcoding `share' there, so that WEB_INSPECTOR_DIR and WEB_INSPECTOR_INSTALL_DIR only differ in their installation prefix.

Make sense.
I tried to use DATA_INSTALL_DIR in WEB_INSPECTOR_DIR.

[Ryuan Choi]

Created attachment 168338 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 168338 [details]
Patch

Looks fine. But, Rakuco might want to have a final look.

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 168338 [details]
Patch

LGTM, thanks!

[WebKit Review Bot]

Comment on attachment 168338 [details]
Patch

Clearing flags on attachment: 168338

Committed r131189: <http://trac.webkit.org/changeset/131189>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by bug 99187

[Ryuan Choi]

Created attachment 168602 [details]
Patch

[Ryuan Choi]

(In reply to comment #13)
> Re-opened since this is blocked by bug 99187

Sorry and sad.

Fixed below typo in WebInspectorProxy::inspectorPageURL from previous patch.

s/inspectorPageURL/inspectorBaseURL

[Gyuyoung Kim]

(In reply to comment #15)
> (In reply to comment #13)
> > Re-opened since this is blocked by bug 99187
> 
> Sorry and sad.
> 
> Fixed below typo in WebInspectorProxy::inspectorPageURL from previous patch.
> 
> s/inspectorPageURL/inspectorBaseURL

Do you check if this patch doesn't influence on EFL layout test anymore ?

[Ryuan Choi]

(In reply to comment #16)
> (In reply to comment #15)
> > (In reply to comment #13)
> > > Re-opened since this is blocked by bug 99187
> > 
> > Sorry and sad.
> > 
> > Fixed below typo in WebInspectorProxy::inspectorPageURL from previous patch.
> > 
> > s/inspectorPageURL/inspectorBaseURL
> 
> Do you check if this patch doesn't influence on EFL layout test anymore ?

I checked inspector and http/tets/inspector in my local.

[Gyuyoung Kim]

Comment on attachment 168602 [details]
Patch

Please check this if there is no problem after landing.

[Ryuan Choi]

Comment on attachment 168602 [details]
Patch

I will check once more after landed.

[WebKit Review Bot]

Comment on attachment 168602 [details]
Patch

Clearing flags on attachment: 168602

Committed r131323: <http://trac.webkit.org/changeset/131323>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.