Add font support to PageSerializer

Add font support to PageSerializer

Previously the PageSerializer (used to serialize pages before saving them
as e.g. MHTML files) didn't have support for web fonts.

R=abarth@chromium.org
BUG=275518
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=157670

[Tiger, 2013-08-19 13:26:58]

The font is missing here,since it is a binary file, and have will have to be
committed separately.

I'm a bit curious if the CSSFontFaceRule.h additions can be avoided somehow (the
same method is used for getting images). Ideas, thoughts?

[abarth-chromium, 2013-08-19 19:30:04]

Seems fine to me.  We'll need someone who knows the CSS directory better to
review that part of the change.

https://codereview.chromium.org/23202008/diff/1/Source/core/css/CSSFontFaceRu...
File Source/core/css/CSSFontFaceRule.h (right):

https://codereview.chromium.org/23202008/diff/1/Source/core/css/CSSFontFaceRu...
Source/core/css/CSSFontFaceRule.h:45: // TODO(tiger): Not CSSOM. Remove.
I would skip this comment.  If we don't want this function, we shouldn't include
it in this CL.

https://codereview.chromium.org/23202008/diff/1/Source/core/page/PageSerializ...
File Source/core/page/PageSerializer.h (right):

https://codereview.chromium.org/23202008/diff/1/Source/core/page/PageSerializ...
Source/core/page/PageSerializer.h:78: bool shouldAddUrl(const KURL&);
shouldAddUrl -> shouldAddURL

[Tiger, 2013-08-26 10:50:35]

On 2013/08/19 19:30:04, abarth wrote:
> Seems fine to me.  We'll need someone who knows the CSS directory better to
> review that part of the change.
> 
>
https://codereview.chromium.org/23202008/diff/1/Source/core/css/CSSFontFaceRu...
> File Source/core/css/CSSFontFaceRule.h (right):
> 
>
https://codereview.chromium.org/23202008/diff/1/Source/core/css/CSSFontFaceRu...
> Source/core/css/CSSFontFaceRule.h:45: // TODO(tiger): Not CSSOM. Remove.
> I would skip this comment.  If we don't want this function, we shouldn't
include
> it in this CL.
> 
>
https://codereview.chromium.org/23202008/diff/1/Source/core/page/PageSerializ...
> File Source/core/page/PageSerializer.h (right):
> 
>
https://codereview.chromium.org/23202008/diff/1/Source/core/page/PageSerializ...
> Source/core/page/PageSerializer.h:78: bool shouldAddUrl(const KURL&);
> shouldAddUrl -> shouldAddURL

I've addressed these issue. Who can take a look at the CSS part of this?

[abarth-chromium, 2013-08-26 19:23:21]

On 2013/08/26 10:50:35, Tiger wrote:
> I've addressed these issue. Who can take a look at the CSS part of this?

I'm not sure.  I would look at the blame for the CSS files you're changing to
see who has been reviewing changes to them recently.

[Tiger, 2013-08-27 14:39:26]

On 2013/08/26 19:23:21, abarth wrote:
> On 2013/08/26 10:50:35, Tiger wrote:
> > I've addressed these issue. Who can take a look at the CSS part of this?
> 
> I'm not sure.  I would look at the blame for the CSS files you're changing to
> see who has been reviewing changes to them recently.

I've added a couple of people that seem to work in these parts. Could any of you
take a look at this?

[Kunihiko Sakamoto, 2013-08-28 10:56:00]

On 2013/08/27 14:39:26, Tiger wrote:
> On 2013/08/26 19:23:21, abarth wrote:
> > On 2013/08/26 10:50:35, Tiger wrote:
> > > I've addressed these issue. Who can take a look at the CSS part of this?
> > 
> > I'm not sure.  I would look at the blame for the CSS files you're changing
to
> > see who has been reviewing changes to them recently.
> 
> I've added a couple of people that seem to work in these parts. Could any of
you
> take a look at this?

css/ changes seems OK to me (I'm not an OWNER though).
Once we support the CSS3 CSSFontFaceRule(*), we'll be able to remove
styleRule().

(*) http://www.w3.org/TR/css3-fonts/#cssfontfacerule

[Tiger, 2013-09-02 10:45:17]

On 2013/08/28 10:56:00, ksakamoto wrote:
> On 2013/08/27 14:39:26, Tiger wrote:
> > On 2013/08/26 19:23:21, abarth wrote:
> > > On 2013/08/26 10:50:35, Tiger wrote:
> > > > I've addressed these issue. Who can take a look at the CSS part of this?
> > > 
> > > I'm not sure.  I would look at the blame for the CSS files you're changing
> to
> > > see who has been reviewing changes to them recently.
> > 
> > I've added a couple of people that seem to work in these parts. Could any of
> you
> > take a look at this?
> 
> css/ changes seems OK to me (I'm not an OWNER though).
> Once we support the CSS3 CSSFontFaceRule(*), we'll be able to remove
> styleRule().
> 
> (*) http://www.w3.org/TR/css3-fonts/#cssfontfacerule

Sounds good. @jchaffraix, could you take a look at this when you get back?

[Julien - ping for review, 2013-09-05 17:38:48]

lgtm, abarth is probably a better resource for PageSerializer. Please fix the
style violations and static_casts (feel free to ask me to review again on irc ->
jchaffraix).

https://codereview.chromium.org/23202008/diff/6001/Source/core/page/PageSeria...
File Source/core/page/PageSerializer.cpp (right):

https://codereview.chromium.org/23202008/diff/6001/Source/core/page/PageSeria...
Source/core/page/PageSerializer.cpp:278:
retrieveResourcesForProperties((static_cast<CSSFontFaceRule*>(rule)->styleRule())->properties(),
document);
Doing direct static_cast should be avoided. You should just expose the following
method in CSSFontFaceRule.h

inline CSSFontFaceRule* toCSSFontFaceRule(CSSRule* rule)
{
    ASSERT_WITH_SECURITY_IMPLICATION(rule->type() == CSSRule::FONT_FACE_RULE);
// Or whatever check to ensure that the cast is OK
    return static_cast<CSSFontFaceRule*>(rule);
}

https://codereview.chromium.org/23202008/diff/6001/Source/core/page/PageSeria...
Source/core/page/PageSerializer.cpp:280:
retrieveResourcesForProperties((static_cast<CSSStyleRule*>(rule)->styleRule())->properties(),
document);
I would apply the same pattern here too. I won't repeat this comment for the
other static casts.

https://codereview.chromium.org/23202008/diff/6001/Source/core/page/PageSeria...
Source/core/page/PageSerializer.cpp:300: void
PageSerializer::addToResources(Resource *resource, PassRefPtr<SharedBuffer>
data, const KURL& url)
Style violation, it should be: Resource* resource, ...

https://codereview.chromium.org/23202008/diff/6001/Source/core/page/PageSeria...
Source/core/page/PageSerializer.cpp:372: for (unsigned i = 0; i <
cssValueList->length(); i++) {
One line statement don't need curly braces.

https://codereview.chromium.org/23202008/diff/6001/Source/web/tests/data/page...
File Source/web/tests/data/pageserializer/font/font.html (right):

https://codereview.chromium.org/23202008/diff/6001/Source/web/tests/data/page...
Source/web/tests/data/pageserializer/font/font.html:2: <html lang="en">
Do we need the lang attribute?

https://codereview.chromium.org/23202008/diff/6001/Source/web/tests/data/page...
Source/web/tests/data/pageserializer/font/font.html:4: <meta charset="utf-8">
Do we need this meta tag?

https://codereview.chromium.org/23202008/diff/6001/Source/web/tests/data/page...
Source/web/tests/data/pageserializer/font/font.html:6: <style type="text/css">
Unneeded type attribute.

[Tiger, 2013-09-10 09:10:20]

I've fixed the issues here. This test requires a ttf font to be present at
'Source/web/tests/data/pageserializer/font/font.ttf'. I've been using the
'Source/testing/data/fonts/WebKit Layout Tests.ttf' font which is quite small.
Could someone copy that there, or is there a better solution for this?

[commit-bot: I haz the power, 2013-09-11 13:25:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tiger@opera.com/23202008/25001

[commit-bot: I haz the power, 2013-09-11 13:26:03]

Can't process patch for file Source/web/tests/data/pageserializer/font/font.ttf.
Binary file support is temporarilly disabled due to a bug. Please commit blindly
the binary files first then commit the source change as a separate CL. Sorry for
the annoyance.

[commit-bot: I haz the power, 2013-09-12 07:08:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tiger@opera.com/23202008/25001

[commit-bot: I haz the power, 2013-09-12 07:20:19]

Retried try job too often on blink_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=blink_pres...

[Tiger, 2013-09-12 07:24:32]

On 2013/09/12 07:20:19, I haz the power (commit-bot) wrote:
> Retried try job too often on blink_presubmit for step(s) presubmit
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=blink_pres...

Okay, so this actually seems to work now. Just waiting for an LGTM from @abarth
or some other owner of Source/web/tests/ folder then (I've also added @tkent and
@jochen if you have time to look at this).

[jochen (gone - plz use gerrit), 2013-09-12 09:48:48]

lgtm

[commit-bot: I haz the power, 2013-09-12 10:04:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tiger@opera.com/23202008/25001

[commit-bot: I haz the power, 2013-09-12 11:30:43]

Change committed as 157670