Closed Bug 886041 Opened 11 years ago Closed 9 years ago

Make the font inspector remotable

Categories

(DevTools :: Inspector, defect)

x86
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 36

People

(Reporter: dcamp, Assigned: harth)

References

Details

Attachments

(1 file, 6 obsolete files)

I don't actually know how we're going to do this - the remote side might have access to fonts we don't have, right?  So preview can't always work?
(In reply to Dave Camp (:dcamp) from comment #0)
> I don't actually know how we're going to do this - the remote side might
> have access to fonts we don't have, right?  So preview can't always work?

Remote fonts will work (because accessible from the client), but not builtin fonts.

We should build the preview client side in a canvas (or in an iframe + drawWindow), and send the image as a dataURL.
See Also: → 872078
We now have a preview that will render the fonts and send back a data URL (since Bug 987797).  And we will need to add a server method (probably on style actor) for getting the list of fonts with getUsedFontFaces.
Assignee: nobody → fayearthur
We might have to get rid of the nice feature where you can edit the preview text. It definitely can't exist in its current form where you can edit text directly, since we're switching to an image of the text. 

I think the first round of this will have to get rid of it, and we might be able to add it back in some form later (like clicking to show an input box to enter the new preview text).
(In reply to Heather Arthur  [:harth] from comment #3)
> We might have to get rid of the nice feature where you can edit the preview
> text. It definitely can't exist in its current form where you can edit text
> directly, since we're switching to an image of the text.

Wow, I didn't even realize it did that currently until you just mentioned it!

It's not very easy to discover as-is, so hopefully not too many people would be sad if it disappeared for a little while...
(In reply to J. Ryan Stinnett [:jryans] from comment #4)
> (In reply to Heather Arthur  [:harth] from comment #3)
> > We might have to get rid of the nice feature where you can edit the preview
> > text. It definitely can't exist in its current form where you can edit text
> > directly, since we're switching to an image of the text.
> 
> Wow, I didn't even realize it did that currently until you just mentioned it!
> 
> It's not very easy to discover as-is, so hopefully not too many people would
> be sad if it disappeared for a little while...

Yes, it is totally hidden in the UI, although there is a note in the docs about it: https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector#Fonts_view.  I'd be fine with removing this to make it remotable as long as the docs get updated.  If/when we add it back we can make it more obvious.
Status: NEW → ASSIGNED
Blocks: 1046174
This patch makes the font inspector remote. It also fixes up the test and enables it on e10s.

It adds a getUsedFontFaces() method to the PageStyleActor, and shares the preview-generating code with the inspector. 

It behaves the same as the old font inspector except: the edit-text feature is gone (see c3), the font-weight/style of the font is applied to the preview (helpful if there are various weights of the same font), and removes empty parentheses if there's no font format.
Attachment #8505911 - Flags: review?(bgrinstead)
Add a couple comment fixes, sorry. Also forgot to mention that this patch adds a head.js file, but most of those functions are stolen from the inspector's head.js
Attachment #8505911 - Attachment is obsolete: true
Attachment #8505911 - Flags: review?(bgrinstead)
Attachment #8505916 - Flags: review?(bgrinstead)
Comment on attachment 8505916 [details] [diff] [review]
Remote font inspector + e10s enabled

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

Looks like a good approach, I just have a couple of questions.

Also, we should pass back a white text version of the preview when the dark theme is applied.  That should be pretty easy - we are already doing this for the font family tooltip.

::: browser/devtools/fontinspector/font-inspector.js
@@ +132,5 @@
> +    let formatElem = s.querySelector(".font-format");
> +    if (font.format) {
> +      formatElem.textContent = font.format;
> +    }
> +    else {

Nit: newline before else

::: browser/devtools/fontinspector/test/head.js
@@ +1,1 @@
> + /* vim: set ts=2 et sw=2 tw=80: */

I so wish that we had a shared head.js file for all the devtools tests.  Oh well, it'd probably be tricky to do at this point since all of them probably use slightly different functions.  If there is a way to include a different head.js (whichever one this is based off of, maybe the inspector's?) that could help avoid adding extra code.

::: toolkit/devtools/server/actors/styles.js
@@ +266,5 @@
> +        localName: font.localName,
> +        metadata: font.metadata
> +      }
> +
> +      if (font.rule) {

What is the case where the font doesn't have a rule included, and what are the repercussions of that?  Please leave a comment explaining

@@ +279,5 @@
> +          fontStyle += font.rule.style.getPropertyValue("font-style") + " " +
> +                       font.rule.style.getPropertyValue("font-weight");
> +        }
> +        else {
> +          let name = font.name.toLowerCase();

I wish there was a different way to do this - seems like checking the font name won't be a guarantee.  I don't understand why we need to do this here but not for the getFontFamilyDataURL call for the font preview tooltips.  I'll probably understand the issue better after hearing the answer to my previous question
Attachment #8505916 - Flags: review?(bgrinstead)
(In reply to Brian Grinstead [:bgrins] from comment #9)
>
> we should pass back a white text version of the preview when the dark
> theme is applied.  That should be pretty easy - we are already doing this
> for the font family tooltip.

Will do.

> 
> ::: browser/devtools/fontinspector/test/head.js
> @@ +1,1 @@
> > + /* vim: set ts=2 et sw=2 tw=80: */
> 
> I so wish that we had a shared head.js file for all the devtools tests.  Oh
> well, it'd probably be tricky to do at this point since all of them probably
> use slightly different functions.  If there is a way to include a different
> head.js (whichever one this is based off of, maybe the inspector's?) that
> could help avoid adding extra code.

I think there've been discussions about this, but I don't think sharing a head.js is possible right now. You could definitely have a helper module or something though I think. I'd rather not breach it right now for this patch, as there are duplicate functions in markupview and layoutview, etc. that would need to be combined too.
 
> ::: toolkit/devtools/server/actors/styles.js
> @@ +266,5 @@
> > +        localName: font.localName,
> > +        metadata: font.metadata
> > +      }
> > +
> > +      if (font.rule) {
> 
> What is the case where the font doesn't have a rule included, and what are
> the repercussions of that?  Please leave a comment explaining

Will do. The 'rule' property is the @font-face rule associated with the font. So there's no rule if the font didn't come from a @font-face rule.

> 
> @@ +279,5 @@
> > +          fontStyle += font.rule.style.getPropertyValue("font-style") + " " +
> > +                       font.rule.style.getPropertyValue("font-weight");
> > +        }
> > +        else {
> > +          let name = font.name.toLowerCase();
> 
> I wish there was a different way to do this - seems like checking the font
> name won't be a guarantee.  I don't understand why we need to do this here
> but not for the getFontFamilyDataURL call for the font preview tooltips. 
> I'll probably understand the issue better after hearing the answer to my
> previous question

Checking the font name is definitely not ideal, but is the only way if the font doesn't come from a @font-face rule (where font-weight is specified). I think it'll catch almost all cases though.

I don't think it's a good idea to do the weight/style for the inspector tooltip. The tooltip is over the font-family, which just specifies the font names with no mention of font weight or style.
An illustration of why we need the font-weight sniffing:

http://i.imgur.com/nYwqgdm.png?1

Visually, it looks like we're showing duplicate fonts in the panel, until you read the names and realize one of them is supposed to be bold. It's even more of a problem if you have three or more of the same font, like regular, italic and bold. And even more bad if you have a @font-face font, as the "Bold" isn't in the name and is in the code, so you really have no idea why it's showing you multiples of the same font.
Rebased and updated to comments.
Attachment #8505916 - Attachment is obsolete: true
Attachment #8508970 - Flags: review?(bgrinstead)
Attachment #8508970 - Flags: review?(bgrinstead)
The difference between this patch and the last is:

1) We now sort the list of fonts primarily by whether it's from a @font-face rule (we were doing this before), secondarily by the font's name, and tertiarily by the font weight. So fonts of the same family are together.

2) To test this I added a new bold font to the test, plus its license, which was missing before. The license is the SIL Open Font License that I've seen included for fonts in reftests.
Attachment #8508970 - Attachment is obsolete: true
Attachment #8509147 - Flags: review?(bgrinstead)
Comment on attachment 8509147 [details] [diff] [review]
Remote font inspector + comments + sorting

Failing try, let me figure that out.
Attachment #8509147 - Flags: review?(bgrinstead)
Alright, figured out the try failures.
Attachment #8509147 - Attachment is obsolete: true
Attachment #8510693 - Flags: review?(bgrinstead)
Blocks: 1097150
Comment on attachment 8510693 [details] [diff] [review]
Remote font inspector + test fixes

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

::: browser/devtools/fontinspector/test/OstrichLicense.txt
@@ +1,1 @@
> +This Font Software is licensed under the SIL Open Font License, Version 1.1.

It seems like this license should be compatible, but let's double check.  Gerv, is this Ostrich font OK to use (under the Open Font License)?
Attachment #8510693 - Flags: feedback?(gerv)
Comment on attachment 8510693 [details] [diff] [review]
Remote font inspector + test fixes

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

First, sorry for the very bad delay on getting this review done.  I went through this today, and everything looks good to me except for the process of matching system weight and style from the font name (ie "Helvetica Bold" is the font face when setting font-family: Helvetica; font-weight: bold).

After discussing a bit on IRC I think that approach probably makes sense, but since it is an existing I'd prefer add (and review) this part in a separate patch.  I think that would help write a clear history of the changes to these files, and help narrow down possible regressions that are directly due to the remoting vs adding this font weight/style handling.

::: toolkit/devtools/server/actors/styles.js
@@ +91,5 @@
>    rules: "array:domstylerule",
>    sheets: "array:stylesheet"
>  });
>  
> +types.addDictType("fontpreview", {

Is there a way to reuse the "imageData" dictType from inspector.js?  If not, could you name them the same thing since they seem to be for the same thing?

@@ +259,5 @@
> +    for (let i = 0; i < fonts.length; i++) {
> +      let font = fonts.item(i);
> +      let fontFace = {
> +        name: font.name,
> +        CSSFamilyName: font.CSSFamilyName,

maybe fontFamilyName instead of CSSFamilyName?  Your choice

@@ +279,5 @@
> +      if (font.rule) {
> +        weight = font.rule.style.getPropertyValue("font-weight") || 400;
> +        if (weight == "bold") {
> +          weight = 700;
> +        }

Nit: please use consistent whitespace usage with the rest of the file (else on same line as closing bracket).
Attachment #8510693 - Flags: review?(bgrinstead)
Turning feedback from Comment 16 into a needinfo
Flags: needinfo?(gerv)
Attachment #8510693 - Flags: feedback?(gerv)
(In reply to Brian Grinstead [:bgrins] from comment #16)
> It seems like this license should be compatible, but let's double check. 
> Gerv, is this Ostrich font OK to use (under the Open Font License)?

Thanks for asking :-) SIL OFL 1.1 is generally fine. Is this font to ship with Firefox or any other product? If so, we have documentation requirements. If not, nothing to do.

Gerv
Flags: needinfo?(gerv)
> Thanks for asking :-) SIL OFL 1.1 is generally fine. Is this font to ship
> with Firefox or any other product? If so, we have documentation
> requirements. If not, nothing to do.

It's only used inside the test file so I'm assuming that no, it won't ship with Firefox.
Here it is without any of the font weight sniffing, just remoting.
Attachment #8510693 - Attachment is obsolete: true
Attachment #8528223 - Flags: review?(bgrinstead)
Comment on attachment 8528223 [details] [diff] [review]
Remote font inspector without weight sniffing

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

Looks good - pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=3fdd69cb8902.

FYI this patch needed rebasing on inspector-panel.js
Attachment #8528223 - Flags: review?(bgrinstead) → review+
Rebased patch from the try run above, ready for check in.
Attachment #8528223 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/2273193cc525
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 36
Depends on: 1105808
Depends on: 1158634
No longer depends on: 1158634
See Also: → 1158634
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.