Comment enum matching assert to allow changes in Blink.

Comment enum matching assert to allow changes in Blink.

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

[jochen (gone - plz use gerrit), 2014-03-27 20:55:13]

We should change chromium/blink such that at any time we have a working system.

you could add something like this to chrome

#if defined(TEMPORARY_DEFINE_FOR_SCREEN_ORIENTATION)
// ... assert matching enums with new enums
#else
// ... current code
#endif

and then change blink to #define TEMPORARY_DEFINE_FOR_SCREEN_ORIENTATION when
you change the enums in blink

[mlamouri (slow - plz ping), 2014-03-31 16:17:16]

On 2014/03/27 20:55:13, jochen wrote:
> We should change chromium/blink such that at any time we have a working
system.
> 
> you could add something like this to chrome
> 
> #if defined(TEMPORARY_DEFINE_FOR_SCREEN_ORIENTATION)
> // ... assert matching enums with new enums
> #else
> // ... current code
> #endif
> 
> and then change blink to #define TEMPORARY_DEFINE_FOR_SCREEN_ORIENTATION when
> you change the enums in blink

I unfortunately can't do that because PORTRAIT_PRIMARY, PORTRAIT_SECONDARY, etc
are defined in content/public/common/screen_orientation_values_list.h which is
included in Java (to generate a list) and C++ to check the values. To be able to
do what you suggested, I would have to #include WebScreenOrientation.h in that
file but I can't #include files there because Java doesn't like it ;)

Sadly, I think commenting is the only solution we have here given the situation.
Alternatively, I can put some bandaid in Blink until the Chrome side is fixed. I
have a patch doing that but it is basically the same idea so I prefer to get
your opinion so I do not cheat and work around your wishes ;)

[jochen (gone - plz use gerrit), 2014-04-01 11:33:06]

On 2014/03/31 16:17:16, Mounir Lamouri wrote:
> On 2014/03/27 20:55:13, jochen wrote:
> > We should change chromium/blink such that at any time we have a working
> system.
> > 
> > you could add something like this to chrome
> > 
> > #if defined(TEMPORARY_DEFINE_FOR_SCREEN_ORIENTATION)
> > // ... assert matching enums with new enums
> > #else
> > // ... current code
> > #endif
> > 
> > and then change blink to #define TEMPORARY_DEFINE_FOR_SCREEN_ORIENTATION
when
> > you change the enums in blink
> 
> I unfortunately can't do that because PORTRAIT_PRIMARY, PORTRAIT_SECONDARY,
etc
> are defined in content/public/common/screen_orientation_values_list.h which is
> included in Java (to generate a list) and C++ to check the values. To be able
to
> do what you suggested, I would have to #include WebScreenOrientation.h in that
> file but I can't #include files there because Java doesn't like it ;)
> 
> Sadly, I think commenting is the only solution we have here given the
situation.
> Alternatively, I can put some bandaid in Blink until the Chrome side is fixed.
I
> have a patch doing that but it is basically the same idea so I prefer to get
> your opinion so I do not cheat and work around your wishes ;)

so my concern with this patch is that if it lands and then you switch the blink
side, there's a window where the enums actually don't match, no?

If you can make sure with a blink side patch that such a time window does not
exists, that is fine by me!

[mlamouri (slow - plz ping), 2014-04-01 12:24:58]

On 2014/04/01 11:33:06, jochen wrote:
> so my concern with this patch is that if it lands and then you switch the
blink
> side, there's a window where the enums actually don't match, no?
> 
> If you can make sure with a blink side patch that such a time window does not
> exists, that is fine by me!

There will be such a window. The only thing I can think of to prevent that
window from happening would be to add new values in the list or hard code some
values in the Java side. However, given that the feature is still behind a flag,
I wouldn't worry too much about having it broken during a short time. I'm
worried that we might spend too much effort trying to avoid a window of breakage
that no one will notice.

[jochen (gone - plz use gerrit), 2014-04-01 18:56:04]

On 2014/04/01 12:24:58, Mounir Lamouri wrote:
> On 2014/04/01 11:33:06, jochen wrote:
> > so my concern with this patch is that if it lands and then you switch the
> blink
> > side, there's a window where the enums actually don't match, no?
> > 
> > If you can make sure with a blink side patch that such a time window does
not
> > exists, that is fine by me!
> 
> There will be such a window. The only thing I can think of to prevent that
> window from happening would be to add new values in the list or hard code some
> values in the Java side. However, given that the feature is still behind a
flag,
> I wouldn't worry too much about having it broken during a short time. I'm
> worried that we might spend too much effort trying to avoid a window of
breakage
> that no one will notice.

so there's no test that would break? o_O

[mlamouri (slow - plz ping), 2014-04-01 19:31:42]

On 2014/04/01 18:56:04, jochen wrote:
> so there's no test that would break? o_O

There is no integration test because ContentShellTest on Android does not seem
to enable test and experimental features. I have that marked in my list of
things to investigate but otherwise, I believe that all the tests are in small
components so whether or not the Java side understand the values isn't really
there business. Regarding the Java tests, they check that getting a specific
value does the expected thing. The fact that the value is known by the caller is
out of their scope. Somehow, those COMPILE_ASSERT are doing the link between
everything.

[jochen (gone - plz use gerrit), 2014-04-02 09:39:42]

On 2014/04/01 19:31:42, Mounir Lamouri wrote:
> On 2014/04/01 18:56:04, jochen wrote:
> > so there's no test that would break? o_O
> 
> There is no integration test because ContentShellTest on Android does not seem
> to enable test and experimental features. I have that marked in my list of
> things to investigate but otherwise, I believe that all the tests are in small
> components so whether or not the Java side understand the values isn't really
> there business. Regarding the Java tests, they check that getting a specific
> value does the expected thing. The fact that the value is known by the caller
is
> out of their scope. Somehow, those COMPILE_ASSERT are doing the link between
> everything.

ok

so not having any test that breaks if you entirely break the feature (and I
expect that the constants not matching up should break the feature entirely) is
a problem. I guess there's no point in blocking this further, but please file a
bug about adding a test that would catch such a mismatch in the future.

[jochen (gone - plz use gerrit), 2014-04-02 09:39:47]

lgtm

[mlamouri (slow - plz ping), 2014-04-02 10:01:17]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2014-04-02 10:01:25]

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

[commit-bot: I haz the power, 2014-04-02 16:54:39]

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

[commit-bot: I haz the power, 2014-04-02 16:54:39]

Retried try job too often on android_aosp for step(s) compile
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_ao...

[mlamouri (slow - plz ping), 2014-04-02 21:24:39]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2014-04-02 21:25:32]

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

[commit-bot: I haz the power, 2014-04-03 06:28:06]

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

[commit-bot: I haz the power, 2014-04-03 06:28:07]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_rel

[mlamouri (slow - plz ping), 2014-04-03 09:41:58]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2014-04-03 09:43:21]

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

[commit-bot: I haz the power, 2014-04-03 10:05:07]

Change committed as 261332