Bas Schouten (:bas.schouten) Assignee
	Description • 15 years ago

Direct2D is the 'new way' to go for rendering on updated Windows Vista and Windows 7 platforms. It's worthwhile investigating the possibility of creating a Cairo backend for it.

	Bas Schouten (:bas.schouten) Assignee
	Comment 1 • 15 years ago

Attaching diffs for making this work.

	Bas Schouten (:bas.schouten) Assignee
	Comment 2 • 15 years ago

These changes go into the tree. Note some things in here are somewhat hacky. I assume win32 surfaces work and gfxWindowsSurface still generates those when asking for something else than an hwnd surface. This means only uxtheme still gets to use Win32 surfaces. Also USE_WIN_SURFACE in libpr0n is disabled. Which might slightly affect GDI performance.

	Bas Schouten (:bas.schouten) Assignee
	Comment 3 • 15 years ago

This time patch with all the files in it.

	Bas Schouten (:bas.schouten) Assignee
	Comment 4 • 15 years ago

And also complete set of mozilla tree changes

	Bas Schouten (:bas.schouten) Assignee
	Comment 6 • 15 years ago

I've added the current DWrite and D2D backends, would be good to get some eyes on that already.

Note the implementation is not cairo-correct. It doesn't implement all possible cairo-operators. It does function correctly for practically all Firefox uses (that I've seen so far) though.

	Sylvain Pasche
	Comment 7 • 15 years ago

I tried the d2d build from http://www.bassified.nl/firefox-3.7a1pre.en-US.win32.d2d.zip and noticed some issues when the dpi setting is not the default (of 96).

The screenshot compares the d2d build on the left and a "normal" build on the right. My system is configured to 106 dpi (110% font size).
It looks like Windows tries to zoom the interface itself (as it would when the application is not flagged as DPI aware). Note that the clickable regions are not scaled (the hit area for a button doesn't map to the drawn region of that button for instance).

I'm wondering if using a manifest instead of calling SetProcessDPIAware() would help here. From http://msdn.microsoft.com/en-us/library/ms633543%28VS.85%29.aspx:
"Note - SetProcessDPIAware is subject to a possible race condition if a DLL caches dpi settings during initialization. For this reason, it is recommended that dpi-aware be set through the application (.exe) manifest rather than by calling SetProcessDPIAware."

	Bas Schouten (:bas.schouten) Assignee
	Comment 8 • 15 years ago

(In reply to comment #7)
> Created an attachment (id=414333) [details]
> Screenshot of a d2d build with 106 dpi
> I tried the d2d build from
> http://www.bassified.nl/firefox-3.7a1pre.en-US.win32.d2d.zip and noticed some
> issues when the dpi setting is not the default (of 96).
> The screenshot compares the d2d build on the left and a "normal" build on the
> right. My system is configured to 106 dpi (110% font size).
> It looks like Windows tries to zoom the interface itself (as it would when the
> application is not flagged as DPI aware). Note that the clickable regions are
> not scaled (the hit area for a button doesn't map to the drawn region of that
> button for instance).
> I'm wondering if using a manifest instead of calling SetProcessDPIAware() would
> help here. From
> http://msdn.microsoft.com/en-us/library/ms633543%28VS.85%29.aspx:
> "Note - SetProcessDPIAware is subject to a possible race condition if a DLL
> caches dpi settings during initialization. For this reason, it is recommended
> that dpi-aware be set through the application (.exe) manifest rather than by
> calling SetProcessDPIAware."

I suspect part of this issue is something with uxtheme and how I blend it. Another part is probably simply the fact that I'm no obeying D2D DPI considerations everywhere yet. I'll ook into this, thanks forthe screenshot. It could be that the theme library atleast needs to do something wit SetProcessDPIAware, it could also very well be a blending issue of the theme.

	Bas Schouten (:bas.schouten) Assignee
	Comment 9 • 15 years ago

(In reply to comment #7)
> Created an attachment (id=414333) [details]
> Screenshot of a d2d build with 106 dpi
> 
> I tried the d2d build from
> http://www.bassified.nl/firefox-3.7a1pre.en-US.win32.d2d.zip and noticed some
> issues when the dpi setting is not the default (of 96).
> 
> The screenshot compares the d2d build on the left and a "normal" build on the
> right. My system is configured to 106 dpi (110% font size).
> It looks like Windows tries to zoom the interface itself (as it would when the
> application is not flagged as DPI aware). Note that the clickable regions are
> not scaled (the hit area for a button doesn't map to the drawn region of that
> button for instance).
> 
> I'm wondering if using a manifest instead of calling SetProcessDPIAware() would
> help here. From
> http://msdn.microsoft.com/en-us/library/ms633543%28VS.85%29.aspx:
> "Note - SetProcessDPIAware is subject to a possible race condition if a DLL
> caches dpi settings during initialization. For this reason, it is recommended
> that dpi-aware be set through the application (.exe) manifest rather than by
> calling SetProcessDPIAware."
I believe I've fixed your issue (well, I've adjusted the build to give the same result as GDI, this is different from other browsers though, which actually render sites bigger). Please let me know if it worked! I uploaded the package to the same URL as before.

	Sylvain Pasche
	Comment 10 • 15 years ago

Yes, it works. Well done :-)

	Jeff Muizelaar [:jrmuizel]
	Comment 11 • 15 years ago

Here's the start of some comments:

1. A couple of functions have inconsistent casting like the following:

+    if (((cairo_scaled_font_t*)scaled_font)->backend->type ==
+	CAIRO_FONT_TYPE_DW) {
+	    return CAIRO_INT_STATUS_UNSUPPORTED;
+    }
+    cairo_dw_scaled_font_t *dwsf = static_cast<cairo_dw_scaled_font_t*>(scaled_font);

2. compute_mask_alpha and invert_mask_alpha in cairo-dw-font.c look like copies from cairo-win32-font.c. Note: this code seems like it has bugs too (see bug 363861)

3. GeometryRecorder could use a comment saying that it's for getting glyph_paths and the class definition could be moved closer to the only user.

4. cairo_d2d_surface_create_with_size() should just be called cairo_d2d_surface_create()

5. _cairo_d2d_show_glyphs can return an uninitialized status

6. All of the (cairo_int_status_t)CAIRO_STATUS_SUCCESS are pretty painful to look at. Perhaps we could add a CAIRO_STATUS_INT_SUCCESS macro to the backend.

7. A bunch of the backend functions do:
    if (((cairo_surface_t*)surface)->type != CAIRO_SURFACE_TYPE_D2D) {
	return (cairo_status_t)CAIRO_INT_STATUS_UNSUPPORTED;
    }
  which shouldn't be needed.

	Jeff Muizelaar [:jrmuizel]
	Comment 12 • 15 years ago

More comments:

8. cairo_dw_show_glyphs_on_d2d_surface() should probably be in cairo-d2d-surface.c unless having it near to the other dw_show_glyphs is of value.

9. What's the performance difference between _dw_draw_glyphs_to_gdi_surface_d2d
and dw_draw_glyphs_to_gdi_surface_gdi. Under what conditions do we expect _dw_draw_glyphs_to_gdi_surface_d2d fail but _dw_draw_glyphs_to_gdi_surface_gdi to succeed?

	Jeff Muizelaar [:jrmuizel]
	Comment 13 • 15 years ago

10. instead of using _cairo_fixed_to_double() in _d2d_point_from_cairo_point we should add a _cairo_fixed_to_float() method and use that.

11. The line wrapping seems a little weird in _d2d_snapshot_detached(), cairo doesn't have a hard limit on line length and unwrapping the call to cairo_surface_get_user_data() should make things more readable.

12. It might also make sense to change the type of existingBitmap to cached_bitmap * and cast the return value of cairo_surface_get_user_data().

In fact how about:

    cached_bitmap *existingBitmap = (cached_bitmap*)cairo_surface_get_user_data (
                                                             surface,
                                                             &bitmap_key);
    
    if (existingBitmap) {
	    exisitingBitmap->dirty = true;
    }

13. Related to the above, the cairo style when wrapping function calls is:

a_function_with_a_really_really_long_name(param1,
                                          param2,
                                          param3);

	Jeff Muizelaar [:jrmuizel]
	Comment 14 • 15 years ago

14. RadialGradientBrushProperties() could use _cairo_fixed_to_float too

15. No need to wrap:
if (surfacePatter->surface->type == CAIRO_SURFACE_TYPE_D2D)
and similar.

16.

+	    if (surfacePattern->surface->type ==
+		CAIRO_SURFACE_TYPE_IMAGE) {
+		cairo_matrix_t translate;
+		memset(&translate, 0, sizeof(cairo_matrix_t));
+		cairo_matrix_t origmat = mat;
+		translate.xx = 1.0;
+		translate.yy = 1.0;
+		translate.x0 = -1.0;
+		translate.y0 = -1.0;
+		cairo_matrix_multiply(&mat, &translate, &mat);
+	    }

looks like it could just be cairo_matrix_translate (&mat, -1, -1);

17.

+		    /**
+		     * Trick here, we create a temporary rectangular
+		     * surface with 1 pixel margin on each side. This
+		     * provides a rectangular transparent border, that
+		     * will ensure CLAMP acts as EXTEND_NONE.
+		     */
+		    unsigned int tmpWidth = srcSurf->width + 2;
+		    unsigned int tmpHeight = srcSurf->height + 2;
+		    unsigned char *tmp = new unsigned char[tmpWidth * tmpHeight * 4];
+		    memset(tmp, 0, tmpWidth * tmpHeight * 4);
+		    for (int y = 0; y < srcSurf->height; y++) {
+			memcpy(
+			    tmp + tmpWidth * 4 * y + tmpWidth * 4 + 4, 
+			    srcSurf->data + y * srcSurf->stride, 
+			    srcSurf->width * 4);
+		    }
+

I think it might be worth moving this to a utility function. We could optimize this by avoiding memsetting the whole array, but I don't care that much about the performance of EXTEND_NONE so I comment about the potential optimization is probably enough.


18.

+	 * Nice axist aligned rectangular clip, try and do our best to keep it
+	 * that way.

'axist' has a typo

	Jeff Muizelaar [:jrmuizel]
	Comment 15 • 15 years ago

I'm wondering, how often do we hit _cairo_d2d_acquire_dest_image and _cairo_d2d_acquire_source_image during normal browsing and under what circumstances do we hit it?

	Jeff Muizelaar [:jrmuizel]
	Comment 16 • 15 years ago

Also, how do these backends do on our reftests?

	Jeff Muizelaar [:jrmuizel]
	Comment 17 • 15 years ago

If you click on the spinning box on this page (http://www.zachstronaut.com/posts/2009/08/07/jquery-animate-css-rotate-scale.html) a couple times. The text seems to switch between large and small once per rotation.

	Jeff Muizelaar [:jrmuizel]
	Comment 18 • 15 years ago

19. I think we should be able to use IDWriteGlyphRunAnalysis in _cairo_dw_scaled_show_glyphs instead of trying to manually extract the mask.

	Bas Schouten (:bas.schouten) Assignee
	Comment 19 • 15 years ago

So, first of all, thanks for the really quick review:
Here's a quick reply to all your points, some still have questions, once those are answered I'll upload a new patch.
 
1. Done
2. No longer needed, see point 19.
3. Done
4. Done
5. Done
6. Done
7. Done

8. It just seemed to make sense there, to have all that code in one centralised
place, so that if anyone fixes a bug there, they won't forget to fix it in the other
show_glyphs.

9. Depends wildly on the machine. Due to some comments on the DWrite bug I've now
switched to always using GDI for the GDI surfaces, and only trying D2D on GDI
when there's a special #define set.

10. Done
11. Done
12. Done

13. Eww... that means you get really long lines when having long arguments.. oh well. Want me to change -all- of those?

14. Done
15. Done
16. Done

17. Comment done.. you really think this will be more readable when put in a seperate
function? It wouldn't be used anywhere else...

18. Done
19. Done.

	Jeff Muizelaar [:jrmuizel]
	Comment 20 • 15 years ago

(In reply to comment #19)
> 
> 8. It just seemed to make sense there, to have all that code in one centralised
> place, so that if anyone fixes a bug there, they won't forget to fix it in the
> other
> show_glyphs.

Fair enough.

> 13. Eww... that means you get really long lines when having long arguments.. oh
> well. Want me to change -all- of those?

It would be nice :) I find the style where arguments only wrap to an additional indent level harder to read. Most of the time it shouldn't make the lines that much longer.

> 17. Comment done.. you really think this will be more readable when put in a
> seperate
> function? It wouldn't be used anywhere else...

Maybe, I don't feel that strongly about it though.

	Bas Schouten (:bas.schouten) Assignee
	Comment 21 • 15 years ago

Patch with jrmuizel's review comments processed.

	Gabriele Best [:Gabri]
	Comment 22 • 15 years ago

Can you upload a tryserver build?
Thanks!

	Bas Schouten (:bas.schouten) Assignee
	Comment 23 • 15 years ago

(In reply to comment #22)
> Can you upload a tryserver build?
> Thanks!

I cannot, since the try servers do not have the DirectX SDK installed yet. This is work in progress :). I did make a build myself though, which can be gotten from my blog (http://www.basschouten.com/)

	Jeff Muizelaar [:jrmuizel]
	Comment 24 • 15 years ago

This hunk could use a comment explaining what's going on. i.e. the format conversion taking place. Aligning the equals symbols will also make it easier to read.

+	for (unsigned int y = 0; y < height; y++) {
+	    for (unsigned int x = 0; x < width; x++) {
+		mask_surface->data[y * mask_surface->stride + x * 4] = surface[y * width * 3 + x * 3 + 2];
+		mask_surface->data[y * mask_surface->stride + x * 4 + 1] = surface[y * width * 3 + x * 3 + 1];
+		mask_surface->data[y * mask_surface->stride + x * 4 + 2] = surface[y * width * 3 + x * 3];
+		mask_surface->data[y * mask_surface->stride + x * 4 + 3] = surface[y * width * 3 + x * 3 + 2];
+	    }
+	}

	Jeff Muizelaar [:jrmuizel]
	Comment 25 • 15 years ago

1. It doesn't look we respect the font antialiasing options that have been set.

	Jeff Muizelaar [:jrmuizel]
	Comment 26 • 15 years ago

(In reply to comment #19)
> 13. Eww... that means you get really long lines when having long arguments.. oh
> well. Want me to change -all- of those?

You got close but I see a bunch of lines that are indented like:

long_function_name(param_a,
                  param_b,
                  param_c);

instead of:

long_function_name(param_a,
                   param_b,
                   param_c);

as it should be.

	Jeff Muizelaar [:jrmuizel]
	Comment 27 • 15 years ago

2. 
+/**
+ * Get D2D point from cairo point
+ *
+ * \param point Cairo point
+ * \return D2D point
+ */
+D2D1_POINT_2F
+_d2d_point_from_cairo_point(const cairo_point_t *point)
+{
+    return D2D1::Point2F(_cairo_fixed_to_float(point->x),
+			 _cairo_fixed_to_float(point->y));
+}
+
+/**
+ * Get D2D color from cairo color
+ *
+ * \param color Cairo color
+ * \return D2D color
+ */
+D2D1_COLOR_F
+_cairo_d2d_color_from_cairo_color(const cairo_color_t &color)
+{
+    return D2D1::ColorF((FLOAT)color.red, 
+			(FLOAT)color.green, 
+			(FLOAT)color.blue,
+			(FLOAT)color.alpha);
+}

The comments for these functions don't really add any value. I'd suggest removing them.

3.
+/**
+ * Via a little trick this is just used to determine when a surface has been
+ * modified.
+ */
+void _d2d_snapshot_detached(cairo_surface_t *surface)
+{
+    cached_bitmap *existingBitmap = (cached_bitmap*)cairo_surface_get_user_data(surface, &bitmap_key);
+    if (existingBitmap) {
+	    cached_bitmap *cachebitmap = (cached_bitmap*)existingBitmap;
+	    cachebitmap->dirty = true;
+    }
+}

No need for cached_bitmap to exist like I suggested previously.

4. _d2d_release_bitmap is probably a better name than _d2d_clear_bitmap. There are also three different 'bitmap's in that function, if we can come up with different names for them that would be better.

5. _cairo_d2d_create_brush_for_pattern feels really dense to me. It would be nice if we could add some line breaks to break it into more paragraphy pieces so it's easier to read. Here are some suggestions:

after this hunk:

+		    memset(tmp, 0, tmpWidth * tmpHeight * Bpp);
+		    for (int y = 0; y < srcSurf->height; y++) {
+			memcpy(
+			    tmp + tmpWidth * Bpp * y + tmpWidth * Bpp + Bpp, 
+			    srcSurf->data + y * srcSurf->stride, 
+			    srcSurf->width * Bpp);
+		    }

after this hunk:

+	    DXGI_FORMAT format;
+	    unsigned int Bpp;
+	    if (srcSurf->format == CAIRO_FORMAT_ARGB32) {
+		format = DXGI_FORMAT_B8G8R8A8_UNORM;
+		Bpp = 4;
+	    } else if (srcSurf->format == CAIRO_FORMAT_RGB24) {
+		format = DXGI_FORMAT_B8G8R8A8_UNORM;
+		Bpp = 4;
+	    } else if (srcSurf->format == CAIRO_FORMAT_A8) {
+		format = DXGI_FORMAT_A8_UNORM;
+		Bpp = 1;
+	    } else {
+		return NULL;
+	    }

etc.

This hunk has weird wrapping:
+	    cached_bitmap *cachebitmap = 
+		(cached_bitmap*)cairo_surface_get_user_data(
+		surfacePattern->surface,
+		&bitmap_key);

This line has an extra space:

+	    D2D1_MATRIX_3X2_F matrix =  _cairo_d2d_matrix_from_matrix(&mat);

	Jeff Muizelaar [:jrmuizel]
	Comment 28 • 15 years ago

6. cairo_dwrite_show_glyphs_on_d2d_surface is still public.

7. What happens when you pass a NULL brush to the D2D functions? We do this in a bunch of places, when we should perhaps be returning CAIRO_STATUS_INT_UNSUPPORTED.

8. Why does _cairo_d2d_present_backbuffer take a void * instead of a cairo_surface_t *?

9. I'd suggest putting line breaks after each section that compute a stroke property in _cairo_d2d_create_strokestyle_for_stroke_style.

10. The error handling in _cairo_d2d_create_similar might be better in the goto style like _cairo_win32_scaled_font_type1_text_to_glyphs(). However, if you do go this way I'd suggest more descriptive names than FAIL[0-3].

11. _cairo_d2d_acquire_dest_image doesn't seem to deal with surfaces of types other than CAIRO_CONTENT_COLOR_ALPHA. I believe it should.

12. This hunk should use MIN() instead of open-coding it.

+	    /** We have a clip rect, intersect of two rects is simple */
+	    D2D1_RECT_F newRect;
+	    newRect.top = d2dsurf->clipRect->top < _cairo_fixed_to_float(box.p1.y) ?
+		_cairo_fixed_to_float(box.p1.y) : d2dsurf->clipRect->top;
+	    newRect.left = d2dsurf->clipRect->left < _cairo_fixed_to_float(box.p1.x) ?
+		_cairo_fixed_to_float(box.p1.x) : d2dsurf->clipRect->left;
+	    newRect.bottom = d2dsurf->clipRect->bottom < _cairo_fixed_to_float(box.p2.y) ?
+		d2dsurf->clipRect->bottom : _cairo_fixed_to_float(box.p2.y);
+	    newRect.right = d2dsurf->clipRect->right < _cairo_fixed_to_float(box.p2.x) ?
+		d2dsurf->clipRect->right : _cairo_fixed_to_float(box.p2.x);
+	    *d2dsurf->clipRect = newRect;
+	    return CAIRO_INT_STATUS_SUCCESS;

We should be able to do without newRect too and just assign directly to clipRect too right?

13. Can _cairo_dwrite_scaled_font_init_glyph_metrics fail? If not it should return void instead of cairo_status_t.

	(not currently active) Ted Mielczarek
	Comment 29 • 15 years ago

(In reply to comment #23)
> I cannot, since the try servers do not have the DirectX SDK installed yet. This
> is work in progress :). I did make a build myself though, which can be gotten
> from my blog (http://www.basschouten.com/)

You should file a bug in mozilla.org : Release Engineering on this, and make it blocking this bug. If the Try Servers don't have this SDK, then I will bet that the build machines don't have it either, so you won't be able to land this without that happening.

	Bas Schouten (:bas.schouten) Assignee
	Comment 30 • 15 years ago

Re: Last review stuff.

2. I agree, but since they were helper functions and not implementations of existing, documented functions it seemed to make sense for them to have documentation. To have them searchable and stuff if you'd build documentations from the comment.
3. Doh, I changed the type of existing bitmap, but not the rest, my bad!
7. I can only see me forgetting to null check the brush in Paint (which is a bug obviously :)), did I miss anything? D2D will definitely crash in !debug mode.
11. Good catch, surprised that didn't give any problems.
12. Erm.. yeah, what I did is silly. We should do that.
13. It seemed consistent with how the Win32 backend did it. It probably -should- be able to fail and properly error check.

	Jeff Muizelaar [:jrmuizel]
	Comment 31 • 15 years ago

(In reply to comment #30)
> Re: Last review stuff.
> 
> 2. I agree, but since they were helper functions and not implementations of
> existing, documented functions it seemed to make sense for them to have
> documentation. To have them searchable and stuff if you'd build documentations
> from the comment.

Does anyone build this documentation? I've never seen it and I don't really know who would use it.

> 3. Doh, I changed the type of existing bitmap, but not the rest, my bad!
> 7. I can only see me forgetting to null check the brush in Paint (which is a
> bug obviously :)), did I miss anything? D2D will definitely crash in !debug
> mode.

cairo_dwrite_show_glyphs_on_d2d_surface also doesn't check.

	Bas Schouten (:bas.schouten) Assignee
	Comment 32 • 15 years ago

A new patch with jrmuizel's comments processed (hopefully correctly this time!) Also some changes in line with some changes made based on the review comments(i.e. create_similar actually supports its content types), and some bug fixes.

The FAIL labels should probably be used in the other two create surface functions as well, but I'm still thinking on how to do that exactly. Done for create_similar though.

	u88484
	Comment 33 • 15 years ago

Bas, very snappy builds and I haven't found really any problems except one.  When searching on google, the right third or so of the page content flashes grey.

STR:
1) GO to http://www.google.com/search?hl=en&q=adblock&btnG=Search&aq=f&oq=&aqi=
2) Hit the search button or refresh the page

	Bas Schouten (:bas.schouten) Assignee
	Comment 34 • 15 years ago

Latest version of my D2D backend code. Featuring horrible loops to facilitate tiling on large surfaces and other such fun! Also lazily creates a lot more for performance reasons, disabled V-sync, and some other general fixes.

	Jeff Muizelaar [:jrmuizel]
	Comment 35 • 15 years ago

1. Why do you use D3D10_CREATE_DEVICE_PREVENT_INTERNAL_THREADING_OPTIMIZATIONS?
2. All of the internal methods should be static.

	Jeff Muizelaar [:jrmuizel]
	Comment 36 • 15 years ago

3. I think we need a better name for bufferTexture. I don't think buffer is the best name for what it does. Also _cairo_d2d_buffer_texture() is a little confusing because it reads like we are buffering a texture instead of getting the buffer texture.

4. I don't really understand this comment:
 * Ensure that the surface has an up to date surface bitmap. Used for
 * window surfaces which cannot have a surface bitmap directle related
 * to their backbuffer for some reason.
What does it mean for window surfaces to have a surface bitmap directly relate to their backbuffer?

Some nits: directle is typo'ed and "up to date" can be hyphenated.

5. Why do we use DXGI_FORMAT_UNKNOWN in _cairo_d2d_update_surface_bitmap()? Does the format not matter?

6. cairo_d2d_scroll takes a "void *" surface when it should take a cairo_surface_t*.

7. _cairo_d2d_present_backbuffer shouldn't have a hyphen if it's a public function and both _cairo_d2d_present_backbuffer and cairo_d2d_scroll should both do surface type testing like cairo_xlib_surface_get_drawable()

	Bas Schouten (:bas.schouten) Assignee
	Comment 37 • 15 years ago

1. A CG researcher e-mailed me after looking at the patches that Direct2D uses that internally when you let D2D create the device so that I should try it. It gave a small performance boost on my machine, so I decided to keep it :). Doesn't mean I shouldn't look into why it helps though :)

4. You cannot create a bitmap around a backbuffer surface for reasons I do not completely understand (it will fail with an E_INVALIDARG). Meaning they need a special texture to store their graphical data which is wrapped by a D2D bitmap if a window surface is ever used in a surface pattern. All other D2D surfaces use a texture as their backing store so can have a bitmap directly.

5. It will then automatically use the format of the texture, preventing us having to do a conditional there.

Other than that - gotya.

	Jeff Muizelaar [:jrmuizel]
	Comment 38 • 15 years ago

(In reply to comment #37)
> 1. A CG researcher e-mailed me after looking at the patches that Direct2D uses
> that internally when you let D2D create the device so that I should try it. It
> gave a small performance boost on my machine, so I decided to keep it :).
> Doesn't mean I shouldn't look into why it helps though :)

The msdn documentation for D3D10_CREATE_DEVICE_PREVENT_INTERNAL_THREADING_OPTIMIZATIONS suggests against using it, so we should put as much info into a comment as we know.

> 4. You cannot create a bitmap around a backbuffer surface for reasons I do not
> completely understand (it will fail with an E_INVALIDARG). Meaning they need a
> special texture to store their graphical data which is wrapped by a D2D bitmap
> if a window surface is ever used in a surface pattern. All other D2D surfaces
> use a texture as their backing store so can have a bitmap directly.

This explanation looks like it would be a better comment than the one we currently have :)

> 
> 5. It will then automatically use the format of the texture, preventing us
> having to do a conditional there.

It's worth adding a comment to this effect there.

	Musawir Ali
	Comment 39 • 15 years ago

I would be that "CG researcher " ;). I can understand how the msdn documentation on D3D10_CREATE_DEVICE_PREVENT_INTERNAL_THREADING_OPTIMIZATIONS can be misleading. In fact, that flag gives no such indication. I pointed this out to Bas in my email. However, Microsoft is in fact using this flag to indicate "light weight" DX applications. By light weight they are essentially referring to applications that are not games. The idea is that when you create a DX game, the driver assumes that you will pretty much have a single instance and therefore it doesn't try to hold back when it comes to GPU resource allocation as long as it can crank out performance. In other words, the priority in regular DX applications is to make that one application run as fast as you can. For "light weight" applications, including D2D applications, the priorities are a bit different. Now you are no longer going to have a single (or very few) instances. You can have a lot of them (say, for example, a separate DX context/device per browser tab). In such cases, the GPU resource allocation scheme changes.

Since D2D is implemented on top of DX10, Microsoft uses this particular flag to indicate to the graphics driver that a "light weight" application is being created. I know that this is a bit confusing when a user looks up the documentation on MSDN, but thats just how it is at the moment. Perhaps in the future Microsoft will create a separate flag to indicate this more explicitly. But until then, there is no reason why the optimizations should not be made use of.

	Jeff Muizelaar [:jrmuizel]
	Comment 40 • 15 years ago

(In reply to comment #39)
> I would be that "CG researcher " ;). I can understand how the msdn
[snip]
> of.

This looks like a good comment to add :) Thanks.

	Jeff Muizelaar [:jrmuizel]
	Comment 41 • 15 years ago

Bas, can you post a patch of the mozilla integration so I try and find out what's going on in bug 535396.

	Jeff Muizelaar [:jrmuizel]
	Comment 42 • 15 years ago

8. _cairo_d2d_paint() assumes that CAIRO_OPERATOR_CLEAR is done with a solid pattern. CLEAR shouldn't use the source pattern at all.

9. Have you tried running the cairo test suite?

	Bas Schouten (:bas.schouten) Assignee
	Comment 46 • 15 years ago

Note that with the above patches, it should work. They might not be commented very well yet though, I still need to address that, especially the DirectWrite code. Although that code in this form is only temporary (until jfkthame can land his stuff), it's probably still good to document that a lot better.

You also need to make sure USE_WIN_SURFACE is undefined in modules/libpr0n, and ideally we'll want a fix for 534787. Although that will only show on some image loading/animated gifs.

	Bas Schouten (:bas.schouten) Assignee
	Comment 47 • 15 years ago

Corrected for latest repos head.

	Jeff Muizelaar [:jrmuizel]
	Comment 50 • 15 years ago

A quick comment on the the thebes patch: we should get rid of the Microsoft sample  code if possible.

	Musawir Ali
	Comment 51 • 15 years ago

Looks like Cairo DirectWrite and Direct2D Backends need to be updated as well. First, there was a build error because cairo_d2d_present_backbuffer() is actually declared as _cairo_d2d_present_backbuffer(). I corrected that, and the build was successful. However, the executable crashes on launch due to a NULL pointer access  in function: _cairo_d2d_release_dest_image() at the following line:

d2dsurf->surfaceBitmap->CopyFromMemory(&rect, data.pData, data.RowPitch);

Here, d2dsurf->surfaceBitmap is NULL.

	Bas Schouten (:bas.schouten) Assignee
	Comment 52 • 15 years ago

(In reply to comment #51)
> Looks like Cairo DirectWrite and Direct2D Backends need to be updated as well.
> First, there was a build error because cairo_d2d_present_backbuffer() is
> actually declared as _cairo_d2d_present_backbuffer(). I corrected that, and the
> build was successful. However, the executable crashes on launch due to a NULL
> pointer access  in function: _cairo_d2d_release_dest_image() at the following
> line:
> 
> d2dsurf->surfaceBitmap->CopyFromMemory(&rect, data.pData, data.RowPitch);
> 
> Here, d2dsurf->surfaceBitmap is NULL.

That's not unlikely actually, that function -should- never be called. I know what the bug is. Forgot to update it because I'd never experience it. It'd be interesting to know why it moves to the software fallback functions.

	Musawir Ali
	Comment 53 • 15 years ago

looking at the code, the fallback functions seem to be called unconditionally. The logic seems to be:

if (backend_func_available)
{
    status = call_backend_func();
    if (status == UNSUPPORTED) return error;
}

status = call_fallback_func();

So unless the backend function returns unsupported, the fallback function will always be called along with it. I see this sort of logic all across the call stack.

	Rob Arnold [:robarnold]
	Comment 54 • 15 years ago

Comment on attachment 418208 [details] [diff] [review]
Thebes DirectWrite and Direct2D Code v2

>+// xxx - used in FontEntry.  should be trimmed, moz code doesn't use
>+//       exceptions.  use gfxSparseBitSet instead?
>+#include <bitset>

Less STL is a good thing for our codebase. Maybe there's something under XPCOM to handle this?

>diff --git a/gfx/thebes/src/gfxD2DSurface.cpp b/gfx/thebes/src/gfxD2DSurface.cpp
>new file mode 100644
>--- /dev/null
>+++ b/gfx/thebes/src/gfxD2DSurface.cpp
>@@ -0,0 +1,48 @@
>+#include "gfxD2DSurface.h"
>+#include "cairo.h"
>+#include "cairo-win32.h"
>+
>+gfxD2DSurface::gfxD2DSurface(HWND aWnd)
>+{
>+#ifdef CAIRO_HAS_D2D_SURFACE
>+    Init(cairo_d2d_surface_create_for_hwnd(aWnd));
>+#endif
>+}
>+
>+gfxD2DSurface::gfxD2DSurface(cairo_surface_t *csurf)
>+{
>+    Init(csurf);
>+}
>+
>+gfxD2DSurface::gfxD2DSurface(const gfxIntSize& size,
>+                             gfxImageFormat imageFormat)
>+{
>+#ifdef CAIRO_HAS_D2D_SURFACE
>+    Init(cairo_d2d_surface_create((cairo_format_t)imageFormat, size.width, size.height));
>+#endif
>+}
>+
>+gfxD2DSurface::~gfxD2DSurface()
>+{
>+}
>+
>+void
>+gfxD2DSurface::Present()
>+{
>+#ifdef CAIRO_HAS_D2D_SURFACE
>+    cairo_d2d_present_backbuffer(CairoSurface());
>+#endif
>+}
>+
>+void
>+gfxD2DSurface::Scroll(const nsIntPoint &aDelta, const nsIntRect &aClip)
>+{
>+    cairo_rectangle_t rect;
>+    rect.x = aClip.x;
>+    rect.y = aClip.y;
>+    rect.width = aClip.width;
>+    rect.height = aClip.height;
>+#ifdef CAIRO_HAS_D2D_SURFACE
>+    cairo_d2d_scroll(CairoSurface(), aDelta.x, aDelta.y, &rect);
>+#endif
>+}

What is the point of the #define? Would it be simpler to not define gfxD2DSurface in this case? At the least this could be collapsed to a single #ifdef block.

>+// -- From Microsoft Samples --
>+
>+// The following macros define the minimum required platform.  The minimum required platform
>+// is the earliest version of Windows, Internet Explorer etc. that has the necessary features to run 
>+// your application.  The macros work by enabling all features available on platform versions up to and 
>+// including the version specified.
>+
>+// Modify the following defines if you have to target a platform prior to the ones specified below.
>+// Refer to MSDN for the latest info on corresponding values for different platforms.
>+
>+#ifndef WINVER                  // Minimum platform is Windows 7
>+#define WINVER 0x0601
>+#endif
>+
>+#ifndef _WIN32_WINNT            // Minimum platform is Windows 7
>+#define _WIN32_WINNT 0x0601
>+#endif
>+
>+#ifndef _WIN32_WINDOWS          // Minimum platform is Windows 7
>+#define _WIN32_WINDOWS 0x0601
>+#endif
>+
>+#ifndef WIN32_LEAN_AND_MEAN     // Exclude rarely-used stuff from Windows headers
>+#define WIN32_LEAN_AND_MEAN
>+#endif
>+
>+#ifndef NOMINMAX                // Use the standard's templated min/max
>+#define NOMINMAX
>+#endif
>+
>+#ifndef _USE_MATH_DEFINES       // want PI defined
>+#define _USE_MATH_DEFINES
>+#endif

I am skeptical that we need do these defines to get things to work. Overriding these values in a header is extremely dangerous. I don't have a link handy, but these represent the minimum supported version of any file that includes this header.

>+
>+////////////////////////////////////////
>+// Common headers:
>+
>+// C headers:
>+#include <stdlib.h>
>+#include <malloc.h>
>+#include <memory.h>
>+#include <math.h>
>+
>+// C++ headers:
>+#include <algorithm>
>+#include <numeric>
>+#include <string>
>+#include <vector>
>+#include <utility>
>+#include <limits>

None of these should be necessary.

>+// Windows headers:
>+
>+#include <windows.h>
>+#include <windowsx.h>
>+#include <unknwn.h>

I would be very surprised if we need these two headers.
>+
>+#include <dwrite.h>
>+#include <intsafe.h>
>+#include <strsafe.h>

And these last two.

>+
>+////////////////////////////////////////
>+// Common macro definitions:
>+
>+#ifndef HINST_THISCOMPONENT
>+EXTERN_C IMAGE_DOS_HEADER __ImageBase;
>+#define HINST_THISCOMPONENT ((HINSTANCE)&__ImageBase)
>+#endif
>+
>+// Use the double macro technique to stringize the actual value of s
>+#define STRINGIZE_(s) STRINGIZE2_(s)
>+#define STRINGIZE2_(s) #s
>+
>+#define FAILURE_LOCATION L"\r\nFunction: " TEXT(__FUNCTION__) L"\r\nLine: " TEXT(STRINGIZE_(__LINE__))

None of these macros are used in this patch.

>+#if (_MSC_VER >= 1200) // want to use std::min and std::max
>+#undef min
>+#undef max
>+#define min(x,y) _cpp_min(x,y)
>+#define max(x,y) _cpp_max(x,y)
>+#endif

I believe we have our own versions of these and this patch doesn't seem to use them. Redefining these in a header is poor form.

>+
>+// Ignore unreferenced parameters, since they are very common
>+// when implementing callbacks.
>+#pragma warning(disable : 4100)

DANGER.

>+////////////////////////////////////////
>+// COM inheritance helpers.
>+
>+
>+// Releases a COM object and nullifies pointer.
>+template <typename InterfaceType>
>+inline void SafeRelease(InterfaceType** currentObject)
>+{
>+    if (*currentObject != NULL)
>+    {
>+        (*currentObject)->Release();
>+        *currentObject = NULL;
>+    }
>+}
>+
>+
>+// Acquires an additional reference, if non-null.
>+template <typename InterfaceType>
>+inline InterfaceType* SafeAcquire(InterfaceType* newObject)
>+{
>+    if (newObject != NULL)
>+        newObject->AddRef();
>+
>+    return newObject;
>+}
>+
>+
>+// Sets a new COM object, releasing the old one.
>+template <typename InterfaceType>
>+inline void SafeSet(InterfaceType** currentObject, InterfaceType* newObject)
>+{
>+    SafeAcquire(newObject);
>+    SafeRelease(currentObject);
>+    *currentObject = newObject;
>+}
>+

None of these are used.

>+
>+// Maps exceptions to equivalent HRESULTs,
>+inline HRESULT ExceptionToHResult() throw()
>+{
>+    try
>+    {
>+        throw;  // Rethrow previous exception.
>+    }
>+    catch(std::bad_alloc&)
>+    {
>+        return E_OUTOFMEMORY;
>+    }
>+    catch (...)
>+    {
>+        return E_FAIL;
>+    }
>+}
>+
>+
>+// Generic COM base implementation for classes, since DirectWrite uses
>+// callbacks for several different kinds of objects, particularly the
>+// script analysis source/sink.
>+//
>+// Example:
>+//
>+//  class TextAnalysis : public ComBase<QiList<IDWriteTextAnalysisSink> >
>+//
>+template <typename InterfaceChain>
>+class ComBase : public InterfaceChain
>+{
>+public:
>+    explicit ComBase() throw()
>+    :   refValue_(0)
>+    { }
>+
>+    // IUnknown interface
>+    IFACEMETHOD(QueryInterface)(IID const& iid, OUT void** ppObject)
>+    {
>+        *ppObject = NULL;
>+        InterfaceChain::QueryInterfaceInternal(iid, ppObject);
>+        if (*ppObject == NULL)
>+            return E_NOINTERFACE;
>+
>+        AddRef();
>+        return S_OK;
>+    }
>+
>+    IFACEMETHOD_(ULONG, AddRef)()
>+    {
>+        return InterlockedIncrement(&refValue_);
>+    }
>+
>+    IFACEMETHOD_(ULONG, Release)()
>+    {
>+        ULONG newCount = InterlockedDecrement(&refValue_);
>+        if (newCount == 0)
>+            delete this;
>+
>+        return newCount;
>+    }

I don't think these need to be interlocked.

>+
>+    virtual ~ComBase()
>+    { }
>+
>+protected:
>+    ULONG refValue_;

This should be mRefValue.

>+
>+private:
>+    // No copy construction allowed.
>+    ComBase(const ComBase& b);
>+    ComBase& operator=(ComBase const&);
>+};
>+
>+
>+struct QiListNil
>+{
>+};
>+
>+
>+// When the QueryInterface list refers to itself as class,
>+// which hasn't fully been defined yet.
>+template <typename InterfaceName, typename InterfaceChain>
>+class QiListSelf : public InterfaceChain
>+{
>+public:
>+    inline void QueryInterfaceInternal(IID const& iid, OUT void** ppObject) throw()
>+    {
>+        if (iid != __uuidof(InterfaceName))
>+            return InterfaceChain::QueryInterfaceInternal(iid, ppObject);
>+
>+        *ppObject = static_cast<InterfaceName*>(this);
>+    }
>+};
>+
>+
>+// When this interface is implemented and more follow.
>+template <typename InterfaceName, typename InterfaceChain = QiListNil>
>+class QiList : public InterfaceName, public InterfaceChain
>+{
>+public:
>+    inline void QueryInterfaceInternal(IID const& iid, OUT void** ppObject) throw()
>+    {
>+        if (iid != __uuidof(InterfaceName))
>+            return InterfaceChain::QueryInterfaceInternal(iid, ppObject);
>+
>+        *ppObject = static_cast<InterfaceName*>(this);
>+    }
>+};
>+
>+
>+// When the this is the last implemented interface in the list.
>+template <typename InterfaceName>
>+class QiList<InterfaceName, QiListNil> : public InterfaceName
>+{
>+public:
>+    inline void QueryInterfaceInternal(IID const& iid, OUT void** ppObject) throw()
>+    {
>+        if (iid != __uuidof(InterfaceName))
>+            return;
>+
>+        *ppObject = static_cast<InterfaceName*>(this);
>+    }
>+};
>+
>+// -- End of MS sample code --

This QiList machinery is neat and clever but is it really necessary for the implementation?

>+// Singleton accessor for DirectWriteFactory. Threadsafe. This
>+// will obviously leak the factory at shutdown.

You could observe the xpcom-shutdown event to safely shut this down I think.

>+// DirectWrite is not available on all platforms.
>+typedef HRESULT (WINAPI*DWriteCreateFactoryFunc)(
>+  __in   DWRITE_FACTORY_TYPE factoryType,
>+  __in   REFIID iid,
>+  __out  IUnknown **factory
>+);

Please move this type declaration into the DWriteFactory class.

>+class DWriteFactory
>+{
>+public:
>+    static IDWriteFactory *Instance()
>+    {
>+        if (!PR_AtomicSet(&mInitialized, 1)) {
>+            nsresult rv;
>+            PRBool forceGDI = PR_FALSE;
>+            nsCOMPtr<nsIPrefService> prefSvc = do_GetService(NS_PREFSERVICE_CONTRACTID, &rv);
>+            if (NS_SUCCEEDED(rv)) {
>+		        nsCOMPtr<nsIPrefBranch> prefBranch;
>+		        rv = prefSvc->GetBranch("font.engine.", getter_AddRefs(prefBranch));
>+		        if (NS_SUCCEEDED(rv)) {

Nit: remove the tabs here.

>+                    rv = prefBranch->GetBoolPref("forcegdi", &forceGDI);
>+                    if (NS_FAILED(rv)) {
>+                        forceGDI = PR_FALSE;
>+                    }
>+                }
>+            }
>+#ifndef CAIRO_HAS_DWRITE_FONT
>+            // If this is the case we need to make sure we use GDI.
>+            forceGDI = PR_TRUE;
>+#endif

You could refactor this to not check the pref if we're not compiled with DWrite

>+            mInitialized = PR_TRUE;
>+	        // If we force GDI we become initialized but will never give a factory.

Nit: tab

>+            if (!forceGDI) {
>+                DWriteCreateFactoryFunc createDWriteFactory = (DWriteCreateFactoryFunc)
>+                    GetProcAddress(LoadLibraryW(L"dwrite.dll"), "DWriteCreateFactory");
>+
>+                if (createDWriteFactory) {
>+                    HRESULT hr = createDWriteFactory(
>+                        DWRITE_FACTORY_TYPE_SHARED,
>+                        __uuidof(IDWriteFactory),
>+                        reinterpret_cast<IUnknown**>(&mInstance));
>+                }
>+            }

This need not be compiled in if DWRITE support isn't enabled. This may in fact cause a perf hit.

>+        }
>+	    return mInstance;

Nit: tab

>+HRESULT STDMETHODCALLTYPE
>+gfxDWriteFontFileLoader::CreateStreamFromKey(const void *fontFileReferenceKey, 
>+					     UINT32 fontFileReferenceKeySize, 
>+					     IDWriteFontFileStream **fontFileStream)

Ok, it's probably easier to just let you remove all the tabs and not point them out each time.

>+IFACEMETHODIMP TextAnalysis::GetTextBeforePosition(
>+    UINT32 textPosition,
>+    OUT WCHAR const** textString,
>+    OUT UINT32* textLength
>+    ) throw()

Are these throw()s absolutely necessary? I think it carries quite a performance hit.

>+gfxWindowsFontEntry *
>+gfxWindowsDWriteFontFamily::FindFontEntry(const gfxFontStyle &aFontStyle)
>+{
>+    PRBool needsBold;
>+    return static_cast<gfxWindowsDWriteFontEntry*> (FindFontForStyle(aFontStyle, needsBold));

Hmm...intentional? The following is unreachable code.

>+    IDWriteFont *font;
>+
>+    DWRITE_FONT_STRETCH stretch;
>+    if (!aFontStyle.stretch) {
>+        stretch = DWRITE_FONT_STRETCH_NORMAL;
>+    } else {
>+        // TODO: Remove 'magic number' 4 and do proper code.
>+        stretch = static_cast<DWRITE_FONT_STRETCH>(aFontStyle.stretch - 4);
>+    }
>+
>+    DWRITE_FONT_STYLE style;
>+    switch(aFontStyle.style) {
>+        case NS_FONT_STYLE_NORMAL:
>+            style = DWRITE_FONT_STYLE_NORMAL;
>+            break;
>+        case NS_FONT_STYLE_ITALIC:
>+            style = DWRITE_FONT_STYLE_ITALIC;
>+            break;
>+        case NS_FONT_STYLE_OBLIQUE:
>+            style = DWRITE_FONT_STYLE_OBLIQUE;
>+            break;
>+    }
>+    

Nit: trailing spaces

>+PRBool
>+gfxWindowsDWriteFontEntry::MatchesGenericFamily(const nsACString &aGeneric) const
>+{
>+    // TODO: Implement me.
>+    return PR_TRUE;
>+}

Still a TODO?

>+PRBool
>+gfxWindowsDWriteFontEntry::SupportsRange(PRUint8 aRangeBit)
>+{
>+    // TODO: Implement me!!
>+    return PR_TRUE;
>+}

Ditto.

>+    DWRITE_FONT_STRETCH stretch;
>+    if (!fontGroup->mStyle.stretch) {
>+        stretch = DWRITE_FONT_STRETCH_NORMAL;
>+    } else {
>+        // TODO: Remove 'magic number' 4 and do proper code.
>+        stretch = static_cast<DWRITE_FONT_STRETCH>(fontGroup->mStyle.stretch - 4);

What is this code doing? The -4 must go.

>+
>+gfxWindowsDWriteFontGroup::~gfxWindowsDWriteFontGroup()
>+{
>+    mFontCollection->Release();
>+}

Can't we use nsRefPtr/nsCOMPtr for this instead of manually releasing?

>+void 
>+gfxWindowsDWriteFontGroup::UpdateFontList()
>+{
>+    // if user font set is set, check to see if font list needs updating
>+    if (mUserFontSet && mCurrGeneration != GetGeneration()) {
>+        // xxx - can probably improve this to detect when all fonts were found, so no need to update list
>+        mFonts.Clear();
>+        mFontEntries.Clear();
>+        InitFontList();
>+        mCurrGeneration = GetGeneration();

Hmm, I'm not sure what this code is supposed to do but the "xxx" is of course worrying.

>+    /**
>+     * There's an internal 16-bit limit on some things inside the analyzer.
>+     * to be on the safe side here we split up any ranges over 25000 characters.
>+     * TODO: Figure out what exactly is going on, and what is a safe number, and why.
>+     */
>+    for(int i = 0; i < ranges.Length(); i++) {
>+	if (ranges[i].Length() > 25000) {
>+	    ranges.InsertElementAt(i + 1, gfxTextRange(ranges[i].start + 25000, ranges[i].end));
>+	    ranges[i + 1].font = ranges[i].font;
>+	    ranges[i].end = ranges[i].start + 25000;
>+	}

Can you #define 25000 as a named constant somewhere?

>diff --git a/gfx/thebes/src/gfxWindowsGDIFonts.cpp b/gfx/thebes/src/gfxWindowsGDIFonts.cpp
>+//#define FORCE_UNISCRIBE 1

Why is this commented out?

>+#define FORCE_PR_LOG

Debugging leftovers?

>+        IDWriteFontFamily *family;
>+        for (UINT32 i = 0; i < systemFonts->GetFontFamilyCount(); i++) {
>+            systemFonts->GetFontFamily(i, &family);
>+            IDWriteLocalizedStrings *familyNames;
>+            family->GetFamilyNames(&familyNames);
>+            WCHAR tbuf[256];
>+            familyNames->GetString(0, tbuf, 256);

Can we lose the magic numbers in favor of named constants? What if the familyName is too long? (does this case come up? seems unlikely).

>+    if (DWriteFactory::Instance()) {
>+        // TODO: Need to do more intelligent matching like down with normal win32 fonts.
>+        const PRUint32 ch = data->mCh;

What does this mean?

> gfxWindowsSurface::gfxWindowsSurface(const gfxIntSize& size, gfxImageFormat imageFormat) :
>     mOwnsDC(PR_FALSE), mForPrinting(PR_FALSE), mWnd(nsnull)
> {
>     if (!CheckSurfaceSize(size))
>         return;
>-
>     cairo_surface_t *surf = cairo_win32_surface_create_with_dib((cairo_format_t)imageFormat,
>-                                                                size.width, size.height);
>+								size.width, size.height);
>     Init(surf);
> 
>     if (CairoStatus() == 0)
>-        mDC = cairo_win32_surface_get_dc(CairoSurface());
>+	mDC = cairo_win32_surface_get_dc(CairoSurface());
>     else
>-        mDC = nsnull;
>+	mDC = nsnull;
> }
> 
> gfxWindowsSurface::gfxWindowsSurface(HDC dc, const gfxIntSize& size, gfxImageFormat imageFormat) :
>     mOwnsDC(PR_FALSE), mForPrinting(PR_FALSE), mWnd(nsnull)
> {
>     if (!CheckSurfaceSize(size))
>         return;
> 
>@@ -130,18 +130,19 @@ gfxWindowsSurface::GetImageSurface()
>         NS_WARNING ("GetImageSurface on an invalid (null) surface; who's calling this without checking for surface errors?");
>         return nsnull;
>     }
> 
>     NS_ASSERTION(CairoSurface() != nsnull, "CairoSurface() shouldn't be nsnull when mSurfaceValid is TRUE!");
> 
>     if (mForPrinting)
>         return nsnull;
>+    cairo_surface_t *isurf;
>+    isurf = cairo_win32_surface_get_image(CairoSurface());
> 
>-    cairo_surface_t *isurf = cairo_win32_surface_get_image(CairoSurface());
>     if (!isurf)
>         return nsnull;
> 
>     nsRefPtr<gfxASurface> asurf = gfxASurface::Wrap(isurf);
>     gfxImageSurface *imgsurf = (gfxImageSurface*) asurf.get();
>     NS_ADDREF(imgsurf);
>     return imgsurf;
> }

Some unnecessary changes here. Lose the tabs.

	John Daggett (:jtd)
	Comment 55 • 15 years ago

Bas, I think the DirectWrite and Direct2D changes should be kept separate, since we might take each on different schedules (e.g. take DirectWrite but wait on Direct2D).  Keep the DirectWrite code on a patch in 517642 and note the dependency between the code here.  

Also, the DirectWrite code is dependent on 493280 and 502906.

	(not currently active) Ted Mielczarek
	Comment 56 • 15 years ago

(In reply to comment #54)
> I am skeptical that we need do these defines to get things to work. Overriding
> these values in a header is extremely dangerous. I don't have a link handy, but
> these represent the minimum supported version of any file that includes this
> header.

Also note that we set some of these in configure:
http://mxr.mozilla.org/mozilla-central/source/configure.in#734

based on the value passed to --with-windows-version.

	Bas Schouten (:bas.schouten) Assignee
	Comment 57 • 15 years ago

(In reply to comment #55)
> Bas, I think the DirectWrite and Direct2D changes should be kept separate,
> since we might take each on different schedules (e.g. take DirectWrite but wait
> on Direct2D).  Keep the DirectWrite code on a patch in 517642 and note the
> dependency between the code here.  
> 
> Also, the DirectWrite code is dependent on 493280 and 502906.

Hrm, we could, I suppose do that. Not sure what the rest thinks. I could tear them apart, but we'd need to decide on the order. I could never make two patches which can both lad on the current repository head.

@Ted/Rob: Those common defines are all going as I replace the MS sample code used there.

	Jeff Muizelaar [:jrmuizel]
	Comment 58 • 15 years ago

My build crashes for me too.

	Jeff Muizelaar [:jrmuizel]
	Comment 60 • 15 years ago

v6 works for me, but doesn't draw a bunch of images.

	Musawir Ali
	Comment 61 • 15 years ago

OK, that fixes the crash issue. I am able to build and run the executable. But the rendering seems to be a bit off. I'm attaching a screenshot. The browser's navigation controls are not being blitted properly (they appear and disappear on mouse over), and the webpage content rendering is also out of place.

	Bas Schouten (:bas.schouten) Assignee
	Comment 63 • 15 years ago

This fixes the crash on fallback, and a memory leak. Do note it's very important we find out why it's falling back in your builds, since it will have a huge performance impact, and really shouldn't happen. This is most likely a fill operation, we need to understand why cairo_d2d_fill fails.

	Bas Schouten (:bas.schouten) Assignee
	Comment 64 • 15 years ago

Thebes code with no more MS sample code. And part of Rob's comments processed. Those issues that I copy-pasted from WindowsGDIFonts I didn't fix yet.

	Musawir Ali
	Comment 65 • 15 years ago

Bas, I have debugged this a bit and it looks like the reason why the fallback is kicking in is due to the failure of the D2D brush creation. In particular, the brush creation is failing when the type is CAIRO_PATTERN_TYPE_SURFACE, and the surface type is CAIRO_SURFACE_TYPE_WIN32. You don't have a case handler for that type. You have:

if ( type == CAIRO_SURFACE_TYPE_D2D)
{
...
}
else if ( type = CAIRO_SURFACE_TYPE_IMAGE )
{
...
}
else
{
return NULL;
}

so you're ending up returning NULL here and the fallback kicks in.

	Musawir Ali
	Comment 66 • 15 years ago

typo above:
else if ( type = CAIRO_SURFACE_TYPE_IMAGE )

is

else if ( type == CAIRO_SURFACE_TYPE_IMAGE )

	Bas Schouten (:bas.schouten) Assignee
	Comment 67 • 15 years ago

(In reply to comment #66)
> typo above:
> else if ( type = CAIRO_SURFACE_TYPE_IMAGE )
> 
> is
> 
> else if ( type == CAIRO_SURFACE_TYPE_IMAGE )

Doh! Ofcourse, Joe's patches for Image Frames. My appologies for not being more clear about this. You need my hack from bug 534787 until that is resolved! With that applied you should be all good. See my comment 46.

	Musawir Ali
	Comment 68 • 15 years ago

yep, that did the trick. everything is now functional in my build.

	Jeff Muizelaar [:jrmuizel]
	Comment 69 • 15 years ago

1. cairo_dwrite_show_glyphs_on_surface still has a cairo_public attribute. It shouldn't and should maybe start with an underscore... I can't remember the precedent here.

	Jeff Muizelaar [:jrmuizel]
	Comment 70 • 15 years ago

Some comments on the thebes patch:

1. All of the ifdef CAIRO_HAS_D2D_SURFACE in gfxD2DSurface.cpp
 should probably go away. Do we have any use for gfxD2DSurface.cpp when we don't have CAIRO_HAS_D2D_SURFACE?

2. gfxWindowsDWriteFont::ComputeMetrics() is a wall of code add some line breaks please.

3. Do you change gfxWindowsGDIFont other than renaming it? In fact, can we keep it as gfxWindowsFont for now, and change it's name in a letter patch. i.e. we'll have gfxWindowsFont and gfxWindowsDWriteFont instead of gfxWindowsGDIFont and gfxWindowsDWriteFont. That will cut down on the amount of noise and make reviewing easier.

	Bas Schouten (:bas.schouten) Assignee
	Comment 71 • 15 years ago

1a. Yeah, fair enough, missed that, sorry.
1b. Well, as previously discussed, if the class ceases to exist in that case it would mean ifdefs in gfxWindowsPlatform and nsWindowGfx.cpp and another file in Widget. Whereas now the ifdefs only exist in gfxD2DSurface.cpp, I don't mind either way.
3. gfxWindowsFont is now an abstract base class that also contains some non pure virtual functions implemented there that were previously in gfxWindowsGDIFonts. So unless we make that gfxWindowsFontCommon(which is less pretty I think) that won't work. We'd also still have changes to gfxWindowsFonts for the functions that are removed.

	John Daggett (:jtd)
	Comment 72 • 15 years ago

I'm seeing funky rendering, similar to that noted attachment 418445 [details].

Also, a bunch of downloadable font tests fail with the when building trunk with the four attached patches.

http://people.mozilla.org/~jdaggett/webfonts/chunkfive-test.html (font doesn't load)
http://people.mozilla.org/~jdaggett/webfonts/droidserif-test.html (some fonts don't load, rendering incorrect)
http://people.mozilla.org/~jdaggett/webfonts/gentium-test.html (fonts don't load)
http://people.mozilla.org/~jdaggett/webfonts/goudy1911-test.html
http://people.mozilla.org/~jdaggett/webfonts/historicaltext.html (images don't render)
http://people.mozilla.org/~jdaggett/tests/titilliumtest.html (fonts don't load)

	John Daggett (:jtd)
	Comment 73 • 15 years ago

Test config:

Dell XPS 9000 Core i7 
NVIDIA GeForce GTS 240
Windows 7 ja

	John Daggett (:jtd)
	Comment 74 • 15 years ago

One thing to note here, it looks like these patches require the Aug 2009 DX SDK to build:

http://www.microsoft.com/downloads/details.aspx?FamilyID=B66E14B8-8505-4B17-BF80-EDB2DF5ABAD4&displaylang=en

	Bas Schouten (:bas.schouten) Assignee
	Comment 75 • 15 years ago

(In reply to comment #72)
> Created an attachment (id=420043) [details]
> screenshot, missing browser buttons
> 
> I'm seeing funky rendering, similar to that noted attachment 418445 [details].

The solution to that problem, is listed above, it has to do with the USE_WIN_SURFACE define in modules/libpr0n.

	John Daggett (:jtd)
	Comment 76 • 15 years ago

(In reply to comment #75)
> The solution to that problem, is listed above, it has to do with the
> USE_WIN_SURFACE define in modules/libpr0n.

So you mean USE_WIN_SURFACE needs to be commented out in modules/libpr0n/src/imgFrame.cpp?  Or that this is simply a "known" issue?  Do these patches rely on a fix for a bug that is not in the depends list?

	Bas Schouten (:bas.schouten) Assignee
	Comment 77 • 15 years ago

(In reply to comment #76)
> (In reply to comment #75)
> > The solution to that problem, is listed above, it has to do with the
> > USE_WIN_SURFACE define in modules/libpr0n.
> 
> So you mean USE_WIN_SURFACE needs to be commented out in
> modules/libpr0n/src/imgFrame.cpp?  Or that this is simply a "known" issue?  Do
> these patches rely on a fix for a bug that is not in the depends list?
Yep, that's what I mean. See comment 46. I updated the depends list as well.

	John Daggett (:jtd)
	Comment 78 • 15 years ago

The reason many pages with downloadable fonts render incorrectly is that you don't initialize the new gfxWindowsDWriteFontEntry with the weight, style, and stretch properties from the proxy entry:

+    BOOL isSupported;
+    DWRITE_FONT_FILE_TYPE fileType;
+    UINT32 numFaces;
+
+    gfxWindowsDWriteFontEntry *entry = new gfxWindowsDWriteFontEntry(uniqueName, NULL, fontFile);
+
+    fontFile->Analyze(&isSupported, &fileType, &entry->mFaceType, &numFaces);
+    if (!isSupported) {
+        delete entry;
+        return nsnull;
+    }
+    entry->mIsUserFont = PR_TRUE;

Instead you need to initialize the gfxWindowsDWriteFontEntry similar to the way the font entry is initialized in gfxWindowsGDIFontEntry::LoadFont:

    PRUint16 w = (aProxyEntry.mWeight == 0 ? 400 : aProxyEntry.mWeight);

    gfxWindowsGDIFontEntry *fe = gfxWindowsGDIFontEntry::CreateFontEntry(uniqueName, 
        gfxWindowsFontType(isCFF ? GFX_FONT_TYPE_PS_OPENTYPE : GFX_FONT_TYPE_TRUETYPE) /*type*/, 
        PRUint32(aProxyEntry.mItalic ? FONT_STYLE_ITALIC : FONT_STYLE_NORMAL), 
        w, winUserFontData);

With this change, the testpages below should render just as they do in 3.5:

http://people.mozilla.org/~jdaggett/webfonts/gentium-test.html
http://people.mozilla.org/~jdaggett/tests/titilliumtest.html
http://people.mozilla.org/~jdaggett/webfonts/droidserif-test.html

The code in gfxWindowsDWriteFontEntry::LoadLocalFont is also wrong, you're confusing family name lookups with face name lookups.  GDI muddles the distinction, you can use a family name (e.g. Arial) or a fullname (e.g. Arial Bold) to lookup a font using CreateFontIndirect.  You need to mimic the logic in gfxWindowsGDIFontEntry::LoadLocalFont which consists of:

1. Lookup a font using the fullname
2. Dig into the name table of the matched font to verify that the font fullname matches correctly (GDI returns a default font when the face isn't found)

Looking over the DWrite API docs this doesn't look easy.  Maybe you could use the IDWriteGdiInterop::CreateFontFromLOGFONT call?

Done correctly, this reftest below will render the same way for both pages:

http://people.mozilla.org/~jdaggett/font-face/src-list-local-full.html
http://people.mozilla.org/~jdaggett/font-face/src-list-local-full-ref.html

	Mark Perris
	Comment 79 • 15 years ago

Where does this fit with the layers work (https://wiki.mozilla.org/Gecko:Layers) coming up from roc? Am i correct in assuming that this bug focuses on the hardware rendering side, and roc's work on a higher level component to enable more efficient use of it?

	Bas Schouten (:bas.schouten) Assignee
	Comment 80 • 15 years ago

(In reply to comment #79)
> Where does this fit with the layers work
> (https://wiki.mozilla.org/Gecko:Layers) coming up from roc? Am i correct in
> assuming that this bug focuses on the hardware rendering side, and roc's work
> on a higher level component to enable more efficient use of it?
They're not that closely related. Roc's (and Jeff/Mine) layers work is about surface retention and hardware based surface composition. This is about actual hardware based vector graphics rendering.

	Bas Schouten (:bas.schouten) Assignee
	Comment 81 • 15 years ago

(In reply to comment #78)
> Lengthy, useful comments from :jtd.
Thanks for investigating. I'll see what I can do to fix these issues.

Personally I think using the interop is a 'very bad idea'(tm). I really do think it's important to move away from GDI, using bits and pieces here and there seems like very bad engineering to me. We'll have to see how we can make DWrite do what we want in my opinion. If you have any ideas, please let me know.

	Bas Schouten (:bas.schouten) Assignee
	Comment 82 • 15 years ago

I've fixed both bugs you mentioned. Without using GDI interop. One thing I'm still pondering about is that right now I'm comparing to the "en-us" locale when comparing the fullnames. I wonder if that's the correct behavior or if we should look at some specific locale? Once I know this I'll upload a new patch.

	Jeff Muizelaar [:jrmuizel]
	Comment 83 • 15 years ago

gfxWindowsSurface.cpp contains a bunch of whitespace changes. Let's avoid that for now.

	John Daggett (:jtd)
	Comment 84 • 15 years ago

(In reply to comment #82)
> I've fixed both bugs you mentioned. Without using GDI interop. One thing I'm
> still pondering about is that right now I'm comparing to the "en-us" locale
> when comparing the fullnames. I wonder if that's the correct behavior or if we
> should look at some specific locale? Once I know this I'll upload a new patch.

The locale is 'en-us' most of the time but you shouldn't need to decide that.  Use the same logic as the existing code, pass the name table to gfxFontUtils::ReadCanonicalName which contains fallback logic as well (for the case where an en-us name doesn't exist).

With your new patch, do reftests pass?  Especially reftests in layout/reftests/font-face?

	John Daggett (:jtd)
	Comment 85 • 15 years ago

BTW, what sort of Ts/Tp numbers do you see?  What's the increase/decrease in time spent within UpdateFontList during startup?

	Bas Schouten (:bas.schouten) Assignee
	Comment 86 • 15 years ago

Added a new thebes patch with the changes suggested by jtd.

	Jeff Muizelaar [:jrmuizel]
	Comment 87 • 15 years ago

1. what's the call to IASetPrimitiveTopology() for?

2. The alignment of the parameters of the prototypes at the top of
cairo-d2d-surface.cpp is off. If you reorder the file so that the backend
structure initialization happens at the end of the file (like other backends)
and you make these functions static (they should be), then you should be able
to avoid having the prototypes entirely.

3. A question about the 'unique' parameter to
_cairo_d2d_create_brush_for_pattern:
It's not clear to me why we need more than one version of a brush.

4. "We can't actually map this type of texture, so we create one in CPU memory,
and the". What type is "this type"?

	Bas Schouten (:bas.schouten) Assignee
	Comment 88 • 15 years ago

(In reply to comment #87)
> 1. what's the call to IASetPrimitiveTopology() for?
It prevents a debug warning if you have the D3D debug layer switched on. I can remove this without consequence if you prefer.

> 
> 2. The alignment of the parameters of the prototypes at the top of
> cairo-d2d-surface.cpp is off. If you reorder the file so that the backend
> structure initialization happens at the end of the file (like other backends)
> and you make these functions static (they should be), then you should be able
> to avoid having the prototypes entirely.

I believe the prototypes are a good idea, if only to concentrate the documentation of the function headers in one place. We could put the structure initialization happen at the end (or like in other backends in the middle somewhere or wherever they felt like it :)), but personally I think my structure of the file is -much- more readable. The functions which are implemented are one of the most important things to know when looking at a backend, hence I think the top, concentrated with the rest of central class information, is a good idea.

I'm fine with making them static, obviously.
> 
> 3. A question about the 'unique' parameter to
> _cairo_d2d_create_brush_for_pattern:
> It's not clear to me why we need more than one version of a brush.
The unique parameter means for a Color Brush and the Bitmap Brush, it doesn't use the ones stored on the class. But it creates new ones (brush creation has overhead, so I try to cache the brushes). This is needed for masking where we have a source pattern and a mask pattern.

> 
> 4. "We can't actually map this type of texture, so we create one in CPU memory,
> and the". What type is "this type"?

Erm, the surface storage texture. We can't map that directly, that comment is unclear for historical reasons. I'll fix that.

	Jeff Muizelaar [:jrmuizel]
	Comment 89 • 15 years ago

(In reply to comment #88)
> (In reply to comment #87)
> > 1. what's the call to IASetPrimitiveTopology() for?
> It prevents a debug warning if you have the D3D debug layer switched on. I can
> remove this without consequence if you prefer.
> 

Just add a comment.

> > 
> > 2. The alignment of the parameters of the prototypes at the top of
> > cairo-d2d-surface.cpp is off. If you reorder the file so that the backend
> > structure initialization happens at the end of the file (like other backends)
> > and you make these functions static (they should be), then you should be able
> > to avoid having the prototypes entirely.
> 
> I believe the prototypes are a good idea, if only to concentrate the
> documentation of the function headers in one place. We could put the structure
> initialization happen at the end (or like in other backends in the middle
> somewhere or wherever they felt like it :)), but personally I think my
> structure of the file is -much- more readable. The functions which are
> implemented are one of the most important things to know when looking at a
> backend, hence I think the top, concentrated with the rest of central class
> information, is a good idea.

Fair enough.

> I'm fine with making them static, obviously.
> > 
> > 3. A question about the 'unique' parameter to
> > _cairo_d2d_create_brush_for_pattern:
> > It's not clear to me why we need more than one version of a brush.
> The unique parameter means for a Color Brush and the Bitmap Brush, it doesn't
> use the ones stored on the class. But it creates new ones (brush creation has
> overhead, so I try to cache the brushes). This is needed for masking where we
> have a source pattern and a mask pattern.
> 

Why can't we use the same brush for the source and the mask?

	Bas Schouten (:bas.schouten) Assignee
	Comment 90 • 15 years ago

(In reply to comment #89)
> 
> Why can't we use the same brush for the source and the mask?

If you look at cairo-d2d-surface.cpp:1694, you'll notice we push a layer with one brush, then we draw with another (the source pattern) and then pop that layer. I have no idea if it is valid to change the opacity brush for that layer around while it is in use (I mean, obviously it holds a reference, so we could release it, but using the shared brush means we would actually be changing it). Maybe it is, it might be worth investigating since that would mean in theory we could get away without the unique brush.

	Jeff Muizelaar [:jrmuizel]
	Comment 91 • 15 years ago

It looks like these patches break the windows mobile/ce build. Have at the bottom of this:

http://tinderbox.mozilla.org/showlog.cgi?tree=MozillaTry&errorparser=unix&logfile=1262995531.1262999274.7126.gz&buildtime=1262995531&buildname=WinCE%20try%20hg%20build&fulltext=1#err3

	Bas Schouten (:bas.schouten) Assignee
	Comment 92 • 15 years ago

(In reply to comment #91)
> It looks like these patches break the windows mobile/ce build. Have at the
> bottom of this:
> 
> http://tinderbox.mozilla.org/showlog.cgi?tree=MozillaTry&errorparser=unix&logfile=1262995531.1262999274.7126.gz&buildtime=1262995531&buildname=WinCE%20try%20hg%20build&fulltext=1#err3
This is correct. There's a bunch of changes required to fix this in the font code. Hence me needing the DX SDK on the try servers. Although I'm also currently trying to get a mobile build going locally.

	Bas Schouten (:bas.schouten) Assignee
	Comment 93 • 15 years ago

Processed comments.

	Bas Schouten (:bas.schouten) Assignee
	Comment 94 • 15 years ago

An update to the thebes patch. Adjusted for the new, recently commited font list code by jfkthame. This should make the patch queue work against current trunk again.

	Bas Schouten (:bas.schouten) Assignee
	Comment 95 • 15 years ago

Patch fixes license headers and a bunch of include issues.

	Bas Schouten (:bas.schouten) Assignee
	Comment 96 • 15 years ago

With this patch and the latest Thebes one this should now no longer break the windows mobile build.

	Jeff Muizelaar [:jrmuizel]
	Comment 97 • 15 years ago

Bas, Do you have a new build system patch that doesn't require the DX sdk coming?

	Bas Schouten (:bas.schouten) Assignee
	Comment 98 • 15 years ago

(In reply to comment #97)
> Bas, Do you have a new build system patch that doesn't require the DX sdk
> coming?

I do, currently doing some clean builds to test that, should be up here soon. It's been modified to check for the WinSDK version.

	Bas Schouten (:bas.schouten) Assignee
	Comment 99 • 15 years ago

Updated buildsystem patch to no longer require DirectX SDK. Just the Windows 7 SDK in order to have D2D/DWrite support.

	Bas Schouten (:bas.schouten) Assignee
	Comment 100 • 15 years ago

Fix a bug where wrapping a cairo surface would not correctly call gfxASurface::Init.

	John Daggett (:jtd)
	Comment 101 • 15 years ago

Couple quick comments:

+    DWRITE_FONT_METRICS fontMetrics;
+    mFontFace->GetMetrics(&fontMetrics);
+    mMetrics->xHeight = ((gfxFloat)fontMetrics.xHeight / fontMetrics.designUnitsPerEm) * mStyle.size;

In this and *many* other places you're ignoring errors coming back from DirectWrite API calls.  Font quality varies dramatically and with downloadable fonts there's no guarantee that the font data isn't poop on a stick, so you really, really need to check for errors and handle them rather than assuming these calls always succeed.

+gfxFontEntry *
+gfxDWriteFontList::LookupLocalFont(const gfxProxyFontEntry *aProxyEntry,
+                                   const nsAString& aFontName)
+{
+    HRESULT hr;
+    IDWriteFontCollection *systemFonts;
+    hr = DWriteFactory::Instance()->GetSystemFontCollection(&systemFonts);
+    NS_ASSERTION(SUCCEEDED(hr), "GetSystemFontCollection failed!");
+
+    nsString lookupName(aFontName);
+    ToLowerCase(lookupName);
+    IDWriteFont *foundFont = NULL;
+    for (PRUint32 i = 0; i < systemFonts->GetFontFamilyCount(); i++) {
+        IDWriteFontFamily *family;
+        HRESULT hr;
+        hr = systemFonts->GetFontFamily(i, &family);
+        if (FAILED(hr)) {
+            continue;
+        }
+        IDWriteLocalizedStrings *familyNames;
+        family->GetFamilyNames(&familyNames);
+        UINT32 idx = 0;
+        BOOL exists;
+        WCHAR familyNameChar[256];
+        familyNames->FindLocaleName(L"en-us", &idx, &exists);
+        familyNames->GetString(idx, familyNameChar, 256);
+        familyNames->Release();
+        nsString familyName(familyNameChar);
+        ToLowerCase(familyName);
+        for (PRUint32 c = 0; c < family->GetFontCount(); c++) {
+            IDWriteFont *font;

Iterating over all font faces on a user's system is not a performant way of doing this.  Any user with tons of fonts on their system (e.g. Stuart!) will die during local() lookups.  If DirectWrite doesn't provide a way of doing direct fullname lookups, just use GDI for the lookup and convert that to a DirectWrite face.  Not ideal but far less sucky than iterating over all font faces.

	John Daggett (:jtd)
	Comment 102 • 15 years ago

Widget d2d patch has lots of trailing CR's and doesn't apply cleanly.

	Bas Schouten (:bas.schouten) Assignee
	Comment 103 • 15 years ago

(In reply to comment #101)
> Iterating over all font faces on a user's system is not a performant way of
> doing this.  Any user with tons of fonts on their system (e.g. Stuart!) will
> die during local() lookups.  If DirectWrite doesn't provide a way of doing
> direct fullname lookups, just use GDI for the lookup and convert that to a
> DirectWrite face.  Not ideal but far less sucky than iterating over all font
> faces.
Well, I'm very much against doing any GDI API's. Simply because GDI will be deprecated at some point. And we want this to be the DirectWrite version. Not the GDI one. Optionally I could store some information in a hash table and use that for the lookup?

On a sidenote, there does not appear to be a measurable difference in performance between using GDI lookup and this. I could gather exact timings on this though. But I'm sure that using some sort of hash lookup would be faster ofcourse.

	Bas Schouten (:bas.schouten) Assignee
	Comment 104 • 15 years ago

Fix widget patch whitespacing. I must've accidently normalized the line-endings in the patch. We have some inconsistent newlines in the widget directory.

	Bas Schouten (:bas.schouten) Assignee
	Comment 105 • 15 years ago

(In reply to comment #101)
> Couple quick comments:
> 
> +    DWRITE_FONT_METRICS fontMetrics;
> +    mFontFace->GetMetrics(&fontMetrics);
> +    mMetrics->xHeight = ((gfxFloat)fontMetrics.xHeight /
> fontMetrics.designUnitsPerEm) * mStyle.size;
> 
> In this and *many* other places you're ignoring errors coming back from
> DirectWrite API calls.  Font quality varies dramatically and with downloadable
> fonts there's no guarantee that the font data isn't poop on a stick, so you
> really, really need to check for errors and handle them rather than assuming
> these calls always succeed.
In this case, as well as several others, our API's don't have facilities for returning failure. Meaning that these font metrics I'm returning are just as much uninitialized memory as the ones I'll be returning if I just return here. In theory I think DWrite should parse the font and user font loading, and therefor this -should- never fail. I'll add a bunch of more checks in other places though to make sure we don't miss any potential failures.

	John Daggett (:jtd)
	Comment 106 • 15 years ago

(In reply to comment #105)
> (In reply to comment #101)
> > Couple quick comments:
> > 
> > +    DWRITE_FONT_METRICS fontMetrics;
> > +    mFontFace->GetMetrics(&fontMetrics);
> > +    mMetrics->xHeight = ((gfxFloat)fontMetrics.xHeight /
> > fontMetrics.designUnitsPerEm) * mStyle.size;
> > 
> > In this and *many* other places you're ignoring errors coming back from
> > DirectWrite API calls.  Font quality varies dramatically and with downloadable
> > fonts there's no guarantee that the font data isn't poop on a stick, so you
> > really, really need to check for errors and handle them rather than assuming
> > these calls always succeed.
> In this case, as well as several others, our API's don't have facilities for
> returning failure. Meaning that these font metrics I'm returning are just as
> much uninitialized memory as the ones I'll be returning if I just return here.
> In theory I think DWrite should parse the font and user font loading, and
> therefor this -should- never fail. I'll add a bunch of more checks in other
> places though to make sure we don't miss any potential failures.

GetMetrics returns a HRESULT value.  Please check it.

"If the method succeeds, it returns S_OK. Otherwise, it returns an HRESULT error code."

http://msdn.microsoft.com/en-us/library/dd371011%28VS.85%29.aspx

Compare your code with the existing GDI version of GetMetrics.  That code checks for an error and tries to deal with it other ways if there's an error.  If all else fails, mark the font not valid.  Using uninitialized data is neither valid nor safe.

http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/src/gfxWindowsFonts.cpp#274

If you think there are places where we should be doing a better job of recovering from errors, propose a fix.

	John Daggett (:jtd)
	Comment 107 • 15 years ago

(In reply to comment #103)
> Well, I'm very much against doing any GDI API's. Simply because GDI will be
> deprecated at some point. And we want this to be the DirectWrite version. Not
> the GDI one. 

There is no requirement currently to rid our code of GDI use.  Sure, it would be nicer, but if it's the difference between creating simple code that uses a GDI API and a larger piece of code that is slower or takes up extra memory, we should use GDI.

	(not currently active) Ted Mielczarek
	Comment 108 • 15 years ago

Comment on attachment 421052 [details] [diff] [review]
Build System DWrite/Direct2D mods v3

>diff --git a/configure.in b/configure.in
>--- a/configure.in
>+++ b/configure.in
>@@ -740,22 +740,23 @@ EOF
>     # version, error out
>     AC_MSG_CHECKING([for Windows SDK being recent enough])
>     if $PERL -e "exit(0x$MOZ_WINSDK_TARGETVER > $MOZ_WINSDK_MAXVER)"; then
>         AC_MSG_RESULT("yes")
>     else
>         AC_MSG_RESULT("no")
>         AC_MSG_ERROR([You are targeting Windows version 0x$MOZ_WINSDK_TARGETVER, but your SDK only supports up to version $MOZ_WINSDK_MAXVER. Install and use an updated SDK, or target a lower version using --with-windows-version. See https://developer.mozilla.org/En/Windows_SDK_versions for more details on fixing this.])
>     fi
>-
>+    
>     AC_DEFINE_UNQUOTED(MOZ_WINSDK_TARGETVER,0x$MOZ_WINSDK_TARGETVER)
>     # Definitions matching sdkddkver.h
>     AC_DEFINE_UNQUOTED(MOZ_NTDDI_WS03, 0x05020000)
>     AC_DEFINE_UNQUOTED(MOZ_NTDDI_LONGHORN, 0x06000000)
>     AC_DEFINE_UNQUOTED(MOZ_NTDDI_WIN7, 0x06010000)
>+    
>     ;;

Looks like you have some unintentional extra whitespace here. Please clean that up.

Looks good otherwise, r=me with that fixed.

	Bas Schouten (:bas.schouten) Assignee
	Comment 109 • 15 years ago

(In reply to comment #107)
> (In reply to comment #103)
> > Well, I'm very much against doing any GDI API's. Simply because GDI will be
> > deprecated at some point. And we want this to be the DirectWrite version. Not
> > the GDI one. 
> 
> There is no requirement currently to rid our code of GDI use.  Sure, it would
> be nicer, but if it's the difference between creating simple code that uses a
> GDI API and a larger piece of code that is slower or takes up extra memory, we
> should use GDI.
Hrm, I personally think this is one of those cases where at some point in the future, part of the API gets removed, or changed, or might not work anymore as expected, and we end up going all over the tree having to fix it. I personally still favor a DWrite only solution (with a little hash table storing the fullname and the family & face index. That would be the fullname + two unsigned ints per font face as additional memory usage). Or at least doing exact timings to see if that GDI interop is any faster(a bunch of them are incredibly slow in many cases, DirectWrite makes use of a cross-process font cache, whereas GDI does not, I'm not sure how the interop will behave in this case).

As for the uninitialized font metrics. I'll put in the same behavior as GDI has which seems to make sense. I don't think the API actually ever fails (loading custom fonts returns an error code if it doesn't understand the font file), but you may very well be right and that may not check for valid metrics.

	Bas Schouten (:bas.schouten) Assignee
	Comment 110 • 15 years ago

(In reply to comment #109)
> (In reply to comment #107)
> > (In reply to comment #103)
> As for the uninitialized font metrics. I'll put in the same behavior as GDI has
> which seems to make sense. I don't think the API actually ever fails (loading
> custom fonts returns an error code if it doesn't understand the font file), but
> you may very well be right and that may not check for valid metrics.


Scratch that... I just noticed why there wasn't a check in there when trying to add one. The documentation on MSDN is just wrong, dwrite.h:1013 has that method returning void. So it cannot actually fail. I replaced the assertion on creating the FontFace with setting mIsValid to PR_FALSE though, since I suspect it might detect font corruption upon creating the font face.

	[Baboo]
	Comment 111 • 15 years ago

(In reply to comment #103)
> Well, I'm very much against doing any GDI API's.

There could be also other reasons for sticking to the GDI API when dealing with fonts:
http://blog.getpaint.net/2009/12/21/paintnet-v352-is-on-the-way-%E2%80%93-better-font-support-for-win7vista/

	Jonathan Kew [:jfkthame]
	Comment 112 • 15 years ago

Comment on attachment 421150 [details] [diff] [review]
Thebes DirectWrite and Direct2D Code v7

A few initial comments from skimming through the patch... (mostly little nits that happened to catch my eye, as I haven't really understood a lot of the actual code yet!)

> -EXPORTS +=	gfxWindowsFonts.h 
> +EXPORTS +=  gfxWindowsGDIFonts.h
> +EXPORTS +=  gfxWindowsDWriteFonts.h
> +EXPORTS +=  gfxD2DSurface.h

It might make merging a little bit simpler if you don't rename the existing gfxWindowsFonts files and classes (which will be radically restructured in bug 502906 part 2 anyway), though I suppose that can be handled with a few global-replace operations there.

Would it be OK to drop the "Windows" from names such as gfxWindowsDWriteFonts? I'd go for gfxDWriteFonts or maybe gfxDirectWriteFonts instead.

(For comparison, my current intention in bug 502906 aims to replace gfxWindowsFonts.* with gfxGDIFontAccessor.* and gfxUniscribeShaper.* ... I'd anticipate similarly having gfxDWriteFontAccessor and gfxDWriteShaper, eventually. Including "Windows" in all these names makes for excessive verbosity, IMO.)


> + * The Initial Developer of the Original Code is Oracle Corporation.
> + * Portions created by the Initial Developer are Copyright (C) 2005
> + * the Initial Developer. All Rights Reserved.

The occurrence of Oracle here surprised me, is that correct? The other files say Mozilla Foundation.

Also the copyright date presumably needs updating (similarly in all files).

> diff --git a/gfx/thebes/public/gfxWindowsSurface.h 
...
> +#include "nsPoint.h"
> +#include "nsRect.h"

Do we really need these added to the header, or can they just be included by the .cpp file if required there?


>  	   gfxDDrawSurface.cpp \
> -	   gfxFT2FontList.cpp \
> +	   gfxFT2FontList.cpp
>  	   $(NULL)

Removal of the continuation-backslash here is a mistake.


> +// Singleton accessor for DirectWriteFactory. Threadsafe. This
> +// will obviously leak the factory at shutdown.

I think we normally try to clean up stuff like this; could you hook in to the platform Shutdown method to do this, or the platform font-list destructor, or something?


> +        fe->mItalic = (font->GetStyle() == DWRITE_FONT_STYLE_ITALIC);

Should also check for DWRITE_FONT_STYLE_OBLIQUE, I think. (We don't currently distinguish Oblique and Italic properly, though we should.....)


> +    UINT32 idx = 0;
> +    BOOL exists;
> +    systemFonts->FindFamilyName(L"tahoma", &idx, &exists);

I was surprised to see "tahoma" here, is that really the appropriate default?

What happens if it doesn't exist? The code doesn't seem to check the result in 'exists'.


> +            IDWriteLocalizedStrings *faceNames;
> +            font->GetFaceNames(&faceNames);
> +            UINT32 index = 0;
> +            /**
> +             * Use the English Locale, when not available index will remain
> +             * 0 and we will use the first name in the table.
> +             */
> +            faceNames->FindLocaleName(L"en-us", &index, &exists);
> +            faceNames->GetString(index, faceNameChar, 256);

This is wrong; MSDN doc for FindLocaleName says that "If the specified locale name does not exist, the return value is S_OK, but index is UINT_MAX and exists is FALSE." So you need to check the result of exists, and reset index if false.

> +        UINT32 idx = 0;
> +        BOOL exists;
> +        names->FindLocaleName(L"en-us", &idx, &exists);
> +        WCHAR famName[256];
> +        names->GetString(idx, famName, 256);

Ditto.


> +    // Call each of the analyzers in sequence, recording their results.
> +    if (SUCCEEDED(hr = textAnalyzer->AnalyzeLineBreakpoints(   this, 0, mTextLength, this))
> +    &&  SUCCEEDED(hr = textAnalyzer->AnalyzeBidi(              this, 0, mTextLength, this))
> +    &&  SUCCEEDED(hr = textAnalyzer->AnalyzeScript(            this, 0, mTextLength, this))
> +    &&  SUCCEEDED(hr = textAnalyzer->AnalyzeNumberSubstitution(this, 0, mTextLength, this))
> +    )

I believe Mozilla style says to break AFTER the operator such as &&.


> +{
> +    if (textLength > 0)
> +    {
> +        memcpy(mBreakpoints + textPosition, lineBreakpoints, textLength * sizeof(DWRITE_LINE_BREAKPOINT));
> +    }
> +    return S_OK;
> +}

There are a number of instances here where bracing style needs to be fixed; Mozilla style calls for the opening brace on the same line as the condition.

> +    if (*textLength < mCurrentRun->mTextLength)
> +    {
> +        SplitCurrentRun(mCurrentRun->mTextStart + *textLength);
> +    }
> +    else
> +    {
> +        // Just advance the current run.
> +        mCurrentRun = mCurrentRun->nextRun;
> +    }

....and "else" goes on the same line as the braces, too.


> +    // IUnknown interface
> +    IFACEMETHOD(QueryInterface)(IID const& iid, OUT void** ppObject)
> +    {
> +        if (iid == __uuidof(IDWriteTextAnalysisSource)) {
> +	    *ppObject = static_cast<IDWriteTextAnalysisSource*>(this);

Indentation.

> +cairo_font_face_t *
> +gfxWindowsDWriteFont::CairoFontFace()
> +{
> +    if (!mCairoFontFace) {
> +#ifdef CAIRO_HAS_DWRITE_FONT
> +        mCairoFontFace = 
> +	    cairo_dwrite_font_face_create_for_dwrite_fontface(
> +	    ((gfxWindowsDWriteFontEntry*)mFontEntry.get())->mFont, mFontFace);
> +#endif

Indentation.

Oh, and we normally use static_cast<> etc rather than C-style casts. I've probably been overlooking those up till now.

> +    gfxWindowsDWriteFontGroup *fontGroup =
> +        (gfxWindowsDWriteFontGroup*)closure;

static_cast<>.

> +        entry = (gfxWindowsDWriteFontEntry*)fontGroup->mUserFontSet->FindFontEntry(aName, fontGroup->mStyle, needsBold);

static_cast, and excessive line length.

> +        if (searchFont->GetStyle() != style || searchFont->GetStretch() != DWriteFontStretchFromStretch(fontGroup->mStyle.stretch)) {

Line length.

> +    if (mFontEntries.Length() == 0) {
> +        // It is pretty important that we have at least one font, so
> +        // try a few system fonts that should be there.
> +        nsAutoString str;
> +        HGDIOBJ hGDI = ::GetStockObject(DEFAULT_GUI_FONT);
> +        LOGFONTW logFont;
> +        if (hGDI && ::GetObjectW(hGDI, sizeof(logFont), &logFont)) {
> +            str.AppendLiteral("\"");
> +            str.Append(nsDependentString(logFont.lfFaceName));
> +            str.AppendLiteral("\"");
> +        }

Is there no DWrite equivalent to the GDI usage here? It seems unfortunate to mix the two, if it's possible to avoid.

> +gfxTextRun *
> +gfxWindowsDWriteFontGroup::MakeTextRun(const PRUint8 *aString, PRUint32 aLength,
> +                                   const Parameters *aParams, PRUint32 aFlags)
> +{
> +    nsDependentCString string((const char*)aString);

Offhand I'm not sure whether aString is guaranteed to be null-terminated; should you pass in aLength here, to be on the safe side?

> +	
>          mContext->Save();
> +	mContext->SetAntialiasMode(gfxContext::MODE_ALIASED);
>          mContext->Translate(mNativeRect.pos);
>          mContext->NewPath();
> -        mContext->Rectangle(gfxRect(gfxPoint(0.0, 0.0), mNativeRect.size));
> +        mContext->Rectangle(gfxRect(gfxPoint(0.0, 0.0), 
> +	    gfxSize(NS_ceil(mNativeRect.size.width), NS_ceil(mNativeRect.size.height)
> +	    )));
>  
>          nsRefPtr<gfxPattern> pat = new gfxPattern(alphaSurface);
>  
> +	pat->SetFilter(gfxPattern::FILTER_NEAREST);

Indentation looks strange; aha, I just realized there are "real" tabs here. Please expand to spaces. That also explains some earlier "indentation" issues.

	Bas Schouten (:bas.schouten) Assignee
	Comment 113 • 15 years ago

(In reply to comment #112)
> (From update of attachment 421150 [details] [diff] [review])
I replied to the things that weren't completely obvious :). Thanks for the quick feedback, my appologies on the tabs space mixes.. Just to explain:

Widget: Spaces, indent 2
Thebes: Spaces, indent 4
Cairo: Tabs, tab-width 8, indent 4

I must've forgotten to change modes a couple of times.

> It might make merging a little bit simpler if you don't rename the existing
> gfxWindowsFonts files and classes (which will be radically restructured in bug
> 502906 part 2 anyway), though I suppose that can be handled with a few
> global-replace operations there.
> 
> Would it be OK to drop the "Windows" from names such as gfxWindowsDWriteFonts?
> I'd go for gfxDWriteFonts or maybe gfxDirectWriteFonts instead.

I'm fine with all that, you guys decide :) I'd go for DWrite though. Since that's what MS uses everywhere.
> > + * The Initial Developer of the Original Code is Oracle Corporation.
> > + * Portions created by the Initial Developer are Copyright (C) 2005
> > + * the Initial Developer. All Rights Reserved.
> 
> The occurrence of Oracle here surprised me, is that correct? The other files
> say Mozilla Foundation.
> 
> Also the copyright date presumably needs updating (similarly in all files).

Erm, yes, I need to fix up the license plates, that Oracle thing is a big Oops. Will fix this.

> 
> > diff --git a/gfx/thebes/public/gfxWindowsSurface.h 
> ...
> > +#include "nsPoint.h"
> > +#include "nsRect.h"
> 
> Do we really need these added to the header, or can they just be included by
> the .cpp file if required there?
Good point, no they don't. They're from the old D2D code that was in there a couple of iterations back.

> 
> 
> >  	   gfxDDrawSurface.cpp \
> > -	   gfxFT2FontList.cpp \
> > +	   gfxFT2FontList.cpp
> >  	   $(NULL)
> 
> Removal of the continuation-backslash here is a mistake.
Heh, fixed that on my mobile test build, forgot to merge back to my normal D2D tree.
> 
> 
> > +// Singleton accessor for DirectWriteFactory. Threadsafe. This
> > +// will obviously leak the factory at shutdown.
> 
> I think we normally try to clean up stuff like this; could you hook in to the
> platform Shutdown method to do this, or the platform font-list destructor, or
> something?

Robarnold made a good suggestion here for what we can do I think. I'll look into doing that.
> 
> 
> > +        fe->mItalic = (font->GetStyle() == DWRITE_FONT_STYLE_ITALIC);
> 
> Should also check for DWRITE_FONT_STYLE_OBLIQUE, I think. (We don't currently
> distinguish Oblique and Italic properly, though we should.....)
> 
> 
> > +    UINT32 idx = 0;
> > +    BOOL exists;
> > +    systemFonts->FindFamilyName(L"tahoma", &idx, &exists);
> 
> I was surprised to see "tahoma" here, is that really the appropriate default?
> 
> What happens if it doesn't exist? The code doesn't seem to check the result in
> 'exists'.
IDX will remain 0, so it will take the first font in the font list. Tahoma is the default font according to http://msdn.microsoft.com/en-us/library/dd144925%28VS.85%29.aspx. (we don't have a DEFAULT_GUI_FONT thing that I can find in DWrite, maybe we could use the Theme API's somehow).
> 
> 
> > +            IDWriteLocalizedStrings *faceNames;
> > +            font->GetFaceNames(&faceNames);
> > +            UINT32 index = 0;
> > +            /**
> > +             * Use the English Locale, when not available index will remain
> > +             * 0 and we will use the first name in the table.
> > +             */
> > +            faceNames->FindLocaleName(L"en-us", &index, &exists);
> > +            faceNames->GetString(index, faceNameChar, 256);
> 
> This is wrong; MSDN doc for FindLocaleName says that "If the specified locale
> name does not exist, the return value is S_OK, but index is UINT_MAX and exists
> is FALSE." So you need to check the result of exists, and reset index if false.
Well spotted :-).
> > +    if (mFontEntries.Length() == 0) {
> > +        // It is pretty important that we have at least one font, so
> > +        // try a few system fonts that should be there.
> > +        nsAutoString str;
> > +        HGDIOBJ hGDI = ::GetStockObject(DEFAULT_GUI_FONT);
> > +        LOGFONTW logFont;
> > +        if (hGDI && ::GetObjectW(hGDI, sizeof(logFont), &logFont)) {
> > +            str.AppendLiteral("\"");
> > +            str.Append(nsDependentString(logFont.lfFaceName));
> > +            str.AppendLiteral("\"");
> > +        }
> 
> Is there no DWrite equivalent to the GDI usage here? It seems unfortunate to
> mix the two, if it's possible to avoid.
I could do the same trick here mentioned earlier for the default system font.

	Bas Schouten (:bas.schouten) Assignee
	Comment 114 • 15 years ago

(In reply to comment #113)
> (In reply to comment #112)
> > (From update of attachment 421150 [details] [diff] [review] [details])
> IDX will remain 0, so it will take the first font in the font list. Tahoma is
> the default font according to
> http://msdn.microsoft.com/en-us/library/dd144925%28VS.85%29.aspx. (we don't
> have a DEFAULT_GUI_FONT thing that I can find in DWrite, maybe we could use the
> Theme API's somehow).

Obviously I was plain wrong here, it will become UINT_MAX just like in the other case you spotted. Will fix those.

	Bas Schouten (:bas.schouten) Assignee
	Comment 115 • 15 years ago

Patch with majority of jfkthame's preliminary comments. (no file/class renames yet)

	Jeff Muizelaar [:jrmuizel]
	Comment 116 • 15 years ago

Comment on attachment 420988 [details] [diff] [review]
Cairo DirectWrite and Direct2D Backends v8

1. _cairo_dwrite_scaled_font_init_glyph_surface has the wrong argument alignment and has some pretty dense blocks of code. Adding some line breaks will help here.

2. _cairo_d2d_flush is not hooked up and should be.

3. We assert on GetFirstMatchingFont failing. We should probably handle the error instead.

Other than that it looks good enough to get into the tree. We can make further improvements when it's there.

	ICHIMARU Takeshi(a.k.a. Ayakawa)
	Comment 117 • 15 years ago

You use "en-us" locale at gfxWindowsDWriteFontFamily::FindStyleVariations,
gfxDWriteFontList::LookupLocalFont, gfxDWriteFontList:InitFontList.
These cause wrong font choosing in environment other than "en-us".

you don't use "en-us", get user locale with "GetUserDefaultLocaleName".

	John Daggett (:jtd)
	Comment 118 • 15 years ago

(In reply to comment #117)
> Created an attachment (id=421395) [details]
> Font problem
> 
> You use "en-us" locale at gfxWindowsDWriteFontFamily::FindStyleVariations,
> gfxDWriteFontList::LookupLocalFont, gfxDWriteFontList:InitFontList.
> These cause wrong font choosing in environment other than "en-us".
> 
> you don't use "en-us", get user locale with "GetUserDefaultLocaleName".

The LookupLocalFont method is used to lookup fonts by their fullname in @font-face rules.  As such, it should only use the en-us name or the first locale name if an en-us version does not exist.  This is used as a canonical fullname to avoid interoperability problems with large numbers of localizations of style names like bold, italic, etc.

But any user-facing name should be using the locale-specific name as noted.

	ICHIMARU Takeshi(a.k.a. Ayakawa)
	Comment 119 • 15 years ago

notice dialog's font. d2d ver choise wrong font.

	John Daggett (:jtd)
	Comment 120 • 15 years ago

(In reply to comment #119)
> notice dialog's font. d2d ver choise wrong font.

Right, definitely wrong.  My comment only pertains to the use of the 'en-us' locale in LookupLocalFont.

	John Daggett (:jtd)
	Comment 121 • 15 years ago

Looks like synthetic rendering for downloadable fonts is busted too:

http://people.mozilla.org/~jdaggett/font-face/synthetic-variations.html

Should see plain, fake bold, fake italic, fake bold/italic in the four columns.  Instead all four columns use plain face, no fake nuttin.  Compare to pre-1/8 trunk builds.

	Bas Schouten (:bas.schouten) Assignee
	Comment 122 • 15 years ago

Processed latest jrmuizel comments.

	John Daggett (:jtd)
	Comment 124 • 15 years ago

+already_AddRefed<gfxWindowsDWriteFont>
+gfxWindowsDWriteFont::GetOrMakeFont(gfxFontEntry *aFontEntry,
+				    const gfxFontStyle *aStyle,
+				    PRBool aNeedsBold)
+{
+    // because we know the FontEntry has the weight we really want, use it for matching
+    // things in the cache so we don't end up with things like 402 in there.
+    gfxFontStyle style(*aStyle);
+
+    if (aFontEntry->mIsUserFont && !aFontEntry->IsBold()) {
+        // determine whether synthetic bolding is needed
+        PRInt8 baseWeight, weightDistance;
+        aStyle->ComputeWeightAndOffset(&baseWeight, &weightDistance);
+
+        if ((weightDistance == 0 && baseWeight >= 6) || (weightDistance > 0 && aNeedsBold)) {
+            style.weight = 700;  
+        } else {
+            style.weight = aFontEntry->mWeight;
+        }
+    } else {
+        style.weight = aFontEntry->mWeight;
+    }
+    nsRefPtr<gfxFont> font = gfxFontCache::GetCache()->Lookup(aFontEntry->Name(), aStyle);
+
+    if (!font) {
+        font = new gfxWindowsDWriteFont(static_cast<gfxWindowsDWriteFontEntry*>(aFontEntry), aStyle);
+        if (!font) {
+            return nsnull;
+        }
+        gfxFontCache::GetCache()->AddNew(font);
+    }
+
+    font->AddRef();
+    return static_cast<gfxWindowsDWriteFont*>(font.get());
+}

After making a local copy of the style, you don't use it.  You need to pass &style instead of aStyle to the cache lookup method and the constructor.

	Bas Schouten (:bas.schouten) Assignee
	Comment 125 • 15 years ago

(In reply to comment #124)
> 
> After making a local copy of the style, you don't use it.  You need to pass
> &style instead of aStyle to the cache lookup method and the constructor.

Indeed, I changed them back to aStyle to test another font lookup issue that I created with my new code. And forgot to change them back when I went back to the synthetic bolding issues. Thanks a lot for helping me find that one.

	John Daggett (:jtd)
	Comment 126 • 15 years ago

Also looks like italics are incorrect for normal fonts (i.e. not @font-face fonts).  Italic fonts are rendering with synthetic italics rather than the real italic face.

Compare these:
http://people.mozilla.org/~jdaggett/font-face/src-list-local-full.html
http://people.mozilla.org/~jdaggett/font-face/src-list-local-full-ref.html

The -ref version is incorrect, it's obliquing the regular face rather than using the italic face.

	Jonathan Kew [:jfkthame]
	Comment 127 • 15 years ago

Comment on attachment 421283 [details] [diff] [review]
Thebes DirectWrite and Direct2D Code v8

Maybe it's just paranoia, given how many problems we've seen caused by bad fonts and unpredictable platform API response to them, but I'd really like to see error checking added in places like this.... if the DWrite APIs return result codes, don't ignore them.

>+void
>+gfxWindowsDWriteFontFamily::FindStyleVariations()
>+{
>+    mAvailableFonts.SetLength(mDWFamily->GetFontCount());
>+
>+    for (UINT32 i = 0; i < mDWFamily->GetFontCount(); i++) {
>+        IDWriteFont *font;
>+        mDWFamily->GetFont(i, &font);

Check for failure. I know it shouldn't occur, but weird things happen when users install random collections of "1000 fonts for $10", and most of them are flaky.....

(And if error occurs, log a warning (in debug builds) and skip this face; we just won't use it.)

>+        IDWriteLocalizedStrings *names;
>+        font->GetFaceNames(&names);

Ditto.

>+        WCHAR faceName[256];
>+        UINT32 idx = 0;
>+        BOOL exists;
>+        names->FindLocaleName(L"en-us", &idx, &exists);

Check result for failure; check 'exists'. It is not guaranteed that a font has en-us names! (Exceptions are very rare, but possible.)

>+        names->GetString(idx, faceName, 256);

Check result for failure.

Don't use a fixed-size buffer and hard-coded length limit; use GetStringLength to resize a stack-based nsAutoString or nsAutoTArray with an auto-size large enough for normal names (so there's no actual allocation cost except in pathological cases). In principle, names in OpenType can be up to 64K long (although I'm sure plenty of other software would fail on them too!)

>+        names->Release();
>+        nsString fullName(mName);
>+        fullName.AppendLiteral(" ");
>+        fullName.Append(faceName);

I wish DWrite gave us access to the real fullname from the font. In the absence of that, you should probably check whether the faceName is "Regular", and if so, don't append it. (The OpenType spec says, "Full font name; this should be a combination of strings 1 and 2. Exception: if the font is “Regular” as indicated in string 2, then use only the family name contained in string 1.")

>+        gfxWindowsDWriteFontEntry *fe = new gfxWindowsDWriteFontEntry(fullName, font);
>+        fe->mItalic = (font->GetStyle() == DWRITE_FONT_STYLE_ITALIC ||
>+                       font->GetStyle() == DWRITE_FONT_STYLE_OBLIQUE);
>+        fe->mStretch = FontStretchFromDWriteStretch(font->GetStretch());
>+        fe->mWeight = font->GetWeight();

It's not entirely clear from the DWrite documentation whether GetWeight() can ONLY return defined values from the DWRITE_FONT_WEIGHT enumeration, or if it returns whatever integer it finds in the font. CSS weights, OTOH, are only allowed to be round hundreds in the range 100..900. So you should at least clamp the result to this range (i.e. we can't accept the DWRITE_FONT_WEIGHT_EXTRA_BLACK = 950 value here), and it might be wisest to explicitly round to the nearest multiple of 100. (Either that, or create a test font with an intermediate usWeight value, and check how DWrite actually handles it.)

>+        mAvailableFonts[i] = fe;

Append the entries rather than stuffing them into fixed locations, to allow for potential skipped faces if the calls above return failure.

>+    }
>+}

There'll be similar error-handling considerations in some other places....

	Bas Schouten (:bas.schouten) Assignee
	Comment 128 • 15 years ago

(In reply to comment #127)
> (From update of attachment 421283 [details] [diff] [review])
> Maybe it's just paranoia, given how many problems we've seen caused by bad
> fonts and unpredictable platform API response to them, but I'd really like to
> see error checking added in places like this.... if the DWrite APIs return
> result codes, don't ignore them.
> 

I honestly think that in many of these cases they return result codes to indicate API usage errors (i.e. E_POINTER) or things like that. If we're sure to ask for an index smaller than GetFontCount() I honestly don't think it can -actually- fail. I believe DirectWrite will have thrown bad fonts out already by then. (for example, if we read a user font we will be calling an 'analyze' function first which returns an 'isSupported' bool) I think adding actual checks on all of them would be a waste of countless conditional branches. I'd prefer to add assertions there so we'll know when maybe that assumption turns out to be wrong, but I doubt it, and I really don't think adding all these checks is very benificial. Something similar goes for GetNames and such.

	Bas Schouten (:bas.schouten) Assignee
	Comment 129 • 15 years ago

Process comments from jfkthame and jdaggett. Should fix all problems listed here.

	Jonathan Kew [:jfkthame]
	Comment 130 • 15 years ago

Comment on attachment 421623 [details] [diff] [review]
Thebes DirectWrite and Direct2D Code v9

>+ * The Initial Developer of the Original Code is Oracle Corporation.

Hmm, this didn't get fixed yet...

>+    for (UINT32 i = 0; i < mDWFamily->GetFontCount(); i++) {
>+        IDWriteFont *font;
>+        hr = mDWFamily->GetFont(i, &font);
>+	if (FAILED(hr)) {
>+	    // This should never happen.
>+	    NS_WARNING("Failed to get existing font from family.");
>+	    continue;
>+	}

Oops, your editor was in the wrong mode - real tabs here. (Several times in this function.)

>+        if (!exists) {
>+            englishIdx = 0;
>+        }

Perhaps add a comment about falling back to whatever language is first in the list, as "englishIdx" is not in fact English in this case?

>+        hr = names->GetString(englishIdx, faceName, 256);

It'd still be better not to use a hard-coded size limit, IMO.

>+	nsString fullNameCase(mName);
>+	fullNameCase.AppendLiteral(" ");
>+	fullNameCase.Append(faceName);

Would it be appropriate to skip appending "Regular" here, too? And in that case, you could simply lowercase this string later for the fullname table, rather than having to construct a second version of it. I can't think of a reason you'd need a to maintain a different form of the name here.

>+	PRUint16 weight = font->GetWeight();
>+	// Round to nearest 100.
>+	weight += 50;
>+	weight /= 100;
>+	weight *= 100;
>+	if (weight < 100) {
>+	    weight = 100;
>+	}
>+	if (weight > 900) {
>+	    weight = 900;
>+	}

I've realized we have some convenience macros you could use here, to make the code easier to read: how about

  PRUint16 weight = PR_ROUNDUP(font->GetWeight() - 50, 100);
  weight = PR_MAX(100, weight);
  weight = PR_MIN(900, weight);

The resulting code should be equivalent, but it's a lot more concise here in the source.

>+        fe->mWeight = font->GetWeight();

Oops, having sanitized the weight value, be sure to use it!

>+        nsString faceNameString(faceName);
>+        nsString fullName(mName);
>+        ToLowerCase(faceNameString);
>+        ToLowerCase(fullName);
>+        if (!faceNameString.EqualsLiteral("regular") &&
>+            !faceNameString.EqualsLiteral("normal")) {
>+		fullName.AppendLiteral(" ");
>+		fullName.Append(faceNameString);
>+                // Put this in as the 'standard' face.
>+        }

See above, can you just reuse the same "fullname" (lowercased) here?

>+	if (font->GetStyle() != DWRITE_FONT_STYLE_OBLIQUE) {
>+	    /**
>+	     * Oblique fonts don't go in here, since we cannot distinquish them
>+	     * from italic with the current gfxFontEntry structure.
>+	     */
>+            mAvailableFonts.AppendElement(fe);
>+	}

I think we'd only want to omit oblique fonts here if the family ALSO has separate italic faces; if it only has upright and oblique, then we do want to use the oblique. (It's better to use a true oblique face, if available, than to synthesize one.)

Which means, I suppose, that you have to collect all the faces, and then filter out the oblique ones only if they are "shadowed" by italics. (Offhand, I can only think of a handful of families that actually have both.)

>+    }
>+}

Before returning from FindStyleVariations, I think it would be good to check that at least one face was successfully added to mAvailableFonts, and at least issue an NS_WARNING if not. (Ideally, I think we should remove any such "broken" families from our font list, but I don't think any of the platforms currently handle that.)


>+gfxFontEntry *
>+gfxDWriteFontList::GetDefaultFont(const gfxFontStyle *aStyle,
>+                                  PRBool &aNeedsBold)

The error-handling code here also has tabs. :(


>+    /**
>+     * TODO: This may not work correctly if this is accessed before
>+     * FindStyleVariations has been called on all families. We may be able
>+     * to "accelerate" runloader if it is needed here.
>+     */

I think we do need to handle this, otherwise src:local(...) will fail if it's on the startup page (or, for example, a page that's open in a session that's being restored when the browser launches).

One way to mitigate the impact of that somewhat would be to separate the loading of cmaps from the population of font families with their faces; we only need to populate the families in order to do name lookups, so cmap reading could be deferred to a second stage of font-loader operation.


>+gfxFontEntry *
>+gfxDWriteFontList::MakePlatformFont(const gfxProxyFontEntry *aProxyEntry,
>+                                    const PRUint8 *aFontData,
>+                                    PRUint32 aLength)

This function is responsible for ensuring aFontData gets deleted when it is no longer needed. (You could probably just borrow the stack-based helper approach from the gfxGDIFontList implementation.)

>+    // Postscript-style glyphs, swizzle name table, load directly

This comment is no longer true, as you're loading all fonts this way regardless of the type of glyph data.

>+    PRUint16 w = (aProxyEntry->mWeight == 0 ? 400 : aProxyEntry->mWeight);
>+    gfxWindowsDWriteFontEntry *entry = new gfxWindowsDWriteFontEntry(uniqueName, NULL, fontFile);
>+    entry->mItalic = aProxyEntry->mItalic;
>+    entry->mWeight = aProxyEntry->mWeight;

This should presumably be w, not aProxyEntry->mWeight.

>+    fontFile->Analyze(&isSupported, &fileType, &entry->mFaceType, &numFaces);
>+    if (!isSupported) {
>+        delete entry;
>+        return nsnull;
>+    }

Would it be ok to call Analyze() and check isSupported before creating the new fontEntry?


>+void
>+gfxDWriteFontList::InitFontList()
>+{
>+    HRESULT hr;
>+    gfxFontCache *fc = gfxFontCache::GetCache();
>+    if (fc) {
>+        fc->AgeAllGenerations();
>+    }
>+    mFontFamilies.Clear();

Also need to clear the fullname table.

>+        WCHAR famName[256];
>+        WCHAR famLocalName[256];

I still don't much like the hard-coded sizes here. There should be no significant cost to using e.g. an nsAutoTArray<WCHAR,32> instead, and checking the actual length.

	Shawn Wilsher :sdwilsh
	Comment 131 • 15 years ago

(In reply to comment #130)
>   PRUint16 weight = PR_ROUNDUP(font->GetWeight() - 50, 100);
>   weight = PR_MAX(100, weight);
>   weight = PR_MIN(900, weight);
Don't do it!  You want to us NS_MIN and NS_MAX here, not the NSPR functions.  See bug 518502.

	Bas Schouten (:bas.schouten) Assignee
	Comment 132 • 15 years ago

(In reply to comment #130)
> (From update of attachment 421623 [details] [diff] [review])
> >+	if (font->GetStyle() != DWRITE_FONT_STYLE_OBLIQUE) {
> >+	    /**
> >+	     * Oblique fonts don't go in here, since we cannot distinquish them
> >+	     * from italic with the current gfxFontEntry structure.
> >+	     */
> >+            mAvailableFonts.AppendElement(fe);
> >+	}
> 
> I think we'd only want to omit oblique fonts here if the family ALSO has
> separate italic faces; if it only has upright and oblique, then we do want to
> use the oblique. (It's better to use a true oblique face, if available, than to
> synthesize one.)
> 
> Which means, I suppose, that you have to collect all the faces, and then filter
> out the oblique ones only if they are "shadowed" by italics. (Offhand, I can
> only think of a handful of families that actually have both.)

I think we need to spec the -exact- desired behavior here, i.e. what if we have a 200 oblique, a 400 italic, a 600 oblique and an 800 italic?

> >+    /**
> >+     * TODO: This may not work correctly if this is accessed before
> >+     * FindStyleVariations has been called on all families. We may be able
> >+     * to "accelerate" runloader if it is needed here.
> >+     */
> 
> I think we do need to handle this, otherwise src:local(...) will fail if it's
> on the startup page (or, for example, a page that's open in a session that's
> being restored when the browser launches).
> 
> One way to mitigate the impact of that somewhat would be to separate the
> loading of cmaps from the population of font families with their faces; we only
> need to populate the families in order to do name lookups, so cmap reading
> could be deferred to a second stage of font-loader operation.

Where do you think we should do this?
> 
> 
> >+gfxFontEntry *
> >+gfxDWriteFontList::MakePlatformFont(const gfxProxyFontEntry *aProxyEntry,
> >+                                    const PRUint8 *aFontData,
> >+                                    PRUint32 aLength)
> 
> This function is responsible for ensuring aFontData gets deleted when it is no
> longer needed. (You could probably just borrow the stack-based helper approach
> from the gfxGDIFontList implementation.)

I only have one additional return point when I need to NS_Free it. So I'll just to it manually.
> >+    fontFile->Analyze(&isSupported, &fileType, &entry->mFaceType, &numFaces);
> >+    if (!isSupported) {
> >+        delete entry;
> >+        return nsnull;
> >+    }
> 
> Would it be ok to call Analyze() and check isSupported before creating the new
> fontEntry?
Well, otherwise we'd have to store the facetype in a temp and then assign it to the font entry member (since it we pass it directly into analyze now), but we could do it. I don't see a real point though, failure should be the rare case, and a deallocation isn't that expensive. Let me know what you think we should do.

	Jonathan Kew [:jfkthame]
	Comment 133 • 15 years ago

(In reply to comment #131)
> (In reply to comment #130)
> >   PRUint16 weight = PR_ROUNDUP(font->GetWeight() - 50, 100);
> >   weight = PR_MAX(100, weight);
> >   weight = PR_MIN(900, weight);
> Don't do it!  You want to us NS_MIN and NS_MAX here, not the NSPR functions. 
> See bug 518502.

Oops. Right.

In that case, it might be nice to create an NS_ROUND (and/or NS_ROUNDUP) too, though PR_ROUNDUP is only used in a few places.

	Jonathan Kew [:jfkthame]
	Comment 134 • 15 years ago

(In reply to comment #132)
> I think we need to spec the -exact- desired behavior here, i.e. what if we have
> a 200 oblique, a 400 italic, a 600 oblique and an 800 italic?

The CSS Fonts spec describes the matching algorithm we're suppose to use (which  gfxFontFamily::FindFontForStyle is responsible to implement). So the font list should just collect all the faces in the family, and let that function make the choices.

Our gfxFontStyle actually supports the italic/oblique distinction, IIRC; maybe it's time to fix the gfxFontEntry so that it can, too.


> > One way to mitigate the impact of that somewhat would be to separate the
> > loading of cmaps from the population of font families with their faces; we only
> > need to populate the families in order to do name lookups, so cmap reading
> > could be deferred to a second stage of font-loader operation.
> 
> Where do you think we should do this?

We could modify the font info loader so that it loops over the families twice instead of once, loading styles the first time and cmaps the second time; then implement a "ForceLoadStyles" function that ensures the first loop has been completed.


> > This function is responsible for ensuring aFontData gets deleted when it is no
> > longer needed. (You could probably just borrow the stack-based helper approach
> > from the gfxGDIFontList implementation.)
> 
> I only have one additional return point when I need to NS_Free it. So I'll just
> to it manually.

Fine, if that's most convenient.


> > Would it be ok to call Analyze() and check isSupported before creating the new
> > fontEntry?
> Well, otherwise we'd have to store the facetype in a temp and then assign it to
> the font entry member (since it we pass it directly into analyze now), but we
> could do it. I don't see a real point though, failure should be the rare case,
> and a deallocation isn't that expensive. Let me know what you think we should
> do.

I don't suppose it really matters. (In the failure case, it would avoid both the allocation and deallocation, of course, but it's still not a big deal.)

	Jonathan Kew [:jfkthame]
	Comment 135 • 15 years ago

A few more comments, then I'm away for the week so you'll have some peace! :)

> +    // We do not use this for now, store anyway
> +    DWRITE_LINE_BREAKPOINT *mBreakpoints;

I'd suggest removing this and the code that uses it, as it's not being used; or disable it with an #ifdef USE_DWRITE_BREAKPOINTS for now. That will eliminate the allocation/deallocation of the (currently redundant) mBreakpoints array, and the call to AnalyzeLineBreakpoints. Keeping the code within an #ifdef will mean we can easily bring it back if we want to use DWrite's line-breaking at some point in the future.

> +	DWRITE_FONT_SIMULATIONS sims = DWRITE_FONT_SIMULATIONS_NONE;
> +	if ((GetStyle()->style & (FONT_STYLE_ITALIC | FONT_STYLE_OBLIQUE)) &&
> +	    !fe->IsItalic()) {
> +		sims |= DWRITE_FONT_SIMULATIONS_OBLIQUE;
> +	}

Oops, more tabs.... please run a global search of some kind across the files to clean them all up.

> +gfxWindowsDWriteFont::~gfxWindowsDWriteFont()
> +{
> +    if (mFontFace) {
> +        mFontFace->Release();
> +    }
> +}

Does this need to free the Cairo fontface and scaledfont as well, or did I miss that somewhere?


> +nsString
> +gfxWindowsDWriteFont::GetUniqueName()
> +{
> +    nsString familyName = mFontEntry->FamilyName();
> +    nsString fontName = mFontEntry->Name();
> +    return familyName + NS_LITERAL_STRING(" ") + fontName;
> +}

I wonder if this should use the "fullname" rule of not appending "Regular"? Sorry, I ran out of time to look into where/how it's used....


> +void 
> +gfxWindowsDWriteFontGroup::UpdateFontList()
> +{
> +    // if user font set is set, check to see if font list needs updating
> +    if (mUserFontSet && mCurrGeneration != GetGeneration()) {
> +        // xxx - can probably improve this to detect when all fonts were found, so no need to update list
> +        mFonts.Clear();
> +        InitFontList();
> +        mCurrGeneration = GetGeneration();
> +    }
> +
> +}

I think this needs to do

+    mUnderlineOffset = UNDERLINE_OFFSET_NOT_SET;

as well, otherwise you're likely to run into bug 523717.


> +    /**
> +     * There's an internal 16-bit limit on some things inside the analyzer.
> +     * to be on the safe side here we split up any ranges over MAX_RANGE_LENGTH characters.
> +     * TODO: Figure out what exactly is going on, and what is a safe number, and why.
> +     */

Do you know what could break if, for example, the total number of glyphs generated exceeds 64K? It's possible for the number of glyphs resulting from a text run to be several times greater than the number of characters. (E.g. some Urdu fonts are examples of this.)

	Bas Schouten (:bas.schouten) Assignee
	Comment 136 • 15 years ago

(In reply to comment #135)
> A few more comments, then I'm away for the week so you'll have some peace! :)
No problem! It's great to get quick reviews so I can fix up all this font code stuff. I knew in advance this would be the tricky part :-).

I can a search and replaced all tabs btw. Or atleast I hope I got them all.

> 
> > +    /**
> > +     * There's an internal 16-bit limit on some things inside the analyzer.
> > +     * to be on the safe side here we split up any ranges over MAX_RANGE_LENGTH characters.
> > +     * TODO: Figure out what exactly is going on, and what is a safe number, and why.
> > +     */
> 
> Do you know what could break if, for example, the total number of glyphs
> generated exceeds 64K? It's possible for the number of glyphs resulting from a
> text run to be several times greater than the number of characters. (E.g. some
> Urdu fonts are examples of this.)

Nothing will truly break, but part of the text run won't be rendered.

	Bas Schouten (:bas.schouten) Assignee
	Comment 137 • 15 years ago

New DirectWrite code, also adjusted to latest changes to the font classes. Synthetic bolding for regular fonts still needs to be double checked. It probably won't work in all cases yet.

	ICHIMARU Takeshi(a.k.a. Ayakawa)
	Comment 138 • 15 years ago

font list show localized name, but still choice wrong font.

	Bas Schouten (:bas.schouten) Assignee
	Comment 139 • 15 years ago

(In reply to comment #138)
> Created an attachment (id=422183) [details]
> fon tproblem with thebes patch v10
> 
> font list show localized name, but still choice wrong font.

Hrm, odd, jdaggett any ideas? I wonder if this is the result of a bug jfkthame and I found in the old GDI code that somehow made things work. As far as I understand lookup should en-us locales so localization should -only- affect how fonts are displayed, not how they are chosen.

	ICHIMARU Takeshi(a.k.a. Ayakawa)
	Comment 140 • 15 years ago

I tried to make mod ver that replace 'en-us' to localeName in FindStyleVariations and InitFontList. Mod ver Fx select right font.

	Bas Schouten (:bas.schouten) Assignee
	Comment 141 • 15 years ago

(In reply to comment #140)
> I tried to make mod ver that replace 'en-us' to localeName in
> FindStyleVariations and InitFontList. Mod ver Fx select right font.

Correct, but technically those should be using the en-us names. Since only UI facing names should be localized from what jfkthame tells me.

	ICHIMARU Takeshi(a.k.a. Ayakawa)
	Comment 142 • 15 years ago

In gfxGDIFontList::InitFontList, this func used EnumFontFamiliesExW that return Localized font name(ex. メイリオ(en-us local name as Meiryo)), and store localized font name in fontList. Therefore GDI ver choice right font, may be.
Upper layer may need localized font names.

	Bas Schouten (:bas.schouten) Assignee
	Comment 143 • 15 years ago

(In reply to comment #142)
> In gfxGDIFontList::InitFontList, this func used EnumFontFamiliesExW that return
> Localized font name(ex. メイリオ(en-us local name as Meiryo)), and store localized
> font name in fontList. Therefore GDI ver choice right font, may be.
> Upper layer may need localized font names.
Correct, this is the font bug in GDI I already discussed with jfkthame. We just need to figure out why that bug causes correct behavior in your case :).

	Jonathan Kew [:jfkthame]
	Comment 144 • 15 years ago

(In reply to comment #143)
> (In reply to comment #142)
> > In gfxGDIFontList::InitFontList, this func used EnumFontFamiliesExW that return
> > Localized font name(ex. メイリオ(en-us local name as Meiryo)), and store localized
> > font name in fontList. Therefore GDI ver choice right font, may be.
> > Upper layer may need localized font names.
> Correct, this is the font bug in GDI I already discussed with jfkthame. We just
> need to figure out why that bug causes correct behavior in your case :).

I think it's not quite as simple as that (sorry if I gave you a wrong impression when we chatted). As I understand it, we should always be able to recognize the "canonical" (en-us) names, but we should ALSO recognize localized family names when they exist. I know we have code in the Mac implementation to support this. I'd have to re-examine the Windows GDI code to check how (or whether) it's handled there.

	John Daggett (:jtd)
	Comment 145 • 15 years ago

Bas, I'm going to do some more testing with your latest patch tomorrow but I did notice one thing while testing today.  You need to test mHasStyles in FindStyleVariations, otherwise you'll populate many times (weee!).

What's the scoop with all the Oblique and Bold Oblique faces?  Those are autogenerated by DirectWrite?  For a font with n weights it appears to be generating n oblique faces, groan.

	John Daggett (:jtd)
	Comment 146 • 15 years ago

So the reason the existing code displays the correctly localized fontname is that the default in GDI is to return the name using the default system locale.  DirectWrite always returns the English name, which as it is on OSX is used as a canonical name.

The way to fix this is:

1. Implement the GetFontTable override (gfxWindowsDWriteFontEntry::GetFontTable)
2. Implement the LocalizedName override (gfxWindowsDWriteFontFamily::LocalizedName) using the same logic as in gfxMacFontFamily::LocalizedName, you just need to change [NSFontManager localizedNameForFamily] to the DWrite equivalent.
3. Get rid of mLocalizedName in gfxWindowsDWriteFontFamily and the code associated with it.

Even for users running on CJK systems the large majority of fonts will not have localized font names.  For example, with a default Japanese Windows 7 install there are 196 font families, only 5 of them have Japanese names.  The existing code in gfxPlatformFontList is set up to handle this efficiently with the methods described above.

	John Daggett (:jtd)
	Comment 147 • 15 years ago

Also, all four of these patch files have lots of tabs in them, that's a bad thing for the thebes and widget patches, not sure about the build system one.

cat -t xxx.diff | grep '\^I'

	John Daggett (:jtd)
	Comment 148 • 15 years ago

The sample code on the docs page for IDWriteFontFamily::GetFamilyNames looks like precisely what you want for getting the family name in the default locale in gfxWindowsDWriteFontFamily::LocalizedName:

IDWriteFontFamily::GetFamilyNames Method
http://msdn.microsoft.com/en-us/library/dd371047(VS.85).aspx

	Bas Schouten (:bas.schouten) Assignee
	Comment 149 • 15 years ago

(In reply to comment #148)
> The sample code on the docs page for IDWriteFontFamily::GetFamilyNames looks
> like precisely what you want for getting the family name in the default locale
> in gfxWindowsDWriteFontFamily::LocalizedName:
> 
> IDWriteFontFamily::GetFamilyNames Method
> http://msdn.microsoft.com/en-us/library/dd371047(VS.85).aspx
This is exacylu what I currently do for mLocalizedName I believe. Only I cache it (i.e. I store it once in FindFontFamilies). I don't use ::GetUserDefaultLocaleName since that is vista and higher only. I could optionally get the function pointer here, but I chose to use Firefox's system locale.

	Bas Schouten (:bas.schouten) Assignee
	Comment 150 • 15 years ago

Update that should fix the localization font family issues.

	John Daggett (:jtd)
	Comment 151 • 15 years ago

Another thing to note here is that DirectWrite is populating font families with "fake" fonts representing the synthetic bold/italic faces.

MS PGothic (family with a single face):
(Regular) face to family (MS PGothic) with style: normal weight: 400 stretch: 0
(Oblique) face to family (MS PGothic) with style: italic weight: 400 stretch: 0
(Bold) face to family (MS PGothic) with style: normal weight: 700 stretch: 0
(Bold Oblique) face to family (MS PGothic) with style: italic weight: 700 stretch: 0

	ICHIMARU Takeshi(a.k.a. Ayakawa)
	Comment 152 • 15 years ago

Still wrong.
Font dialog with v11 patch choice wrong font. Same as v10.

	John Daggett (:jtd)
	Comment 153 • 15 years ago

+        nsString fullName(mName);
+        nsString faceNameString(faceName.Elements());
+        ToLowerCase(faceNameString);
+        if (!faceNameString.EqualsLiteral("regular") &&
+            !faceNameString.EqualsLiteral("normal")) {
+            fullName.AppendLiteral(" ");
+            fullName.Append(faceName.Elements());
+            // Put this in as the 'standard' face.
+        }

The logic in LookupLocalFont is incorrect, the fullname of the font is a separate data tag in font data from the family name and the face name.  Usually fullname = family + style but there are exceptions, the correct search is for a font for which the fullname in local() matches the fullname in the font data.  This is what the logic in gfxGDIFontList::LookupLocalFont is doing, as noted in comment 78.  I actually don't see a way of pulling out the fullname from individual faces using the DirectWrite API, it appears to only provide access to select fields in the name table.

As I originally suggested, rather than spending time in code that is critical path (FindStyleVariations will be in the startup path if the first page rendered requires font fallback), it makes more sense to use GDI API's in combination with the GDI interop object.  The attached patch is applied on top of thebes v11 and uses the CreateFontFaceFromHdc method to fish out the DirectWrite font.  It also creates a new font entry for this, because the font entry needs to use font properties defined in the @font-face rule (i.e. use the proxy entry settings).  See bug 465463.

I'm no expert on COM+ programming but it looks to me like your code is full of leaks in various places, you're not always calling Release() on objects after using them.  In some cases, this only occurs along error paths.  Getting this right is a complete pain so in my patch I set up a class AutoReleaseIUnknown to deal with auto-cleanup of these objects.  The static function CheckFullname should probably be shared with the GDI code, maybe a static method of gfxWindowsPlatform?

	Bas Schouten (:bas.schouten) Assignee
	Comment 154 • 15 years ago

(In reply to comment #153)
> Created an attachment (id=422925) [details]
> patch, use GDI interop methods for looking up fullname
> 
> +        nsString fullName(mName);
> +        nsString faceNameString(faceName.Elements());
> +        ToLowerCase(faceNameString);
> +        if (!faceNameString.EqualsLiteral("regular") &&
> +            !faceNameString.EqualsLiteral("normal")) {
> +            fullName.AppendLiteral(" ");
> +            fullName.Append(faceName.Elements());
> +            // Put this in as the 'standard' face.
> +        }
> 
> The logic in LookupLocalFont is incorrect, the fullname of the font is a
> separate data tag in font data from the family name and the face name.  Usually
> fullname = family + style but there are exceptions, the correct search is for a
> font for which the fullname in local() matches the fullname in the font data. 
> This is what the logic in gfxGDIFontList::LookupLocalFont is doing, as noted in
> comment 78.  I actually don't see a way of pulling out the fullname from
> individual faces using the DirectWrite API, it appears to only provide access
> to select fields in the name table.
> 
> As I originally suggested, rather than spending time in code that is critical
> path (FindStyleVariations will be in the startup path if the first page
> rendered requires font fallback), it makes more sense to use GDI API's in
> combination with the GDI interop object.  The attached patch is applied on top
> of thebes v11 and uses the CreateFontFaceFromHdc method to fish out the
> DirectWrite font.  It also creates a new font entry for this, because the font
> entry needs to use font properties defined in the @font-face rule (i.e. use the
> proxy entry settings).  See bug 465463.
> 
> I'm no expert on COM+ programming but it looks to me like your code is full of
> leaks in various places, you're not always calling Release() on objects after
> using them.  In some cases, this only occurs along error paths.  Getting this
> right is a complete pain so in my patch I set up a class AutoReleaseIUnknown to
> deal with auto-cleanup of these objects.  The static function CheckFullname
> should probably be shared with the GDI code, maybe a static method of
> gfxWindowsPlatform?
Personally, I still think this is a -very- bad idea, if we could do it with the interop using the LOGFONT it would be alright (but it doesn't seem to work). But this way we let GDI load the font completely. This means we bypass DirectWrite's cross-platform font cache, and we load every font used in this way in GDI. I think performance wise this is a very bad idea. I wanted to do some measurements on its performance but it pretty much doesn't work on my machine (GetFontData in CheckFullName returns GDI_ERROR on many occasions). 

If the current name is incorrect, I think we should look for a way of fixing this, rather than doing this. Perhaps we could use TryGetFontTable to get the canonical name out of DirectWrite, and use those to populate the hash table. For creating the font entry we should then make the hashtable point to the IDWriteFont rather than the gfxFontEntry, we could then create a new gfxFontEntry surrounding that.

On the COM part/stack based helper, technically we should use the MS CComPtr as the stack based helper to do these things. If we do want to use a stack based helper that is. This also allows all passing byref and such to go right. I'm not sure if we're allowed to pull parts of the ATL into Mozilla code though? If we can't a stack based helper such as AutoReleaseIUnknown would be an option. But we'd probably need to expand it a bit (i.e. it needs to 'forget' a pointer if it's kept around as a class member, or something like that).

	Bas Schouten (:bas.schouten) Assignee
	Comment 155 • 15 years ago

(In reply to comment #153)
> I'm no expert on COM+ programming but it looks to me like your code is full of
> leaks in various places, you're not always calling Release() on objects after
> using them.  In some cases, this only occurs along error paths.
Hrm, I've only been able to find three cases(indeed in error paths). Where else do you see a lot of issues? Not that I'm against using a stack based helper where possible.

	Bas Schouten (:bas.schouten) Assignee
	Comment 156 • 15 years ago

Correct license headers.

	Jeff Muizelaar [:jrmuizel]
	Comment 157 • 15 years ago

I've landed the build changes and the cairo backend:
http://hg.mozilla.org/mozilla-central/rev/95b94b4206fb
http://hg.mozilla.org/mozilla-central/rev/6ca33f430fca

	Jim Mathies [:jimm]
	Comment 158 • 15 years ago

Comment on attachment 421221 [details] [diff] [review]
Widget Direct2D Code v4

Sorry it took so long, especially considering how small this was. One question though, why did you remove that big comment block for ScrollWindowEx?

	WulfTheSaxon [:Wulf]
	Comment 159 • 15 years ago

Has anybody considered reaching out to Microsoft regarding the missing features in DWrite vs GDI?

	John Daggett (:jtd)
	Comment 160 • 15 years ago

I'm seeing asserts and warnings when loading the latest HTML5 spec (my proxy for "big honkin web doc"):

http://dev.w3.org/html5/spec/Overview.html

Asserts/warnings:
0[78b770]: WARNING: Could not get glyph extents (no aContext): file c:/builds/mozcentral/gfx/thebes/src/gfxFont.cpp, line 1250
0[78b770]: ###!!! ASSERTION: illegal height for combined area: 'aCombinedArea.height >= 0', file c:/builds/mozcentral/layout/generic/nsLineBox.cpp, line 515

Lots of the first warning, two of the asserts.  Probably something metrics-related.

	John Daggett (:jtd)
	Comment 161 • 15 years ago

This patch fixes local lookup by using GDI (comment 153) and also fixes the GetFontTable method of gfxDWriteFontList.  I don't think the original version of the GetFontTable method was tested very much, apparently the DirectWrite TryGetFontTable method takes a table tag in reverse endian order compared to GDI's GetFontTable so it was always failing.  Also, the routine was leaking font face objects which caused a failure later on while trying to read in cmap's. With this patch, applied on top of the Thebes v11 patch, the font names for Japanese display correctly.

	John Daggett (:jtd)
	Comment 162 • 15 years ago

(In reply to comment #154)

> Personally, I still think this is a -very- bad idea, if we could do it with the
> interop using the LOGFONT it would be alright (but it doesn't seem to work).
> But this way we let GDI load the font completely. This means we bypass
> DirectWrite's cross-platform font cache, and we load every font used in this
> way in GDI. I think performance wise this is a very bad idea. I wanted to do
> some measurements on its performance but it pretty much doesn't work on my
> machine (GetFontData in CheckFullName returns GDI_ERROR on many occasions). 

Just to be clear, LookupLocalFont is *only* used for looking up local fonts:

body { font-family: Arial, Helevetica, sans-serif; } /* A */

@font-face {
  font-family: Test;
  src: local(Test), url(test.ttf); /* B */
}

@font-face {
  font-family: Test;
  src: local(Test Bold), url(test-bold.ttf); /* B */
}

p { font-family: Test; }

In the style rules above, LookupLocalFont would only be used for cases B, not for general font family lookup (case A).  As such, it's definitely not the common path case by any stretch and only occurs once when the @font-face font is resolved.  Any performance improvement gained by maintaining a fullname hash table has to be weighed against the additional code and memory needed to maintain this.  The code in my patch is basically reusing existing code and assures consistency with the GDI rendering path.  I don't see any need for a hash table unless we can prove that there's a significant performance advantage to maintaining our own hash of font fullnames.

Are you certain that Windows is maintaining a per-app GDI font cache in such a way that using GDI font calls increases our memory footprint?  Have you seen this in practice?

	John Daggett (:jtd)
	Comment 163 • 15 years ago

(In reply to comment #155)
> (In reply to comment #153)
> > I'm no expert on COM+ programming but it looks to me like your code is full of
> > leaks in various places, you're not always calling Release() on objects after
> > using them.  In some cases, this only occurs along error paths.
> Hrm, I've only been able to find three cases(indeed in error paths). Where else
> do you see a lot of issues? Not that I'm against using a stack based helper
> where possible.

Here's what I found, although I easily could have missed one ;)

+void
+gfxWindowsDWriteFontFamily::FindStyleVariations()
+{

+        UINT32 length;
+        hr = names->GetStringLength(englishIdx, &length);
+        if (FAILED(hr)) {
+            font->Release();
+            continue;
+        }

Leaks IDWriteLocalizedStrings.

+nsresult
+gfxWindowsDWriteFontEntry::GetFontTable(PRUint32 aTableTag, nsTArray<PRUint8> &aBuffer)

Leaks IDWriteFontFace (both normal and error paths).

+nsresult
+gfxWindowsDWriteFontEntry::ReadCMAP()
+{

+    hr = fontFace->TryGetFontTable(DWRITE_MAKE_OPENTYPE_TAG('c', 'm', 'a', 'p'),
+                                   (const void**)&tableData,
+                                   &len,
+                                   &tableContext,
+                                   &exists);
+    if (FAILED(hr)) {
+        return NS_ERROR_FAILURE;
+    }

Leaks IDWriteFontFace.

+void
+gfxDWriteFontList::InitFontList()
+{

Leaks IDWriteFontCollection (system font collection), all paths.

I think a simple stack-based cleanup class works to simplify code like this.  It's not a silver bullet but it avoids problems as others touch the code later on.

Also, is the code below really kosher?

+class gfxDWriteFontFileLoader : public IDWriteFontFileLoader
+{
+public:
+
+    IFACEMETHOD_(ULONG, AddRef)()
+    {
+        return 1;
+    }
+
+    IFACEMETHOD_(ULONG, Release)()
+    {
+        return 1;
+    }

Again, I'm no COM+ whiz but this makes be nervous...

	John Daggett (:jtd)
	Comment 164 • 15 years ago

Tryserver build, Thebes v11 + fixup patch
http://tinyurl.com/directd2dtry

I ran some startup timing tests to compare the existing GDI code to the DirectWrite code, using the method described in bug 519445, comment 25.  It basically times the startup + first page load for saved pages from a number of sites.  I used the same build, toggling the 'font.engine.forcegdi' boolean to toggle the two codepaths.

The startup time seems to have improved but in many cases the first page load time is longer.  The sites below are significantly worse with DirectWrite enabled.  I'm guessing system font fallback is occurring and the problem lies along that codepath.

The three numbers are:
  startup (time until onload fires)
  page load (time for an iframe page to load)
  total (sum of the two) 

wikipedia.org dwrite 481 1006 1487
wikipedia.org dwrite 496 1011 1507
wikipedia.org dwrite 497 1009 1506
wikipedia.org dwrite 499 1010 1509
wikipedia.org dwrite 531 980 1511
wikipedia.org dwrite 534 974 1508
wikipedia.org gdi 538 391 929
wikipedia.org gdi 540 414 954
wikipedia.org gdi 542 394 936
wikipedia.org gdi 550 389 939
wikipedia.org gdi 555 415 970
wikipedia.org gdi 556 413 969
www.yahoo.co.jp dwrite 480 938 1418
www.yahoo.co.jp dwrite 487 1042 1529
www.yahoo.co.jp dwrite 491 951 1442
www.yahoo.co.jp dwrite 492 957 1449
www.yahoo.co.jp dwrite 493 928 1421
www.yahoo.co.jp dwrite 496 947 1443
www.yahoo.co.jp gdi 528 277 805
www.yahoo.co.jp gdi 531 278 809
www.yahoo.co.jp gdi 538 276 814
www.yahoo.co.jp gdi 538 278 816
www.yahoo.co.jp gdi 542 275 817
www.yahoo.co.jp gdi 550 281 831
www.yahoo.com dwrite 485 1198 1683
www.yahoo.com dwrite 488 1199 1687
www.yahoo.com dwrite 489 1204 1693
www.yahoo.com dwrite 494 1190 1684
www.yahoo.com dwrite 494 1214 1708
www.yahoo.com dwrite 501 1222 1723
www.yahoo.com gdi 530 483 1013
www.yahoo.com gdi 537 485 1022
www.yahoo.com gdi 537 488 1025
www.yahoo.com gdi 539 525 1064
www.yahoo.com gdi 547 529 1076
www.yahoo.com gdi 552 530 1082

	John Daggett (:jtd)
	Comment 165 • 15 years ago

(In reply to comment #159)
> Has anybody considered reaching out to Microsoft regarding the missing features
> in DWrite vs GDI?

I pinged them today about this, we'll see what the response is.

	Radek 'sysKin' Czyz
	Comment 166 • 15 years ago

(In reply to comment #164)
> http://tinyurl.com/directd2dtry

Tried it out of curiosity and I'm wondering if he following is known:

1. Fonts look different, as if they were softer/thinner/antialiased more
2. Autoscroll indicator is invisible
3. I am able to break entire browser on one website by zooming in enough - program seems to continue working fine but everything stops rendering (in all windows)
4. Some images (dynamically-added with non-1:1 scaling) are mostly black

For the last two I can easily confirm with new profile and make testcases, if needed.

	John Daggett (:jtd)
	Comment 167 • 15 years ago

Looks like the thebes code also needs to be tweaked to assure that WinCE/WinMobile builds work:

e:/builds/moz2_slave/winmo-hg/build/gfx/thebes/src/gfxWindowsPlatform.cpp(147) : error C2653: 'DWriteFactory' : is not a class or namespace name
e:/builds/moz2_slave/winmo-hg/build/gfx/thebes/src/gfxWindowsPlatform.cpp(147) : error C3861: 'Instance': identifier not found
e:/builds/moz2_slave/winmo-hg/build/gfx/thebes/src/gfxWindowsPlatform.cpp(150) : error C2653: 'DWriteFactory' : is not a class or namespace name
e:/builds/moz2_slave/winmo-hg/build/gfx/thebes/src/gfxWindowsPlatform.cpp(150) : error C2227: left of '->Release' must point to class/struct/union/generic type
        type is ''unknown-type''
e:/builds/moz2_slave/winmo-hg/build/gfx/thebes/src/gfxWindowsPlatform.cpp(150) : error C3861: 'Instance': identifier not found

	Bas Schouten (:bas.schouten) Assignee
	Comment 168 • 15 years ago

(In reply to comment #162)
> (In reply to comment #154)
> 
> > Personally, I still think this is a -very- bad idea, if we could do it with the
> > interop using the LOGFONT it would be alright (but it doesn't seem to work).
> > But this way we let GDI load the font completely. This means we bypass
> > DirectWrite's cross-platform font cache, and we load every font used in this
> > way in GDI. I think performance wise this is a very bad idea. I wanted to do
> > some measurements on its performance but it pretty much doesn't work on my
> > machine (GetFontData in CheckFullName returns GDI_ERROR on many occasions). 
> 
> Just to be clear, LookupLocalFont is *only* used for looking up local fonts:
> 
> body { font-family: Arial, Helevetica, sans-serif; } /* A */
> 
> @font-face {
>   font-family: Test;
>   src: local(Test), url(test.ttf); /* B */
> }
> 
> @font-face {
>   font-family: Test;
>   src: local(Test Bold), url(test-bold.ttf); /* B */
> }
> 
> p { font-family: Test; }
> 
> In the style rules above, LookupLocalFont would only be used for cases B, not
> for general font family lookup (case A).  As such, it's definitely not the
> common path case by any stretch and only occurs once when the @font-face font
> is resolved.  Any performance improvement gained by maintaining a fullname hash
> table has to be weighed against the additional code and memory needed to
> maintain this.  The code in my patch is basically reusing existing code and
> assures consistency with the GDI rendering path.  I don't see any need for a
> hash table unless we can prove that there's a significant performance advantage
> to maintaining our own hash of font fullnames.
> 
> Are you certain that Windows is maintaining a per-app GDI font cache in such a
> way that using GDI font calls increases our memory footprint?  Have you seen
> this in practice?
Well, I want to test this and analyze it, but as I said, the code isn't working for me? Any ideas?

	Bas Schouten (:bas.schouten) Assignee
	Comment 169 • 15 years ago

(In reply to comment #166)
> (In reply to comment #164)
> > http://tinyurl.com/directd2dtry
> 
> Tried it out of curiosity and I'm wondering if he following is known:
> 
> 1. Fonts look different, as if they were softer/thinner/antialiased more
This is because of directwrite.

> 2. Autoscroll indicator is invisible
Hrm, I can reproduce this, odd.

> 3. I am able to break entire browser on one website by zooming in enough -
> program seems to continue working fine but everything stops rendering (in all
> windows)
> 4. Some images (dynamically-added with non-1:1 scaling) are mostly black
> 
> For the last two I can easily confirm with new profile and make testcases, if
> needed.
That would be really nice :-).

	Bas Schouten (:bas.schouten) Assignee
	Comment 170 • 15 years ago

(In reply to comment #164)
> Created an attachment (id=423315) [details]
> startup timings, directwrite vs. gdi
> 
> Tryserver build, Thebes v11 + fixup patch
> http://tinyurl.com/directd2dtry
> 
> I ran some startup timing tests to compare the existing GDI code to the
> DirectWrite code, using the method described in bug 519445, comment 25.  It
> basically times the startup + first page load for saved pages from a number of
> sites.  I used the same build, toggling the 'font.engine.forcegdi' boolean to
> toggle the two codepaths.
> 
> The startup time seems to have improved but in many cases the first page load
> time is longer.  The sites below are significantly worse with DirectWrite
> enabled.  I'm guessing system font fallback is occurring and the problem lies
> along that codepath.

Although this could be the case, there's two other things to consider: Direct2D could be slower for some reason or another. The entire page rendering would be different. The second option is that DirectWrite simply does significantly more than GDI, sub-pixel positioning and vertical anti-aliasing would cause an increase in work.

	Bas Schouten (:bas.schouten) Assignee
	Comment 171 • 15 years ago

(In reply to comment #163)
> (In reply to comment #155)
> > (In reply to comment #153)
> Here's what I found, although I easily could have missed one ;)
> 
> +void
> +gfxWindowsDWriteFontFamily::FindStyleVariations()
> +{
> 
> +        UINT32 length;
> +        hr = names->GetStringLength(englishIdx, &length);
> +        if (FAILED(hr)) {
> +            font->Release();
> +            continue;
> +        }
> 
> Leaks IDWriteLocalizedStrings.
> 
> +nsresult
> +gfxWindowsDWriteFontEntry::GetFontTable(PRUint32 aTableTag, nsTArray<PRUint8>
> &aBuffer)
> 
> Leaks IDWriteFontFace (both normal and error paths).
> 
> 
> +void
> +gfxDWriteFontList::InitFontList()
> +{
> 
> Leaks IDWriteFontCollection (system font collection), all paths.
> 
Ah, I found a couple of others, yes, but I missed these two, thanks!

> I think a simple stack-based cleanup class works to simplify code like this. 
> It's not a silver bullet but it avoids problems as others touch the code later
> on.
> 
Yes, I agree. Although now that I think about it, doesn't one of Mozilla's auto pointers also work on IUnknowns on windows? I seem to recall seeing code for this.

> Also, is the code below really kosher?
> 
> +class gfxDWriteFontFileLoader : public IDWriteFontFileLoader
> +{
> +public:
> +
> +    IFACEMETHOD_(ULONG, AddRef)()
> +    {
> +        return 1;
> +    }
> +
> +    IFACEMETHOD_(ULONG, Release)()
> +    {
> +        return 1;
> +    }
> 
> Again, I'm no COM+ whiz but this makes be nervous...
This should be fine. A problem could occur if someone tries to release until it returns 0, but that's very bad practice anyway, and I've only ever done that when debugging :-). Other than that the classes comes and goes on the stack, I know of several MS examples that do this too. Perhaps Robarnold can confirm, as he's more knowledgeable on COM than I am.

	Radek 'sysKin' Czyz
	Comment 172 • 15 years ago

(In reply to comment #169)
> That would be really nice :-).

I tried to minimise the "stops rendering" testcase and got a crashing testcase instead. This website consists of just one character - long dash. I'm guessing I could just paste it to a comment but I won't be evil :)

Black-images testcase next.

	Jonathan Kew [:jfkthame]
	Comment 173 • 15 years ago

(In reply to comment #172)
> I tried to minimise the "stops rendering" testcase and got a crashing testcase
> instead. This website consists of just one character - long dash. I'm guessing
> I could just paste it to a comment but I won't be evil :)

Although this page claims to be UTF-8, it appears that it is in fact encoded as Latin-1 (ISO 8859-1 or Windows codepage 1252). The intended "long dash", when interpreted as UTF-8, is therefore an encoding error and should be replaced with U+FFFD.

So I'd guess the failure is caused by the presence of U+FFFD in the text to be rendered. Of course, this shouldn't cause a crash, but it is likely to trigger system font fallback (at least), and may not be available in any font (in which case it should fall back to hexbox rendering). Perhaps one of these paths is broken.

	Jonathan Kew [:jfkthame]
	Comment 174 • 15 years ago

(In reply to comment #151)
> Another thing to note here is that DirectWrite is populating font families with
> "fake" fonts representing the synthetic bold/italic faces.
> 
> MS PGothic (family with a single face):
> (Regular) face to family (MS PGothic) with style: normal weight: 400 stretch: 0
> (Oblique) face to family (MS PGothic) with style: italic weight: 400 stretch: 0
> (Bold) face to family (MS PGothic) with style: normal weight: 700 stretch: 0
> (Bold Oblique) face to family (MS PGothic) with style: italic weight: 700
> stretch: 0

If it's at all feasible, I'd prefer that we don't create such "fake font entries"; the font family should contain only the real faces that are actually available, and any synthetic styling needed should be handled at the point where an actual Cairo font is instantiated from one of the available faces.

Creating fake font entries causes us to waste time and space reading the same cmap multiple times. This is particularly unfortunate considering that many of the fonts that lack a "full" set of (at least) the 4 standard styles are large-repertoire CJK fonts.

	Radek 'sysKin' Czyz
	Comment 175 • 15 years ago

dynamically adds images. They don't decode well, not incrementally.
Uncomment the width change to make their b0rk black, otherwise it's transparent.
(hopefully adding http images works from https?)

	Bas Schouten (:bas.schouten) Assignee
	Comment 176 • 15 years ago

(In reply to comment #175)
> Created an attachment (id=423338) [details]
> image adding b0rk
> 
> dynamically adds images. They don't decode well, not incrementally.
> Uncomment the width change to make their b0rk black, otherwise it's
> transparent.
> (hopefully adding http images works from https?)

This is due to bug 534787. I have a fix for this on my tree, and a proper fix will be landed at some point.

	Shawn Wilsher :sdwilsh
	Comment 177 • 15 years ago

(In reply to comment #171)
> This should be fine. A problem could occur if someone tries to release until it
> returns 0, but that's very bad practice anyway, and I've only ever done that
> when debugging :-). Other than that the classes comes and goes on the stack, I
> know of several MS examples that do this too. Perhaps Robarnold can confirm, as
> he's more knowledgeable on COM than I am.
You can use nsRefPtr with COM objects.

	Bas Schouten (:bas.schouten) Assignee
	Comment 178 • 15 years ago

Fix the crash on invalid characters. Also fix windows mobile build, and use nsRefPtr in main thebes code.

	Bas Schouten (:bas.schouten) Assignee
	Comment 179 • 15 years ago

New patch for thebes. This uses the new code on trunk for fullname lookup, and it also fixed a bunch of glyph cluster issues. This now passes all the ref tests that I've been able to find for fonts. Except for a couple of mixed-charset bidi issues (where the non-mixed version used kerning, but the mixed-charset version does not, according to jfkthame on XP neither used kerning, which is why the reftests passed. On my windows 7 machine, they also fail when using GDI.)

	John Daggett (:jtd)
	Comment 180 • 15 years ago

Comparison of Calluna (OpenType PS font) rendered with DWrite (v13 patch) and with GDI, rendered at 8x magnification.  Best viewed sitting several meters away from your monitor.

http://people.mozilla.org/~jdaggett/tests/subpixelspacing.html

Note how the stem of the initial T slides from one pixel to the next in the DWrite case.  With GDI is "jumps" at 0.5 pixels of shift.  Larger sizes of TrueType fonts also display without the jaggies present in the GDI case (yay!).

	John Daggett (:jtd)
	Comment 181 • 15 years ago

(In reply to comment #179)
> Created an attachment (id=424484) [details]
> Thebes DirectWrite and Direct2D Code v13

General cleanup:

Several of the files in this patch still contain tab characters.  Those need to be cleaned out.  Also, there are lots of places where the 80-character per line rule is ignored.  We already have gfx code that ignores this in different places but that's a bug, we shouldn't be creating new code that does this.

+++ b/gfx/thebes/public/gfxWindowsDWriteFonts.h
+++ b/gfx/thebes/src/gfxWindowsDWriteFonts.cpp
+++ b/gfx/thebes/public/gfxWindowsGDIFonts.h
+++ b/gfx/thebes/src/gfxWindowsGDIFonts.cpp

As Jonathan noted in comment 112, I think it would be *much* simpler to skip changing gfxWindowsFonts to gfxWindowsGDIFonts and trim the 'Windows' from the DWrite files.  This code will go through another thrash as part of Jonathan's harfbuzz work, we can figure out the right name changes at that point.

+++ b/gfx/thebes/src/gfxDWriteCommon.h
+++ b/gfx/thebes/src/gfxDWriteCustomFonts.cpp
+++ b/gfx/thebes/src/gfxDWriteCustomFonts.h

I don't see the need for these to be separate files, I think it would be better to keep them all in gfxDWriteFonts.h/cpp.

+class DWriteFactory
+{
+public:
+    static IDWriteFactory *Instance()

So this class has a single static method and a couple statics associated with it?  Seems like this belongs in gfxWindowsPlatform instead.

+    static IDWriteFactory *mInstance;

Use nsRefPtr<IDWriteFactory> instead and skip the release.

+                rv = prefSvc->GetBranch("font.engine.", getter_AddRefs(prefBranch));
+                if (NS_SUCCEEDED(rv)) {
+                    rv = prefBranch->GetBoolPref("forcegdi", &forceGDI);
+                    if (NS_FAILED(rv)) {
+                        forceGDI = PR_FALSE;
+                    }

Rather than 'font.engine.forcegdi', use 'gfx.font_rendering.directwrite.enabled' and add an entry to modules/libpref/src/init/all.js in the Windows-only section.  We'll need to add other conditionals like this for forcing Cleartype-rendering on XP, I'd like to keep those grouped together.  For initial checkin we should leave this set to 'false' and we can flip it to 'true' once it's had a bit of time to get banged on.

+IDWriteFontFileLoader* gfxDWriteFontFileLoader::mInstance = NULL;

Use nsRefPtr<IDWriteFontFileLoader>.

+        gfxWindowsDWriteFontEntry *fe = 
+            new gfxWindowsDWriteFontEntry(fullName, font);
+        fe->mItalic = (font->GetStyle() == DWRITE_FONT_STYLE_ITALIC ||
+                      font->GetStyle() == DWRITE_FONT_STYLE_OBLIQUE);
+        fe->mStretch = FontStretchFromDWriteStretch(font->GetStretch());
+        PRUint16 weight = PR_ROUNDUP(font->GetWeight() - 50, 100);
+        	
+        weight = NS_MAX<PRUint16>(100, weight);
+        weight = NS_MIN<PRUint16>(900, weight);
+	
+        fe->mWeight = weight;

Code like this appears several times.  Make the weight, style, stretch part of the constructor.

+gfxDWriteFontFileStream::gfxDWriteFontFileStream(PRUint8 *aData, PRUint32 aLength)
+{
+    mData = new PRUint8[aLength];
+    mDataSize = aLength;
+    memcpy(mData, aData, aLength);
+}

Why is this copy needed?  Seems like could rework the code in MakePlatformFont to avoid this copy.

+    gfxWindowsDWriteFontEntry(const nsAString& aFaceName,
+                              IDWriteFont *aFont, 
+                              IDWriteFontFile *aFontFile = NULL) 
+      : gfxFontEntry(aFaceName), mFont(aFont), mFontFile(aFontFile)
+    {
+    }

Make two constructors, one with a IDWriteFont, the other with a IDWriteFontFile.

+void
+gfxWindowsDWriteFontFamily::FindStyleVariations()

There's a lot of code here to get the fullname for each face, I don't think that's needed at all, this is only used by the font cache and never for OS lookups (correct me here if I missed something).  Seems like just mName + i would work just as well for example.

+        if (exists) {
+            UINT32 eng;
+            unspec->FindLocaleName(L"en-us", &eng, &exists);
+            WCHAR test[256];
+            if (exists) {
+                unspec->GetString(eng, test, 256);
+            }
+            int a = 1;
+        }

Delete debugging (?) code.

+    /**
+     * Second pass, for all oblique entries we search for an identical italic
+     * entry. If one exists we remove the oblique entry from the available
+     * fonts list, since gfxFontEntry can't make the distinction.
+     *
+     * TODO: Current algorithm is not the most efficient but it does the job
+     * with little code complexity. Need to consider if something more advanced
+     * is required.
+     */
+    for (unsigned int i = 0; i < obliqueEntries.Length(); i++) {
+        for (unsigned int c = 0; c < mAvailableFonts.Length(); c++) {
+            if (mAvailableFonts[c].get() != obliqueEntries[i] &&
+                mAvailableFonts[c]->mWeight == obliqueEntries[i]->mWeight &&
+                mAvailableFonts[c]->mItalic == PR_TRUE &&
+                mAvailableFonts[c]->mStretch == obliqueEntries[i]->mStretch) {
+                    mAvailableFonts.RemoveElement(obliqueEntries[i]);
+                    break;
+            }
+        }
+    }

Ack.  I think we can avoid this by simply skipping faces that have a "simulation" (i.e. fake bold/oblique) set based on font->GetSimulations().  My guess is that Windows 7 does not ship with any "real" oblique fonts.

+PRBool
+gfxWindowsDWriteFontEntry::SupportsLangGroup(const nsACString &aLangGroup) const
+{
+    // TODO: Implement me.
+    return PR_TRUE;
+}
+
+PRBool
+gfxWindowsDWriteFontEntry::MatchesGenericFamily(const nsACString &aGeneric) const
+{
+    // TODO: Implement me.
+    return PR_TRUE;
+}
+
+PRBool
+gfxWindowsDWriteFontEntry::SupportsRange(PRUint8 aRangeBit)
+{
+    // TODO: Implement me!!
+    return PR_TRUE;
+}

Implementation needed.

+    if (mFont) {
+        hr = mFont->CreateFontFace(getter_AddRefs(fontFace));
+    } else if (mFontFile) {
+        IDWriteFontFile *fontFile = mFontFile.get();
+        hr = DWriteFactory::Instance()->CreateFontFace(
+            mFaceType,
+            1,
+            &fontFile,
+            0,
+            DWRITE_FONT_SIMULATIONS_NONE,
+            getter_AddRefs(fontFace));
+    }

Create a private helper method 'GetFontFace' so that you're not repeating code in multiple places.

+gfxFontEntry *
+gfxDWriteFontList::GetDefaultFont(const gfxFontStyle *aStyle,
+                                  PRBool &aNeedsBold)
+{
+    HRESULT hr;
+    nsRefPtr<IDWriteFontCollection> systemFonts;
+    hr = DWriteFactory::Instance()->GetSystemFontCollection(getter_AddRefs(systemFonts));
+    NS_ASSERTION(SUCCEEDED(hr), "GetSystemFontCollection failed!");
+    UINT32 idx = 0;
+    BOOL exists;
+    hr = systemFonts->FindFamilyName(L"tahoma", &idx, &exists);

This code needs to match the logic in gfxGDIFontList::GetDefaultFont, the default font varies based on the OS locale, just setting this to Tahoma is wrong.

+class gfxWindowsDWriteFontFamily : public gfxFontFamily
+{
+public:
+    /**
+     * Constructs a new DWriteFont Family.
+     *
+     * \param aName Name identifying the family
+     * \param aFamily IDWriteFontFamily object representing the directwrite
+     * family object.
+     */
+    gfxWindowsDWriteFontFamily(const nsAString& aName, 
+                               IDWriteFontFamily *aFamily,
+                               const nsAString& aLocalName) :
+        gfxFontFamily(aName), mDWFamily(aFamily),
+        mLocalizedName(aLocalName) { }
+    virtual ~gfxWindowsDWriteFontFamily();
+    
+    virtual void FindStyleVariations();
+
+    virtual void LocalizedName(nsAString& aLocalizedName); 
+protected:
+    /** This font family's directwrite fontfamily object */
+    nsRefPtr<IDWriteFontFamily> mDWFamily;
+    nsString mLocalizedName;
+};

We don't need to store the localized family, this is *only* used when the user opens the content prefs panel.  Better to look this up within LocalizedName() rather than spend time in gfxDWriteFontList::InitFontList which affects startup time.

+void
+gfxPlatformFontList::EnsureStylesLoaded()
+{
+    // Prevent freezing the main thread if mIncrement somehow became 0.
+    if (mStartIndex < mNumFamilies && mIncrement > 0) {
+        while (!RunLoader()) {}
+        CancelLoader();
+    }
+}
+

This isn't called anywhere, trim.

Jeff or Joe should probably review the gfxD2DSurface code.

	Jonathan Kew [:jfkthame]
	Comment 182 • 15 years ago

Comment on attachment 424484 [details] [diff] [review]
Thebes DirectWrite and Direct2D Code v13

Please grep your files/patches for TAB characters (again), as it looks like a few have crept back in during editing.

> +        nsRefPtr<IDWriteLocalizedStrings> unspec;
> +        BOOL exists;
> +        hr = font->GetInformationalStrings(DWRITE_INFORMATIONAL_STRING_WIN32_SUBFAMILY_NAMES,
> +                                           getter_AddRefs(unspec),
> +                                           &exists);
> +        if (exists) {
> +            UINT32 eng;
> +            unspec->FindLocaleName(L"en-us", &eng, &exists);
> +            WCHAR test[256];
> +            if (exists) {
> +                unspec->GetString(eng, test, 256);
> +            }
> +            int a = 1;
> +        }

This looks like leftover debugging/investigative code!

> +        nsString fullName(mName);
> +        nsString faceNameString(faceName.Elements());
> +        ToLowerCase(faceNameString);
> +        if (!faceNameString.EqualsLiteral("regular") &&
> +            !faceNameString.EqualsLiteral("normal")) {

I don't think "normal" should be excluded here, the "rule" (or rather recommendation) only applies to the specific name "regular"...

> +            fullName.AppendLiteral(" ");
> +            fullName.Append(faceName.Elements());
> +            // Otherwise put this is the 'standard' face.
> +        }

...but do we still need to do this at all, with the new fullname-reading code available? I don't like the idea that we could end up getting two different versions of the "full name" for a face, one by concatenating family + style (here) and another later by reading the actual name table.


> +    PRUint8 *tableData;
> +    PRUint32 len;
> +    void *tableContext = NULL;
> +    BOOL exists;
> +    hr = fontFace->TryGetFontTable(NS_SWAP32(aTableTag),
> +                                   (const void**)&tableData,
> +                                   &len,
> +                                   &tableContext,
> +                                   &exists);
> +
> +    if (FAILED(hr) || !exists) {
> +        return NS_ERROR_FAILURE;
> +    }
> +    aBuffer.SetLength(len);

nsTArray<T>::SetLength returns a boolean indicating success/failure; should check it (here and elsewhere), and bail with NS_ERROR_OUT_OF_MEMORY if it fails.


> +    if (exists) {
> +        rv = gfxFontUtils::ReadCMAP(tableData,
> +                                    len,
> +                                    mCharacterMap,
> +                                    isUnicode,
> +                                    isSymbol);
> +    }
> +
> +    if (tableContext) {
> +        fontFace->ReleaseFontTable(tableContext);
> +    }
> +
> +    mCmapInitialized = PR_TRUE;
> +    return NS_OK;

Shouldn't this return rv (the result from gfxFontUtils::ReadCMAP), rather than just NS_OK? Although the table data was found, interpreting it may have failed.


> +        famName.SetLength(length + 1);

> +        famLocalName.SetLength(length + 1);

Check for failures.


> +/**
> + * gfxWindowsDWFontFamily is a class that describes one of the fonts on the
> + * users system.  It holds each gfxWindowsDWFontEntry (maps more directly to
> + * a font face) which holds font type, charset info and character map info.
> + */

Class names in the comment are out of date - the actual code has "DWrite", not "DW".


> +    void IndexFullNames();

This is obsolete, I believe.


> +void TextAnalysis::SplitCurrentRun(UINT32 splitPosition)
> +{
> +    if (!mCurrentRun) {
> +        // Shouldn't be calling this when no current run is set!

In that case, I'd make it an assertion rather than just a comment. If someone breaks the logic, we should tell them.


> +gfxWindowsDWriteFont::~gfxWindowsDWriteFont()
> +{
> +    if (mCairoFontFace) {
> +        cairo_font_face_destroy(mCairoFontFace);
> +    }
> +    if (mCairoScaledFont) {
> +        cairo_scaled_font_destroy(mCairoScaledFont);
> +    }
> +}

Looks like this will leak the font's mMetrics, I think.


> +    UINT32 ucs = L'0';
> +    mFontFace->GetGlyphIndicesA(&ucs, 1, &glyph);
> +    mFontFace->GetDesignGlyphMetrics(&glyph, 1, &metrics);

What happens if there's no "0" character in the font? Will GetGlyphIndicesA fail? Should be checking the result code.

> +    mMetrics->zeroOrAveCharWidth = ((gfxFloat)metrics.advanceWidth / fontMetrics.designUnitsPerEm) * mStyle.size;
> +    ucs = L'x';
> +    mFontFace->GetGlyphIndicesA(&ucs, 1, &glyph);
> +    mFontFace->GetDesignGlyphMetrics(&glyph, 1, &metrics);    

Again, what if there's no "x"? Also, trailing whitespace here.


> +cairo_font_face_t *
> +gfxWindowsDWriteFont::CairoFontFace()
> +{
> +    if (!mCairoFontFace) {
> +#ifdef CAIRO_HAS_DWRITE_FONT
> +        mCairoFontFace = 
> +            cairo_dwrite_font_face_create_for_dwrite_fontface(
> +            ((gfxWindowsDWriteFontEntry*)mFontEntry.get())->mFont, mFontFace);
> +#endif
> +    }
> +    return mCairoFontFace;
> +}

What would happen if the #ifdef here is false? Seems like that could be a problem - should there be a configure-time dependency or something so that we don't get into that situation?


> +        UINT32 maxGlyphs = 3 * range.Length() / 2 + 16;
> +        nsAutoTArray<UINT16, 400> clusters;
> +        nsAutoTArray<UINT16, 400> indices;
> +        clusters.SetLength(range.Length());
> +        indices.SetLength(maxGlyphs);
> +        nsAutoTArray<DWRITE_SHAPING_TEXT_PROPERTIES, 400> textProperties;
> +        nsAutoTArray<DWRITE_SHAPING_GLYPH_PROPERTIES, 400> glyphProperties;
> +        textProperties.SetLength(maxGlyphs);
> +        glyphProperties.SetLength(maxGlyphs);

Need to check the SetLength calls for failure.

> +        advances.SetLength(actualGlyphs);
> +        glyphOffsets.SetLength(actualGlyphs);

Ditto.

	Bas Schouten (:bas.schouten) Assignee
	Comment 183 • 15 years ago

I've replied in the places where I have questions :).

(In reply to comment #181)
> (In reply to comment #179)
> 
> +++ b/gfx/thebes/src/gfxDWriteCommon.h
> +++ b/gfx/thebes/src/gfxDWriteCustomFonts.cpp
> +++ b/gfx/thebes/src/gfxDWriteCustomFonts.h
> 
> I don't see the need for these to be separate files, I think it would be better
> to keep them all in gfxDWriteFonts.h/cpp.
Personally I think this is a bad idea. There's a lot of places in our tree where I feel the large amount of code/classes in a single file is already compromising readability and making the code harder to understand. I wouldn't mind merging some of these files to less files, but I'm not sure how to merge them in that case :). I could put all the stuff from CustomFonts.h in Common.h and create a common.cpp, clearing out one file. Of course if you guys feel I should just stuff all this in the main files, I can do that.
> 
> +class DWriteFactory
> +{
> +public:
> +    static IDWriteFactory *Instance()
> 
> So this class has a single static method and a couple statics associated with
> it?  Seems like this belongs in gfxWindowsPlatform instead.
> 
> +    static IDWriteFactory *mInstance;
> 
> Use nsRefPtr<IDWriteFactory> instead and skip the release.
> 
> +                rv = prefSvc->GetBranch("font.engine.",
> getter_AddRefs(prefBranch));
> +                if (NS_SUCCEEDED(rv)) {
> +                    rv = prefBranch->GetBoolPref("forcegdi", &forceGDI);
> +                    if (NS_FAILED(rv)) {
> +                        forceGDI = PR_FALSE;
> +                    }
> 
> Rather than 'font.engine.forcegdi', use
> 'gfx.font_rendering.directwrite.enabled' and add an entry to
> modules/libpref/src/init/all.js in the Windows-only section.  We'll need to add
> other conditionals like this for forcing Cleartype-rendering on XP, I'd like to
> keep those grouped together.  For initial checkin we should leave this set to
> 'false' and we can flip it to 'true' once it's had a bit of time to get banged
> on.

This will be done in the final iteration where I also remove using Direct2D by default.
> 
> +IDWriteFontFileLoader* gfxDWriteFontFileLoader::mInstance = NULL;
> 
> Use nsRefPtr<IDWriteFontFileLoader>.
> 
> +        gfxWindowsDWriteFontEntry *fe = 
> +            new gfxWindowsDWriteFontEntry(fullName, font);
> +        fe->mItalic = (font->GetStyle() == DWRITE_FONT_STYLE_ITALIC ||
> +                      font->GetStyle() == DWRITE_FONT_STYLE_OBLIQUE);
> +        fe->mStretch = FontStretchFromDWriteStretch(font->GetStretch());
> +        PRUint16 weight = PR_ROUNDUP(font->GetWeight() - 50, 100);
> +            
> +        weight = NS_MAX<PRUint16>(100, weight);
> +        weight = NS_MIN<PRUint16>(900, weight);
> +    
> +        fe->mWeight = weight;
> 
> Code like this appears several times.  Make the weight, style, stretch part of
> the constructor.
Shouldn't we make this constructor arguments on the baseclass then? Every implementation I've looked at seems to either add code for this in the constructor, or do what I did. Seems to me like we'd be better off adding this to the initializer list on the base class.

> 
> +gfxDWriteFontFileStream::gfxDWriteFontFileStream(PRUint8 *aData, PRUint32
> aLength)
> +{
> +    mData = new PRUint8[aLength];
> +    mDataSize = aLength;
> +    memcpy(mData, aData, aLength);
> +}
> 
> Why is this copy needed?  Seems like could rework the code in MakePlatformFont
> to avoid this copy.
Well, I need to change ownership of the data. A change as you suggest would require a big warning sign though. Since this would mean I need to use NS_Free in the destructor. It means -ONLY- an array allocated with NS_Alloc can be passed in here. This is a dangerous situation which is why I copied it. The RAM copy has negligible overhead I believe.

> 
> +void
> +gfxWindowsDWriteFontFamily::FindStyleVariations()
> 
> There's a lot of code here to get the fullname for each face, I don't think
> that's needed at all, this is only used by the font cache and never for OS
> lookups (correct me here if I missed something).  Seems like just mName + i
> would work just as well for example.
At the moment, you're correct (I know this because I did what you're suggested at some point :)). However I think for debugging/maintenance purposes this is a bad idea. IF we do this it should be renamed Identifier() or something like that. We can't go calling it 'name' and then not actually have it contain the name. That would make for horrible API behaviour.
> +    /**
> +     * Second pass, for all oblique entries we search for an identical italic
> +     * entry. If one exists we remove the oblique entry from the available
> +     * fonts list, since gfxFontEntry can't make the distinction.
> +     *
> +     * TODO: Current algorithm is not the most efficient but it does the job
> +     * with little code complexity. Need to consider if something more
> advanced
> +     * is required.
> +     */
> +    for (unsigned int i = 0; i < obliqueEntries.Length(); i++) {
> +        for (unsigned int c = 0; c < mAvailableFonts.Length(); c++) {
> +            if (mAvailableFonts[c].get() != obliqueEntries[i] &&
> +                mAvailableFonts[c]->mWeight == obliqueEntries[i]->mWeight &&
> +                mAvailableFonts[c]->mItalic == PR_TRUE &&
> +                mAvailableFonts[c]->mStretch == obliqueEntries[i]->mStretch) {
> +                    mAvailableFonts.RemoveElement(obliqueEntries[i]);
> +                    break;
> +            }
> +        }
> +    }
> 
> Ack.  I think we can avoid this by simply skipping faces that have a
> "simulation" (i.e. fake bold/oblique) set based on font->GetSimulations().  My
> guess is that Windows 7 does not ship with any "real" oblique fonts.
True, we can, GetSimulations() indicates it's a simulation for most oblique fonts. Despite that, however, I felt they should still be listed if there's no italic font available? If this is incorrect ofcourse, it's -much- easier to just take out all Oblique simulations.
> 
> Implementation needed.

Yeah... can't find any DirectWrite code to do any of these things at this point though. Most of them can be solved, MatchesGenericFamily I'm not so sure though. DirectWrite seems to have no concept of generic families.

> This code needs to match the logic in gfxGDIFontList::GetDefaultFont, the
> default font varies based on the OS locale, just setting this to Tahoma is
> wrong.

Yep, I realize that. I can't seem to find anything to do this in DirectWrite though, I will look some more.

> 
> We don't need to store the localized family, this is *only* used when the user
> opens the content prefs panel.  Better to look this up within LocalizedName()
> rather than spend time in gfxDWriteFontList::InitFontList which affects startup
> time.

We basically already have it. Since we've already gotten the localized strings set. The time spent in the lookup of the localized name is extremely small (we've already gotten the IDWriteLocalizedStrings set, so the table's already been grabbed). Do you really think it's worth doing this lookup in LocalizedName(), I realise this is not used a lot at the moment, but for future API robustness I'd say it's wiser to store this.

	Bas Schouten (:bas.schouten) Assignee
	Comment 184 • 15 years ago

(In reply to comment #182)
> (From update of attachment 424484 [details] [diff] [review])
> > +cairo_font_face_t *
> > +gfxWindowsDWriteFont::CairoFontFace()
> > +{
> > +    if (!mCairoFontFace) {
> > +#ifdef CAIRO_HAS_DWRITE_FONT
> > +        mCairoFontFace = 
> > +            cairo_dwrite_font_face_create_for_dwrite_fontface(
> > +            ((gfxWindowsDWriteFontEntry*)mFontEntry.get())->mFont, mFontFace);
> > +#endif
> > +    }
> > +    return mCairoFontFace;
> > +}
> 
> What would happen if the #ifdef here is false? Seems like that could be a
> problem - should there be a configure-time dependency or something so that we
> don't get into that situation?

It should be unreachable code. Since DWriteFactory would never construct then. And therefor the gfxWindowsPlatform code would never create a DWriteFontList. I realize this isn't too pretty :(.