[blink-in-js] Migrate resources required for blink-in-js to grd - part 3

[blink-in-js] Migrate resources required for blink-in-js to grd - part 3

Remove all the embedded resources from blink and migrate to using grd generated
pak files

BUG=312586
R=haraken@chromium.org, jochen@chromium.org

Part 1: https://codereview.chromium.org/558933002
Part 2: https://codereview.chromium.org/561653002

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=181913

[vivekg, 2014-09-10 05:46:10]

vivekg@chromium.org changed reviewers:
+ vivekg@chromium.org

[vivekg, 2014-09-10 05:46:11]

PTAL, thanks!

[haraken, 2014-09-10 07:30:33]

Looks good!

https://codereview.chromium.org/560763002/diff/1/Source/core/core_generated.gyp
File Source/core/core_generated.gyp (right):

https://codereview.chromium.org/560763002/diff/1/Source/core/core_generated.g...
Source/core/core_generated.gyp:186: '--for-testing',

--verbose ?

https://codereview.chromium.org/560763002/diff/1/Source/platform/PlatformReso...
File Source/platform/PlatformResourceLoader.cpp (right):

https://codereview.chromium.org/560763002/diff/1/Source/platform/PlatformReso...
Source/platform/PlatformResourceLoader.cpp:17: ASSERT(!dataString.isEmpty() &&
dataString.containsOnlyASCII());

What is this check for?

Other call sites of Platform::current()->loadResource are not checking the
condition.

e.g.,
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

If the check is not a worth, you can just directly write
Platform::current()->loadResource in PrivateScriptRunner.cpp and remove
PlatformResourceLoader.{h,cpp}.

[jochen (gone - plz use gerrit), 2014-09-10 07:35:47]

https://codereview.chromium.org/560763002/diff/1/Source/build/scripts/make_pr...
File Source/build/scripts/make_private_script_source.py (right):

https://codereview.chromium.org/560763002/diff/1/Source/build/scripts/make_pr...
Source/build/scripts/make_private_script_source.py:36: if '--for-testing' in
sys.argv:
maybe use optparse?

[vivekg, 2014-09-10 07:42:45]

https://codereview.chromium.org/560763002/diff/1/Source/platform/PlatformReso...
File Source/platform/PlatformResourceLoader.cpp (right):

https://codereview.chromium.org/560763002/diff/1/Source/platform/PlatformReso...
Source/platform/PlatformResourceLoader.cpp:17: ASSERT(!dataString.isEmpty() &&
dataString.containsOnlyASCII());
On 2014/09/10 at 07:30:33, haraken wrote:
> What is this check for?
> 

This is to make sure the script/stylesheets are indeed in ascii format. @abarth
suggested it here https://codereview.chromium.org/438843004/#msg2

> Other call sites of Platform::current()->loadResource are not checking the
condition.
> 
> e.g.,
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 
> If the check is not a worth, you can just directly write
Platform::current()->loadResource in PrivateScriptRunner.cpp and remove
PlatformResourceLoader.{h,cpp}.

This will be required here: https://codereview.chromium.org/438843004/ as there
are many call sites doing the same. Its a helper to minimize that effort.
Actually this was supposed to go along with that patch. As thats getting
delayed, I am pushing it via this patch which anyway will be used.

[vivekg, 2014-09-10 07:46:56]

https://codereview.chromium.org/560763002/diff/1/Source/core/core_generated.gyp
File Source/core/core_generated.gyp (right):

https://codereview.chromium.org/560763002/diff/1/Source/core/core_generated.g...
Source/core/core_generated.gyp:186: '--for-testing',
On 2014/09/10 at 07:30:32, haraken wrote:
> --verbose ?

The test resources are not packaged in the grd generated pak files. For the
tests, these will be still embedded in the binary. I am working on how to
migrate the test resources also out of the binary but that shouldn't stop us
from landing these changes.

To differentiate that, I used '--for-testing'. I think '--verbose' wouldn't
serve the purpose due to different literal meaning. WDYT?

[haraken, 2014-09-10 07:49:03]

On 2014/09/10 07:42:45, vivekg_ wrote:
>
https://codereview.chromium.org/560763002/diff/1/Source/platform/PlatformReso...
> File Source/platform/PlatformResourceLoader.cpp (right):
> 
>
https://codereview.chromium.org/560763002/diff/1/Source/platform/PlatformReso...
> Source/platform/PlatformResourceLoader.cpp:17: ASSERT(!dataString.isEmpty() &&
> dataString.containsOnlyASCII());
> On 2014/09/10 at 07:30:33, haraken wrote:
> > What is this check for?
> > 
> 
> This is to make sure the script/stylesheets are indeed in ascii format.
@abarth
> suggested it here https://codereview.chromium.org/438843004/#msg2
> 
> > Other call sites of Platform::current()->loadResource are not checking the
> condition.
> > 
> > e.g.,
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> > 
> > If the check is not a worth, you can just directly write
> Platform::current()->loadResource in PrivateScriptRunner.cpp and remove
> PlatformResourceLoader.{h,cpp}.
> 
> This will be required here: https://codereview.chromium.org/438843004/ as
there
> are many call sites doing the same. Its a helper to minimize that effort.
> Actually this was supposed to go along with that patch. As thats getting
> delayed, I am pushing it via this patch which anyway will be used.

Thanks, makes sense. Let's use the helper method in other places as well in a
follow-up CL :)

[haraken, 2014-09-10 07:49:26]

On 2014/09/10 07:46:56, vivekg_ wrote:
>
https://codereview.chromium.org/560763002/diff/1/Source/core/core_generated.gyp
> File Source/core/core_generated.gyp (right):
> 
>
https://codereview.chromium.org/560763002/diff/1/Source/core/core_generated.g...
> Source/core/core_generated.gyp:186: '--for-testing',
> On 2014/09/10 at 07:30:32, haraken wrote:
> > --verbose ?
> 
> The test resources are not packaged in the grd generated pak files. For the
> tests, these will be still embedded in the binary. I am working on how to
> migrate the test resources also out of the binary but that shouldn't stop us
> from landing these changes.
> 
> To differentiate that, I used '--for-testing'. I think '--verbose' wouldn't
> serve the purpose due to different literal meaning. WDYT?

Sorry, I was confused with what --for-testing does.

LGTM.

[vivekg, 2014-09-10 23:07:49]

The CQ bit was checked by vivekg@chromium.org

[commit-bot: I haz the power, 2014-09-10 23:08:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/560763002/1

[commit-bot: I haz the power, 2014-09-10 23:22:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-10 23:22:39]

Try jobs failed on following builders:
  mac_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...)

[vivekg, 2014-09-12 09:17:44]

The CQ bit was checked by vivekg@chromium.org

[commit-bot: I haz the power, 2014-09-12 09:18:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/560763002/1

[commit-bot: I haz the power, 2014-09-12 09:27:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-12 09:27:51]

Try jobs failed on following builders:
  mac_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...)

[vivekg, 2014-09-12 10:32:28]

The CQ bit was checked by vivekg@chromium.org

[commit-bot: I haz the power, 2014-09-12 10:33:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/560763002/20001

[vivekg, 2014-09-12 11:21:43]

The CQ bit was unchecked by vivekg@chromium.org

[vivekg, 2014-09-12 11:30:54]

On 2014/09/12 at 11:21:43, vivekg_ wrote:
> The CQ bit was unchecked by vivekg@chromium.org

@jochen, sorry had missed your comment previously about using optparse. I have
made the patch using optparse. PTAL.

@haraken, now that the dependent patches, [1, 2 & 3], have already landed, we
are ready to land this.

[1] https://android-review.googlesource.com/107500
[2] https://codereview.chromium.org/548023002
[3] https://codereview.chromium.org/456413002

[vivekg, 2014-09-12 15:10:07]

The CQ bit was checked by vivekg@chromium.org

[commit-bot: I haz the power, 2014-09-12 15:10:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/560763002/40001

[commit-bot: I haz the power, 2014-09-12 16:03:53]

Committed patchset #3 (id:40001) as 181913

[loislo, 2014-09-13 16:44:05]

On 2014/09/12 16:03:53, I haz the power (commit-bot) wrote:
> Committed patchset #3 (id:40001) as 181913

revert is coming. It breaks Linux GN (dbg) which is tree closer.

[vivekg, 2014-09-13 17:02:23]

On 2014/09/13 at 16:44:05, loislo wrote:
> On 2014/09/12 16:03:53, I haz the power (commit-bot) wrote:
> > Committed patchset #3 (id:40001) as 181913
> 
> revert is coming. It breaks Linux GN (dbg) which is tree closer.

Is there a linux gn dbg bot running this? I tried running linux_chromium_gn_rel
which passed... Am I missing anything? If you could paste the build failure log,
would be helpful. Thanks!