Don't use SK_ASSERT in header.

Don't use SK_ASSERT in header.

It can cause link error. For example of chromium debug build,
../../third_party/skia/include/core/SkImageInfo.h:109: error: undefined reference to 'SkDebugf_FileLine(char const*, int, bool, char const*, ...)'
collect2: error: ld returned 1 exit status

It's because SkDebugf can be overridden by a client.

[dshwang, 2015-01-23 15:37:09]

dongseong.hwang@intel.com changed reviewers:
+ bsalomon@google.com, reed@google.com

[dshwang, 2015-01-23 15:37:09]

could you review?
currently chromium linux debug build using gcc is broken.

[dshwang, 2015-01-23 15:37:49]

https://codereview.chromium.org/870023002/diff/1/include/core/SkImageInfo.h
File include/core/SkImageInfo.h (left):

https://codereview.chromium.org/870023002/diff/1/include/core/SkImageInfo.h#o...
include/core/SkImageInfo.h:109: SkASSERT((size_t)ct < SK_ARRAY_COUNT(gSize));
I think this ASSERT is covered by above SK_COMPILE_ASSERT and c++ type system.

[dshwang, 2015-01-23 15:54:00]

in more detail, following is the dependency graph causing this bug.

src/thirdparty/skia <- src/skia <- src/media <- src/media/blink

src/skia implements SkDebugf.
one cc file in src/media/blink includes SkCanvasVideoRenderer.h in src/media,
which includes SkImageInfo.h
src/media module links src/skia module but src/media/blink module doesn't link
src/skia module

Another solution is that src/media/blink module links src/skia module, but it
looks not correct way.

[bsalomon, 2015-01-23 15:55:50]

https://codereview.chromium.org/870023002/diff/1/include/core/SkImageInfo.h
File include/core/SkImageInfo.h (left):

https://codereview.chromium.org/870023002/diff/1/include/core/SkImageInfo.h#o...
include/core/SkImageInfo.h:109: SkASSERT((size_t)ct < SK_ARRAY_COUNT(gSize));
On 2015/01/23 15:37:48, dshwang wrote:
> I think this ASSERT is covered by above SK_COMPILE_ASSERT and c++ type system.

I believe the assert is to guard against the case when ct was unpacked from some
bitfield or whatnot and cast to SkColorType. The type system doesn't prevent you
from doing (SkColorType)(1 << 24).

The client is allowed to override SkDebugf but AFAIK the client is responsible
for ensuring that it is linked into anything that is built with skia headers.

[dshwang, 2015-01-23 20:16:37]

> The client is allowed to override SkDebugf but AFAIK the client is responsible
> for ensuring that it is linked into anything that is built with skia headers.

Your opinion is reasonable, but chromium has practical issue.

SkBitmap.h includes SkImageInfo.h and SkBitmap.h is included in so many header
files in chromium.
Think about you build chromium in component build, and a component is totally
not related to skia, but a including header indirectly includes SkBitmap.h.
The component should link to skia static library. It looks overkill.
WDYT?

[reed1, 2015-01-23 20:45:50]

On 2015/01/23 15:54:00, dshwang wrote:
> in more detail, following is the dependency graph causing this bug.
> 
> src/thirdparty/skia <- src/skia <- src/media <- src/media/blink
> 
> src/skia implements SkDebugf.
> one cc file in src/media/blink includes SkCanvasVideoRenderer.h in src/media,
> which includes SkImageInfo.h
> src/media module links src/skia module but src/media/blink module doesn't link
> src/skia module
> 
> Another solution is that src/media/blink module links src/skia module, but it
> looks not correct way.

What if later skia decides to make that method not inline. Wouldn't media have
to link against skia then?

[dshwang, 2015-01-23 20:57:12]

On 2015/01/23 20:45:50, reed1 wrote:
> On 2015/01/23 15:54:00, dshwang wrote:
> > in more detail, following is the dependency graph causing this bug.
> > 
> > src/thirdparty/skia <- src/skia <- src/media <- src/media/blink
> > 
> > src/skia implements SkDebugf.
> > one cc file in src/media/blink includes SkCanvasVideoRenderer.h in
src/media,
> > which includes SkImageInfo.h
> > src/media module links src/skia module but src/media/blink module doesn't
link
> > src/skia module
> > 
> > Another solution is that src/media/blink module links src/skia module, but
it
> > looks not correct way.
> 
> What if later skia decides to make that method not inline. Wouldn't media have
> to link against skia then?

that's good idea. implementing it in cpp might work. let me try.

[reed1, 2015-01-23 21:07:01]

On 2015/01/23 20:57:12, dshwang wrote:
> On 2015/01/23 20:45:50, reed1 wrote:
> > On 2015/01/23 15:54:00, dshwang wrote:
> > > in more detail, following is the dependency graph causing this bug.
> > > 
> > > src/thirdparty/skia <- src/skia <- src/media <- src/media/blink
> > > 
> > > src/skia implements SkDebugf.
> > > one cc file in src/media/blink includes SkCanvasVideoRenderer.h in
> src/media,
> > > which includes SkImageInfo.h
> > > src/media module links src/skia module but src/media/blink module doesn't
> link
> > > src/skia module
> > > 
> > > Another solution is that src/media/blink module links src/skia module, but
> it
> > > looks not correct way.
> > 
> > What if later skia decides to make that method not inline. Wouldn't media
have
> > to link against skia then?
> 
> that's good idea. implementing it in cpp might work. let me try.

My question was motivated by the thought that any client of Skia's headers *has*
to be ready to link against the skia lib, since what is inlined today may be
non-inlined tomorrow (and vise-versa). 

Perhaps the solution to SkDebugf_FileLine is just to be sure that function is
marked SK_API so it can be exported?

[dshwang, 2015-01-30 21:09:07]

On 2015/01/23 21:07:01, reed1 wrote:
> On 2015/01/23 20:57:12, dshwang wrote:
> > On 2015/01/23 20:45:50, reed1 wrote:
> > > On 2015/01/23 15:54:00, dshwang wrote:
> > > > in more detail, following is the dependency graph causing this bug.
> > > > 
> > > > src/thirdparty/skia <- src/skia <- src/media <- src/media/blink
> > > > 
> > > > src/skia implements SkDebugf.
> > > > one cc file in src/media/blink includes SkCanvasVideoRenderer.h in
> > src/media,
> > > > which includes SkImageInfo.h
> > > > src/media module links src/skia module but src/media/blink module
doesn't
> > link
> > > > src/skia module
> > > > 
> > > > Another solution is that src/media/blink module links src/skia module,
but
> > it
> > > > looks not correct way.
> > > 
> > > What if later skia decides to make that method not inline. Wouldn't media
> have
> > > to link against skia then?
> > 
> > that's good idea. implementing it in cpp might work. let me try.
> 
> My question was motivated by the thought that any client of Skia's headers
*has*
> to be ready to link against the skia lib, since what is inlined today may be
> non-inlined tomorrow (and vise-versa). 
> 
> Perhaps the solution to SkDebugf_FileLine is just to be sure that function is
> marked SK_API so it can be exported?

sorry for delaying response.

SkDebugf_FileLine is already exported in src/skia/config/SkUserConfig.h
https://code.google.com/p/chromium/codesearch#chromium/src/skia/config/SkUser...

The root problem is that any module including SkImageInfo.h tries to compile
SkColorTypeBytesPerPixel() function but the module don't have reference of
SkDebugf implementation.

[reed1, 2015-01-30 21:15:11]

I think any module that includes a skia header has to link against skia.

If we followed that, would that fix the problem?

[dshwang, 2015-01-30 21:15:17]

> My question was motivated by the thought that any client of Skia's headers
*has*
> to be ready to link against the skia lib, since what is inlined today may be
> non-inlined tomorrow (and vise-versa). 

If inlined tomorrow, we need to fix it again.
all inline function and method in skia headers should be compiled by themselves.

https://codereview.chromium.org/870023002/diff/20001/include/core/SkImageInfo.h
File include/core/SkImageInfo.h (right):

https://codereview.chromium.org/870023002/diff/20001/include/core/SkImageInfo...
include/core/SkImageInfo.h:230: SK_API int bytesPerPixel() const;
move this definition to cpp because SkColorTypeBytesPerPixel() is not exported.
worth to note is that SkImageInfo is not exported while many chromium
implementation uses it. It's possible because most of methods are inline, which
means all chromium module using it compiles it.
I add SK_API because chromium uses it.

[dshwang, 2015-01-30 21:18:54]

On 2015/01/30 21:15:11, reed1 wrote:
> I think any module that includes a skia header has to link against skia.
> 
> If we followed that, would that fix the problem?

that's true but it requires lots of practical headache, because all indirect
dependent modules need to link skia. I think 90% modules in chromium indirectly
depends on skia.

e.g.
src/thirdparty/skia <- src/skia <- src/media <- src/media/blink

src/media/blink totally don't use skia but it needs to link skia because one
header in src/media includes SkBitmap.h

[reed1, 2015-01-30 21:23:51]

reed@google.com changed reviewers:
+ bungeman@google.com, mtklein@google.com

[reed1, 2015-01-30 21:23:52]

alternate CL to export SkImageInfo
https://codereview.chromium.org/893603005/

[dshwang, 2015-01-30 21:26:38]

On 2015/01/30 21:15:17, dshwang wrote:
> > My question was motivated by the thought that any client of Skia's headers
> *has*
> > to be ready to link against the skia lib, since what is inlined today may be
> > non-inlined tomorrow (and vise-versa). 
> 
> If inlined tomorrow, we need to fix it again.
> all inline function and method in skia headers should be compiled by
themselves.

The root problem is that SkImageInfo.h cannot be compiled by itself.
As you know, all cc or cpp files include SkImageInfo.h compile separately all
inline function and methods in SkImageInfo.h, because all definitions is in the
header file already.
However, SkColorTypeMinRowBytes() can not be compiled because SkDebugf is not
defined in header.

The rule of thumb should be
"all exported inline method and function definition in header must use only
exported inline method and function."

[dshwang, 2015-01-30 21:29:37]

On 2015/01/30 21:23:52, reed1 wrote:
> alternate CL to export SkImageInfo
> https://codereview.chromium.org/893603005/

yes, it's alternative way to export bytesPerPixel() but your CL cannot fix this
compile fail because SkDebugf is still not referenced.
btw, your CL is better than only exporting bytesPerPixel(), because chromium
uses this class heavily.

[mtklein, 2015-01-30 21:31:14]

On 2015/01/30 21:26:38, dshwang wrote:
> On 2015/01/30 21:15:17, dshwang wrote:
> > > My question was motivated by the thought that any client of Skia's headers
> > *has*
> > > to be ready to link against the skia lib, since what is inlined today may
be
> > > non-inlined tomorrow (and vise-versa). 
> > 
> > If inlined tomorrow, we need to fix it again.
> > all inline function and method in skia headers should be compiled by
> themselves.
> 
> The root problem is that SkImageInfo.h cannot be compiled by itself.
> As you know, all cc or cpp files include SkImageInfo.h compile separately all
> inline function and methods in SkImageInfo.h, because all definitions is in
the
> header file already.
> However, SkColorTypeMinRowBytes() can not be compiled because SkDebugf is not
> defined in header.
> 
> The rule of thumb should be
> "all exported inline method and function definition in header must use only
> exported inline method and function."

This is _intentionally_ not how Skia works.  If you're going to use any Skia
header, you must link Skia.

[dshwang, 2015-01-30 21:36:14]

On 2015/01/30 21:31:14, mtklein wrote:
> On 2015/01/30 21:26:38, dshwang wrote:
> > On 2015/01/30 21:15:17, dshwang wrote:
> > > > My question was motivated by the thought that any client of Skia's
headers
> > > *has*
> > > > to be ready to link against the skia lib, since what is inlined today
may
> be
> > > > non-inlined tomorrow (and vise-versa). 
> > > 
> > > If inlined tomorrow, we need to fix it again.
> > > all inline function and method in skia headers should be compiled by
> > themselves.
> > 
> > The root problem is that SkImageInfo.h cannot be compiled by itself.
> > As you know, all cc or cpp files include SkImageInfo.h compile separately
all
> > inline function and methods in SkImageInfo.h, because all definitions is in
> the
> > header file already.
> > However, SkColorTypeMinRowBytes() can not be compiled because SkDebugf is
not
> > defined in header.
> > 
> > The rule of thumb should be
> > "all exported inline method and function definition in header must use only
> > exported inline method and function."
> 
> This is _intentionally_ not how Skia works.  If you're going to use any Skia
> header, you must link Skia.

could you read #13?

in chromium, lots of top modules (e.g. ui/gfx, ui/gl, blink, cc) include skia
headers.
It means that all indirect dependent modules must link to skia if we compile
chromium by component build because skia is static_library.

[mtklein, 2015-01-30 21:39:33]

> in chromium, lots of top modules (e.g. ui/gfx, ui/gl, blink, cc) include skia
> headers.
> It means that all indirect dependent modules must link to skia if we compile
> chromium by component build because skia is static_library.

Yes, and they do.

[dshwang, 2015-01-30 21:42:42]

On 2015/01/30 21:39:33, mtklein wrote:
> > in chromium, lots of top modules (e.g. ui/gfx, ui/gl, blink, cc) include
skia
> > headers.
> > It means that all indirect dependent modules must link to skia if we compile
> > chromium by component build because skia is static_library.
> 
> Yes, and they do.

ok, thx for confirming.

I'll make src/media/blink link to skia in chromium.

I close this CL as WontFix