Make fID and MixedID calculations private

BUG=skia:

(mtklein from here on)
No public API changes.
TBR=reed@google.com

Committed: https://skia.googlesource.com/skia/+/f8d24e2c0c7b44b7ccf20e40890514db4cde7b15

Committed: https://skia.googlesource.com/skia/+/b69d0e0ac45e13f667bc11a937dcb547072bc93d

[herb_g, 2015-02-20 04:35:28]

herb@google.com changed reviewers:
+ mtklein@google.com

[mtklein, 2015-02-23 17:30:36]

https://codereview.chromium.org/939123002/diff/40001/src/core/SkGlyph.h
File src/core/SkGlyph.h (right):

https://codereview.chromium.org/939123002/diff/40001/src/core/SkGlyph.h#newco...
src/core/SkGlyph.h:31: kSubShiftX = kSubBits,
funky indent?

https://codereview.chromium.org/939123002/diff/40001/src/core/SkGlyph.h#newco...
src/core/SkGlyph.h:52: void initWithGlyphIDXY(uint32_t glyph_id, SkFixed x,
SkFixed y) {
Huh, unused.  Must be only SkGlyphCache does this?  Let's leave it out if we're
going to keep SkGlyphCache as a superfriend.

https://codereview.chromium.org/939123002/diff/40001/src/core/SkGlyph.h#newco...
src/core/SkGlyph.h:56: void initFromGlyph(const SkGlyph& glyph) {
maybe copyFrom() ?

https://codereview.chromium.org/939123002/diff/40001/src/core/SkGlyph.h#newco...
src/core/SkGlyph.h:128: return (id & kCodeMask) - 1;
Now that it's clear this is safe, let's undo this for this CL.  It's weird to
land it without the extra context of why we're trying to carve zero out as an
invalid value.  Really, when SkGlyphCache is no longer a friend, we'll probably
want an SkGlyph::initInvalid() to hide away how we do that too.  It may be
clearer, for example, to use ~0 instead of 0 as the invalid value.

[herb_g, 2015-02-23 23:24:53]

PTAL

https://codereview.chromium.org/939123002/diff/40001/src/core/SkGlyph.h
File src/core/SkGlyph.h (right):

https://codereview.chromium.org/939123002/diff/40001/src/core/SkGlyph.h#newco...
src/core/SkGlyph.h:31: kSubShiftX = kSubBits,
On 2015/02/23 17:30:36, mtklein wrote:
> funky indent?

Done.

https://codereview.chromium.org/939123002/diff/40001/src/core/SkGlyph.h#newco...
src/core/SkGlyph.h:52: void initWithGlyphIDXY(uint32_t glyph_id, SkFixed x,
SkFixed y) {
On 2015/02/23 17:30:36, mtklein wrote:
> Huh, unused.  Must be only SkGlyphCache does this?  Let's leave it out if
we're
> going to keep SkGlyphCache as a superfriend.

Done.

https://codereview.chromium.org/939123002/diff/40001/src/core/SkGlyph.h#newco...
src/core/SkGlyph.h:56: void initFromGlyph(const SkGlyph& glyph) {
On 2015/02/23 17:30:36, mtklein wrote:
> maybe copyFrom() ?

This is not really copying the whole structure, but only the glyph id. It's
really an init. Tried a better name.

https://codereview.chromium.org/939123002/diff/40001/src/core/SkGlyph.h#newco...
src/core/SkGlyph.h:128: return (id & kCodeMask) - 1;
On 2015/02/23 17:30:36, mtklein wrote:
> Now that it's clear this is safe, let's undo this for this CL.  It's weird to
> land it without the extra context of why we're trying to carve zero out as an
> invalid value.  Really, when SkGlyphCache is no longer a friend, we'll
probably
> want an SkGlyph::initInvalid() to hide away how we do that too.  It may be
> clearer, for example, to use ~0 instead of 0 as the invalid value.

If you use ~0, then one record will always be at the end of the array, so you
will always be adjust an extra record. With sentinal == 0 then you will never
move the extra record.

[mtklein, 2015-02-23 23:28:52]

lgtm.

[mtklein, 2015-02-23 23:29:28]

The CQ bit was checked by mtklein@google.com

[mtklein, 2015-02-23 23:29:50]

The CQ bit was unchecked by mtklein@google.com

[mtklein, 2015-02-23 23:30:32]

https://codereview.chromium.org/939123002/diff/60001/src/core/SkGlyph.h
File src/core/SkGlyph.h (right):

https://codereview.chromium.org/939123002/diff/60001/src/core/SkGlyph.h#newco...
src/core/SkGlyph.h:145: return code + 1;
Whoops, I think we missed this one.

[herb_g, 2015-02-24 00:22:51]

Last +1 removed.

https://codereview.chromium.org/939123002/diff/60001/src/core/SkGlyph.h
File src/core/SkGlyph.h (right):

https://codereview.chromium.org/939123002/diff/60001/src/core/SkGlyph.h#newco...
src/core/SkGlyph.h:145: return code + 1;
On 2015/02/23 23:30:32, mtklein wrote:
> Whoops, I think we missed this one.

Done.

[herb_g, 2015-02-24 13:05:03]

The CQ bit was checked by herb@google.com

[herb_g, 2015-02-24 13:05:03]

The patchset sent to the CQ was uploaded after l-g-t-m from mtklein@google.com
Link to the patchset: https://codereview.chromium.org/939123002/#ps80001 (title:
"Last adjustment.")

[commit-bot: I haz the power, 2015-02-24 13:05:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/939123002/80001

[commit-bot: I haz the power, 2015-02-24 13:12:10]

Committed patchset #5 (id:80001) as
https://skia.googlesource.com/skia/+/f8d24e2c0c7b44b7ccf20e40890514db4cde7b15

[scroggo, 2015-02-24 14:08:24]

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.chromium.org/951353002/ by scroggo@google.com.

The reason for reverting is: This actually *does* change the public API - fID is
now private, and it was being used on Android. See
https://android-build.storage.googleapis.com/builds/git_master-skia-linux-vol...
:

In file included from frameworks/base/libs/hwui/tests/../font/Font.cpp:26:0:
external/skia/src/core/SkGlyph.h: In member function
'android::uirenderer::CachedGlyphInfo*
android::uirenderer::Font::cacheGlyph(const SkPaint*, glyph_t, bool)':
external/skia/src/core/SkGlyph.h:157:17: error: 'uint32_t SkGlyph::fID' is
private
     uint32_t    fID;
                 ^
frameworks/base/libs/hwui/tests/../font/Font.cpp:482:39: error: within this
context
     newGlyph->mGlyphIndex = skiaGlyph.fID;

We need to update Android in order to hide fID..

[scroggo, 2015-02-24 14:11:45]

scroggo@google.com changed reviewers:
+ djsollen@google.com, scroggo@google.com

[scroggo, 2015-02-24 14:11:45]

On 2015/02/24 14:08:24, scroggo wrote:
> A revert of this CL (patchset #5 id:80001) has been created in
> https://codereview.chromium.org/951353002/ by mailto:scroggo@google.com.
> 
> The reason for reverting is: This actually *does* change the public API - fID
is
> now private, and it was being used on Android. See
>
https://android-build.storage.googleapis.com/builds/git_master-skia-linux-vol...
> :
> 
> In file included from frameworks/base/libs/hwui/tests/../font/Font.cpp:26:0:
> external/skia/src/core/SkGlyph.h: In member function
> 'android::uirenderer::CachedGlyphInfo*
> android::uirenderer::Font::cacheGlyph(const SkPaint*, glyph_t, bool)':
> external/skia/src/core/SkGlyph.h:157:17: error: 'uint32_t SkGlyph::fID' is
> private
>      uint32_t    fID;
>                  ^
> frameworks/base/libs/hwui/tests/../font/Font.cpp:482:39: error: within this
> context
>      newGlyph->mGlyphIndex = skiaGlyph.fID;
> 
> We need to update Android in order to hide fID..

Mike pointed out that SkGlyphCache is in src/, so Android shouldn't be referring
to it. I don't know how much work it will be to remove their dependency.

[djsollen, 2015-02-24 14:26:57]

The short answer is that this is a known issue and the reason it hasn't been
addressed is that it is a lot of work.

[herb_g, 2015-02-24 23:29:18]

Added private bypass for Android.

[mtklein, 2015-02-24 23:33:49]

https://codereview.chromium.org/939123002/diff/100001/src/core/SkGlyph.h
File src/core/SkGlyph.h (right):

https://codereview.chromium.org/939123002/diff/100001/src/core/SkGlyph.h#newc...
src/core/SkGlyph.h:158: // accesses fID. Remove when fID accesses are cleaned
up.
As far as dirty hacks go, I don't think this makes things any dirtier, but won't
this prevent you from doing the +1, -1 later?

Seems like we need to add a public getter (hopefully not a setter?), switch
android over to that, then make the field private?

[scroggo, 2015-02-25 14:08:00]

lgtm

Mike already approved, and it looks good to me now that it should not break
Android.

https://codereview.chromium.org/939123002/diff/100001/src/core/SkGlyph.h
File src/core/SkGlyph.h (right):

https://codereview.chromium.org/939123002/diff/100001/src/core/SkGlyph.h#newc...
src/core/SkGlyph.h:158: // accesses fID. Remove when fID accesses are cleaned
up.
On 2015/02/24 23:33:48, mtklein wrote:
> Seems like we need to add a public getter (hopefully not a setter?), switch
> android over to that, then make the field private?

Android does not need a setter. That seems like the right approach...

> As far as dirty hacks go, I don't think this makes things any dirtier, but
won't
> this prevent you from doing the +1, -1 later?

If the plan is to modify it later, then perhaps the getter won't work. Android
stores the fID and then uses it later on. If Android still *should* be looking
at the original fID, we don't have a problem. I don't know enough about this
code, or Android's use of it to make that call though.

[mtklein, 2015-02-25 14:23:10]

> If the plan is to modify it later, then perhaps the getter won't work. Android
> stores the fID and then uses it later on. If Android still *should* be looking
> at the original fID, we don't have a problem. I don't know enough about this
> code, or Android's use of it to make that call though.

It's more like we'll want to secretly store ID 0 as ID 1, ID 2 as ID 3, etc.  If
Android uses the normal getter, there should be no problems (it'll do the -1 on
the other side).  We want to keep the physical 0 value as a special undefined
value for the glyph cache.

[scroggo, 2015-02-25 14:40:04]

The CQ bit was checked by scroggo@google.com

[scroggo, 2015-02-25 14:40:04]

The patchset sent to the CQ was uploaded after l-g-t-m from mtklein@google.com
Link to the patchset: https://codereview.chromium.org/939123002/#ps100001
(title: "Support android build")

[commit-bot: I haz the power, 2015-02-25 14:40:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/939123002/100001

[commit-bot: I haz the power, 2015-02-25 14:47:11]

Committed patchset #6 (id:100001) as
https://skia.googlesource.com/skia/+/b69d0e0ac45e13f667bc11a937dcb547072bc93d