DevTools: Front-end for platform font inspection

DevTools: Front-end for platform font inspection

This patch adds UI for the 'platform font inspection' feature.

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

[aandrey, 2013-08-22 13:05:35]

https://codereview.chromium.org/22875023/diff/3001/Source/devtools/front_end/...
File Source/devtools/front_end/PlatformFontsSidebarPane.js (right):

https://codereview.chromium.org/22875023/diff/3001/Source/devtools/front_end/...
Source/devtools/front_end/PlatformFontsSidebarPane.js:2: * Copyright (C) 2007
Apple Inc.  All rights reserved.
fix license

https://codereview.chromium.org/22875023/diff/3001/Source/devtools/front_end/...
Source/devtools/front_end/PlatformFontsSidebarPane.js:37:
WebInspector.domAgent.addEventListener(WebInspector.DOMAgent.Events.AttrModified,
this._innerUpdate, this);
This can be a performance issue. See https://codereview.chromium.org/22548006/
where I fix slow breadcrumbs. Maybe you also want to listen to the
ElementsTreeUpdated event instead to sync with the elements panel UI updates? Or
at least consider throttling these events (_innerUpdate seems pretty "heavy").

https://codereview.chromium.org/22875023/diff/3001/Source/devtools/front_end/...
Source/devtools/front_end/PlatformFontsSidebarPane.js:56: this._node = node;
The old callbacks produced by callbackBarrier may be still alive at this point.
You do not handle the [this._platformFontsCallback, this._computedStyleCallback,
this._render] being called for an old node.

https://codereview.chromium.org/22875023/diff/3001/Source/devtools/front_end/...
Source/devtools/front_end/PlatformFontsSidebarPane.js:101: }.bind(this));
no need to bind

https://codereview.chromium.org/22875023/diff/3001/Source/devtools/front_end/...
Source/devtools/front_end/PlatformFontsSidebarPane.js:103: var cssFontSection =
this.element.createChild("div");
this.element.createChild("div", "stats-section monospace");

https://codereview.chromium.org/22875023/diff/3001/Source/devtools/front_end/...
Source/devtools/front_end/PlatformFontsSidebarPane.js:105: var cssFontPrefix =
cssFontSection.createChild("span");
ditto

https://codereview.chromium.org/22875023/diff/3001/Source/devtools/front_end/...
Source/devtools/front_end/PlatformFontsSidebarPane.js:108: var cssFontColumn =
cssFontSection.createChild("span");
use createTextChild()?

https://codereview.chromium.org/22875023/diff/3001/Source/devtools/front_end/...
Source/devtools/front_end/PlatformFontsSidebarPane.js:110: var cssFontValue =
cssFontSection.createChild("span");
cssFontSection.createChild("span", "css-font-value");

https://codereview.chromium.org/22875023/diff/3001/Source/devtools/front_end/...
Source/devtools/front_end/PlatformFontsSidebarPane.js:117: var fontStatElement =
fontStatsSection.createChild("div");
ditto here and elsewhere

https://codereview.chromium.org/22875023/diff/3001/Source/devtools/front_end/...
File Source/devtools/front_end/elementsPanel.css (right):

https://codereview.chromium.org/22875023/diff/3001/Source/devtools/front_end/...
Source/devtools/front_end/elementsPanel.css:590: margin: 0px 1ex 0px 1ex;
0px -> 0

[lushnikov, 2013-08-22 14:52:39]

Thank you for the review! Fixed everything

https://codereview.chromium.org/22875023/diff/3001/Source/devtools/front_end/...
File Source/devtools/front_end/PlatformFontsSidebarPane.js (right):

https://codereview.chromium.org/22875023/diff/3001/Source/devtools/front_end/...
Source/devtools/front_end/PlatformFontsSidebarPane.js:2: * Copyright (C) 2007
Apple Inc.  All rights reserved.
On 2013/08/22 13:05:35, aandrey wrote:
> fix license

Done.

https://codereview.chromium.org/22875023/diff/3001/Source/devtools/front_end/...
Source/devtools/front_end/PlatformFontsSidebarPane.js:37:
WebInspector.domAgent.addEventListener(WebInspector.DOMAgent.Events.AttrModified,
this._innerUpdate, this);
On 2013/08/22 13:05:35, aandrey wrote:
> This can be a performance issue. See https://codereview.chromium.org/22548006/
> where I fix slow breadcrumbs. Maybe you also want to listen to the
> ElementsTreeUpdated event instead to sync with the elements panel UI updates?
Or
> at least consider throttling these events (_innerUpdate seems pretty "heavy").

Added throttling here. Done.

https://codereview.chromium.org/22875023/diff/3001/Source/devtools/front_end/...
Source/devtools/front_end/PlatformFontsSidebarPane.js:56: this._node = node;
On 2013/08/22 13:05:35, aandrey wrote:
> The old callbacks produced by callbackBarrier may be still alive at this
point.
> You do not handle the [this._platformFontsCallback,
this._computedStyleCallback,
> this._render] being called for an old node.

Done.

https://codereview.chromium.org/22875023/diff/3001/Source/devtools/front_end/...
Source/devtools/front_end/PlatformFontsSidebarPane.js:101: }.bind(this));
On 2013/08/22 13:05:35, aandrey wrote:
> no need to bind

Done.

https://codereview.chromium.org/22875023/diff/3001/Source/devtools/front_end/...
Source/devtools/front_end/PlatformFontsSidebarPane.js:103: var cssFontSection =
this.element.createChild("div");
On 2013/08/22 13:05:35, aandrey wrote:
> this.element.createChild("div", "stats-section monospace");

Done.

https://codereview.chromium.org/22875023/diff/3001/Source/devtools/front_end/...
Source/devtools/front_end/PlatformFontsSidebarPane.js:105: var cssFontPrefix =
cssFontSection.createChild("span");
On 2013/08/22 13:05:35, aandrey wrote:
> ditto

Done.

https://codereview.chromium.org/22875023/diff/3001/Source/devtools/front_end/...
Source/devtools/front_end/PlatformFontsSidebarPane.js:108: var cssFontColumn =
cssFontSection.createChild("span");
On 2013/08/22 13:05:35, aandrey wrote:
> use createTextChild()?

Done.

https://codereview.chromium.org/22875023/diff/3001/Source/devtools/front_end/...
Source/devtools/front_end/PlatformFontsSidebarPane.js:110: var cssFontValue =
cssFontSection.createChild("span");
On 2013/08/22 13:05:35, aandrey wrote:
> cssFontSection.createChild("span", "css-font-value");

Done.

https://codereview.chromium.org/22875023/diff/3001/Source/devtools/front_end/...
Source/devtools/front_end/PlatformFontsSidebarPane.js:117: var fontStatElement =
fontStatsSection.createChild("div");
On 2013/08/22 13:05:35, aandrey wrote:
> ditto here and elsewhere

Done.

https://codereview.chromium.org/22875023/diff/3001/Source/devtools/front_end/...
File Source/devtools/front_end/elementsPanel.css (right):

https://codereview.chromium.org/22875023/diff/3001/Source/devtools/front_end/...
Source/devtools/front_end/elementsPanel.css:590: margin: 0px 1ex 0px 1ex;
On 2013/08/22 13:05:35, aandrey wrote:
> 0px -> 0

Done.

[aandrey, 2013-08-22 15:16:43]

https://codereview.chromium.org/22875023/diff/11001/Source/devtools/front_end...
File Source/devtools/front_end/PlatformFontsSidebarPane.js (right):

https://codereview.chromium.org/22875023/diff/11001/Source/devtools/front_end...
Source/devtools/front_end/PlatformFontsSidebarPane.js:92: this._platformFonts =
platformFonts;
this async callback can be called for an old node (wrong platformFonts)
bind the same way, as this._render()?

https://codereview.chromium.org/22875023/diff/11001/Source/devtools/front_end...
Source/devtools/front_end/PlatformFontsSidebarPane.js:100: this._computedStyles
= computedStyles;
ditto

https://codereview.chromium.org/22875023/diff/11001/Source/devtools/front_end...
Source/devtools/front_end/PlatformFontsSidebarPane.js:103: _render:
function(node)
jsdocs plz

[aandrey, 2013-08-22 15:22:32]

https://codereview.chromium.org/22875023/diff/11001/Source/devtools/front_end...
File Source/devtools/front_end/PlatformFontsSidebarPane.js (right):

https://codereview.chromium.org/22875023/diff/11001/Source/devtools/front_end...
Source/devtools/front_end/PlatformFontsSidebarPane.js:50:
clearTimeout(this._innerUpdateTimeout);
return;

otherwise you call setTimeout on every event - not the best throthling

https://codereview.chromium.org/22875023/diff/11001/Source/devtools/front_end...
Source/devtools/front_end/PlatformFontsSidebarPane.js:72: if (!this._node)
if (this._innerUpdateTimeout) {
  clear...
  delete this._innerUpdateTimeout;
}

[lushnikov, 2013-08-22 15:52:41]

Fixed all the glitches

https://codereview.chromium.org/22875023/diff/11001/Source/devtools/front_end...
File Source/devtools/front_end/PlatformFontsSidebarPane.js (right):

https://codereview.chromium.org/22875023/diff/11001/Source/devtools/front_end...
Source/devtools/front_end/PlatformFontsSidebarPane.js:50:
clearTimeout(this._innerUpdateTimeout);
On 2013/08/22 15:22:32, aandrey wrote:
> return;
> 
> otherwise you call setTimeout on every event - not the best throthling

Still setting timeout is much faster then rendering

https://codereview.chromium.org/22875023/diff/11001/Source/devtools/front_end...
Source/devtools/front_end/PlatformFontsSidebarPane.js:72: if (!this._node)
On 2013/08/22 15:22:32, aandrey wrote:
> if (this._innerUpdateTimeout) {
>   clear...
>   delete this._innerUpdateTimeout;
> }

Done.

https://codereview.chromium.org/22875023/diff/11001/Source/devtools/front_end...
Source/devtools/front_end/PlatformFontsSidebarPane.js:92: this._platformFonts =
platformFonts;
On 2013/08/22 15:16:43, aandrey wrote:
> this async callback can be called for an old node (wrong platformFonts)
> bind the same way, as this._render()?

Done.

https://codereview.chromium.org/22875023/diff/11001/Source/devtools/front_end...
Source/devtools/front_end/PlatformFontsSidebarPane.js:100: this._computedStyles
= computedStyles;
On 2013/08/22 15:16:43, aandrey wrote:
> ditto

Done.

https://codereview.chromium.org/22875023/diff/11001/Source/devtools/front_end...
Source/devtools/front_end/PlatformFontsSidebarPane.js:103: _render:
function(node)
On 2013/08/22 15:16:43, aandrey wrote:
> jsdocs plz

Done.

[pfeldman, 2013-08-22 16:39:51]

https://codereview.chromium.org/22875023/diff/15001/Source/devtools/front_end...
File Source/devtools/front_end/PlatformFontsSidebarPane.js (right):

https://codereview.chromium.org/22875023/diff/15001/Source/devtools/front_end...
Source/devtools/front_end/PlatformFontsSidebarPane.js:82:
WebInspector.cssModel.getPlatformFontsForNode(this._node.id,
platformFontsCallback);
Serve font family as a part of this request.

https://codereview.chromium.org/22875023/diff/15001/Source/devtools/front_end...
Source/devtools/front_end/PlatformFontsSidebarPane.js:83:
WebInspector.cssModel.getComputedStyleAsync(this._node.id,
computedStyleCallback);
This operation is extremely expensive (up to 1 second)

https://codereview.chromium.org/22875023/diff/15001/Source/devtools/front_end...
Source/devtools/front_end/PlatformFontsSidebarPane.js:130:
cssFontPrefix.textContent = "font-family";
You don't have to re-create this part.

[aandrey, 2013-08-22 18:10:15]

https://codereview.chromium.org/22875023/diff/15001/Source/devtools/front_end...
File Source/devtools/front_end/PlatformFontsSidebarPane.js (right):

https://codereview.chromium.org/22875023/diff/15001/Source/devtools/front_end...
Source/devtools/front_end/PlatformFontsSidebarPane.js:51:
this._innerUpdateTimeout = setTimeout(this._innerUpdate.bind(this), 100);
If _onNodeChanged is fired every 100ms for some period of time, you'll never
have a chance to update the UI. Is this OK?

[aandrey, 2013-08-22 18:12:45]

lgtm (not owner)

[aandrey, 2013-08-22 18:30:46]

https://codereview.chromium.org/22875023/diff/15001/Source/devtools/front_end...
File Source/devtools/front_end/PlatformFontsSidebarPane.js (right):

https://codereview.chromium.org/22875023/diff/15001/Source/devtools/front_end...
Source/devtools/front_end/PlatformFontsSidebarPane.js:83:
WebInspector.cssModel.getComputedStyleAsync(this._node.id,
computedStyleCallback);
On 2013/08/22 16:39:51, pfeldman wrote:
> This operation is extremely expensive (up to 1 second)

Yeah, you should not call it again until you get the callback for the old node,
otherwise there can be an unlimited backlog queue. You should have a flag like
this._rebuildUpdateInProgress in WebInspector.StylesSidebarPane, or
this._pendingReplayTraceLogEvent in WebInspector.CanvasProfileView. 

nit: we usually make callbacks like _platformFontsCallback and
_computedStyleCallback local functions, so that you no longer bind this._node.

[lushnikov, 2013-08-26 10:01:24]

please take a look

[pfeldman, 2013-08-26 12:10:49]

lgtm given the nits are fixed.

https://chromiumcodereview.appspot.com/22875023/diff/26001/Source/devtools/de...
File Source/devtools/devtools.gyp (right):

https://chromiumcodereview.appspot.com/22875023/diff/26001/Source/devtools/de...
Source/devtools/devtools.gyp:248: 'front_end/PlatformFontsSidebarPane.js',
P < S

https://chromiumcodereview.appspot.com/22875023/diff/26001/Source/devtools/fr...
File Source/devtools/front_end/ElementsPanel.js (right):

https://chromiumcodereview.appspot.com/22875023/diff/26001/Source/devtools/fr...
Source/devtools/front_end/ElementsPanel.js:37:
importScript("PlatformFontsSidebarPane.js");
ditto

https://chromiumcodereview.appspot.com/22875023/diff/26001/Source/devtools/fr...
File Source/devtools/front_end/PlatformFontsSidebarPane.js (right):

https://chromiumcodereview.appspot.com/22875023/diff/26001/Source/devtools/fr...
Source/devtools/front_end/PlatformFontsSidebarPane.js:66:
this.element.removeChildren();
Is this a bug?

https://chromiumcodereview.appspot.com/22875023/diff/26001/Source/devtools/fr...
Source/devtools/front_end/PlatformFontsSidebarPane.js:89: _render:
function(node, cssFamilyName, platformFonts)
_refreshUI

https://chromiumcodereview.appspot.com/22875023/diff/26001/Source/devtools/fr...
Source/devtools/front_end/PlatformFontsSidebarPane.js:101:
this._fontStatsSection.remove();
don't

https://chromiumcodereview.appspot.com/22875023/diff/26001/Source/devtools/fr...
Source/devtools/front_end/PlatformFontsSidebarPane.js:109:
fontDelimeterElement.textContent = "—";
Please use \u notation

https://chromiumcodereview.appspot.com/22875023/diff/26001/Source/devtools/sc...
File Source/devtools/scripts/compile_frontend.py (right):

https://chromiumcodereview.appspot.com/22875023/diff/26001/Source/devtools/sc...
Source/devtools/scripts/compile_frontend.py:213: "PlatformFontsSidebarPane.js"
ditto

[commit-bot: I haz the power, 2013-08-26 12:54:47]

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

[commit-bot: I haz the power, 2013-08-26 14:45:40]

Change committed as 156700