Move the user agent styles sheets to blink_resources.grd (Part 3)

Move the user agent styles sheets to blink_resources.grd (Part 3)

Blink should make use of blink_resources.grd for the inline resources of user agent stylesheets.
This removes the dependency upon using make-file-arrays.py which embeds these resources as strings. 

Also the .rodata section of libblink_web (in component build mode) is reduced by ~33kb.

Part 1: https://codereview.chromium.org/436843004/
Part 2: https://codereview.chromium.org/422023008
Part 2 Unit Test support: https://codereview.chromium.org/456413002/

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

[vivekg, 2014-08-01 18:19:31]

PTAL, thank you.

[abarth-chromium, 2014-08-01 19:15:15]

LGTM

https://codereview.chromium.org/438843004/diff/40001/Source/core/css/CSSDefau...
File Source/core/css/CSSDefaultStyleSheets.cpp (right):

https://codereview.chromium.org/438843004/diff/40001/Source/core/css/CSSDefau...
Source/core/css/CSSDefaultStyleSheets.cpp:103: const blink::WebData&
htmlCssResource = blink::Platform::current()->loadResource("html.css");
No need for "blink::"

https://codereview.chromium.org/438843004/diff/40001/Source/core/rendering/Re...
File Source/core/rendering/RenderThemeChromiumAndroid.cpp (right):

https://codereview.chromium.org/438843004/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderThemeChromiumAndroid.cpp:64: return
String(mediaControlsAndroidCssResource.data(),
mediaControlsAndroidCssResource.size());
Should we add a function to WebData that returns a String?  maybe it should
ASSERT that the WebData is all ASCII.

[vivekg, 2014-08-02 03:45:35]

On 2014/08/01 19:15:15, abarth wrote:
> LGTM
> 
>
https://codereview.chromium.org/438843004/diff/40001/Source/core/css/CSSDefau...
> File Source/core/css/CSSDefaultStyleSheets.cpp (right):
> 
>
https://codereview.chromium.org/438843004/diff/40001/Source/core/css/CSSDefau...
> Source/core/css/CSSDefaultStyleSheets.cpp:103: const blink::WebData&
> htmlCssResource = blink::Platform::current()->loadResource("html.css");
> No need for "blink::"
> 
Done. 

>
https://codereview.chromium.org/438843004/diff/40001/Source/core/rendering/Re...
> File Source/core/rendering/RenderThemeChromiumAndroid.cpp (right):
> 
>
https://codereview.chromium.org/438843004/diff/40001/Source/core/rendering/Re...
> Source/core/rendering/RenderThemeChromiumAndroid.cpp:64: return
> String(mediaControlsAndroidCssResource.data(),
> mediaControlsAndroidCssResource.size());
> Should we add a function to WebData that returns a String?  maybe it should
> ASSERT that the WebData is all ASCII.

Done.

[abarth-chromium, 2014-08-02 17:19:51]

https://codereview.chromium.org/438843004/diff/80001/public/platform/WebData.h
File public/platform/WebData.h (right):

https://codereview.chromium.org/438843004/diff/80001/public/platform/WebData....
public/platform/WebData.h:79: WebCString asCString() const;
I meant an actual String:

#if INSIDE_BLINK
    BLINK_PLATFORM_EXPORT String toASCIIString();
#endif

Then in WebData.cpp:

String WebData::toASCIIString()
{
    String result(data(), size());
    ASSERT(result.containsOnlyASCII());
    return result;
}

By going through WebCString, you've introduced an extra copy of the data
(SharedBuffer -> WebCStringBuffer -> StringImpl).  The approach above has one
fewer copy (SharedBuffer -> StringImpl).

[vivekg, 2014-08-03 05:19:08]

On 2014/08/02 17:19:51, abarth wrote:
> https://codereview.chromium.org/438843004/diff/80001/public/platform/WebData.h
> File public/platform/WebData.h (right):
> 
>
https://codereview.chromium.org/438843004/diff/80001/public/platform/WebData....
> public/platform/WebData.h:79: WebCString asCString() const;
> I meant an actual String:
> 
> #if INSIDE_BLINK
>     BLINK_PLATFORM_EXPORT String toASCIIString();
> #endif
> 
> Then in WebData.cpp:
> 
> String WebData::toASCIIString()
> {
>     String result(data(), size());
>     ASSERT(result.containsOnlyASCII());
>     return result;
> }
> 
> By going through WebCString, you've introduced an extra copy of the data
> (SharedBuffer -> WebCStringBuffer -> StringImpl).  The approach above has one
> fewer copy (SharedBuffer -> StringImpl).

Sure, should have used WTF::String at first place. Thanks! Have fixed it, PTAL.

[abarth-chromium, 2014-08-03 14:53:31]

lgtm

https://codereview.chromium.org/438843004/diff/120001/public/platform/WebData.h
File public/platform/WebData.h (right):

https://codereview.chromium.org/438843004/diff/120001/public/platform/WebData...
public/platform/WebData.h:34: #include "WebCString.h"
This include is no longer necessary.

[vivekg, 2014-08-05 14:35:56]

The CQ bit was checked by vivekg@chromium.org

[vivekg, 2014-08-05 14:35:59]

The CQ bit was unchecked by vivekg@chromium.org

[vivekg, 2014-08-05 16:16:11]

The CQ bit was checked by vivekg@chromium.org

[commit-bot: I haz the power, 2014-08-05 16:18:12]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/vivek.vg@samsung.com/438843004/140001

[commit-bot: I haz the power, 2014-08-05 21:11:25]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...)
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/18500)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/21070)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/42682)

[commit-bot: I haz the power, 2014-08-05 21:23:40]

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

[commit-bot: I haz the power, 2014-08-05 21:23:41]

Try jobs failed on following builders:
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/18529)

[vivekg, 2014-08-11 13:07:03]

This patch needs the unit test plumbing done in
https://codereview.chromium.org/456413002/

[vivekg, 2014-09-02 11:13:08]

Patch rebased.

[eseidel, 2014-09-02 17:15:07]

This looks great.  If you get rid of the duplicate code everywehre. :)  No need
to add 3 lines in 18 places when you could add a 3 line function somewhere and
then add 1 line in 18 places. :)

https://codereview.chromium.org/438843004/diff/180001/Source/core/css/CSSDefa...
File Source/core/css/CSSDefaultStyleSheets.cpp (right):

https://codereview.chromium.org/438843004/diff/180001/Source/core/css/CSSDefa...
Source/core/css/CSSDefaultStyleSheets.cpp:144: String navigationTransitionsRules
= navigationTransitionsCssResource.toASCIIString();
It appears you want a helper wrapper.

loadCSSAsString("navigationTransitions.css")

Which does the load, the ASCII conversion and the ASSERT.

[vivekg, 2014-09-03 15:12:17]

On 2014/09/02 at 17:15:07, eseidel wrote:
> This looks great.  If you get rid of the duplicate code everywehre. :)  No
need to add 3 lines in 18 places when you could add a 3 line function somewhere
and then add 1 line in 18 places. :)
> 

Thank you :) 

>
https://codereview.chromium.org/438843004/diff/180001/Source/core/css/CSSDefa...
> File Source/core/css/CSSDefaultStyleSheets.cpp (right):
> 
>
https://codereview.chromium.org/438843004/diff/180001/Source/core/css/CSSDefa...
> Source/core/css/CSSDefaultStyleSheets.cpp:144: String
navigationTransitionsRules = navigationTransitionsCssResource.toASCIIString();
> It appears you want a helper wrapper.
> 
> loadCSSAsString("navigationTransitions.css")
> 
> Which does the load, the ASCII conversion and the ASSERT.

Done. Added loadResourceAsASCIIString() helper function. PTAL, thanks!

[eseidel, 2014-09-04 07:05:34]

https://codereview.chromium.org/438843004/diff/220001/Source/platform/Platfor...
File Source/platform/PlatformResourceLoader.cpp (right):

https://codereview.chromium.org/438843004/diff/220001/Source/platform/Platfor...
Source/platform/PlatformResourceLoader.cpp:13: inline WTF::String
loadResourceAsASCIIString(const char* resource)
inline doesn't do anything here.  No need for the WTF:: prefix.

https://codereview.chromium.org/438843004/diff/220001/Source/platform/Platfor...
File Source/platform/PlatformResourceLoader.h (right):

https://codereview.chromium.org/438843004/diff/220001/Source/platform/Platfor...
Source/platform/PlatformResourceLoader.h:12: WTF::String
loadResourceAsASCIIString(const char* resource);
No need for WTF::

https://codereview.chromium.org/438843004/diff/220001/public/platform/WebData.h
File public/platform/WebData.h (right):

https://codereview.chromium.org/438843004/diff/220001/public/platform/WebData...
public/platform/WebData.h:78: String toASCIIString() const;
I think we normally use CString for ascii-only strings.

[vivekg, 2014-09-04 07:44:28]

Thank you Eric.

https://codereview.chromium.org/438843004/diff/220001/Source/platform/Platfor...
File Source/platform/PlatformResourceLoader.cpp (right):

https://codereview.chromium.org/438843004/diff/220001/Source/platform/Platfor...
Source/platform/PlatformResourceLoader.cpp:13: inline WTF::String
loadResourceAsASCIIString(const char* resource)
On 2014/09/04 at 07:05:34, eseidel wrote:
> inline doesn't do anything here.  No need for the WTF:: prefix.

Done.

https://codereview.chromium.org/438843004/diff/220001/Source/platform/Platfor...
File Source/platform/PlatformResourceLoader.h (right):

https://codereview.chromium.org/438843004/diff/220001/Source/platform/Platfor...
Source/platform/PlatformResourceLoader.h:12: WTF::String
loadResourceAsASCIIString(const char* resource);
On 2014/09/04 at 07:05:34, eseidel wrote:
> No need for WTF::

Done. Also added missing  PLATFORM_EXPORT

https://codereview.chromium.org/438843004/diff/220001/public/platform/WebData.h
File public/platform/WebData.h (right):

https://codereview.chromium.org/438843004/diff/220001/public/platform/WebData...
public/platform/WebData.h:78: String toASCIIString() const;
On 2014/09/04 at 07:05:34, eseidel wrote:
> I think we normally use CString for ascii-only strings.

This function was required earlier. Now that we have the PlatformResourceLoader
helper function, this is not required anymore.

[eseidel, 2014-09-04 16:13:35]

The CQ bit was checked by eseidel@chromium.org

[eseidel, 2014-09-04 16:13:36]

lgtm

[commit-bot: I haz the power, 2014-09-04 16:13:56]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/vivek.vg@samsung.com/438843004/240001

[vivekg, 2014-09-04 16:14:37]

The CQ bit was unchecked by vivekg@chromium.org

[vivekg, 2014-09-04 16:15:38]

On 2014/09/04 at 16:13:35, eseidel wrote:
> The CQ bit was checked by eseidel@chromium.org

Thank you Eric, but this has a dependency over
https://codereview.chromium.org/456413002 for supporting the webkit_unit_tests.
Once that lands, I will land this one.

[vivekg, 2014-09-12 16:09:56]

The CQ bit was checked by vivekg@chromium.org

[commit-bot: I haz the power, 2014-09-12 16:10:11]

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

[vivekg, 2014-09-12 16:10:21]

The CQ bit was unchecked by vivekg@chromium.org

[vivekg, 2014-09-12 16:59:50]

The CQ bit was checked by vivekg@chromium.org

[commit-bot: I haz the power, 2014-09-12 17:00:53]

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

[commit-bot: I haz the power, 2014-09-12 17:15:39]

Committed patchset #10 (id:260001) as 181917

[jianli, 2014-09-12 22:01:15]

A revert of this CL (patchset #10 id:260001) has been created in
https://codereview.chromium.org/567073004/ by jianli@chromium.org.

The reason for reverting is: Speculative revert for breaking quite a few
browser_tests and content_browsertests across multiple platforms:
https://build.chromium.org/p/chromium.webkit/builders/Mac10.6%20Tests/builds/...
http://build.chromium.org/p/chromium.webkit/builders/Mac10.8%20Tests/builds/9662
http://build.chromium.org/p/chromium.webkit/builders/Win7%20Tests/builds/5566
.