Ensure we use the current locale when looking up fallback fonts per character

Ensure we use the current locale when looking up fallback fonts per character

http://crbug.com/183830

R=scroggo@google.com

Committed: https://code.google.com/p/skia/source/detail?r=9751

[reed1, 2013-06-25 16:28:44]

https://codereview.chromium.org/17691002/diff/5/src/ports/SkFontConfigInterfa...
File src/ports/SkFontConfigInterface_android.cpp (right):

https://codereview.chromium.org/17691002/diff/5/src/ports/SkFontConfigInterfa...
src/ports/SkFontConfigInterface_android.cpp:550: fLocaleFallbackFontList =
this->findFallbackFontList(locale);
Does this leak the old fontlist?

https://codereview.chromium.org/17691002/diff/5/src/ports/SkFontConfigInterfa...
src/ports/SkFontConfigInterface_android.cpp:553: return fLocaleFallbackFontList;
the caller can manipulate this object outside of the mutex...

[djsollen, 2013-06-25 16:38:20]

https://codereview.chromium.org/17691002/diff/5/src/ports/SkFontConfigInterfa...
File src/ports/SkFontConfigInterface_android.cpp (right):

https://codereview.chromium.org/17691002/diff/5/src/ports/SkFontConfigInterfa...
src/ports/SkFontConfigInterface_android.cpp:550: fLocaleFallbackFontList =
this->findFallbackFontList(locale);
The short answer is no.  The fallbackfontlist is part of the object and as such
this pointer is guaranteed to be valid as long as the object is valid.  The
actual cleanup of the list takes place in the object's destructor.

On 2013/06/25 16:28:45, reed1 wrote:
> Does this leak the old fontlist?

https://codereview.chromium.org/17691002/diff/5/src/ports/SkFontConfigInterfa...
src/ports/SkFontConfigInterface_android.cpp:553: return fLocaleFallbackFontList;
removed the mutex in the next patch set.

On 2013/06/25 16:28:45, reed1 wrote:
> the caller can manipulate this object outside of the mutex...

[scroggo, 2013-06-25 16:45:51]

lgtm

[reed1, 2013-06-25 16:48:23]

https://codereview.chromium.org/17691002/diff/5/src/ports/SkFontConfigInterfa...
File src/ports/SkFontConfigInterface_android.cpp (right):

https://codereview.chromium.org/17691002/diff/5/src/ports/SkFontConfigInterfa...
src/ports/SkFontConfigInterface_android.cpp:550: fLocaleFallbackFontList =
this->findFallbackFontList(locale);
On 2013/06/25 16:38:20, djsollen wrote:
> The short answer is no.  The fallbackfontlist is part of the object and as
such
> this pointer is guaranteed to be valid as long as the object is valid.  The
> actual cleanup of the list takes place in the object's destructor.
> 
> On 2013/06/25 16:28:45, reed1 wrote:
> > Does this leak the old fontlist?
> 

In line 550 we assign into fLocalFallbackFontList. What if it was not null?

[djsollen, 2013-06-25 16:56:45]

On 2013/06/25 16:48:23, reed1 wrote:
>
https://codereview.chromium.org/17691002/diff/5/src/ports/SkFontConfigInterfa...
> File src/ports/SkFontConfigInterface_android.cpp (right):
> 
>
https://codereview.chromium.org/17691002/diff/5/src/ports/SkFontConfigInterfa...
> src/ports/SkFontConfigInterface_android.cpp:550: fLocaleFallbackFontList =
> this->findFallbackFontList(locale);
> On 2013/06/25 16:38:20, djsollen wrote:
> > The short answer is no.  The fallbackfontlist is part of the object and as
> such
> > this pointer is guaranteed to be valid as long as the object is valid.  The
> > actual cleanup of the list takes place in the object's destructor.
> > 
> > On 2013/06/25 16:28:45, reed1 wrote:
> > > Does this leak the old fontlist?
> > 
> 
> In line 550 we assign into fLocalFallbackFontList. What if it was not null?

That field is just a cache so that we don't need to do the lookup in the
dictionary.  It isn't changing the ownership of the data to
fLocaleFallbackFontList.  The data pointed to by that field is still owned by
the object and will be cleaned up when the object is destroyed.

[reed1, 2013-06-25 17:44:01]

On 2013/06/25 16:56:45, djsollen wrote:
> On 2013/06/25 16:48:23, reed1 wrote:
> >
>
https://codereview.chromium.org/17691002/diff/5/src/ports/SkFontConfigInterfa...
> > File src/ports/SkFontConfigInterface_android.cpp (right):
> > 
> >
>
https://codereview.chromium.org/17691002/diff/5/src/ports/SkFontConfigInterfa...
> > src/ports/SkFontConfigInterface_android.cpp:550: fLocaleFallbackFontList =
> > this->findFallbackFontList(locale);
> > On 2013/06/25 16:38:20, djsollen wrote:
> > > The short answer is no.  The fallbackfontlist is part of the object and as
> > such
> > > this pointer is guaranteed to be valid as long as the object is valid. 
The
> > > actual cleanup of the list takes place in the object's destructor.
> > > 
> > > On 2013/06/25 16:28:45, reed1 wrote:
> > > > Does this leak the old fontlist?
> > > 
> > 
> > In line 550 we assign into fLocalFallbackFontList. What if it was not null?
> 
> That field is just a cache so that we don't need to do the lookup in the
> dictionary.  It isn't changing the ownership of the data to
> fLocaleFallbackFontList.  The data pointed to by that field is still owned by
> the object and will be cleaned up when the object is destroyed.

Gotcha.

[djsollen, 2013-06-25 18:07:49]

Committed patchset #3 manually as r9751 (presubmit successful).