aw: Add versions to public function tables

aw: Add versions to public function tables

BUG=399370
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=287810

[boliu, 2014-07-31 18:17:33]

Missed these last time :/

[hush (inactive), 2014-07-31 21:03:50]

https://codereview.chromium.org/433603005/diff/1/android_webview/public/brows...
File android_webview/public/browser/draw_gl.h (right):

https://codereview.chromium.org/433603005/diff/1/android_webview/public/brows...
android_webview/public/browser/draw_gl.h:89: static const int
kAwDrawGLFunctionTableVersion = 1;
what is the circumstance that AwDrawGLFunctionTableVersion is different from
AwDrawGLInfoVersion? Should they always be the same version?

[hush (inactive), 2014-07-31 21:08:41]

https://codereview.chromium.org/433603005/diff/1/android_webview/public/brows...
File android_webview/public/browser/draw_gl.h (right):

https://codereview.chromium.org/433603005/diff/1/android_webview/public/brows...
android_webview/public/browser/draw_gl.h:89: static const int
kAwDrawGLFunctionTableVersion = 1;
I guess every time you add something to the struct, you need to increase the
version number by 1. And you are not supposed to delete members from the struct.
Right?
On 2014/07/31 21:03:50, hush wrote:
> what is the circumstance that AwDrawGLFunctionTableVersion is different from
> AwDrawGLInfoVersion? Should they always be the same version?

[boliu, 2014-07-31 21:09:38]

https://codereview.chromium.org/433603005/diff/1/android_webview/public/brows...
File android_webview/public/browser/draw_gl.h (right):

https://codereview.chromium.org/433603005/diff/1/android_webview/public/brows...
android_webview/public/browser/draw_gl.h:89: static const int
kAwDrawGLFunctionTableVersion = 1;
On 2014/07/31 21:03:50, hush wrote:
> what is the circumstance that AwDrawGLFunctionTableVersion is different from
> AwDrawGLInfoVersion? Should they always be the same version?

They can change independently. For example if we switch to idle upload, we can
drop all the gralloc functions from the table below, which is ehh...everything.
But nothing changes to AwDrawGLInfoVersion

[boliu, 2014-07-31 21:10:47]

https://codereview.chromium.org/433603005/diff/1/android_webview/public/brows...
File android_webview/public/browser/draw_gl.h (right):

https://codereview.chromium.org/433603005/diff/1/android_webview/public/brows...
android_webview/public/browser/draw_gl.h:89: static const int
kAwDrawGLFunctionTableVersion = 1;
On 2014/07/31 21:09:38, boliu wrote:
> On 2014/07/31 21:03:50, hush wrote:
> > what is the circumstance that AwDrawGLFunctionTableVersion is different from
> > AwDrawGLInfoVersion? Should they always be the same version?
> 
> They can change independently. For example if we switch to idle upload, we can
> drop all the gralloc functions from the table below, which is
ehh...everything.
> But nothing changes to AwDrawGLInfoVersion

Right that's the idea. And you can't access new members based on version.

Binary stability..

[hush (inactive), 2014-07-31 21:13:18]

On 2014/07/31 21:09:38, boliu wrote:
>
https://codereview.chromium.org/433603005/diff/1/android_webview/public/brows...
> File android_webview/public/browser/draw_gl.h (right):
> 
>
https://codereview.chromium.org/433603005/diff/1/android_webview/public/brows...
> android_webview/public/browser/draw_gl.h:89: static const int
> kAwDrawGLFunctionTableVersion = 1;
> On 2014/07/31 21:03:50, hush wrote:
> > what is the circumstance that AwDrawGLFunctionTableVersion is different from
> > AwDrawGLInfoVersion? Should they always be the same version?
> 
> They can change independently. For example if we switch to idle upload, we can
> drop all the gralloc functions from the table below, which is
ehh...everything.
> But nothing changes to AwDrawGLInfoVersion

Oh. Okay.
So you can remove things from the struct. This means you need to exact match of
version number instead of doing less than/greater than comparison to claim the
version of struct contains the data you want or not.

[boliu, 2014-07-31 21:16:10]

On 2014/07/31 21:13:18, hush wrote:
> Oh. Okay.
> So you can remove things from the struct. This means you need to exact match
of
> version number instead of doing less than/greater than comparison to claim the
> version of struct contains the data you want or not.

Umm....I suppose you can, basically you use a union, and use the version number
to switch. But feels too dangerous.

Basically once things are frozen, you can never remove fields. You can deprecate
them, but never remove them.

Things are not frozen yet though.

[hush (inactive), 2014-07-31 21:19:11]

On 2014/07/31 21:16:10, boliu wrote:
> On 2014/07/31 21:13:18, hush wrote:
> > Oh. Okay.
> > So you can remove things from the struct. This means you need to exact match
> of
> > version number instead of doing less than/greater than comparison to claim
the
> > version of struct contains the data you want or not.
> 
> Umm....I suppose you can, basically you use a union, and use the version
number
> to switch. But feels too dangerous.
> 
> Basically once things are frozen, you can never remove fields. You can
deprecate
> them, but never remove them.
> 
> Things are not frozen yet though.

lgtm

Okay. Let's not remove fields from the struct when you bump a version. 
Maybe a comment there would help?

[boliu, 2014-07-31 21:29:50]

The CQ bit was checked by boliu@chromium.org

[boliu, 2014-07-31 21:30:20]

On 2014/07/31 21:19:11, hush wrote:
> Okay. Let's not remove fields from the struct when you bump a version. 
> Maybe a comment there would help?

Don't anyone touch this file without my say!!

[commit-bot: I haz the power, 2014-07-31 21:32:13]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/boliu@chromium.org/433603005/1

[commit-bot: I haz the power, 2014-08-01 00:02:58]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)

[commit-bot: I haz the power, 2014-08-01 00:19:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-01 00:19:02]

Try jobs failed on following builders:
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)

[hush (inactive), 2014-08-01 00:56:23]

On 2014/08/01 00:19:02, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   android_aosp on tryserver.chromium.linux
>
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)

frameworks/webview/chromium/plat_support/graphics_utils.cpp: In function 'jlong
android::{anonymous}::GetDrawSWFunctionTable(JNIEnv*, jclass)':
frameworks/webview/chromium/plat_support/graphics_utils.cpp:88:3: error: invalid
conversion from 'AwPixelInfo* (*)(JNIEnv*, jobject) {aka AwPixelInfo*
(*)(_JNIEnv*, _jobject*)}' to 'int' [-fpermissive]
   };
   ^
frameworks/webview/chromium/plat_support/graphics_utils.cpp:88:3: error: invalid
conversion from 'void (*)(AwPixelInfo*)' to 'AwPixelInfo* (*)(JNIEnv*, jobject)
{aka AwPixelInfo* (*)(_JNIEnv*, _jobject*)}' [-fpermissive]
frameworks/webview/chromium/plat_support/graphics_utils.cpp:88:3: warning:
missing initializer for member 'AwDrawSWFunctionTable::release_pixels'
[-Wmissing-field-initializers]
frameworks/webview/chromium/plat_support/graphics_utils.cpp: In function 'jlong
android::{anonymous}::GetDrawGLFunctionTable(JNIEnv*, jclass)':
frameworks/webview/chromium/plat_support/graphics_utils.cpp:100:3: error:
invalid conversion from 'long int (*)(int, int)' to 'int' [-fpermissive]
   };
   ^
frameworks/webview/chromium/plat_support/graphics_utils.cpp:100:3: error:
invalid conversion from 'void (*)(long int)' to 'long int (*)(int, int)'
[-fpermissive]
frameworks/webview/chromium/plat_support/graphics_utils.cpp:100:3: error:
invalid conversion from 'int (*)(long int, AwMapMode, void**)' to 'void (*)(long
int)' [-fpermissive]
frameworks/webview/chromium/plat_support/graphics_utils.cpp:100:3: error:
invalid conversion from 'int (*)(long int)' to 'int (*)(long int, AwMapMode,
void**)' [-fpermissive]
frameworks/webview/chromium/plat_support/graphics_utils.cpp:100:3: error:
invalid conversion from 'void* (*)(long int)' to 'int (*)(long int)'
[-fpermissive]
frameworks/webview/chromium/plat_support/graphics_utils.cpp:100:3: error:
invalid conversion from 'uint32_t (*)(long int) {aka unsigned int (*)(long
int)}' to 'void* (*)(long int)' [-fpermissive]
frameworks/webview/chromium/plat_support/graphics_utils.cpp:100:3: warning:
missing initializer for member 'AwDrawGLFunctionTable::get_stride'
[-Wmissing-field-initializers]
Import includes file:
out/target/product/generic/obj/SHARED_LIBRARIES/libjavacore_intermediates/import_includes
make: ***
[out/target/product/generic/obj/SHARED_LIBRARIES/libwebviewchromium_plat_support_intermediates/plat_support/graphics_utils.o]
Error 1
make: *** Waiting for unfinished jobs....

[boliu, 2014-08-01 00:59:08]

Yeah the current glue layer isn't written to allow new fields to be added. Need
to fix that first

[hush (inactive), 2014-08-01 01:01:44]

I think you need a frameworks/webview change for
GraphicsUtils::GetDrawSWFunctionTable

[boliu, 2014-08-06 16:22:03]

The CQ bit was checked by boliu@chromium.org

[commit-bot: I haz the power, 2014-08-06 16:23:22]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/boliu@chromium.org/433603005/20001

[boliu, 2014-08-06 16:25:39]

Rolled aosp btw, so it's green now.

[commit-bot: I haz the power, 2014-08-06 18:35:03]

Change committed as 287810