Bug 120120 – [Windows] Generate Optimized WebInspectorUI in Release Build

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Brent Fulgham]

The OS X build generates an "optimized" WebInspectorUI set consisting of combined/compressed source and resources to let the UI load more quickly.

We should do the same on Windows.

[Radar WebKit Bug Importer]

<rdar://problem/14797132>

[Brent Fulgham]

Created attachment 228641 [details]
Patch

[Joseph Pecoraro]

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 <[e-mail]>
> + * 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!

[Brent Fulgham]

(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 <[e-mail]>
> > + * 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.

[Timothy Hatcher]

Converting that shared script to another language is fine with me.

[Brent Fulgham]

Created attachment 228750 [details]
Converted copy-user-interface-resources.sh to a Perl script that can be shared across all platforms.

[Brent Fulgham]

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.

[Timothy Hatcher]

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?

[Brent Fulgham]

Created attachment 228757 [details]
Patch

[Brent Fulgham]

Created attachment 228777 [details]
Completed Patch.

[Brent Fulgham]

Comment on attachment 228777 [details]
Completed Patch.

This version works on Windows and Mac OS.

[Joseph Pecoraro]

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 <[e-mail]>
> + * 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?

[Joseph Pecoraro]

Comment on attachment 228777 [details]
Completed Patch.

r- for the cp -R question / issue.

[Brent Fulgham]

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.

[Brent Fulgham]

Created attachment 228860 [details]
Patch

[Brent Fulgham]

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.

[Timothy Hatcher]

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?

[Timothy Hatcher]

(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.

[Joseph Pecoraro]

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(...) . "\" >> ..."

[Timothy Hatcher]

(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.

[Brent Fulgham]

(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

[Brent Fulgham]

(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.

[Timothy Hatcher]

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.

[Brent Fulgham]

Created attachment 228884 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Brent Fulgham]

(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.

[Brent Fulgham]

Committed r166962: <http://trac.webkit.org/changeset/166962>