| Summary: | [Windows] Generate Optimized WebInspectorUI in Release Build | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Brent Fulgham <bfulgham> | ||||||||||||||||
| Component: | Web Inspector | Assignee: | Brent Fulgham <bfulgham> | ||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||
| Severity: | Enhancement | CC: | bfulgham, buildbot, joepeck, rniwa, timothy, webkit-bug-importer | ||||||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||
| Hardware: | PC | ||||||||||||||||||
| OS: | Windows 7 | ||||||||||||||||||
| Attachments: |
|
||||||||||||||||||
|
Description
Brent Fulgham
2013-08-21 10:24:27 PDT
Created attachment 228641 [details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=228641&action=review > Source/WebInspectorUI/WebInspectorUI.vcxproj/build-webinspectorui.sh:62 > +export UNLOCALIZED_RESOURCES_FOLDER_PATH="WebInspectorUI" > + > +LICENSE=$(cat <<EOF > +/* > + * Copyright (C) 2007-2014 Apple Inc. All rights reserved. > + * Copyright (C) 2009-2011 Google Inc. All rights reserved. > + * Copyright (C) 2009-2010 Joseph Pecoraro. All rights reserved. > + * Copyright (C) 2008 Matt Lilek. All rights reserved. > + * Copyright (C) 2008-2009 Anthony Ricaud <rik@webkit.org> > + * Copyright (C) 2009 280 North Inc. All Rights Reserved. Having this in multiple places is going to be confusing for those of us that need to update it! How much of this can be shared with: Source/WebInspectorUI/Scripts/copy-user-interface-resources.sh It seems like most of this entire file after windows does the "export"s above! (In reply to comment #3) > View in context: https://bugs.webkit.org/attachment.cgi?id=228641&action=review > > > Source/WebInspectorUI/WebInspectorUI.vcxproj/build-webinspectorui.sh:62 > > +export UNLOCALIZED_RESOURCES_FOLDER_PATH="WebInspectorUI" > > + > > +LICENSE=$(cat <<EOF > > +/* > > + * Copyright (C) 2007-2014 Apple Inc. All rights reserved. > > + * Copyright (C) 2009-2011 Google Inc. All rights reserved. > > + * Copyright (C) 2009-2010 Joseph Pecoraro. All rights reserved. > > + * Copyright (C) 2008 Matt Lilek. All rights reserved. > > + * Copyright (C) 2008-2009 Anthony Ricaud <rik@webkit.org> > > + * Copyright (C) 2009 280 North Inc. All Rights Reserved. > > Having this in multiple places is going to be confusing for those of us that need to update it! > > How much of this can be shared with: > Source/WebInspectorUI/Scripts/copy-user-interface-resources.sh > > It seems like most of this entire file after windows does the "export"s above! The use of 'ditto' prevents us from using it on Windows. But you are right, most of this is duplicated from the Mac version. What if the whole thing was a Ruby or Python script instead? If we could use language file operations, it would avoid all of the futzing around with different copy commands, etc. Converting that shared script to another language is fine with me. Created attachment 228750 [details]
Converted copy-user-interface-resources.sh to a Perl script that can be shared across all platforms.
Comment on attachment 228750 [details]
Converted copy-user-interface-resources.sh to a Perl script that can be shared across all platforms.
WIP. Testing on Mac now that Windows side works. Will push to review once this is confirmed.
Comment on attachment 228750 [details] Converted copy-user-interface-resources.sh to a Perl script that can be shared across all platforms. View in context: https://bugs.webkit.org/attachment.cgi?id=228750&action=review > Source/WebInspectorUI/Scripts/copy-user-interface-resources.pl:62 > + system("\"$ENV{'SRCROOT'}/Scripts/combine-resources.pl\" --input-html \"$ENV{'SRCROOT'}/UserInterface/Main.html\" --derived-sources-dir \"$ENV{'DERIVED_SOURCES_DIR'}\" --output-dir \"$ENV{'DERIVED_SOURCES_DIR'}\" --output-script-name \"Main.js\" --output-style-name \"Main.css\""); system supports a list input and will not use a shell to evaluate the command. Eliminating the string quoting issues. system "$ENV{'SRCROOT'}/Scripts/combine-resources.pl", "--input-html" "$ENV{'SRCROOT'}/UserInterface/Main.html", "--derived-sources-dir", $ENV{"DERIVED_SOURCES_DIR"}, "--output-dir", $ENV{"DERIVED_SOURCES_DIR"}, "--output-script-name", "Main.js", "--output-style-name", "Main.css"; That is the syntax you should use throughout this script. > Source/WebInspectorUI/Scripts/copy-user-interface-resources.pl:85 > + if (open(LICENSE_FILE, '>', "$ENV{'TARGET_BUILD_DIR'}/$ENV{'UNLOCALIZED_RESOURCES_FOLDER_PATH'}/Main.js")) { > + print LICENSE_FILE $LICENSE; > + close(LICENSE_FILE); > + } Maybe a helper function for this? Created attachment 228757 [details]
Patch
Created attachment 228777 [details]
Completed Patch.
Comment on attachment 228777 [details]
Completed Patch.
This version works on Windows and Mac OS.
Comment on attachment 228777 [details] Completed Patch. View in context: https://bugs.webkit.org/attachment.cgi?id=228777&action=review Some comments worth addressing. > Source/WebInspectorUI/Scripts/copy-user-interface-resources.pl:26 > + } elsif ($^O eq 'darwin') { > + system("ditto \"$source\" \"$destination\""); > + } else { > + system("cp -r \"$source\" \"$destination\""); > + } There is an annoying difference between `ditto` and `cp -R`, and why I prefer ditto. `ditto` always copies the resources to a directory at the destination path. `cp -R` changes behavior if the destination does not exist, it copies source TO destination. if destination already existed, it copies source INTO destination. Given this setup: shell> mkdir foo shell> echo "hello" > foo/bar.txt shell> mkdir baz Compare: shell> ditto foo baz shell> find . . ./baz ./baz/bar.txt ./foo ./foo/bar.txt To: shell> cp -R foo baz shell> find . . ./baz ./baz/foo ./baz/foo/bar.txt ./foo ./foo/bar.txt So, long story short, unless we are very careful about always creating new directories, the "cp -R" path of ditto() here, might need to change. Maybe we should abort if $destination already exists, which would let us know that cp -R will exhibit unexpected behavior. > Source/WebInspectorUI/Scripts/copy-user-interface-resources.pl:35 > +my $LICENSE = <<EOF; > /* > * Copyright (C) 2007-2014 Apple Inc. All rights reserved. > * Copyright (C) 2009-2011 Google Inc. All rights reserved. > * Copyright (C) 2009-2010 Joseph Pecoraro. All rights reserved. > * Copyright (C) 2008 Matt Lilek. All rights reserved. > - * Copyright (C) 2008-2009 Anthony Ricaud <rik@webkit.org> > + * Copyright (C) 2008-2009 Anthony Ricaud <rik\@webkit.org> Hehe, this must have been awkward to discover! I think there is an easier fix though: <http://www.perlmonks.org/?node_id=736988> my $LICENSE = <<'EOF'; ... EOF Note the single quotes around 'EOF' imply the contents are like a single quotes string (without variable interpolation) instead of the default double quote (with variable interpolation). > Source/WebInspectorUI/Scripts/copy-user-interface-resources.pl:72 > +my $derivedSourcesDir = "$ENV{'DERIVED_SOURCES_DIR'}"; > +my $scriptsRoot = "$ENV{'SRCROOT'}/Scripts"; > +my $uiRoot = "$ENV{'SRCROOT'}/UserInterface"; > +my $targetResourcePath = "$ENV{'TARGET_BUILD_DIR'}/$ENV{'UNLOCALIZED_RESOURCES_FOLDER_PATH'}"; > +my $protocolDir = "$targetResourcePath/Protocol"; Is it normal to build paths with '/' in them, or is there a perl way to build paths? For instance: my $scriptsRoot = File::Spec->catdir($ENV{'SRCROOT'}, 'Scripts'); However, if the '/' works, then lets keep it, because it would make this script a lot easier to read in the long run. > Source/WebInspectorUI/Scripts/copy-user-interface-resources.pl:91 > + system("\"$scriptsRoot/combine-resources.pl\" --input-html \"$uiRoot/Main.html\" --derived-sources-dir \"$derivedSourcesDir\" --output-dir \"$derivedSourcesDir\" --output-script-name \"Main.js\" --output-style-name \"Main.css\""); I think Timothy Hatcher suggested using the list system syntax: So instead of: system("\"$scriptsRoot/combine-resources.pl\" --input-html \"$uiRoot/Main.html\" ..."); It would just be: system("$scriptsRoot/combine-resources.pl", '--input-html', "$uiRoot/Main.html", ...); Easier to read and avoid mistakes. > Source/WebInspectorUI/Scripts/copy-user-interface-resources.pl:-71 > - python "${SRCROOT}/Scripts/cssmin.py" <"${DERIVED_SOURCES_DIR}/Main.css" >"${TARGET_BUILD_DIR}/${UNLOCALIZED_RESOURCES_FOLDER_PATH}/Main.css" Nice! Looks like we fixed a bug here. Previously it was "> Main.css" which would overwrite the license, now it is ">> Main.css" appending. > Source/WebInspectorUI/Scripts/copy-user-interface-resources.pl:143 > + ditto("$uiRoot", $targetResourcePath); This is where I think `cp -R` will break. I think it will actually create an extra "UserInterface" folder inside of $targetResourcePath, which I don't think `ditto` would have done. > Source/WebInspectorUI/WebInspectorUI.vcxproj/build-webinspectorui.sh:29 > +NUMCPUS=`../../../Tools/Scripts/num-cpus` Needed? Is this used anywhere? > Source/WebInspectorUI/WebInspectorUI.vcxproj/build-webinspectorui.sh:46 > +XSRCROOT="`pwd`/.." > +XSRCROOT=`realpath "$XSRCROOT"` > +# Do a little dance to get the path into 8.3 form to make it safe for gnu make > +# http://bugzilla.opendarwin.org/show_bug.cgi?id=8173 > +XSRCROOT=`cygpath -m -s "$XSRCROOT"` > +XSRCROOT=`cygpath -u "$XSRCROOT"` > +export XSRCROOT > +export SRCROOT=$XSRCROOT > + > +XDSTROOT="$1" > +export XDSTROOT > +# Do a little dance to get the path into 8.3 form to make it safe for gnu make > +# http://bugzilla.opendarwin.org/show_bug.cgi?id=8173 > +XDSTROOT=`cygpath -m -s "$XDSTROOT"` > +XDSTROOT=`cygpath -u "$XDSTROOT"` > +export XDSTROOT Likewise, these comments say "to make it safe for gnu make" but we aren't inside a make here. Just bash / perl from here on out. Maybe the comment on these might want to be updated? Comment on attachment 228777 [details]
Completed Patch.
r- for the cp -R question / issue.
Comment on attachment 228777 [details] Completed Patch. View in context: https://bugs.webkit.org/attachment.cgi?id=228777&action=review >> Source/WebInspectorUI/Scripts/copy-user-interface-resources.pl:26 >> + } > > There is an annoying difference between `ditto` and `cp -R`, and why I prefer ditto. `ditto` always copies the resources to a directory at the destination path. `cp -R` changes behavior if the destination does not exist, it copies source TO destination. if destination already existed, it copies source INTO destination. > > Given this setup: > > shell> mkdir foo > shell> echo "hello" > foo/bar.txt > shell> mkdir baz > > Compare: > > shell> ditto foo baz > shell> find . > . > ./baz > ./baz/bar.txt > ./foo > ./foo/bar.txt > > To: > > shell> cp -R foo baz > shell> find . > . > ./baz > ./baz/foo > ./baz/foo/bar.txt > ./foo > ./foo/bar.txt > > So, long story short, unless we are very careful about always creating new directories, the "cp -R" path of ditto() here, might need to change. > > Maybe we should abort if $destination already exists, which would let us know that cp -R will exhibit unexpected behavior. On OS X we have ditto, so we are safe. On Windows, we have File::Copy::Recursive, which also works properly. How about if we just 'die' in the case where neither of these hold? I don't think any other ports use this code (yet), and they can flesh it out as they see fit. Frankly, Linux probably also has File::Copy::Recursive, so it's probably a moot point. >> Source/WebInspectorUI/Scripts/copy-user-interface-resources.pl:35 >> + * Copyright (C) 2008-2009 Anthony Ricaud <rik\@webkit.org> > > Hehe, this must have been awkward to discover! I think there is an easier fix though: > <http://www.perlmonks.org/?node_id=736988> > > my $LICENSE = <<'EOF'; > ... > EOF > > Note the single quotes around 'EOF' imply the contents are like a single quotes string (without variable interpolation) instead of the default double quote (with variable interpolation). Nice! I didn't know about this feature. Silly Perl! >> Source/WebInspectorUI/Scripts/copy-user-interface-resources.pl:72 >> +my $protocolDir = "$targetResourcePath/Protocol"; > > Is it normal to build paths with '/' in them, or is there a perl way to build paths? For instance: > > my $scriptsRoot = File::Spec->catdir($ENV{'SRCROOT'}, 'Scripts'); > > However, if the '/' works, then lets keep it, because it would make this script a lot easier to read in the long run. Actually, we should use 'catdir' since it means the path separator will be correct for the OS in use. It doesn't really matter for us because we're either darwin, Linux, or Cygwin. However, if we did want to support building on Windows outside of Cygwin, this script wouldn't have to change at all if we were using the 'catdir' method. >> Source/WebInspectorUI/Scripts/copy-user-interface-resources.pl:91 >> + system("\"$scriptsRoot/combine-resources.pl\" --input-html \"$uiRoot/Main.html\" --derived-sources-dir \"$derivedSourcesDir\" --output-dir \"$derivedSourcesDir\" --output-script-name \"Main.js\" --output-style-name \"Main.css\""); > > I think Timothy Hatcher suggested using the list system syntax: > > So instead of: > > system("\"$scriptsRoot/combine-resources.pl\" --input-html \"$uiRoot/Main.html\" ..."); > > It would just be: > > system("$scriptsRoot/combine-resources.pl", '--input-html', "$uiRoot/Main.html", ...); > > Easier to read and avoid mistakes. Good point. We can also get rid of a bunch of escaped quotes, which is always a major source of time-wasting syntax errors. >> Source/WebInspectorUI/Scripts/copy-user-interface-resources.pl:-71 >> - python "${SRCROOT}/Scripts/cssmin.py" <"${DERIVED_SOURCES_DIR}/Main.css" >"${TARGET_BUILD_DIR}/${UNLOCALIZED_RESOURCES_FOLDER_PATH}/Main.css" > > Nice! Looks like we fixed a bug here. Previously it was "> Main.css" which would overwrite the license, now it is ">> Main.css" appending. Yes! I'm sad I can't use the system list syntax here because of the reliance on shell redirection here. In the future, it might be nice to have jssmin.py/cssmin.py accept input/output arguments so we can avoid all of this quoting and escaping. >> Source/WebInspectorUI/Scripts/copy-user-interface-resources.pl:143 >> + ditto("$uiRoot", $targetResourcePath); > > This is where I think `cp -R` will break. I think it will actually create an extra "UserInterface" folder inside of $targetResourcePath, which I don't think `ditto` would have done. I see. I think it's safest just to 'die' here, so that Linux/EFL don't accidentally run into this if they someday start using this script. >> Source/WebInspectorUI/WebInspectorUI.vcxproj/build-webinspectorui.sh:29 >> +NUMCPUS=`../../../Tools/Scripts/num-cpus` > > Needed? Is this used anywhere? Nope! It's gone. (Obviously I copied this from another makefile!) >> Source/WebInspectorUI/WebInspectorUI.vcxproj/build-webinspectorui.sh:46 >> +export XDSTROOT > > Likewise, these comments say "to make it safe for gnu make" but we aren't inside a make here. Just bash / perl from here on out. Maybe the comment on these might want to be updated? Yes -- I can get rid of all of this 'dancing' and just use the 'realpath' output. Created attachment 228860 [details]
Patch
Comment on attachment 228860 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=228860&action=review > Source/WebInspectorUI/Scripts/copy-user-interface-resources.pl:133 > + seedFile($targetCodeMirrorCSS, $LICENSE); Question: Should we be seeding copyright to the Main.css file? We don't seem to have done so in the past. Comment on attachment 228860 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=228860&action=review > Source/WebInspectorUI/WebInspectorUI.vcxproj/build-webinspectorui.sh:46 > +if [[ "${TARGET_BUILD_DIR}/${UNLOCALIZED_RESOURCES_FOLDER_PATH}/CodeMirror.js" ]]; then > + export COMBINE_INSPECTOR_RESOURCES="YES"; > +fi When would this kick in? This requires a build to already exist that has combined resources/ What sets COMBINE_INSPECTOR_RESOURCES the first time? (In reply to comment #16) > (From update of attachment 228860 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=228860&action=review > > > Source/WebInspectorUI/Scripts/copy-user-interface-resources.pl:133 > > + seedFile($targetCodeMirrorCSS, $LICENSE); > > Question: Should we be seeding copyright to the Main.css file? We don't seem to have done so in the past. Yes, it should get the copyright. It was a bug before. View in context: https://bugs.webkit.org/attachment.cgi?id=228860&action=review Looks good to me. > Source/WebInspectorUI/Scripts/copy-user-interface-resources.pl:92 > +my $derivedSourcesDir = $ENV{'DERIVED_SOURCES_DIR'}; > +my $scriptsRoot = File::Spec->catdir($ENV{'SRCROOT'}, "Scripts"); > +my $uiRoot = File::Spec->catdir($ENV{'SRCROOT'}, "UserInterface"); > +my $targetResourcePath = File::Spec->catdir($ENV{'TARGET_BUILD_DIR'}, $ENV{'UNLOCALIZED_RESOURCES_FOLDER_PATH'}); > +my $protocolDir = File::Spec->catdir($targetResourcePath, "Protocol"); > + > +my $codeMirrorLicenseFile = File::Spec->catfile($uiRoot, "External", "CodeMirror", "LICENSE"); > +open(CMLFILE, '<', $codeMirrorLicenseFile) or die 'Unable to open $codeMirrorFile: ' . $!; > +my $CODE_MIRROR_LICENSE = "/*\n"; > +while (<CMLFILE>) { > + $CODE_MIRROR_LICENSE .= " * " . $_; > +} > +close(CMLFILE); > +$CODE_MIRROR_LICENSE .= " */\n"; Nit: There is a lot of jumping between single quoted strings and double quoted strings. I don't know if there is a WebKit perl standard. But I tend to prefer single quoted strings for simple strings (e.g. "Scripts", "External", " * ", above and plenty below). > Source/WebInspectorUI/Scripts/copy-user-interface-resources.pl:107 > + system($combineResourcesCmd, '--input-dir', File::Spec->catdir('External', 'CodeMirror'), '--input-html', File::Spec->catfile($derivedSourcesDir, 'Main.html'), '--input-html-dir', $uiRoot, '--derived-sources-dir', $derivedSourcesDir, '--output-dir', $derivedSourcesDir, '--output-script-name', 'CodeMirror.js', '--output-style-name', 'CodeMirror.css'); This just opens External/CodeMirror. Does it need to be $uiRoot/External/CodeMirror? >> Source/WebInspectorUI/Scripts/copy-user-interface-resources.pl:133 >> + seedFile($targetCodeMirrorCSS, $LICENSE); > > Question: Should we be seeding copyright to the Main.css file? We don't seem to have done so in the past. Hmm, good question. We were trying to do it before, so lets do it. > Source/WebInspectorUI/Scripts/copy-user-interface-resources.pl:139 > + system("python \"$jsMinScript\" < " . File::Spec->catfile($derivedSourcesDir, 'Main.js') . " >> \"$targetMainJS\"") and die "Failed to minify Main.js: $!"; > + system("python \"$cssMinScript\" < \"$mainCSSDerivedFile\" >> \"" . File::Spec->catfile($targetResourcePath, 'Main.css') . "\"") and die "Failed to minify Main.css: $!"; Main.js is not quoted as expected here, right? I would expect: "... < \"" . File::Sec->catfile(...) . "\" >> ..." (In reply to comment #19) > View in context: https://bugs.webkit.org/attachment.cgi?id=228860&action=review > > Looks good to me. > > > Source/WebInspectorUI/Scripts/copy-user-interface-resources.pl:92 > > +my $derivedSourcesDir = $ENV{'DERIVED_SOURCES_DIR'}; > > +my $scriptsRoot = File::Spec->catdir($ENV{'SRCROOT'}, "Scripts"); > > +my $uiRoot = File::Spec->catdir($ENV{'SRCROOT'}, "UserInterface"); > > +my $targetResourcePath = File::Spec->catdir($ENV{'TARGET_BUILD_DIR'}, $ENV{'UNLOCALIZED_RESOURCES_FOLDER_PATH'}); > > +my $protocolDir = File::Spec->catdir($targetResourcePath, "Protocol"); > > + > > +my $codeMirrorLicenseFile = File::Spec->catfile($uiRoot, "External", "CodeMirror", "LICENSE"); > > +open(CMLFILE, '<', $codeMirrorLicenseFile) or die 'Unable to open $codeMirrorFile: ' . $!; > > +my $CODE_MIRROR_LICENSE = "/*\n"; > > +while (<CMLFILE>) { > > + $CODE_MIRROR_LICENSE .= " * " . $_; > > +} > > +close(CMLFILE); > > +$CODE_MIRROR_LICENSE .= " */\n"; > > Nit: There is a lot of jumping between single quoted strings and double quoted strings. I don't know if there is a WebKit perl standard. But I tend to prefer single quoted strings for simple strings (e.g. "Scripts", "External", " * ", above and plenty below). We should use single quotes where possible. In the cases above, for \n to work it needs to be in double quotes or it won't expand. (In reply to comment #17) > (From update of attachment 228860 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=228860&action=review > > > Source/WebInspectorUI/WebInspectorUI.vcxproj/build-webinspectorui.sh:46 > > +if [[ "${TARGET_BUILD_DIR}/${UNLOCALIZED_RESOURCES_FOLDER_PATH}/CodeMirror.js" ]]; then > > + export COMBINE_INSPECTOR_RESOURCES="YES"; > > +fi > > When would this kick in? This requires a build to already exist that has combined resources/ What sets COMBINE_INSPECTOR_RESOURCES the first time? Doh! This is wrong. This got copied/pasted incorrectly at some point: It should be: if [[ ${TARGET_BUILD_DIR} =~ "Release" ]] || [[ ${TARGET_BUILD_DIR} =~ "Production" ]]; then (In reply to comment #19) > Nit: There is a lot of jumping between single quoted strings and double quoted strings. I don't know if there is a WebKit perl standard. But I tend to prefer single quoted strings for simple strings (e.g. "Scripts", "External", " * ", above and plenty below). Will fix. > > Source/WebInspectorUI/Scripts/copy-user-interface-resources.pl:107 > > + system($combineResourcesCmd, '--input-dir', File::Spec->catdir('External', 'CodeMirror'), '--input-html', File::Spec->catfile($derivedSourcesDir, 'Main.html'), '--input-html-dir', $uiRoot, '--derived-sources-dir', $derivedSourcesDir, '--output-dir', $derivedSourcesDir, '--output-script-name', 'CodeMirror.js', '--output-style-name', 'CodeMirror.css'); > > This just opens External/CodeMirror. Does it need to be $uiRoot/External/CodeMirror? Not sure. This is verbatim from the original shell script, but looking at the files on disk, it should be $uiRoot.... > >> Source/WebInspectorUI/Scripts/copy-user-interface-resources.pl:133 > >> + seedFile($targetCodeMirrorCSS, $LICENSE); > > > > Question: Should we be seeding copyright to the Main.css file? We don't seem to have done so in the past. > > Hmm, good question. We were trying to do it before, so lets do it. > > > Source/WebInspectorUI/Scripts/copy-user-interface-resources.pl:139 > > + system("python \"$jsMinScript\" < " . File::Spec->catfile($derivedSourcesDir, 'Main.js') . " >> \"$targetMainJS\"") and die "Failed to minify Main.js: $!"; > > + system("python \"$cssMinScript\" < \"$mainCSSDerivedFile\" >> \"" . File::Spec->catfile($targetResourcePath, 'Main.css') . "\"") and die "Failed to minify Main.css: $!"; > > Main.js is not quoted as expected here, right? I would expect: > > "... < \"" . File::Sec->catfile(...) . "\" >> ..." Yes! Good catch. Comment on attachment 228860 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=228860&action=review > Source/WebInspectorUI/Scripts/copy-user-interface-resources.pl:107 > + system($combineResourcesCmd, '--input-dir', File::Spec->catdir('External', 'CodeMirror'), '--input-html', File::Spec->catfile($derivedSourcesDir, 'Main.html'), '--input-html-dir', $uiRoot, '--derived-sources-dir', $derivedSourcesDir, '--output-dir', $derivedSourcesDir, '--output-script-name', 'CodeMirror.js', '--output-style-name', 'CodeMirror.css'); On second look, the value to input-dir needs to be 'External/CodeMirror'. The patch separator needs to always be "/" since that is how it is in the HTML. This string is used for a regex and needs to match. Created attachment 228884 [details]
Patch
Comment on attachment 228884 [details] Patch Attachment 228884 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/4818030794113024 New failing tests: media/track/w3c/track/webvtt/align_middle_position_50.html platform/mac/fast/scrolling/scroll-iframe-latched-mainframe.html platform/mac/fast/scrolling/scroll-select-latched-mainframe.html platform/mac/fast/scrolling/scroll-div-latched-mainframe.html Created attachment 228889 [details]
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-09 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
(In reply to comment #25) > (From update of attachment 228884 [details]) > Attachment 228884 [details] did not pass mac-wk2-ews (mac-wk2): > Output: http://webkit-queues.appspot.com/results/4818030794113024 > > New failing tests: > media/track/w3c/track/webvtt/align_middle_position_50.html > platform/mac/fast/scrolling/scroll-iframe-latched-mainframe.html > platform/mac/fast/scrolling/scroll-select-latched-mainframe.html > platform/mac/fast/scrolling/scroll-div-latched-mainframe.html The test errors are unrelated to this change. I've actually corrected the three scrolling tests under a separate commit. Committed r166962: <http://trac.webkit.org/changeset/166962> |