DevTools: Add CSS.getPlatformFontsForNode in protocol.json

DevTools: Add CSS.getPlatformFontsForNode in protocol.json

This patch adds method CSS.getPlatformFontsForNode to the protocol.
The method returns platform fonts that were used to render child text
nodes for the given node.

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

[lushnikov, 2013-08-15 18:30:01]

Please take a look

[pfeldman, 2013-08-16 08:16:47]

https://chromiumcodereview.appspot.com/22923010/diff/1/LayoutTests/inspector-...
File LayoutTests/inspector-protocol/dom-get-platform-fonts-expected.txt (right):

https://chromiumcodereview.appspot.com/22923010/diff/1/LayoutTests/inspector-...
LayoutTests/inspector-protocol/dom-get-platform-fonts-expected.txt:3:
{"textNodeList":[{"text":"First line.\n","fonts":["Times New Roman","Courier
New","Courier New","Courier New","Courier New","Courier New","Courier
New","Courier New","Courier New","Courier New","Courier New","Courier
New"]},{"text":"Second
line.","fonts":["Arial","Arial","Arial","Arial","Arial","Arial","Arial","Arial","Arial","Arial","Arial","Arial"]}]}
This will fail on other operating systems.

https://chromiumcodereview.appspot.com/22923010/diff/1/Source/core/inspector/...
File Source/core/inspector/InspectorDOMAgent.cpp (right):

https://chromiumcodereview.appspot.com/22923010/diff/1/Source/core/inspector/...
Source/core/inspector/InspectorDOMAgent.cpp:1238: String
InspectorDOMAgent::familyNameFromSimpleFontData(const SimpleFontData* fontData)
make it static

https://chromiumcodereview.appspot.com/22923010/diff/1/Source/core/inspector/...
Source/core/inspector/InspectorDOMAgent.cpp:1241: SkTypeface::LocalizedStrings*
fontFamilyIterator = platformData.typeface()->createFamilyNameIterator();
No, you can't use Sk here.

https://chromiumcodereview.appspot.com/22923010/diff/1/Source/core/inspector/...
Source/core/inspector/InspectorDOMAgent.cpp:1285: RenderText* renderer =
static_cast<RenderText*>(textNodes[i]->renderer());
toRenderText

https://chromiumcodereview.appspot.com/22923010/diff/1/Source/core/inspector/...
Source/core/inspector/InspectorDOMAgent.cpp:1287: RenderTextFragment*
textFragment = static_cast<RenderTextFragment*>(renderer);
ditto

https://chromiumcodereview.appspot.com/22923010/diff/1/Source/core/inspector/...
Source/core/inspector/InspectorDOMAgent.cpp:1292:
platformFontsForRenderer(static_cast<RenderText*>(current), fontChars, fonts);
collectFontsForRenderer

https://chromiumcodereview.appspot.com/22923010/diff/1/Source/core/inspector/...
Source/core/inspector/InspectorDOMAgent.cpp:1296:
platformFontsForRenderer(renderer, fontChars, fonts);
drop fontChars

https://chromiumcodereview.appspot.com/22923010/diff/1/Source/devtools/protoc...
File Source/devtools/protocol.json (right):

https://chromiumcodereview.appspot.com/22923010/diff/1/Source/devtools/protoc...
Source/devtools/protocol.json:1789: "id": "TextNodePlatformFonts",
hidden: true

https://chromiumcodereview.appspot.com/22923010/diff/1/Source/devtools/protoc...
Source/devtools/protocol.json:2016: "name": "getPlatformFontsForNode",
It looks more like CSS

https://chromiumcodereview.appspot.com/22923010/diff/1/Source/devtools/protoc...
Source/devtools/protocol.json:2021: { "name": "textNodeList", "type": "array",
"items": { "$ref": "TextNodePlatformFonts"}, "description": "Platform fonts
information about every child TextNode." }
name: "fonts"

[lushnikov, 2013-08-19 12:32:55]

please take a look

[pfeldman, 2013-08-19 12:40:12]

lgtm

Please make sure platformfont-specific code is reviewed by the people who know
how it works!

https://codereview.chromium.org/22923010/diff/33001/Source/core/inspector/Ins...
File Source/core/inspector/InspectorCSSAgent.h (right):

https://codereview.chromium.org/22923010/diff/33001/Source/core/inspector/Ins...
Source/core/inspector/InspectorCSSAgent.h:59: class PlatformFontUsage;
S > P > N

https://codereview.chromium.org/22923010/diff/33001/Source/core/platform/grap...
File Source/core/platform/graphics/harfbuzz/FontPlatformDataHarfBuzz.h (right):

https://codereview.chromium.org/22923010/diff/33001/Source/core/platform/grap...
Source/core/platform/graphics/harfbuzz/FontPlatformDataHarfBuzz.h:74: //
-------------------------------------------------------------------------
Blank line

[lushnikov, 2013-08-19 12:57:00]

@pdr, @fmalita: Could you please have a look on this patch? It touches some font
platform-specific code

[pdr., 2013-08-19 17:33:48]

On 2013/08/19 12:57:00, lushnikov wrote:
> @pdr, @fmalita: Could you please have a look on this patch? It touches some
font
> platform-specific code

I'm not the best person for font reviews. Adding eae and eseidel who know much
more about this.

[pdr., 2013-08-19 17:34:07]

On 2013/08/19 17:33:48, pdr wrote:
> On 2013/08/19 12:57:00, lushnikov wrote:
> > @pdr, @fmalita: Could you please have a look on this patch? It touches some
> font
> > platform-specific code
> 
> I'm not the best person for font reviews. Adding eae and eseidel who know much
> more about this.

[eae, 2013-08-19 19:36:43]

https://codereview.chromium.org/22923010/diff/39001/Source/core/platform/grap...
File Source/core/platform/graphics/chromium/FontPlatformDataChromiumWin.cpp
(right):

https://codereview.chromium.org/22923010/diff/39001/Source/core/platform/grap...
Source/core/platform/graphics/chromium/FontPlatformDataChromiumWin.cpp:207:
String FontPlatformData::fontFamilyName() const
Until we switch windows over to use Skia for font selection you'll likely likely
want to get the name from the HFONT (using LookupAltName and GetTextFace)
instead.

https://codereview.chromium.org/22923010/diff/39001/Source/core/platform/grap...
File Source/core/platform/graphics/harfbuzz/FontPlatformDataHarfBuzz.cpp
(right):

https://codereview.chromium.org/22923010/diff/39001/Source/core/platform/grap...
Source/core/platform/graphics/harfbuzz/FontPlatformDataHarfBuzz.cpp:220: return
String(localizedString.fString.c_str());
This will return the first non-empty name, is that really what you want?
Checking for the preferred language and returning that if set (and falling back
on the first non-empty_ seems like a better idea.

Another option would be to always return all of the names.

[lushnikov, 2013-08-21 13:43:14]

@eae, I've fixed PlatformDataChromiumWin to use HFONT instead of skia, could you
please take another look?

https://codereview.chromium.org/22923010/diff/39001/Source/core/platform/grap...
File Source/core/platform/graphics/chromium/FontPlatformDataChromiumWin.cpp
(right):

https://codereview.chromium.org/22923010/diff/39001/Source/core/platform/grap...
Source/core/platform/graphics/chromium/FontPlatformDataChromiumWin.cpp:207:
String FontPlatformData::fontFamilyName() const
On 2013/08/19 19:36:43, eae wrote:
> Until we switch windows over to use Skia for font selection you'll likely
likely
> want to get the name from the HFONT (using LookupAltName and GetTextFace)
> instead.

Could you clarify the need for the LookupAltName?
From here:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
I see that hfont is created iff altName is equal to the winName, which was
returned via GetTextFace. So altText should be the same as winText for every
given hfont, shouldn't it?

https://codereview.chromium.org/22923010/diff/39001/Source/core/platform/grap...
File Source/core/platform/graphics/harfbuzz/FontPlatformDataHarfBuzz.cpp
(right):

https://codereview.chromium.org/22923010/diff/39001/Source/core/platform/grap...
Source/core/platform/graphics/harfbuzz/FontPlatformDataHarfBuzz.cpp:220: return
String(localizedString.fString.c_str());
On 2013/08/19 19:36:43, eae wrote:
> This will return the first non-empty name, is that really what you want?
> Checking for the preferred language and returning that if set (and falling
back
> on the first non-empty_ seems like a better idea.
> 
> Another option would be to always return all of the names.

Yes, that's intended. I hope in future this will be replaces with a simple
SKTypeface.getFamilyName() call, which is currently bogus:
https://code.google.com/p/skia/issues/detail?id=1508&q=lushnikov&colspec=ID%2...

[eae, 2013-08-21 15:34:41]

LGTM.

Ok, as long as you are aware of it and want that behavior.

[commit-bot: I haz the power, 2013-08-21 20:34:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/lushnikov@chromium.org/22923010/47001

[commit-bot: I haz the power, 2013-08-21 21:44:25]

Change committed as 156505