Bug 98991 - [EFL] Share resources installed for inspector
Summary: [EFL] Share resources installed for inspector
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryuan Choi
URL:
Keywords:
Depends on: 99187
Blocks:
  Show dependency treegraph
 
Reported: 2012-10-10 21:34 PDT by Ryuan Choi
Modified: 2012-10-15 11:10 PDT (History)
5 users (show)

See Also:


Attachments
Patch (8.46 KB, patch)
2012-10-10 22:44 PDT, Ryuan Choi
no flags Details | Formatted Diff | Diff
Patch (8.47 KB, patch)
2012-10-11 05:15 PDT, Ryuan Choi
no flags Details | Formatted Diff | Diff
Patch (8.44 KB, patch)
2012-10-11 19:21 PDT, Ryuan Choi
no flags Details | Formatted Diff | Diff
Patch (8.50 KB, patch)
2012-10-11 19:56 PDT, Ryuan Choi
no flags Details | Formatted Diff | Diff
Patch (8.49 KB, patch)
2012-10-14 17:51 PDT, Ryuan Choi
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryuan Choi 2012-10-10 21:34:47 PDT
Now, Webkit1/Efl and WebKit2/Efl install same resources related to inspector to different directory.

Patch will be added.
Comment 1 Ryuan Choi 2012-10-10 22:44:48 PDT
Created attachment 168145 [details]
Patch
Comment 2 Raphael Kubo da Costa (:rakuco) 2012-10-11 04:42:40 PDT
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.
Comment 3 Ryuan Choi 2012-10-11 05:15:31 PDT
Created attachment 168196 [details]
Patch
Comment 4 Ryuan Choi 2012-10-11 05:17:28 PDT
(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.
Comment 5 Raphael Kubo da Costa (:rakuco) 2012-10-11 05:35:26 PDT
(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.
Comment 6 Ryuan Choi 2012-10-11 19:21:36 PDT
Created attachment 168336 [details]
Patch
Comment 7 Ryuan Choi 2012-10-11 19:28:21 PDT
(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.
Comment 8 Ryuan Choi 2012-10-11 19:56:40 PDT
Created attachment 168338 [details]
Patch
Comment 9 Gyuyoung Kim 2012-10-11 21:23:22 PDT
Comment on attachment 168338 [details]
Patch

Looks fine. But, Rakuco might want to have a final look.
Comment 10 Raphael Kubo da Costa (:rakuco) 2012-10-12 09:12:52 PDT
Comment on attachment 168338 [details]
Patch

LGTM, thanks!
Comment 11 WebKit Review Bot 2012-10-12 09:19:50 PDT
Comment on attachment 168338 [details]
Patch

Clearing flags on attachment: 168338

Committed r131189: <http://trac.webkit.org/changeset/131189>
Comment 12 WebKit Review Bot 2012-10-12 09:19:55 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 WebKit Review Bot 2012-10-12 09:52:36 PDT
Re-opened since this is blocked by bug 99187
Comment 14 Ryuan Choi 2012-10-14 17:51:56 PDT
Created attachment 168602 [details]
Patch
Comment 15 Ryuan Choi 2012-10-14 17:53:37 PDT
(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
Comment 16 Gyuyoung Kim 2012-10-14 19:28:25 PDT
(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 ?
Comment 17 Ryuan Choi 2012-10-14 21:16:47 PDT
(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.
Comment 18 Gyuyoung Kim 2012-10-14 21:27:10 PDT
Comment on attachment 168602 [details]
Patch

Please check this if there is no problem after landing.
Comment 19 Ryuan Choi 2012-10-14 21:27:52 PDT
Comment on attachment 168602 [details]
Patch

I will check once more after landed.
Comment 20 WebKit Review Bot 2012-10-15 11:09:54 PDT
Comment on attachment 168602 [details]
Patch

Clearing flags on attachment: 168602

Committed r131323: <http://trac.webkit.org/changeset/131323>
Comment 21 WebKit Review Bot 2012-10-15 11:10:00 PDT
All reviewed patches have been landed.  Closing bug.