Add dynamic web font support to Chrome's port of Webkit. ...

Add dynamic web font support to Chrome's port of Webkit. 

Moving OpenTypeUtilities.{cpp,h} from platform/graphics/win to platform/graphics/opentype
was sent upstream (pending review at https://bugs.webkit.org/show_bug.cgi?id=23028 )

This CL obsoletes http://codereview.chromium.org/14084/show because FontCustomPlatformData.{cpp,h}
were moved out of webkit/port.

BUG=1303
http://crbug.com/1303
TEST=LayoutTests/fast/css/font-face-remote.html
Besides, go to http://www.alistapart.com/articles/cssatten and click on 'html' links for various test cases.



Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=7592

[Nicolas Sylvain, 2008-12-30 22:29:40]

I give full LGTM power to eroman on this one.

[eroman, 2009-01-03 06:05:51]

* Please add the corresponding build file changes

* Are there any corresponding layout-tests that can be removed from the fixable
list?

http://codereview.chromium.org/16494/diff/1/2
File
third_party/WebKit/WebCore/platform/graphics/chromium/FontCustomPlatformData.cpp
(right):

http://codereview.chromium.org/16494/diff/1/2#newcode70
Line 70: std::min(static_cast<size_t>(LF_FACESIZE), 1 + m_name.length()));
This doesn't NULL-terminate in the case where length exceeds LF_FACESIZE.

In fact why not just fail if it is too long, since (a) I don't expect
CreateFontIndirect will find a match on truncated family name (b)
createUniqueFontName() should not be giving us such long names anyway.

http://codereview.chromium.org/16494/diff/1/2#newcode92
Line 92: notImplemented();
Does this compile on linux/mac?

I reckon we need to return a value.

http://codereview.chromium.org/16494/diff/1/2#newcode98
Line 98: class EOTStream {
It is unfortunate that all of this code is duplicated from
platform/graphics/win/FontCustomPlatformData.cpp

Can you add a comment explaining where it comes from?
And a TODO for consolidating it later.

http://codereview.chromium.org/16494/diff/1/2#newcode127
Line 127: size_t bytesFromHeader =std::min(m_eotHeader.size() - m_offset,
count);
nit: Add a space after the equals.

http://codereview.chromium.org/16494/diff/1/2#newcode138
Line 138: bytesToRead);
nit: May as well keep this formatted the same as
"platform/graphics/win/FontCustomPlatformData.cpp", by making it single-line
min.

http://codereview.chromium.org/16494/diff/1/2#newcode143
Line 143: size_t bytesToCopy =std::min(bytesFromData - dstOffset,
nit: Same as above. Also, add a space after the equals.

http://codereview.chromium.org/16494/diff/1/2#newcode211
Line 211: notImplemented();
Perhaps we should return 0 here as well?

[jungshik at Google, 2009-01-05 21:24:11]

On 2009/01/03 06:05:51, eroman wrote:
> * Please add the corresponding build file changes
> 

It's in http://codereview.chromium.org/17036/show
Can I merge it with this CL? I'm a bit confused as to how to deal with 
third_party/WebKit and webkit/*?

> * Are there any corresponding layout-tests that can be removed from the
fixable
> list?

Yes, there is. It's LayoutTests/fast/css/font-face-remote.html. SVG test still
fails (but in a different way from without this patch). Again, can/should I add
the test list change to this CL? 

> http://codereview.chromium.org/16494/diff/1/2
> File
>
third_party/WebKit/WebCore/platform/graphics/chromium/FontCustomPlatformData.cpp
> (right):
> 
> http://codereview.chromium.org/16494/diff/1/2#newcode70
> Line 70: std::min(static_cast<size_t>(LF_FACESIZE), 1 + m_name.length()));
> This doesn't NULL-terminate in the case where length exceeds LF_FACESIZE.
> 
> In fact why not just fail if it is too long, since (a) I don't expect
> CreateFontIndirect will find a match on truncated family name (b)
> createUniqueFontName() should not be giving us such long names anyway.

I added an if-statement and return early in that case (which should not happen.
so, I added ASSERT_NOT_REACHED inside the if-block)

> 
> http://codereview.chromium.org/16494/diff/1/2#newcode92
> Line 92: notImplemented();
> Does this compile on linux/mac?
> 
> I reckon we need to return a value.

Added a return stmnt.

> http://codereview.chromium.org/16494/diff/1/2#newcode98
> Line 98: class EOTStream {
> It is unfortunate that all of this code is duplicated from
> platform/graphics/win/FontCustomPlatformData.cpp
> 
> Can you add a comment explaining where it comes from?
> And a TODO for consolidating it later.

Done

> http://codereview.chromium.org/16494/diff/1/2#newcode127
> Line 127: size_t bytesFromHeader =std::min(m_eotHeader.size() - m_offset,
> count);
> nit: Add a space after the equals.

done
 
> http://codereview.chromium.org/16494/diff/1/2#newcode138
> Line 138: bytesToRead);
> nit: May as well keep this formatted the same as
> "platform/graphics/win/FontCustomPlatformData.cpp", by making it single-line
> min.
> 
> http://codereview.chromium.org/16494/diff/1/2#newcode143
> Line 143: size_t bytesToCopy =std::min(bytesFromData - dstOffset,
> nit: Same as above. Also, add a space after the equals.

done
 
> http://codereview.chromium.org/16494/diff/1/2#newcode211
> Line 211: notImplemented();
> Perhaps we should return 0 here as well?

done

[eroman, 2009-01-06 02:17:05]

LGTM

> It's in http://codereview.chromium.org/17036/show
> Can I merge it with this CL? I'm a bit confused as to how to deal with
> third_party/WebKit and webkit/*?

You are correct, those edits can't be part of this change since they spans
modules. Your current approach is good: (a) commit to third_party/WebKit, (b)
make a change which updates DEPS, the build files, the layout test list.

I just wanted to see what the sister changes looked like.

> Yes, there is. It's LayoutTests/fast/css/font-face-remote.html. SVG test
> still fails (but in a different way from without this patch). Again,
> can/should I add the test list change to this CL?

That is fine, see above.

http://codereview.chromium.org/16494/diff/213/28
File
third_party/WebKit/WebCore/platform/graphics/chromium/FontCustomPlatformData.cpp
(right):

http://codereview.chromium.org/16494/diff/213/28#newcode71
Line 71: if (m_name.length() >= LF_FACESIZE) {
I think this check should be against m_name.length() + 1, since it must also
accomodate the null-terminator.