738228 - Option to display used font faces

Status: RESOLVED FIXED

(Other Applications :: DOM Inspector, enhancement)

Description

[[e-mail]]

inIDOMUtils has a method to extract the fonts used on a particular range. This could be displayed inside DOM Inspector, although we would have to limit it to a node rather than a range.

Comment 1

[[e-mail]]

Oh, I forgot to mention that the method is new in Gecko 7.

Comment 2

[[e-mail]]

Attachment: Proof of concept

This was mostly copy and paste, so I've probably made some howlers...

Comment 3

[Colby Russell :crussell (no longer Mozilla-ing)]

Does this need a whole new viewer?  Seems like it would be nice if we could shoehorn this into the Computed Style viewer or the CSS Rules viewer.  It wouldn't be very pure, but a lot better for usability I think.

Those viewers map directly onto the underlying object models, which is an ideal that should be maintained, so we don't want to misrepresent what's happening.

What about a child row under the font-family row in the Computed Style viewer, or something similar in CSS Rules?  (Colored red, like anonymous nodes?)  Any other ideas?

Comment 4

[[e-mail]]

(In reply to Colby Russell from comment #3)
> Does this need a whole new viewer?  Seems like it would be nice if we could
> shoehorn this into the Computed Style viewer or the CSS Rules viewer.  It
> wouldn't be very pure, but a lot better for usability I think.
I did consider the Computed Style viewer but the idea to use a separate viewer was because it works on text nodes and documents as well as elements.

> What about a child row under the font-family row in the Computed Style
> viewer, or something similar in CSS Rules?  (Colored red, like anonymous
> nodes?)  Any other ideas?
Well, the used font doesn't necessarily relate to the computed font because child elements might change it... I would be happy to move my current tree into a second pane in Computed Style, styled similarly to CSS Rules' panes.

Comment 5

[[e-mail]]

Attachment: Addressed review comments

Comment 6

[Colby Russell :crussell (no longer Mozilla-ing)]

In that case, I guess I prefer the separate viewer approach.

Comment 7

[Colby Russell :crussell (no longer Mozilla-ing)]

Comment on attachment 608451 [details] [diff] [review]
Addressed review comments

See comment 6.

Comment 8

[[e-mail]]

Attachment: Proposed patch

As per attachment 608313 [details] [diff] [review] but including l10n.

Comment 9

[Colby Russell :crussell (no longer Mozilla-ing)]

Comment on attachment 623492 [details] [diff] [review]
Proposed patch

> +      ins:uid="usedFontFaces"
> +      ins:panels="bxObjectPanel bxObjPanel"
> +      ins:description="&usedFontFaces.label;">
> +        <ins:filter><![CDATA[
> +          return object instanceof Components.interfaces.nsIDOMNode &&
...
> +function UsedFontFacesView(aObject)
> +{
> +  if (aObject instanceof Components.interfaces.nsIDOMDocumentType) {
> +    this.mFontList = [];
> +    this.mRowCount = 0;

Shouldn't this check just be in the filter, along with some others, like comment nodes and processing instructions?

> diff --git a/resources/content/viewers/usedFontFaces/usedFontFaces.js b/resources/content/viewers/usedFontFaces/usedFontFaces.js
> new file mode 100644
> --- /dev/null
> +++ b/resources/content/viewers/usedFontFaces/usedFontFaces.js

I see you did a git style diff.  Did you do this because you're basing this off styleRules.js, or was there another reason?  If it's the former, you have to do |hg copy| in conjunction with the git diff to get it to pick up on the fact that this is a copy.  Use |--after| to register it after-the-fact.

> @@ -0,0 +1,182 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/*****************************************************************************
> +* UsedFontFacesViewer --------------------------------------------------------
> +*  The viewer for the computed CSS styles on a DOM element.

Are you sure?

> +  this.mDOMUtils = XPCU.getService("@mozilla.org/inspector/dom-utils;1", "inIDOMUtils");

This is longer than 80 columns.  There are a few other places where this happens.

> +
> +  this.mTree = document.getElementById("olFonts");
> +}
> +
> +//XXX Don't use anonymous functions

:)

> +  initialize: function CSVr_Initialize(aPane)

You probably meant to change the function names.

> +  getCommand: function CSVr_GetCommand(aCommand)
> +  {
> +    if (aCommand == "cmdEditCopy") {
> +      return new cmdEditCopy(this.mTreeView.getSelectedRowObjects());

You probably want to copy the font name, or something, instead of the object.  This copies the string "[xpconnect wrapped nsIDOMFontFace]".

> +    }
> +    return null;

No support for copying the URI (cmdEditCopyFileURI)?

> +function UsedFontFacesView(aObject)
> +{
> +  if (aObject instanceof Components.interfaces.nsIDOMDocumentType) {
> +    this.mFontList = [];
> +    this.mRowCount = 0;
> +  } else {

else on a new line

> +UsedFontFacesView.prototype.getCellText = function UFFV_GetCellText(aRow, aCol)
> +{
...
> +}
> +
...
> +UsedFontFacesView.prototype.getRowObjectFromIndex =
> +  function UFFV_GetRowObjectFromIndex(aIndex)
> +{
> +  return this.mFontList.item(aIndex);
> +}

Nit: Please put semi-colons at the end of these assignments.  I know styleRules.js doesn't have them, but this has already been taken care of in several of the other viewers.

Comment 10

[Colby Russell :crussell (no longer Mozilla-ing)]

I also got some errors in the console when trying this patch, and the viewer died once.  It might have had something to do with exotic nodes (comments, processing instructions), since that's what I was testing at the time.

Comment 11

[[e-mail]]

(In reply to Colby Russell from comment #9)
> (From update of attachment 623492 [details] [diff] [review])
> > +      ins:uid="usedFontFaces"
> > +      ins:panels="bxObjectPanel bxObjPanel"
> > +      ins:description="&usedFontFaces.label;">
> > +        <ins:filter><![CDATA[
> > +          return object instanceof Components.interfaces.nsIDOMNode &&
> ...
> > +function UsedFontFacesView(aObject)
> > +{
> > +  if (aObject instanceof Components.interfaces.nsIDOMDocumentType) {
> > +    this.mFontList = [];
> > +    this.mRowCount = 0;
> Shouldn't this check just be in the filter
I could do but I didn't want the page to flip just because you'd click on a document type.

> along with some others, like comment nodes and processing instructions?
The API actually uses a range. For some reason, you can't select the contents of a document type with a range, but you can select comment nodes and PIs.

> I see you did a git style diff.  Did you do this because you're basing this
> off styleRules.js, or was there another reason?
It's the default for mozillabuild. It's also useful when dealing with images.

> If it's the former, you
> have to do |hg copy| in conjunction with the git diff to get it to pick up
> on the fact that this is a copy.  Use |--after| to register it
> after-the-fact.
It was actually based off computedStyle.js (the comment below should have been a giveaway!) but the copy diff would be bigger than an add so unless you really want it I won't bother.

> > +* UsedFontFacesViewer --------------------------------------------------------
> > +*  The viewer for the computed CSS styles on a DOM element.
> Are you sure?
Oops ;-)

> > +  this.mDOMUtils = XPCU.getService("@mozilla.org/inspector/dom-utils;1", "inIDOMUtils");
> This is longer than 80 columns.  There are a few other places where this
> happens.
Yes, that was copied from computedStyle.js too...

> > +//XXX Don't use anonymous functions
> :)
Also copied...

> > +  initialize: function CSVr_Initialize(aPane)
> You probably meant to change the function names.
Oops... I did in some places, but I missed these.

> > +  getCommand: function CSVr_GetCommand(aCommand)
> > +  {
> > +    if (aCommand == "cmdEditCopy") {
> > +      return new cmdEditCopy(this.mTreeView.getSelectedRowObjects());
> You probably want to copy the font name, or something, instead of the
> object.  This copies the string "[xpconnect wrapped nsIDOMFontFace]".
> > +    }
> > +    return null;
> No support for copying the URI (cmdEditCopyFileURI)?
Again, cargo-culted from computedStyles.js; I will investigate.

Comment 12

[[e-mail]]

Attachment: Revised patch

Note that not all points were addressed, so to speak.

Comment 13

[Colby Russell :crussell (no longer Mozilla-ing)]

Comment on attachment 624879 [details] [diff] [review]
Revised patch

Review of attachment 624879 [details] [diff] [review]:
-----------------------------------------------------------------

::: resources/content/viewers/usedFontFaces/usedFontFaces.js
@@ +199,5 @@
> +  return this.mFontList.item(aIndex);
> +};
> +
> +/**
> + * Copy the URI for a downloaded font onto the clipboard.

s/URI for/name of/

(In reply to [e-mail] from comment #11)
> (In reply to Colby Russell from comment #9)
> > (From update of attachment 623492 [details] [diff] [review])
> > > +      ins:uid="usedFontFaces"
> > > +      ins:panels="bxObjectPanel bxObjPanel"
> > > +      ins:description="&usedFontFaces.label;">
> > > +        <ins:filter><![CDATA[
> > > +          return object instanceof Components.interfaces.nsIDOMNode &&
> > ...
> > > +function UsedFontFacesView(aObject)
> > > +{
> > > +  if (aObject instanceof Components.interfaces.nsIDOMDocumentType) {
> > > +    this.mFontList = [];
> > > +    this.mRowCount = 0;
> > Shouldn't this check just be in the filter
> I could do but I didn't want the page to flip just because you'd click on a
> document type.

That's bug 166749, but oh well.

Comment 14

[Colby Russell :crussell (no longer Mozilla-ing)]

(Colby Russell :crussell wrote in comment #13)
> That's bug 166749, but oh well.

Actually, could you put an XXX comment mentioning bug 166749 in the filter, and a comment in the UsedFontFaceView constructor explaining the doctype/range special case?

Comment 15

[[e-mail]]

Pushed changeset ddaa74ce685c to dom-inspector.