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
Kent Tamura
2010-07-06 02:16:59 PDT
Created attachment 60605 [details]
Patch
Comment on attachment 60605 [details]
Patch
This gyp magic is too deep for me, sorry.
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?
Created attachment 60824 [details]
Patch 2
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? 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 on attachment 60824 [details] Patch 2 Clearing flags on attachment: 60824 Committed r63171: <http://trac.webkit.org/changeset/63171> All reviewed patches have been landed. Closing bug. 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... Please don't land this unless tested on all platforms. I'd like to have us rolled today. 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. Re-landed as r63285: http://trac.webkit.org/changeset/63285 |