Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9384011:
Chrome on Android: adds jni_generator. (Closed)
|
Created:
8 years, 10 months ago by bulach Modified:
8 years, 10 months ago CC:
chromium-reviews, brettw-cc_chromium.org, Peter Beverloo Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionChrome on Android: adds jni_generator.
On Android, we use JNI to call into the system API.
This patch adds the generator for the bindings and all its tests.
We'll soon start adding the API wrappers themselves.
BUG=
TEST=base/android/jni_generator/jni_generator_tests.py
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=123374
Patch Set 1 : Patch #
Total comments: 23
Patch Set 2 : Joth's comments #
Total comments: 19
Patch Set 3 : Mark's comments #Patch Set 4 : Reflow comments. #
Total comments: 2
Patch Set 5 : Wraps python and .h output at 80cols. #
Messages
Total messages: 38 (0 generated)
Hi Mark, This is the patch for the jni generator. I also "upstreamed" the docs I sent you before, they're now available at: https://sites.google.com/a/chromium.org/dev/developers/design-documents/andro... would you mind taking a look, please? I'm also adding joth, as he's reviewed most of this code downstream. Thanks, Marcus
I didn't read any python in detail this time. http://codereview.chromium.org/9384011/diff/4001/base/android/jni_generator/S... File base/android/jni_generator/SampleForTests.java (right): http://codereview.chromium.org/9384011/diff/4001/base/android/jni_generator/S... base/android/jni_generator/SampleForTests.java:1: // Copyright (c) 2011 The Chromium Authors. All rights reserved. 2012 http://codereview.chromium.org/9384011/diff/4001/base/android/jni_generator/S... base/android/jni_generator/SampleForTests.java:5: package com.android.example.jni_generator; may as well be org.chromium... http://codereview.chromium.org/9384011/diff/4001/base/android/jni_generator/S... base/android/jni_generator/SampleForTests.java:74: // Note the "Unchecked" suffix. By default, @CalledByNative will always generate bindings that note to anyone else reading this: .java files have 100 column line limit. I wonder if we can get rietveld to honor this? http://codereview.chromium.org/9384011/diff/4001/base/android/jni_generator/g... File base/android/jni_generator/golden_sample_for_tests_jni.h (right): http://codereview.chromium.org/9384011/diff/4001/base/android/jni_generator/g... base/android/jni_generator/golden_sample_for_tests_jni.h:1: // Copyright (c) 2011 The Chromium Authors. All rights reserved. 2012 ? http://codereview.chromium.org/9384011/diff/4001/base/android/jni_generator/g... base/android/jni_generator/golden_sample_for_tests_jni.h:10: #define com_android_example_jni_generator_SampleForTests_JNI so this will be org_chromium_... http://codereview.chromium.org/9384011/diff/4001/base/android/jni_generator/j... File base/android/jni_generator/jni_generator.gyp (right): http://codereview.chromium.org/9384011/diff/4001/base/android/jni_generator/j... base/android/jni_generator/jni_generator.gyp:1: # Copyright (c) 2010 The Chromium Authors. All rights reserved. 2012 http://codereview.chromium.org/9384011/diff/4001/base/android/jni_generator/j... base/android/jni_generator/jni_generator.gyp:72: # vim: set expandtab tabstop=2 shiftwidth=2: this doesn't look a normal footer? http://codereview.chromium.org/9384011/diff/4001/base/android/jni_generator/j... File base/android/jni_generator/jni_generator.py (right): http://codereview.chromium.org/9384011/diff/4001/base/android/jni_generator/j... base/android/jni_generator/jni_generator.py:187: 'Lorg/chromium/chromeview/ChromeView$FindResultReceivedListener$FindNotificationDetails', nit: 80 http://codereview.chromium.org/9384011/diff/4001/base/android/jni_generator/j... base/android/jni_generator/jni_generator.py:825: g_${JAVA_CLASS}_clazz = *(new base::android::ScopedJavaGlobalRef<jclass>());""") oh I'd never noticed we changed to using ScopedJavaGlobalRef here. Given we never delete it, using a ScopedFoo gives very little benefit for a whole bunch of extra RAM usage (all allocated at startup) :( I think it maybe better to use GetMethodIDFromClassName() which handles caching internally, and means all the class/method lookups will happen lazily too. We can of course do that change at a later date :-) http://codereview.chromium.org/9384011/diff/4001/base/android/jni_generator/j... File base/android/jni_generator/jni_generator_tests.py (right): http://codereview.chromium.org/9384011/diff/4001/base/android/jni_generator/j... base/android/jni_generator/jni_generator_tests.py:54: private static native void nativeCreateHistoricalTabFromState(byte[] state, int tab_index); should .py be 80 col? Plenty to do here... ah hang on, it's embedded java. I don't recall why this is here in addition to having the golden sample files? http://codereview.chromium.org/9384011/diff/4001/base/android/jni_generator/s... File base/android/jni_generator/sample_for_tests.cc (right): http://codereview.chromium.org/9384011/diff/4001/base/android/jni_generator/s... base/android/jni_generator/sample_for_tests.cc:1: // Copyright (c) 2011 The Chromium Authors. All rights reserved. 2012 http://codereview.chromium.org/9384011/diff/4001/base/android/jni_generator/s... base/android/jni_generator/sample_for_tests.cc:4: maybe comment the main purpose of this is to ensure sample_for_tests_jni.h compiles and the functions are declared in it as expected.
thanks joth! all comments addressed, except the scoped jclass that I'll do separately.. https://chromiumcodereview.appspot.com/9384011/diff/4001/base/android/jni_gen... File base/android/jni_generator/SampleForTests.java (right): https://chromiumcodereview.appspot.com/9384011/diff/4001/base/android/jni_gen... base/android/jni_generator/SampleForTests.java:1: // Copyright (c) 2011 The Chromium Authors. All rights reserved. On 2012/02/14 00:46:49, joth wrote: > 2012 :) done. https://chromiumcodereview.appspot.com/9384011/diff/4001/base/android/jni_gen... base/android/jni_generator/SampleForTests.java:5: package com.android.example.jni_generator; On 2012/02/14 00:46:49, joth wrote: > may as well be org.chromium... Done. https://chromiumcodereview.appspot.com/9384011/diff/4001/base/android/jni_gen... File base/android/jni_generator/golden_sample_for_tests_jni.h (right): https://chromiumcodereview.appspot.com/9384011/diff/4001/base/android/jni_gen... base/android/jni_generator/golden_sample_for_tests_jni.h:1: // Copyright (c) 2011 The Chromium Authors. All rights reserved. On 2012/02/14 00:46:49, joth wrote: > 2012 ? Done. https://chromiumcodereview.appspot.com/9384011/diff/4001/base/android/jni_gen... base/android/jni_generator/golden_sample_for_tests_jni.h:10: #define com_android_example_jni_generator_SampleForTests_JNI On 2012/02/14 00:46:49, joth wrote: > so this will be org_chromium_... Done. https://chromiumcodereview.appspot.com/9384011/diff/4001/base/android/jni_gen... File base/android/jni_generator/jni_generator.gyp (right): https://chromiumcodereview.appspot.com/9384011/diff/4001/base/android/jni_gen... base/android/jni_generator/jni_generator.gyp:1: # Copyright (c) 2010 The Chromium Authors. All rights reserved. On 2012/02/14 00:46:49, joth wrote: > 2012 Done. https://chromiumcodereview.appspot.com/9384011/diff/4001/base/android/jni_gen... base/android/jni_generator/jni_generator.gyp:72: # vim: set expandtab tabstop=2 shiftwidth=2: On 2012/02/14 00:46:49, joth wrote: > this doesn't look a normal footer? Done. https://chromiumcodereview.appspot.com/9384011/diff/4001/base/android/jni_gen... File base/android/jni_generator/jni_generator.py (right): https://chromiumcodereview.appspot.com/9384011/diff/4001/base/android/jni_gen... base/android/jni_generator/jni_generator.py:187: 'Lorg/chromium/chromeview/ChromeView$FindResultReceivedListener$FindNotificationDetails', On 2012/02/14 00:46:49, joth wrote: > nit: 80 Done. https://chromiumcodereview.appspot.com/9384011/diff/4001/base/android/jni_gen... base/android/jni_generator/jni_generator.py:825: g_${JAVA_CLASS}_clazz = *(new base::android::ScopedJavaGlobalRef<jclass>());""") On 2012/02/14 00:46:49, joth wrote: > oh I'd never noticed we changed to using ScopedJavaGlobalRef here. Given we > never delete it, using a ScopedFoo gives very little benefit for a whole bunch > of extra RAM usage (all allocated at startup) :( > > I think it maybe better to use GetMethodIDFromClassName() which handles caching > internally, and means all the class/method lookups will happen lazily too. > > We can of course do that change at a later date :-) good point! let me see check how complicated this would be downstream first, it may be best to do as a separate step (either before or after this patch) rather than directly here.. https://chromiumcodereview.appspot.com/9384011/diff/4001/base/android/jni_gen... File base/android/jni_generator/jni_generator_tests.py (right): https://chromiumcodereview.appspot.com/9384011/diff/4001/base/android/jni_gen... base/android/jni_generator/jni_generator_tests.py:54: private static native void nativeCreateHistoricalTabFromState(byte[] state, int tab_index); On 2012/02/14 00:46:49, joth wrote: > should .py be 80 col? Plenty to do here... > > ah hang on, it's embedded java. I don't recall why this is here in addition to > having the golden sample files? these are different levels / aspect of testing than the golden samples.. I'm not sure it'd be feasible to make it in 80cols without compromising the readability of the string contents itself.. I added a comment on top as suggested.. https://chromiumcodereview.appspot.com/9384011/diff/4001/base/android/jni_gen... File base/android/jni_generator/sample_for_tests.cc (right): https://chromiumcodereview.appspot.com/9384011/diff/4001/base/android/jni_gen... base/android/jni_generator/sample_for_tests.cc:1: // Copyright (c) 2011 The Chromium Authors. All rights reserved. On 2012/02/14 00:46:49, joth wrote: > 2012 Done. https://chromiumcodereview.appspot.com/9384011/diff/4001/base/android/jni_gen... base/android/jni_generator/sample_for_tests.cc:4: On 2012/02/14 00:46:49, joth wrote: > maybe comment the main purpose of this is to ensure sample_for_tests_jni.h > compiles and the functions are declared in it as expected. Done.
http://codereview.chromium.org/9384011/diff/11001/base/android/jni_generator/... File base/android/jni_generator/SampleForTests.java (right): http://codereview.chromium.org/9384011/diff/11001/base/android/jni_generator/... base/android/jni_generator/SampleForTests.java:142: private native double nativeMethodOtherP0(int nativeCPPClass /* cpp_namespace::CPPClass */); Long line? http://codereview.chromium.org/9384011/diff/11001/base/android/jni_generator/... File base/android/jni_generator/jni_generator.gyp (right): http://codereview.chromium.org/9384011/diff/11001/base/android/jni_generator/... base/android/jni_generator/jni_generator.gyp:4: { It’s usual to put a blank line before this { to separate it from the boilerplate. http://codereview.chromium.org/9384011/diff/11001/base/android/jni_generator/... base/android/jni_generator/jni_generator.gyp:19: '', Really? No outputs? http://codereview.chromium.org/9384011/diff/11001/base/android/jni_generator/... base/android/jni_generator/jni_generator.gyp:22: 'python', './jni_generator_tests.py', You don’t need the ./ here. http://codereview.chromium.org/9384011/diff/11001/base/android/jni_generator/... File base/android/jni_generator/jni_generator.py (right): http://codereview.chromium.org/9384011/diff/11001/base/android/jni_generator/... base/android/jni_generator/jni_generator.py:1: #!/usr/bin/python I trust that this file has already been reviewed as-is. I can give it a light look-over, but if you can show me any changes from its current state wherever it’s checked in now, or a review history of the existing version, it might be helpful. http://codereview.chromium.org/9384011/diff/11001/base/android/jni_generator/... File base/android/jni_generator/jni_generator_tests.py (right): http://codereview.chromium.org/9384011/diff/11001/base/android/jni_generator/... base/android/jni_generator/jni_generator_tests.py:1: #!/usr/bin/python The same comment as in the previous file applies. http://codereview.chromium.org/9384011/diff/11001/base/android/jni_generator/... base/android/jni_generator/jni_generator_tests.py:1288: Don’t need this blank line. http://codereview.chromium.org/9384011/diff/11001/base/android/jni_generator/... File base/android/jni_generator/sample_for_tests.cc (right): http://codereview.chromium.org/9384011/diff/11001/base/android/jni_generator/... base/android/jni_generator/sample_for_tests.cc:46: // This is how you call a java static method from C++. For readability, it’s good to put a blank line before a comment like this, when the comment doesn’t have anything to do with the thing that comes before it, but has a lot to do with the thing that comes after it. Same on line 48. http://codereview.chromium.org/9384011/diff/11001/base/android/jni_generator/... base/android/jni_generator/sample_for_tests.cc:52: return 0; And then you’d want a blank line before this one, also for readability.
thanks mark! all comments addressed, another look please? http://codereview.chromium.org/9384011/diff/11001/base/android/jni_generator/... File base/android/jni_generator/SampleForTests.java (right): http://codereview.chromium.org/9384011/diff/11001/base/android/jni_generator/... base/android/jni_generator/SampleForTests.java:142: private native double nativeMethodOtherP0(int nativeCPPClass /* cpp_namespace::CPPClass */); On 2012/02/14 20:00:38, Mark Mentovai wrote: > Long line? this is going to be a fun ride! :) java's style guide is 100 cols.. this is just the very first file, but we should probably add a link to that styleguide in chromium? should I do it? http://codereview.chromium.org/9384011/diff/11001/base/android/jni_generator/... File base/android/jni_generator/jni_generator.gyp (right): http://codereview.chromium.org/9384011/diff/11001/base/android/jni_generator/... base/android/jni_generator/jni_generator.gyp:4: { On 2012/02/14 20:00:38, Mark Mentovai wrote: > It’s usual to put a blank line before this { to separate it from the > boilerplate. Done. http://codereview.chromium.org/9384011/diff/11001/base/android/jni_generator/... base/android/jni_generator/jni_generator.gyp:19: '', On 2012/02/14 20:00:38, Mark Mentovai wrote: > Really? No outputs? this action is just to run the python tests, it doesn't produce any output.. http://codereview.chromium.org/9384011/diff/11001/base/android/jni_generator/... base/android/jni_generator/jni_generator.gyp:22: 'python', './jni_generator_tests.py', On 2012/02/14 20:00:38, Mark Mentovai wrote: > You don’t need the ./ here. Done. http://codereview.chromium.org/9384011/diff/11001/base/android/jni_generator/... File base/android/jni_generator/jni_generator.py (right): http://codereview.chromium.org/9384011/diff/11001/base/android/jni_generator/... base/android/jni_generator/jni_generator.py:1: #!/usr/bin/python On 2012/02/14 20:00:38, Mark Mentovai wrote: > I trust that this file has already been reviewed as-is. I can give it a light > look-over, but if you can show me any changes from its current state wherever > it’s checked in now, or a review history of the existing version, it might be > helpful. I sent you separately the link for our forked codebase, I'll chase the admins to give you access to it. just for the records, joth@ (cc:d) reviewed and co-designed this with me, and of course, we're both happy to give you any detailed information to help clarify.. http://codereview.chromium.org/9384011/diff/11001/base/android/jni_generator/... File base/android/jni_generator/jni_generator_tests.py (right): http://codereview.chromium.org/9384011/diff/11001/base/android/jni_generator/... base/android/jni_generator/jni_generator_tests.py:1288: On 2012/02/14 20:00:38, Mark Mentovai wrote: > Don’t need this blank line. Done. http://codereview.chromium.org/9384011/diff/11001/base/android/jni_generator/... File base/android/jni_generator/sample_for_tests.cc (right): http://codereview.chromium.org/9384011/diff/11001/base/android/jni_generator/... base/android/jni_generator/sample_for_tests.cc:46: // This is how you call a java static method from C++. On 2012/02/14 20:00:38, Mark Mentovai wrote: > For readability, it’s good to put a blank line before a comment like this, when > the comment doesn’t have anything to do with the thing that comes before it, but > has a lot to do with the thing that comes after it. > > Same on line 48. Done. http://codereview.chromium.org/9384011/diff/11001/base/android/jni_generator/... base/android/jni_generator/sample_for_tests.cc:52: return 0; On 2012/02/14 20:00:38, Mark Mentovai wrote: > And then you’d want a blank line before this one, also for readability. Done.
http://codereview.chromium.org/9384011/diff/11001/base/android/jni_generator/... File base/android/jni_generator/SampleForTests.java (right): http://codereview.chromium.org/9384011/diff/11001/base/android/jni_generator/... base/android/jni_generator/SampleForTests.java:142: private native double nativeMethodOtherP0(int nativeCPPClass /* cpp_namespace::CPPClass */); bulach wrote: > On 2012/02/14 20:00:38, Mark Mentovai wrote: > > Long line? > > this is going to be a fun ride! :) > java's style guide is 100 cols.. this is just the very first file, but we should > probably add a link to that styleguide in chromium? should I do it? Well, all of your comments are wrapped at 80, so why’d you let this line run longer?
http://codereview.chromium.org/9384011/diff/11001/base/android/jni_generator/... File base/android/jni_generator/SampleForTests.java (right): http://codereview.chromium.org/9384011/diff/11001/base/android/jni_generator/... base/android/jni_generator/SampleForTests.java:142: private native double nativeMethodOtherP0(int nativeCPPClass /* cpp_namespace::CPPClass */); On 2012/02/15 19:32:36, Mark Mentovai wrote: > bulach wrote: > > On 2012/02/14 20:00:38, Mark Mentovai wrote: > > > Long line? > > > > this is going to be a fun ride! :) > > java's style guide is 100 cols.. this is just the very first file, but we > should > > probably add a link to that styleguide in chromium? should I do it? > > Well, all of your comments are wrapped at 80, so why’d you let this line run > longer? there are a few comments, say, 74-75, that aren't.. I don't think it was _intentional_ to wrap them at 80.. since the styleguide (that we'd need to add) says 100, should I re-format all of it at 100 then? this is the very first file we're doing here, so I'm keen to set a good history, whatever you think it's best.
If the file’s going to be 100, then use 100 consistently, don’t wrap at a narrower width. That just wastes space.
On 2012/02/15 21:47:05, Mark Mentovai wrote: > If the file’s going to be 100, then use 100 consistently, don’t wrap at a > narrower width. That just wastes space. sounds good, let me fix this now.. thanks!
On 2012/02/15 22:03:37, bulach wrote: > On 2012/02/15 21:47:05, Mark Mentovai wrote: > > If the file’s going to be 100, then use 100 consistently, don’t wrap at a > > narrower width. That just wastes space. > > sounds good, let me fix this now.. thanks! done.
LGTM, then.
On 2012/02/15 22:28:03, Mark Mentovai wrote: > LGTM, then. cool, thanks! I'll put through the trys and then cq.. really appreciate Mark, this is an important milestone for us!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bulach@chromium.org/9384011/23001
Presubmit check for 9384011-23001 failed and returned exit status 1. Running presubmit commit checks ... ** Presubmit Messages ** If this change has an associated bug, add BUG=[bug number]. ** Presubmit Warnings ** Found lines longer than 80 characters (first 5 shown). base/android/jni_generator/SampleForTests.java, line 7, 97 chars \ base/android/jni_generator/SampleForTests.java, line 13, 91 chars \ base/android/jni_generator/SampleForTests.java, line 16, 95 chars \ base/android/jni_generator/SampleForTests.java, line 20, 81 chars \ base/android/jni_generator/SampleForTests.java, line 31, 99 chars ** Presubmit ERRORS ** Run the command: svn pset svn:eol-style LF \ base/android/jni_generator/SampleForTests.java Presubmit checks took 1.2s to calculate.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bulach@chromium.org/9384011/23001
Presubmit check for 9384011-23001 failed and returned exit status 1. Running presubmit commit checks ... ** Presubmit Messages ** If this change has an associated bug, add BUG=[bug number]. ** Presubmit Warnings ** Found lines longer than 80 characters (first 5 shown). base/android/jni_generator/SampleForTests.java, line 7, 97 chars \ base/android/jni_generator/SampleForTests.java, line 13, 91 chars \ base/android/jni_generator/SampleForTests.java, line 16, 95 chars \ base/android/jni_generator/SampleForTests.java, line 20, 81 chars \ base/android/jni_generator/SampleForTests.java, line 31, 99 chars Presubmit checks took 1.1s to calculate.
opps just noticed a couple other small things since I was last looking at this stuff. http://codereview.chromium.org/9384011/diff/23001/base/android/jni_generator/... File base/android/jni_generator/golden_sample_for_tests_jni.h (right): http://codereview.chromium.org/9384011/diff/23001/base/android/jni_generator/... base/android/jni_generator/golden_sample_for_tests_jni.h:19: using base::android::ScopedJavaLocalRef; shouldn't have using in a .h file. (and it doesn't seem to be used anyway) http://codereview.chromium.org/9384011/diff/23001/base/android/jni_generator/... base/android/jni_generator/golden_sample_for_tests_jni.h:46: static jobject GetNonPODDatatype(JNIEnv* env, jobject obj); In the java it said this would be declared to return ScopedJavaLocalRef, but here it's actually returning naked jobject. (which seems to make more sense, seems this is going back over the JNI boundary anyway and we don't want to release the ref before returning it)
thanks joth! I'm still struggling with the CQ, maruel pointed me to the files and I'm working on them now.. once the infra is in, would you mind if I fix your comments as a follow up? they all make sense and are reasonably straightforward, but I'd prefer to avoid having such a massive patch floating around and do incremental, smaller changes.. how does it sound?
On 16 February 2012 10:37, <bulach@chromium.org> wrote: > thanks joth! I'm still struggling with the CQ, maruel pointed me to the > files > and I'm working on them now.. > > once the infra is in, would you mind if I fix your comments as a follow > up? they > all make sense and are reasonably straightforward, but I'd prefer to avoid > having such a massive patch floating around and do incremental, smaller > changes.. how does it sound? > > Definitely... I forgot this hadn't actually landed. Makes more sense to iterate these as a separate step given the amount of cross repo syncing involved in it at this point. Cheers
landed the presubmit check: http://codereview.chromium.org/9417023 I'll put this on the CQ again..
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bulach@chromium.org/9384011/23001
Presubmit check for 9384011-23001 failed and returned exit status 1. Running presubmit commit checks ... ** Presubmit Messages ** If this change has an associated bug, add BUG=[bug number]. ** Presubmit Warnings ** Found lines longer than 80 characters (first 5 shown). base/android/jni_generator/SampleForTests.java, line 7, 97 chars \ base/android/jni_generator/SampleForTests.java, line 13, 91 chars \ base/android/jni_generator/SampleForTests.java, line 16, 95 chars \ base/android/jni_generator/SampleForTests.java, line 20, 81 chars \ base/android/jni_generator/SampleForTests.java, line 31, 99 chars Presubmit checks took 1.6s to calculate.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bulach@chromium.org/9384011/23001
Presubmit check for 9384011-23001 failed and returned exit status 1. Running presubmit commit checks ... ** Presubmit Messages ** If this change has an associated bug, add BUG=[bug number]. ** Presubmit Warnings ** Found lines longer than 80 characters (first 5 shown). base/android/jni_generator/golden_sample_for_tests_jni.h, line 6, 82 chars \ base/android/jni_generator/golden_sample_for_tests_jni.h, line 23, 104 chars \ base/android/jni_generator/golden_sample_for_tests_jni.h, line 24, 97 chars \ base/android/jni_generator/golden_sample_for_tests_jni.h, line 30, 81 chars \ base/android/jni_generator/golden_sample_for_tests_jni.h, line 70, 95 chars Presubmit checks took 1.6s to calculate.
hi mark, quick question: I fixed the presubmit check to allow java files to have 100cols. there's however this auto-generated header that are used for tests... there are two options here: - bypass presubmit. - do a post processing step to split at 80 columns, and re-generate this golden data. what would you prefer?
The commit bot is complaining about golden_sample_for_tests_jni.h this time, not the Java file. Peter
On 2012/02/22 17:45:01, Peter Beverloo wrote: > The commit bot is complaining about golden_sample_for_tests_jni.h this time, not > the Java file. > > Peter Misread the message, sorry :-).
[as discussed on IM] Either way I propose bypassing the presubmit for this CL, to avoid holding up people waiting on it any longer. We can either edit pre-submit to ignore this file, or change the generator to aggressively wrap lines, as a follow-up. On 22 February 2012 17:43, <bulach@chromium.org> wrote: > hi mark, > > quick question: I fixed the presubmit check to allow java files to have > 100cols. > there's however this auto-generated header that are used for tests... > > there are two options here: > - bypass presubmit. > - do a post processing step to split at 80 columns, and re-generate this > golden > data. > > what would you prefer? > > > http://codereview.chromium.**org/9384011/<http://codereview.chromium.org/9384... >
the presubmit would also complain on the python itself, and the test... :( let me do the right thing and clear all of this up, it shouldn't take too long.. I'll upload a new patch in a couple of hours.
phew! :) it seems that the presubmit tests are now green.. there are no functional changes from the previous patchset in terms of the generated output, the only minor thing is that it now wraps the output (i.e., the generated header file) at 80cols.. I also cleaned up the test.py accordingly. would you mind another look please?
LGTM
thanks Mark! try jobs failures seem unrelated.. hitting the CQ once again.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bulach@chromium.org/9384011/31003
Try job failure for 9384011-31003 (retry) (retry) (retry) on win_rel for step "ui_tests". It's a second try, previously, step "ui_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bulach@chromium.org/9384011/31003
Try job failure for 9384011-31003 (retry) on win_rel for steps "unit_tests, browser_tests, ui_tests". It's a second try, previously, steps "unit_tests, browser_tests, ui_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bulach@chromium.org/9384011/31003
Change committed as 123374 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
