Bug 41669

Summary: [Chromium] Upstreaming inspector_resources target
Product: WebKit Reporter: Kent Tamura <tkent>
Component: Web Inspector (Deprecated)Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, pfeldman, rolandsteiner, tony
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: All   
Bug Depends on: 42144    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch 2 none

Description Kent Tamura 2010-07-06 02:16:59 PDT
Upstreaming 'inspector_resources', which is currently in src/webkit/webkit.gyp of Chromium tree, to WebKit.gyp.
Comment 1 Kent Tamura 2010-07-06 02:22:57 PDT
Created attachment 60605 [details]
Patch
Comment 2 Adam Barth 2010-07-07 03:23:52 PDT
Comment on attachment 60605 [details]
Patch

This gyp magic is too deep for me, sorry.
Comment 3 Tony Chang 2010-07-07 08:30:08 PDT
Comment on attachment 60605 [details]
Patch

I realize that this is mostly a copy/paste from the existing gyp file, but might as well clean up the code a bit.

WebKit/chromium/WebKit.gyp:68
 +          'debug_devtools%': 0,
Nit: It would be nice if there was a comment describing what this does.

WebKit/chromium/WebKit.gyp:562
 +              # 'msvs_guid': '5330F8EE-00F5-D65C-166E-E3150171055D',
Can we just remove this?

WebKit/chromium/WebKit.gyp:566
 +                      'dependencies+': ['concatenated_devtools_js'],
What does the + at the end do?

WebKit/chromium/WebKit.gyp:578
 +                              'files/': [['exclude', '\\.js$']],
What does the / at the end of files do?

WebKit/chromium/WebKit.gyp:599
 +                      '<(chromium_src_dir)/webkit/build/generate_devtools_html.py',
Can we upstream generate_devtools_html.py too?
Comment 4 Kent Tamura 2010-07-07 19:17:39 PDT
Created attachment 60824 [details]
Patch 2
Comment 5 Kent Tamura 2010-07-07 19:23:57 PDT
Thank you for looking at the patch.

(In reply to comment #3)
> WebKit/chromium/WebKit.gyp:68
>  +          'debug_devtools%': 0,
> Nit: It would be nice if there was a comment describing what this does.

Added a comment.

> WebKit/chromium/WebKit.gyp:562
>  +              # 'msvs_guid': '5330F8EE-00F5-D65C-166E-E3150171055D',
> Can we just remove this?

Removed.  According to http://code.google.com/p/gyp/wiki/GypUserDocumentation#Skeleton_of_a_typical_executable_target_in_a_.gyp_file, we don't need msvs_guid anymore, right?

> WebKit/chromium/WebKit.gyp:566
>  +                      'dependencies+': ['concatenated_devtools_js'],
> What does the + at the end do?

Prepending something to 'dependencies' makes no sense.  I removed '+'.

> WebKit/chromium/WebKit.gyp:578
>  +                              'files/': [['exclude', '\\.js$']],
> What does the / at the end of files do?

I think we need to use pattern exclusion here.
http://code.google.com/p/gyp/wiki/InputFormatReference#Pattern_Lists_(/)

> WebKit/chromium/WebKit.gyp:599
>  +                      '<(chromium_src_dir)/webkit/build/generate_devtools_html.py',
> Can we upstream generate_devtools_html.py too?

We can.  Where should we put it to?  WebKitTools/Scripts?
Comment 6 Tony Chang 2010-07-08 10:39:38 PDT
LGTM

'<(chromium_src_dir)/webkit/build/generate_devtools_html.py',
> > Can we upstream generate_devtools_html.py too?
> 
> We can.  Where should we put it to?  WebKitTools/Scripts?

Hmm, good question. I guess I would be nice to make a directory at WebKit/WebKit/chromium/WebKit.gyp/ to put all the build related files (kind of like how we have WebCore/WebCore.gyp/).  It's kind of a pain to move WebKit.gyp now, so maybe just put it in WebKit/WebKit/chromium/ for now.

WebKitTools/Scripts doesn't seem to contain any build related scripts.
Comment 7 Kent Tamura 2010-07-12 22:37:45 PDT
Comment on attachment 60824 [details]
Patch 2

Clearing flags on attachment: 60824

Committed r63171: <http://trac.webkit.org/changeset/63171>
Comment 8 Kent Tamura 2010-07-12 22:37:53 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 Kent Tamura 2010-07-12 23:06:43 PDT
I rolled r63171 out. It failed by the following errors on Chromium Linux canary:

make: *** No rule to make target `third_party/WebKit/WebKit/chromium/v8/tools/codemap.js', needed by `out/Release/resources/inspector/DevTools.js'.
make: *** No rule to make target `third_party/WebKit/WebKit/chromium/v8/tools/consarray.js', needed by `out/Release/resources/inspector/DevTools.js'.
make: *** No rule to make target `third_party/WebKit/WebKit/chromium/v8/tools/csvparser.js', needed by `out/Release/resources/inspector/DevTools.js'.
make: *** No rule to make target `third_party/WebKit/WebKit/chromium/v8/tools/logreader.js', needed by `out/Release/resources/inspector/DevTools.js'.
make: *** No rule to make target `third_party/WebKit/WebKit/chromium/v8/tools/profile.js', needed by `out/Release/resources/inspector/DevTools.js'.
make: *** No rule to make target `third_party/WebKit/WebKit/chromium/v8/tools/profile_view.js', needed by `out/Release/resources/inspector/DevTools.js'.
make: *** No rule to make target `third_party/WebKit/WebKit/chromium/v8/tools/splaytree.js', needed by `out/Release/resources/inspector/DevTools.js'.

I'm investigating...
Comment 10 Pavel Feldman 2010-07-12 23:09:37 PDT
Please don't land this unless tested on all platforms. I'd like to have us rolled today.
Comment 11 Kent Tamura 2010-07-12 23:21:43 PDT
I identified the problem.

(In reply to comment #10)
> Please don't land this unless tested on all platforms. I'd like to have us rolled today.

ok, I'll test and will re-land the change tomorrow just in case.
Comment 12 Kent Tamura 2010-07-14 00:04:57 PDT
Re-landed as r63285: http://trac.webkit.org/changeset/63285