Enable pp::flash::FontFile support on Windows

Enable pp::flash::FontFile support on Windows

Part 1:
Added Windows implementation using Skia for PepperFlashFontFileHost.
This patch adds a simple Windows implementation of PepperFlashFontFileHost
using Skia to access the font data. By implementing this PepperFlash can
remove some use cases of GDI to support Win32k lockdown.

AUTHOR=forshaw@google.com

Part 2:
To support win32k lockdown, Pepper flash needs to switch Win GDI font calls
to pp::flash::FontFile PPAPI. This API was only supported on Linux, and it
is supported on Windows starting from M48.

Considering backward compatibility, we still need to fallback to GDI calls
for Chrome versions where Win32k lockdown is not available yet. We add a new
version of PB_Flash_FontFile interface: PPB_FLASH_FONTFILE_INTERFACE_0_2

This new version does not change any API, its availability shows that
pp::flash:FontFile is supported for Windows, and Pepper flash can use
it to decide whether it should call pp::flash:FontFile API or fall back
to Win GDI calls.

AUTHOR=xzhang@adobe.com

BUG=523278
R=bbudge@chromium.org, raymes@chromium.org, forshaw@google.com

Committed: https://crrev.com/eca2b40dd6ffacdf9e1772e5d18c3623d71b2946
Cr-Commit-Position: refs/heads/master@{#357457}

[Xing, 2015-10-19 15:33:46]

Description was changed from

==========
Enable pp::flash::FontFile support on Windows

Part 1:
Added Windows implementation using Skia for PepperFlashFontFileHost.
This patch adds a simple Windows implementation of PepperFlashFontFileHost
using Skia to access the font data. By implementing this PepperFlash can
remove some use cases of GDI to support Win32k lockdown.

AUTHOR=forshaw@google.com

Part 2:
To support win32k lockdown, Pepper flash needs to switch Win GDI font calls
to pp::flash::FontFile PPAPI. This API was only supported on Linux, and it
is supported on Windows starting from M48.

Considering backward compatibility, we still need to fallback to GDI calls
for Chrome versions where Win32k lockdown is not available yet. We add a new
version of PB_Flash_FontFile interface: PPB_FLASH_FONTFILE_INTERFACE_0_2

This new version does not change any API, its availability shows that
pp::flash:FontFile is supported for Windows, and Pepper flash can use
it to decide whether it should call pp::flash:FontFile API or fall back
to Win GDI calls.

AUTHOR=xzhang@adobe.com

BUG=523278,
R=bbudge@chromium.org, raymes@chromium.org, forshaw@google.com
==========

to

==========
Enable pp::flash::FontFile support on Windows

Part 1:
Added Windows implementation using Skia for PepperFlashFontFileHost.
This patch adds a simple Windows implementation of PepperFlashFontFileHost
using Skia to access the font data. By implementing this PepperFlash can
remove some use cases of GDI to support Win32k lockdown.

AUTHOR=forshaw@google.com

Part 2:
To support win32k lockdown, Pepper flash needs to switch Win GDI font calls
to pp::flash::FontFile PPAPI. This API was only supported on Linux, and it
is supported on Windows starting from M48.

Considering backward compatibility, we still need to fallback to GDI calls
for Chrome versions where Win32k lockdown is not available yet. We add a new
version of PB_Flash_FontFile interface: PPB_FLASH_FONTFILE_INTERFACE_0_2

This new version does not change any API, its availability shows that
pp::flash:FontFile is supported for Windows, and Pepper flash can use
it to decide whether it should call pp::flash:FontFile API or fall back
to Win GDI calls.

AUTHOR=xzhang@adobe.com

BUG=523278,
R=bbudge@chromium.org, raymes@chromium.org, forshaw@google.com
==========

[Xing, 2015-10-19 15:34:01]

Description was changed from

==========
Enable pp::flash::FontFile support on Windows

Part 1:
Added Windows implementation using Skia for PepperFlashFontFileHost.
This patch adds a simple Windows implementation of PepperFlashFontFileHost
using Skia to access the font data. By implementing this PepperFlash can
remove some use cases of GDI to support Win32k lockdown.

AUTHOR=forshaw@google.com

Part 2:
To support win32k lockdown, Pepper flash needs to switch Win GDI font calls
to pp::flash::FontFile PPAPI. This API was only supported on Linux, and it
is supported on Windows starting from M48.

Considering backward compatibility, we still need to fallback to GDI calls
for Chrome versions where Win32k lockdown is not available yet. We add a new
version of PB_Flash_FontFile interface: PPB_FLASH_FONTFILE_INTERFACE_0_2

This new version does not change any API, its availability shows that
pp::flash:FontFile is supported for Windows, and Pepper flash can use
it to decide whether it should call pp::flash:FontFile API or fall back
to Win GDI calls.

AUTHOR=xzhang@adobe.com

BUG=523278,
R=bbudge@chromium.org, raymes@chromium.org, forshaw@google.com
==========

to

==========
Enable pp::flash::FontFile support on Windows

Part 1:
Added Windows implementation using Skia for PepperFlashFontFileHost.
This patch adds a simple Windows implementation of PepperFlashFontFileHost
using Skia to access the font data. By implementing this PepperFlash can
remove some use cases of GDI to support Win32k lockdown.

AUTHOR=forshaw@google.com

Part 2:
To support win32k lockdown, Pepper flash needs to switch Win GDI font calls
to pp::flash::FontFile PPAPI. This API was only supported on Linux, and it
is supported on Windows starting from M48.

Considering backward compatibility, we still need to fallback to GDI calls
for Chrome versions where Win32k lockdown is not available yet. We add a new
version of PB_Flash_FontFile interface: PPB_FLASH_FONTFILE_INTERFACE_0_2

This new version does not change any API, its availability shows that
pp::flash:FontFile is supported for Windows, and Pepper flash can use
it to decide whether it should call pp::flash:FontFile API or fall back
to Win GDI calls.

AUTHOR=xzhang@adobe.com

BUG=523278
R=bbudge@chromium.org, raymes@chromium.org, forshaw@google.com
==========

[bbudge, 2015-10-20 20:17:24]

LGTM w/comment

https://codereview.chromium.org/1416643002/diff/1/chrome/renderer/pepper/pepp...
File chrome/renderer/pepper/pepper_flash_font_file_host.cc (right):

https://codereview.chromium.org/1416643002/diff/1/chrome/renderer/pepper/pepp...
chrome/renderer/pepper/pepper_flash_font_file_host.cc:74: *length =
typeface_->getTableSize(table);
nit: on this code path, length can be changed even if the result is false.

https://codereview.chromium.org/1416643002/diff/1/chrome/renderer/pepper/pepp...
chrome/renderer/pepper/pepper_flash_font_file_host.cc:80: *length = new_length;
nit: not necessary since they're equal.

https://codereview.chromium.org/1416643002/diff/1/ppapi/api/private/ppb_flash...
File ppapi/api/private/ppb_flash_font_file.idl (right):

https://codereview.chromium.org/1416643002/diff/1/ppapi/api/private/ppb_flash...
ppapi/api/private/ppb_flash_font_file.idl:8: */
Have you tried auto-generating the thunk? In other words add this:

[generate_thunk]

[Xing, 2015-10-21 13:26:34]

On 2015/10/20 20:17:24, bbudge wrote:
> LGTM w/comment
> 
>
https://codereview.chromium.org/1416643002/diff/1/chrome/renderer/pepper/pepp...
> File chrome/renderer/pepper/pepper_flash_font_file_host.cc (right):
> 
>
https://codereview.chromium.org/1416643002/diff/1/chrome/renderer/pepper/pepp...
> chrome/renderer/pepper/pepper_flash_font_file_host.cc:74: *length =
> typeface_->getTableSize(table);
> nit: on this code path, length can be changed even if the result is false.
> 
>
https://codereview.chromium.org/1416643002/diff/1/chrome/renderer/pepper/pepp...
> chrome/renderer/pepper/pepper_flash_font_file_host.cc:80: *length =
new_length;
> nit: not necessary since they're equal.
> 
>
https://codereview.chromium.org/1416643002/diff/1/ppapi/api/private/ppb_flash...
> File ppapi/api/private/ppb_flash_font_file.idl (right):
> 
>
https://codereview.chromium.org/1416643002/diff/1/ppapi/api/private/ppb_flash...
> ppapi/api/private/ppb_flash_font_file.idl:8: */
> Have you tried auto-generating the thunk? In other words add this:
> 
> [generate_thunk]

I am not exactly how to use IDL generator, I tried to run
generator.py/idl_thunk.py on many idl files under ppapi/api directory, I got
lots of errors, like certain type cannot be resolved. Could you give me a little
hint on how to use this tool?

[Xing, 2015-10-23 21:59:03]

Figured out how to use ppapi/generators/generator.py, it seems to require
"clang-format.bat" (idl_outfile.py) instead of "clang-format" in my Windows
environment.

Updated the patch with IDL generator generated thunk and header files

Fixed typo in pepper_flash_font_file_host.cc to remove the extra ")"

Added required type cast in pepper_flash_font_file_host.cc to avoid compiling
failure on Linux

Small fixes according comments by bbudge

https://codereview.chromium.org/1416643002/diff/1/chrome/renderer/pepper/pepp...
File chrome/renderer/pepper/pepper_flash_font_file_host.cc (right):

https://codereview.chromium.org/1416643002/diff/1/chrome/renderer/pepper/pepp...
chrome/renderer/pepper/pepper_flash_font_file_host.cc:80: *length = new_length;
On 2015/10/20 20:17:24, bbudge wrote:
> nit: not necessary since they're equal.

Done.

https://codereview.chromium.org/1416643002/diff/1/ppapi/api/private/ppb_flash...
File ppapi/api/private/ppb_flash_font_file.idl (right):

https://codereview.chromium.org/1416643002/diff/1/ppapi/api/private/ppb_flash...
ppapi/api/private/ppb_flash_font_file.idl:8: */
On 2015/10/20 20:17:24, bbudge wrote:
> Have you tried auto-generating the thunk? In other words add this:
> 
> [generate_thunk]
> 

Done.

https://codereview.chromium.org/1416643002/diff/20001/chrome/renderer/pepper/...
File chrome/renderer/pepper/pepper_flash_font_file_host.cc (right):

https://codereview.chromium.org/1416643002/diff/20001/chrome/renderer/pepper/...
chrome/renderer/pepper/pepper_flash_font_file_host.cc:69: result =
content::GetFontTable(fd, table, 0 /* offset */,
Fixed the typo of extra ')', and did the necessary type conversion to avoid
compiler error on Linux

[ilja, 2015-10-27 23:32:05]

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

[ilja, 2015-10-27 23:32:30]

The CQ bit was checked by ihf@chromium.org to run a CQ dry run

[ilja, 2015-10-27 23:33:10]

LGTM, but best if Bill takes a second look. Sending it to the trybots.

[commit-bot: I haz the power, 2015-10-27 23:35:02]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1416643002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1416643002/20001

[bbudge, 2015-10-28 00:06:33]

Some questions I missed last time.

https://codereview.chromium.org/1416643002/diff/1/chrome/renderer/pepper/pepp...
File chrome/renderer/pepper/pepper_flash_font_file_host.cc (right):

https://codereview.chromium.org/1416643002/diff/1/chrome/renderer/pepper/pepp...
chrome/renderer/pepper/pepper_flash_font_file_host.cc:74: *length =
typeface_->getTableSize(table);
On 2015/10/20 20:17:24, bbudge wrote:
> nit: on this code path, length can be changed even if the result is false.

OK I see this is an private method.

https://codereview.chromium.org/1416643002/diff/20001/chrome/renderer/pepper/...
File chrome/renderer/pepper/pepper_flash_font_file_host.cc (right):

https://codereview.chromium.org/1416643002/diff/20001/chrome/renderer/pepper/...
chrome/renderer/pepper/pepper_flash_font_file_host.cc:74: if (buffer == NULL) {
Digging into skia a little bit, it seems like you're duplicating the logic in
SkTypeface::getTableData, which just returns the length if buffer == nullptr.

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/skia/s...

Could you simplify the code here to just call getTableData?

Also, I'm a little confused because getTableData also potentially byte swaps the
tag (again).

[commit-bot: I haz the power, 2015-10-28 00:57:38]

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

[commit-bot: I haz the power, 2015-10-28 00:57:39]

Dry run: This issue passed the CQ dry run.

[Xing, 2015-10-28 21:52:50]

https://codereview.chromium.org/1416643002/diff/20001/chrome/renderer/pepper/...
File chrome/renderer/pepper/pepper_flash_font_file_host.cc (right):

https://codereview.chromium.org/1416643002/diff/20001/chrome/renderer/pepper/...
chrome/renderer/pepper/pepper_flash_font_file_host.cc:74: if (buffer == NULL) {
On 2015/10/28 00:06:33, bbudge wrote:
> Digging into skia a little bit, it seems like you're duplicating the logic in
> SkTypeface::getTableData, which just returns the length if buffer == nullptr.
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/skia/s...
> 

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/skia/s...

> Could you simplify the code here to just call getTableData?
> 
> Also, I'm a little confused because getTableData also potentially byte swaps
the
> tag (again).

It calls into |DWriteFontTypeface::onGetTableData()|, not
|LogFontTypeface::onGetTableData()|, so the logic does not seem to be quite
duplicate.

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/skia/s...

table tag is of type SkFontTableTag which is defined as:

/** Machine endian. */
typedef uint32_t SkFontTableTag;

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/skia/i...

Flash already has the table tag in big endian, so the swap here is just another
format change for skia's interface.

[bbudge, 2015-11-02 20:20:17]

Still LGTM. Thanks for addressing my comments.

https://codereview.chromium.org/1416643002/diff/20001/chrome/renderer/pepper/...
File chrome/renderer/pepper/pepper_flash_font_file_host.cc (right):

https://codereview.chromium.org/1416643002/diff/20001/chrome/renderer/pepper/...
chrome/renderer/pepper/pepper_flash_font_file_host.cc:74: if (buffer == NULL) {
On 2015/10/28 21:52:50, Xing wrote:
> On 2015/10/28 00:06:33, bbudge wrote:
> > Digging into skia a little bit, it seems like you're duplicating the logic
in
> > SkTypeface::getTableData, which just returns the length if buffer ==
nullptr.
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/skia/s...
> > 
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/skia/s...
> 
> > Could you simplify the code here to just call getTableData?
> > 
> > Also, I'm a little confused because getTableData also potentially byte swaps
> the
> > tag (again).
> 
> It calls into |DWriteFontTypeface::onGetTableData()|, not
> |LogFontTypeface::onGetTableData()|, so the logic does not seem to be quite
> duplicate.
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/skia/s...
> 
> table tag is of type SkFontTableTag which is defined as:
> 
> /** Machine endian. */
> typedef uint32_t SkFontTableTag;
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/skia/i...
> 
> Flash already has the table tag in big endian, so the swap here is just
another
> format change for skia's interface.

OK.

[Xing, 2015-11-02 21:26:30]

The CQ bit was checked by xzhang@adobe.com

[commit-bot: I haz the power, 2015-11-02 21:27:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1416643002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1416643002/20001

[commit-bot: I haz the power, 2015-11-02 22:43:02]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-11-02 22:43:55]

Patchset 2 (id:??) landed as
https://crrev.com/eca2b40dd6ffacdf9e1772e5d18c3623d71b2946
Cr-Commit-Position: refs/heads/master@{#357457}