force readbuffer clients to use specialized readFoo for flattenables

force readbuffer clients to use specialized readFoo for flattenables

BUG=
R=mtklein@google.com

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

[reed1, 2013-10-09 15:44:01]

A possible pre-CL to the type-checking CL for flattenables. This CL *only*
modifies the callers of readBuffer, forcing them to use type-specific readers.
The ideas are:

1. There is a finite, closed, bounded, core-known, set of base-types that we
unflatten.

2. The base-type of the object being unflattened is implicitly know by the
caller, and so this allows them to communicate it to the "factory" (though it is
ignored at the moment).

3. I made up an enum, but that is perhaps not needed. It is just another
encapsulation of the bounded set of types that core needs to know about.

4. As my comments in .h show, there are a few "flattenables" that kinda don't
need to inherit from SkFlattenable per-se, since they explicitly don't allow
subclassing, but just needed a way to read/write themselves. Perhaps these guys
can be treated differently in our sanitizing new world...

[sugoi, 2013-10-09 17:08:48]

On 2013/10/09 15:44:01, reed1 wrote:
> A possible pre-CL to the type-checking CL for flattenables. This CL *only*
> modifies the callers of readBuffer, forcing them to use type-specific readers.
> The ideas are:
> 
> 1. There is a finite, closed, bounded, core-known, set of base-types that we
> unflatten.

The enum would be redundant with the object's type or name, which is required
for type checking. Also, I think this can be accomplished by the template, while
making sure that noone can call the default instantiation (we'd need to modify
it a little).

> 2. The base-type of the object being unflattened is implicitly know by the
> caller, and so this allows them to communicate it to the "factory" (though it
is
> ignored at the moment).

A single base class is not enough, we need to know the whole hierarchy of
classes to do type checking. For example, an SkDiffuseLightingImageFilter object
is an SkLightingImageFilter, which is an SkImageFilter, which is an
SkFlattenable. Any caller could know that they should be receiving an
SkFlattenable, or an SkImageFilter, etc, so we can't make this work universally
with a single known base class to check. I also don't understand how to check
for the base class using this code.

> 4. As my comments in .h show, there are a few "flattenables" that kinda don't
> need to inherit from SkFlattenable per-se, since they explicitly don't allow
> subclassing, but just needed a way to read/write themselves. Perhaps these
guys
> can be treated differently in our sanitizing new world...

As long as they have the required functions to serialize and do type checking,
they don't need to inherit from SkFlattenable and we could easily make a
standard for that case. For example, if we use names, we could say that using ""
would mean that we don't explicitly type check an object. Although, why would we
be reading these objects through someReadBuffer::readFlattenable() in the first
place ? If we need to make up new functions for these, then there's no reason
for these objects to interfere with the code in readFlattenable().

[reed1, 2013-10-09 17:13:11]

The missing piece, which *might* answer some of your questions, is a proper impl
of readEffect. It should/could look like the following:

SkFlattenable* readEffect(enum) {
    call_factory_function_ptr(buffer, enum);
}

Where we change the factory signature to take the enum. In that world, the
factories can now inspect the enum, and if their particular instance is not a
subclass (to some degree) of the enum, they return NULL.

[reed1, 2013-10-09 17:14:59]

This proposal explicitly posits that the caller (skia core) does *not* care
about intervening subclasses between the one represented by the string (which
was used to return the factory in the first place) and the base-class
represented by the enum. Since all the caller is going to do is "cast" to the
enum's base-class, that should be the only constraint that it imposes on the
factories.

Perhaps a VC would help.

[sugoi, 2013-10-09 17:25:48]

On 2013/10/09 17:13:11, reed1 wrote:
> The missing piece, which *might* answer some of your questions, is a proper
impl
> of readEffect. It should/could look like the following:
> 
> SkFlattenable* readEffect(enum) {
>     call_factory_function_ptr(buffer, enum);
> }
> 
> Where we change the factory signature to take the enum. In that world, the
> factories can now inspect the enum, and if their particular instance is not a
> subclass (to some degree) of the enum, they return NULL.

I'm sorry for not understanding this right away, but how do you check that an
instance is of a particular subclass ?

Also, doesn't this mean that we'd need to implement a new function every time we
want to read based on a new subclass ? To use my earlier example, to read a
SkDiffuseLightingImageFilter object could require 4 different functions,
depending on who the caller is : readDiffuseLightingImageFilter(),
readLightingImageFilter(), readImageFilter() or readFlattenable() (or the
equivalent code using readEffect() and 4 different enum). Or is it a big switch
case with all the possible enum values in it ?

How would we get the factory function pointer from the base class ? We need the
factory function from the actual class, right ? Not the factory function of the
base class.

[sugoi, 2013-10-09 20:26:55]

Some more reflection on this...

https://codereview.chromium.org/26702002/diff/1/include/core/SkFlattenableBuf...
File include/core/SkFlattenableBuffers.h (left):

https://codereview.chromium.org/26702002/diff/1/include/core/SkFlattenableBuf...
include/core/SkFlattenableBuffers.h:101: template <typename T> T*
readFlattenableT() {
Ok, so I've been thinking about this. Even though the public calls are all
strongly typed, I still think it would be much easier to implement / safer if
the inside of these functions are from a (protected/private) call to a template
function and having something like T::GetType() (or maybe
SkFlattenable::GetTypeFromName(T::GetName())) inside that template function.
We'll need to get the type from the name somehow anyway when reading the stream
to do type checking.

In a more abstract way of saying it, I would really appreciate a design that
would not have us manually link an enum with a class in 2 different spots in the
code (which would be here, and at registration time, if we want type checking to
work -- and we'd need some special registration phase for base classes too, to
link each base class with its enum, I guess, which we don't have now), since I
think that would be error prone. If we could simply link an enum value with each
class ONLY at registration time and use the template here to figure out the type
without explicitly writing it in each function, I would find that much cleaner.

[reed1, 2013-10-10 18:03:39]

next iteration up

[sugoi, 2013-10-10 18:15:13]

https://codereview.chromium.org/26702002/diff/9001/include/core/SkFlattenable...
File include/core/SkFlattenableBuffers.h (right):

https://codereview.chromium.org/26702002/diff/9001/include/core/SkFlattenable...
include/core/SkFlattenableBuffers.h:42: kUnknown_SkEffectType,
Having an "unknown" type defeats the purpose of having a known subset of base
classes that we know we can deserialize here, doesn't it ? It feels like we have
a shiny new lock on the front door, but left a window completely open.

https://codereview.chromium.org/26702002/diff/9001/src/core/SkFlattenableBuff...
File src/core/SkFlattenableBuffers.cpp (right):

https://codereview.chromium.org/26702002/diff/9001/src/core/SkFlattenableBuff...
src/core/SkFlattenableBuffers.cpp:57: 
No actual changes to this file

[mtklein, 2013-10-10 18:59:27]

On 2013/10/10 18:15:13, sugoi wrote:
>
https://codereview.chromium.org/26702002/diff/9001/include/core/SkFlattenable...
> File include/core/SkFlattenableBuffers.h (right):
> 
>
https://codereview.chromium.org/26702002/diff/9001/include/core/SkFlattenable...
> include/core/SkFlattenableBuffers.h:42: kUnknown_SkEffectType,
> Having an "unknown" type defeats the purpose of having a known subset of base
> classes that we know we can deserialize here, doesn't it ? It feels like we
have
> a shiny new lock on the front door, but left a window completely open.
> 
>
https://codereview.chromium.org/26702002/diff/9001/src/core/SkFlattenableBuff...
> File src/core/SkFlattenableBuffers.cpp (right):
> 
>
https://codereview.chromium.org/26702002/diff/9001/src/core/SkFlattenableBuff...
> src/core/SkFlattenableBuffers.cpp:57: 
> No actual changes to this file

lgtm now that I'm reading kUnknown_SkEffectType as "TODO: we know the real types
here, fix up and remove kUnknown_SkEffectType eventually"

[reed1, 2013-10-15 21:41:42]

now with no more kUnknown goodness ...

[reed1, 2013-10-16 13:05:20]

Committed patchset #8 manually as r11803 (presubmit successful).