Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10832034:
Upstreaming AndroidProtocolAdapter. (Closed)
|
Created:
8 years, 5 months ago by felipeg Modified:
8 years, 4 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionUpstreaming AndroidProtocolAdapter.
We currently don't have a way of compiling or running java unittests upstream.
So the java unittest for AndroidProtocolAdapter is being done only downstream for now.
It should be upstreamed once we can.
BUG=136983
TEST=
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=149653
Patch Set 1 #
Total comments: 6
Patch Set 2 : #Patch Set 3 : #
Total comments: 6
Patch Set 4 : #
Total comments: 2
Patch Set 5 : #
Total comments: 29
Patch Set 6 : #Patch Set 7 : #
Total comments: 8
Patch Set 8 : #Patch Set 9 : #Patch Set 10 : #
Total comments: 4
Patch Set 11 : #
Total comments: 10
Patch Set 12 : #Patch Set 13 : #
Total comments: 2
Patch Set 14 : #Patch Set 15 : #Patch Set 16 : #Messages
Total messages: 30 (0 generated)
Hi guys, I would like you to review this cl. thakis@ for chrome/ avi@ for content/ skyostil@ as the original creator of these classes downstream. Thanks
+mkosiba@ who has also been looking at this lately.
No. If in your CL the only change in content is altering a file with constants, and all the other files are in chrome, you're doing it wrong. There is chrome/common/url_constants.h. Use it.
https://chromiumcodereview.appspot.com/10832034/diff/1/content/public/common/... File content/public/common/url_constants.cc (right): https://chromiumcodereview.appspot.com/10832034/diff/1/content/public/common/... content/public/common/url_constants.cc:60: const char kTelScheme[] = "tel"; Ditto. https://chromiumcodereview.appspot.com/10832034/diff/1/content/public/common/... File content/public/common/url_constants.h (right): https://chromiumcodereview.appspot.com/10832034/diff/1/content/public/common/... content/public/common/url_constants.h:69: extern const char kTelScheme[]; Geo and Tel aren't part of content URLs so they should probably be upstreamed separately.
Avi is right. Removed the constant from content:: and using the constant (that surprisingly was already present) from chrome:: https://chromiumcodereview.appspot.com/10832034/diff/1/content/public/common/... File content/public/common/url_constants.cc (right): https://chromiumcodereview.appspot.com/10832034/diff/1/content/public/common/... content/public/common/url_constants.cc:60: const char kTelScheme[] = "tel"; On 2012/07/26 15:12:32, Sami wrote: > Ditto. Done. https://chromiumcodereview.appspot.com/10832034/diff/1/content/public/common/... File content/public/common/url_constants.h (right): https://chromiumcodereview.appspot.com/10832034/diff/1/content/public/common/... content/public/common/url_constants.h:69: extern const char kTelScheme[]; On 2012/07/26 15:12:32, Sami wrote: > Geo and Tel aren't part of content URLs so they should probably be upstreamed > separately. Done.
https://chromiumcodereview.appspot.com/10832034/diff/1/content/public/common/... File content/public/common/url_constants.h (right): https://chromiumcodereview.appspot.com/10832034/diff/1/content/public/common/... content/public/common/url_constants.h:69: extern const char kTelScheme[]; On 2012/07/26 15:12:32, Sami wrote: > Geo and Tel aren't part of content URLs so they should probably be upstreamed > separately. These are leftovers from an old URL intercepting implementation (which had geo and tel whitelisted) and should be removed from downstream code. https://chromiumcodereview.appspot.com/10832034/diff/6001/chrome/browser/andr... File chrome/browser/android/android_protocol_adapter.cc (right): https://chromiumcodereview.appspot.com/10832034/diff/6001/chrome/browser/andr... chrome/browser/android/android_protocol_adapter.cc:5: // URL request job for reading data from an Android Java InputStream. this isn't entirely accurate. The AndroidProtocolAdapter is used to read from resources and assets. https://chromiumcodereview.appspot.com/10832034/diff/6001/chrome/browser/andr... File chrome/browser/android/android_stream_reader_url_request_job.h (right): https://chromiumcodereview.appspot.com/10832034/diff/6001/chrome/browser/andr... chrome/browser/android/android_stream_reader_url_request_job.h:25: class Delegate : public base::NonThreadSafe { I know it's like this downstream but this class shouldn't derive from NonThreadSafe. https://chromiumcodereview.appspot.com/10832034/diff/6001/chrome/chrome_brows... File chrome/chrome_browser.gypi (right): https://chromiumcodereview.appspot.com/10832034/diff/6001/chrome/chrome_brows... chrome/chrome_browser.gypi:5489: 'actions': [ this looks like a rule that could be shared. have you considered moving it under base/ and including it from there?
https://chromiumcodereview.appspot.com/10832034/diff/1/content/public/common/... File content/public/common/url_constants.h (right): https://chromiumcodereview.appspot.com/10832034/diff/1/content/public/common/... content/public/common/url_constants.h:69: extern const char kTelScheme[]; On 2012/07/26 16:01:04, Martin Kosiba wrote: > On 2012/07/26 15:12:32, Sami wrote: > > Geo and Tel aren't part of content URLs so they should probably be upstreamed > > separately. > > These are leftovers from an old URL intercepting implementation (which had geo > and tel whitelisted) and should be removed from downstream code. I will update the downstream code in a new CL. See that I removed those from upstream. https://chromiumcodereview.appspot.com/10832034/diff/6001/chrome/browser/andr... File chrome/browser/android/android_stream_reader_url_request_job.h (right): https://chromiumcodereview.appspot.com/10832034/diff/6001/chrome/browser/andr... chrome/browser/android/android_stream_reader_url_request_job.h:25: class Delegate : public base::NonThreadSafe { On 2012/07/26 16:01:05, Martin Kosiba wrote: > I know it's like this downstream but this class shouldn't derive from > NonThreadSafe. Done.
Thanks Martin, I addressed all comments, ptal. cheers -- Felipeg https://chromiumcodereview.appspot.com/10832034/diff/6001/chrome/browser/andr... File chrome/browser/android/android_protocol_adapter.cc (right): https://chromiumcodereview.appspot.com/10832034/diff/6001/chrome/browser/andr... chrome/browser/android/android_protocol_adapter.cc:5: // URL request job for reading data from an Android Java InputStream. On 2012/07/26 16:01:05, Martin Kosiba wrote: > this isn't entirely accurate. The AndroidProtocolAdapter is used to read from > resources and assets. Done. https://chromiumcodereview.appspot.com/10832034/diff/6001/chrome/chrome_brows... File chrome/chrome_browser.gypi (right): https://chromiumcodereview.appspot.com/10832034/diff/6001/chrome/chrome_brows... chrome/chrome_browser.gypi:5489: 'actions': [ On 2012/07/26 16:01:05, Martin Kosiba wrote: > this looks like a rule that could be shared. have you considered moving it under > base/ and including it from there? Done.
https://chromiumcodereview.appspot.com/10832034/diff/4010/chrome/browser/andr... File chrome/browser/android/android_protocol_adapter.cc (right): https://chromiumcodereview.appspot.com/10832034/diff/4010/chrome/browser/andr... chrome/browser/android/android_protocol_adapter.cc:5: // URL request job for reading data from an Android Java InputStream. doesn't look like this got changed.
https://chromiumcodereview.appspot.com/10832034/diff/4010/chrome/browser/andr... File chrome/browser/android/android_protocol_adapter.cc (right): https://chromiumcodereview.appspot.com/10832034/diff/4010/chrome/browser/andr... chrome/browser/android/android_protocol_adapter.cc:5: // URL request job for reading data from an Android Java InputStream. On 2012/07/27 13:34:25, Martin Kosiba wrote: > doesn't look like this got changed. Sorry, forgot to commit in my git client. Done.
LGTM, but you'll need an OWNER/committer approval as well.
Will look at the rest soon. https://chromiumcodereview.appspot.com/10832034/diff/5005/build/system_classe... File build/system_classes_jni_generator.gypi (right): https://chromiumcodereview.appspot.com/10832034/diff/5005/build/system_classe... build/system_classes_jni_generator.gypi:36: fi)', The Gyp Way is to put anything more complex than a short line in an external script and call the script instead. https://chromiumcodereview.appspot.com/10832034/diff/5005/chrome/android/java... File chrome/android/java/src/org/chromium/chrome/browser/AndroidProtocolAdapter.java (right): https://chromiumcodereview.appspot.com/10832034/diff/5005/chrome/android/java... chrome/android/java/src/org/chromium/chrome/browser/AndroidProtocolAdapter.java:92: context.getResources().getValue(id, value, true); The 5 lines above look like they'd make a good separate method https://chromiumcodereview.appspot.com/10832034/diff/5005/chrome/android/java... chrome/android/java/src/org/chromium/chrome/browser/AndroidProtocolAdapter.java:130: // we have permission to read (see b/3125650). Can you convert this to a public bug and update the comment? (What's the upstreaming plan for bugs? Having links to closed bugs in open code seems bad though, so please do this.) https://chromiumcodereview.appspot.com/10832034/diff/5005/chrome/browser/andr... File chrome/browser/android/android_protocol_adapter.cc (right): https://chromiumcodereview.appspot.com/10832034/diff/5005/chrome/browser/andr... chrome/browser/android/android_protocol_adapter.cc:21: #include "net/url_request/url_request.h" Redundant with .h file https://chromiumcodereview.appspot.com/10832034/diff/5005/chrome/browser/andr... chrome/browser/android/android_protocol_adapter.cc:89: if (scheme == chrome::kFileScheme && url.find(assetPrefix) != 0 && StartsWithASCII() in base/string_util.h would read better https://chromiumcodereview.appspot.com/10832034/diff/5005/chrome/browser/andr... File chrome/browser/android/android_protocol_adapter.h (right): https://chromiumcodereview.appspot.com/10832034/diff/5005/chrome/browser/andr... chrome/browser/android/android_protocol_adapter.h:8: #include <memory> Not needed https://chromiumcodereview.appspot.com/10832034/diff/5005/chrome/browser/andr... chrome/browser/android/android_protocol_adapter.h:11: #include "net/http/http_byte_range.h" Not needed https://chromiumcodereview.appspot.com/10832034/diff/5005/chrome/browser/andr... chrome/browser/android/android_protocol_adapter.h:13: #include "net/url_request/url_request_job.h" Not needed
Can you update http://www.chromium.org/developers/design-documents/android-jni with a note about the system jni file and explain when one would use one or the other? https://chromiumcodereview.appspot.com/10832034/diff/5005/chrome/browser/andr... File chrome/browser/android/android_protocol_adapter.cc (right): https://chromiumcodereview.appspot.com/10832034/diff/5005/chrome/browser/andr... chrome/browser/android/android_protocol_adapter.cc:124: // testing. nit: If this is only for testing, add "ForTesting" to the name https://chromiumcodereview.appspot.com/10832034/diff/5005/chrome/browser/andr... chrome/browser/android/android_protocol_adapter.cc:137: // OK to release, JNI binding. I'm not familiar with JNI enough to judge if this is true. I'm assuming someone who is familiar with it checked this when it was reviewed internally. https://chromiumcodereview.appspot.com/10832034/diff/5005/chrome/browser/andr... chrome/browser/android/android_protocol_adapter.cc:150: // We have to reset as GetApplicationContext() returns a jobject with a nit: s/ We/We/ https://chromiumcodereview.appspot.com/10832034/diff/5005/chrome/browser/andr... File chrome/browser/android/android_stream_reader_url_request_job.cc (right): https://chromiumcodereview.appspot.com/10832034/diff/5005/chrome/browser/andr... chrome/browser/android/android_stream_reader_url_request_job.cc:144: jbyteArray buffer = static_cast<jbyteArray>(buffer_.obj()); You shouldn't need the static_cast https://chromiumcodereview.appspot.com/10832034/diff/5005/chrome/browser/andr... chrome/browser/android/android_stream_reader_url_request_job.cc:154: int byte_count = static_cast<int>( nit: Why not using the corresponding stdint.h types? (int64_t for klongs, int32_t for jints)
https://chromiumcodereview.appspot.com/10832034/diff/5005/build/system_classe... File build/system_classes_jni_generator.gypi (right): https://chromiumcodereview.appspot.com/10832034/diff/5005/build/system_classe... build/system_classes_jni_generator.gypi:36: fi)', On 2012/07/27 14:22:23, Nico wrote: > The Gyp Way is to put anything more complex than a short line in an external > script and call the script instead. Done. https://chromiumcodereview.appspot.com/10832034/diff/5005/chrome/android/java... File chrome/android/java/src/org/chromium/chrome/browser/AndroidProtocolAdapter.java (right): https://chromiumcodereview.appspot.com/10832034/diff/5005/chrome/android/java... chrome/android/java/src/org/chromium/chrome/browser/AndroidProtocolAdapter.java:92: context.getResources().getValue(id, value, true); On 2012/07/27 14:22:23, Nico wrote: > The 5 lines above look like they'd make a good separate method Done. https://chromiumcodereview.appspot.com/10832034/diff/5005/chrome/android/java... chrome/android/java/src/org/chromium/chrome/browser/AndroidProtocolAdapter.java:130: // we have permission to read (see b/3125650). On 2012/07/27 14:22:23, Nico wrote: > Can you convert this to a public bug and update the comment? (What's the > upstreaming plan for bugs? Having links to closed bugs in open code seems bad > though, so please do this.) That bug is fixed, so I am assuming the comment is not needed anymore. Removed. https://chromiumcodereview.appspot.com/10832034/diff/5005/chrome/browser/andr... File chrome/browser/android/android_protocol_adapter.cc (right): https://chromiumcodereview.appspot.com/10832034/diff/5005/chrome/browser/andr... chrome/browser/android/android_protocol_adapter.cc:21: #include "net/url_request/url_request.h" On 2012/07/27 14:22:23, Nico wrote: > Redundant with .h file Done. https://chromiumcodereview.appspot.com/10832034/diff/5005/chrome/browser/andr... chrome/browser/android/android_protocol_adapter.cc:89: if (scheme == chrome::kFileScheme && url.find(assetPrefix) != 0 && On 2012/07/27 14:22:23, Nico wrote: > StartsWithASCII() in base/string_util.h would read better Done. https://chromiumcodereview.appspot.com/10832034/diff/5005/chrome/browser/andr... chrome/browser/android/android_protocol_adapter.cc:124: // testing. On 2012/07/27 15:23:45, Nico wrote: > nit: If this is only for testing, add "ForTesting" to the name Done. https://chromiumcodereview.appspot.com/10832034/diff/5005/chrome/browser/andr... chrome/browser/android/android_protocol_adapter.cc:137: // OK to release, JNI binding. On 2012/07/27 15:23:45, Nico wrote: > I'm not familiar with JNI enough to judge if this is true. I'm assuming someone > who is familiar with it checked this when it was reviewed internally. Yep. https://chromiumcodereview.appspot.com/10832034/diff/5005/chrome/browser/andr... chrome/browser/android/android_protocol_adapter.cc:150: // We have to reset as GetApplicationContext() returns a jobject with a On 2012/07/27 15:23:45, Nico wrote: > nit: s/ We/We/ Done. https://chromiumcodereview.appspot.com/10832034/diff/5005/chrome/browser/andr... File chrome/browser/android/android_protocol_adapter.h (right): https://chromiumcodereview.appspot.com/10832034/diff/5005/chrome/browser/andr... chrome/browser/android/android_protocol_adapter.h:8: #include <memory> On 2012/07/27 14:22:23, Nico wrote: > Not needed Done. https://chromiumcodereview.appspot.com/10832034/diff/5005/chrome/browser/andr... chrome/browser/android/android_protocol_adapter.h:11: #include "net/http/http_byte_range.h" On 2012/07/27 14:22:23, Nico wrote: > Not needed Done. https://chromiumcodereview.appspot.com/10832034/diff/5005/chrome/browser/andr... chrome/browser/android/android_protocol_adapter.h:13: #include "net/url_request/url_request_job.h" On 2012/07/27 14:22:23, Nico wrote: > Not needed Done. https://chromiumcodereview.appspot.com/10832034/diff/5005/chrome/browser/andr... File chrome/browser/android/android_stream_reader_url_request_job.cc (right): https://chromiumcodereview.appspot.com/10832034/diff/5005/chrome/browser/andr... chrome/browser/android/android_stream_reader_url_request_job.cc:144: jbyteArray buffer = static_cast<jbyteArray>(buffer_.obj()); On 2012/07/27 15:23:45, Nico wrote: > You shouldn't need the static_cast Done. https://chromiumcodereview.appspot.com/10832034/diff/5005/chrome/browser/andr... chrome/browser/android/android_stream_reader_url_request_job.cc:154: int byte_count = static_cast<int>( On 2012/07/27 15:23:45, Nico wrote: > nit: Why not using the corresponding stdint.h types? (int64_t for klongs, > int32_t for jints) Done.
LGTM A few minor comments below (the checkdeps is somewhat important). Please land after addressing these as you see fit. https://chromiumcodereview.appspot.com/10832034/diff/5005/chrome/android/java... File chrome/android/java/src/org/chromium/chrome/browser/AndroidProtocolAdapter.java (right): https://chromiumcodereview.appspot.com/10832034/diff/5005/chrome/android/java... chrome/android/java/src/org/chromium/chrome/browser/AndroidProtocolAdapter.java:130: // we have permission to read (see b/3125650). On 2012/07/30 16:48:54, felipeg1 wrote: > On 2012/07/27 14:22:23, Nico wrote: > > Can you convert this to a public bug and update the comment? (What's the > > upstreaming plan for bugs? Having links to closed bugs in open code seems bad > > though, so please do this.) > > > That bug is fixed, so I am assuming the comment is not needed anymore. > Removed. We often leave links to fixed bugs in comments to explain why code is the way it is. But if you think removing the comment is more appropriate than mirroring the internal bug in this case, ok. https://chromiumcodereview.appspot.com/10832034/diff/13005/build/android_jar_... File build/android_jar_location.sh (right): https://chromiumcodereview.appspot.com/10832034/diff/13005/build/android_jar_... build/android_jar_location.sh:1: #!/bin/sh checkdeps will go red after check-in for files with a shebang line that don't have the executable bit set. Either chmod +x this file, or remove the shebang line (up to you). https://chromiumcodereview.appspot.com/10832034/diff/13005/build/android_jar_... build/android_jar_location.sh:6: # Tries to find the locaiton of android.jar in the android SDK root dir. typo location https://chromiumcodereview.appspot.com/10832034/diff/13005/chrome/browser/and... File chrome/browser/android/android_protocol_adapter.cc (right): https://chromiumcodereview.appspot.com/10832034/diff/13005/chrome/browser/and... chrome/browser/android/android_protocol_adapter.cc:91: !StartsWithASCII(url, assetPrefix, true) && nit: If you say |StartsWithASCII(url, assetPrefix, /*case_sensitive=*/true) &&|, then it's obvious what the "true" means. Also in the next line. ("nit:" comments are completely optional, feel free to ignore) https://chromiumcodereview.appspot.com/10832034/diff/13005/chrome/browser/and... File chrome/browser/android/android_protocol_adapter.h (right): https://chromiumcodereview.appspot.com/10832034/diff/13005/chrome/browser/and... chrome/browser/android/android_protocol_adapter.h:8: #include "base/android/scoped_java_ref.h" Not needed either (?)
https://chromiumcodereview.appspot.com/10832034/diff/5005/chrome/android/java... File chrome/android/java/src/org/chromium/chrome/browser/AndroidProtocolAdapter.java (right): https://chromiumcodereview.appspot.com/10832034/diff/5005/chrome/android/java... chrome/android/java/src/org/chromium/chrome/browser/AndroidProtocolAdapter.java:130: // we have permission to read (see b/3125650). On 2012/07/31 14:07:13, Nico wrote: > On 2012/07/30 16:48:54, felipeg1 wrote: > > On 2012/07/27 14:22:23, Nico wrote: > > > Can you convert this to a public bug and update the comment? (What's the > > > upstreaming plan for bugs? Having links to closed bugs in open code seems > bad > > > though, so please do this.) > > > > > > That bug is fixed, so I am assuming the comment is not needed anymore. > > Removed. > > We often leave links to fixed bugs in comments to explain why code is the way it > is. But if you think removing the comment is more appropriate than mirroring the > internal bug in this case, ok. I will remove it, because the internal bug has lots of replies and it will be impractical to move those to crbug. Also the thing that matters is the CL that fixed the bug, and that CL is also internal and there is no easy way of moving it to a public website. I will bring this question to our team and if needed I can add the crbug and put it back here. does that sounds OK ? https://chromiumcodereview.appspot.com/10832034/diff/13005/build/android_jar_... File build/android_jar_location.sh (right): https://chromiumcodereview.appspot.com/10832034/diff/13005/build/android_jar_... build/android_jar_location.sh:1: #!/bin/sh On 2012/07/31 14:07:13, Nico wrote: > checkdeps will go red after check-in for files with a shebang line that don't > have the executable bit set. Either chmod +x this file, or remove the shebang > line (up to you). Done. https://chromiumcodereview.appspot.com/10832034/diff/13005/build/android_jar_... build/android_jar_location.sh:6: # Tries to find the locaiton of android.jar in the android SDK root dir. On 2012/07/31 14:07:13, Nico wrote: > typo location Done. https://chromiumcodereview.appspot.com/10832034/diff/13005/chrome/browser/and... File chrome/browser/android/android_protocol_adapter.cc (right): https://chromiumcodereview.appspot.com/10832034/diff/13005/chrome/browser/and... chrome/browser/android/android_protocol_adapter.cc:91: !StartsWithASCII(url, assetPrefix, true) && On 2012/07/31 14:07:13, Nico wrote: > nit: If you say |StartsWithASCII(url, assetPrefix, /*case_sensitive=*/true) &&|, > then it's obvious what the "true" means. Also in the next line. > > ("nit:" comments are completely optional, feel free to ignore) Done. I like this. thanks https://chromiumcodereview.appspot.com/10832034/diff/13005/chrome/browser/and... File chrome/browser/android/android_protocol_adapter.h (right): https://chromiumcodereview.appspot.com/10832034/diff/13005/chrome/browser/and... chrome/browser/android/android_protocol_adapter.h:8: #include "base/android/scoped_java_ref.h" On 2012/07/31 14:07:13, Nico wrote: > Not needed either (?) was for JNIEnv forward declared.
https://chromiumcodereview.appspot.com/10832034/diff/5005/chrome/android/java... File chrome/android/java/src/org/chromium/chrome/browser/AndroidProtocolAdapter.java (right): https://chromiumcodereview.appspot.com/10832034/diff/5005/chrome/android/java... chrome/android/java/src/org/chromium/chrome/browser/AndroidProtocolAdapter.java:130: // we have permission to read (see b/3125650). sgtm
drive-by comments https://chromiumcodereview.appspot.com/10832034/diff/4027/build/system_classe... File build/system_classes_jni_generator.gypi (right): https://chromiumcodereview.appspot.com/10832034/diff/4027/build/system_classe... build/system_classes_jni_generator.gypi:39: '<(SHARED_INTERMEDIATE_DIR)/chrome/jni/input_stream_jni.h', This isn't really re-usable and doesn't belong in build/ if you're hardcoding a specific class. Furthermore, we've changed it so that generated classes follow their java names so this should be InputStream_jni.h https://chromiumcodereview.appspot.com/10832034/diff/4027/chrome/chrome_brows... File chrome/chrome_browser.gypi (right): https://chromiumcodereview.appspot.com/10832034/diff/4027/chrome/chrome_brows... chrome/chrome_browser.gypi:5482: 'android/java/src/org/chromium/chrome/browser/IntentHelper.java', Does this work? Does it actually generate headers for the other classes too? Personally, I find this combined rule a little confusing. Only one of the files is a "system_class" as per the include below
FYI, I discussed with Yaron, and I am addressing these comments. I will ping when it is ready to be reviewed again. Thanks , felipeg On Tue, Jul 31, 2012 at 5:21 PM, <yfriedman@chromium.org> wrote: > drive-by comments > > > https://chromiumcodereview.**appspot.com/10832034/diff/** > 4027/build/system_classes_jni_**generator.gypi<https://chromiumcodereview.appspot.com/10832034/diff/4027/build/system_classes_jni_generator.gypi> > File build/system_classes_jni_**generator.gypi (right): > > https://chromiumcodereview.**appspot.com/10832034/diff/** > 4027/build/system_classes_jni_**generator.gypi#newcode39<https://chromiumcodereview.appspot.com/10832034/diff/4027/build/system_classes_jni_generator.gypi#newcode39> > build/system_classes_jni_**generator.gypi:39: > '<(SHARED_INTERMEDIATE_DIR)/**chrome/jni/input_stream_jni.h'**, > This isn't really re-usable and doesn't belong in build/ if you're > hardcoding a specific class. Furthermore, we've changed it so that > generated classes follow their java names so this should be > InputStream_jni.h > > https://chromiumcodereview.**appspot.com/10832034/diff/** > 4027/chrome/chrome_browser.**gypi<https://chromiumcodereview.appspot.com/10832034/diff/4027/chrome/chrome_browser.gypi> > File chrome/chrome_browser.gypi (right): > > https://chromiumcodereview.**appspot.com/10832034/diff/** > 4027/chrome/chrome_browser.**gypi#newcode5482<https://chromiumcodereview.appspot.com/10832034/diff/4027/chrome/chrome_browser.gypi#newcode5482> > chrome/chrome_browser.gypi:**5482: > 'android/java/src/org/**chromium/chrome/browser/**IntentHelper.java', > Does this work? Does it actually generate headers for the other classes > too? Personally, I find this combined rule a little confusing. Only one > of the files is a "system_class" as per the include below > > https://chromiumcodereview.**appspot.com/10832034/<https://chromiumcodereview... >
Thanks Yaron. Ptal https://chromiumcodereview.appspot.com/10832034/diff/4027/build/system_classe... File build/system_classes_jni_generator.gypi (right): https://chromiumcodereview.appspot.com/10832034/diff/4027/build/system_classe... build/system_classes_jni_generator.gypi:39: '<(SHARED_INTERMEDIATE_DIR)/chrome/jni/input_stream_jni.h', On 2012/07/31 15:21:30, Yaron wrote: > This isn't really re-usable and doesn't belong in build/ if you're hardcoding a > specific class. Furthermore, we've changed it so that generated classes follow > their java names so this should be InputStream_jni.h Done. https://chromiumcodereview.appspot.com/10832034/diff/4027/chrome/chrome_brows... File chrome/chrome_browser.gypi (right): https://chromiumcodereview.appspot.com/10832034/diff/4027/chrome/chrome_brows... chrome/chrome_browser.gypi:5482: 'android/java/src/org/chromium/chrome/browser/IntentHelper.java', On 2012/07/31 15:21:30, Yaron wrote: > Does this work? Does it actually generate headers for the other classes too? > Personally, I find this combined rule a little confusing. Only one of the files > is a "system_class" as per the include below Done.
Can you land the gyp changes downstream first to make sure everything works. https://chromiumcodereview.appspot.com/10832034/diff/14005/build/android_jar_... File build/android_jar_location.sh (right): https://chromiumcodereview.appspot.com/10832034/diff/14005/build/android_jar_... build/android_jar_location.sh:8: if [ -f $ANDROID_SDK_ROOT/android.jar ]; then You can remove this file. https://chromiumcodereview.appspot.com/10832034/diff/14005/build/jar_file_jni... File build/jar_file_jni_generator.gypi (right): https://chromiumcodereview.appspot.com/10832034/diff/14005/build/jar_file_jni... build/jar_file_jni_generator.gypi:15: # 'android_sdk': '/somewhere/sdk/', This would have to go too. https://chromiumcodereview.appspot.com/10832034/diff/14005/build/jar_file_jni... build/jar_file_jni_generator.gypi:20: # 'includes': [ '../build/system_classes_jni_generator.gypi' ], update https://chromiumcodereview.appspot.com/10832034/diff/14005/build/jar_file_jni... build/jar_file_jni_generator.gypi:35: '<(SHARED_INTERMEDIATE_DIR)/<(jni_gen_dir)/jni/<(RULE_INPUT_ROOT)_jni.h', I don't think this generates it with the right name. In jni_generator.gypi the "inputs" are java files. Here it's a jar. You don't want the generated header to have the jar name in it's path https://chromiumcodereview.appspot.com/10832034/diff/14005/chrome/chrome_brow... File chrome/chrome_browser.gypi (right): https://chromiumcodereview.appspot.com/10832034/diff/14005/chrome/chrome_brow... chrome/chrome_browser.gypi:5496: 'android_sdk%': Why is |android_sdk| being set here? It's now being passed in from gyp. Please remove and let me know if it doesn't work.
https://chromiumcodereview.appspot.com/10832034/diff/14005/build/android_jar_... File build/android_jar_location.sh (right): https://chromiumcodereview.appspot.com/10832034/diff/14005/build/android_jar_... build/android_jar_location.sh:8: if [ -f $ANDROID_SDK_ROOT/android.jar ]; then On 2012/08/01 20:27:26, Yaron wrote: > You can remove this file. Done. https://chromiumcodereview.appspot.com/10832034/diff/14005/build/jar_file_jni... File build/jar_file_jni_generator.gypi (right): https://chromiumcodereview.appspot.com/10832034/diff/14005/build/jar_file_jni... build/jar_file_jni_generator.gypi:15: # 'android_sdk': '/somewhere/sdk/', On 2012/08/01 20:27:26, Yaron wrote: > This would have to go too. Done. https://chromiumcodereview.appspot.com/10832034/diff/14005/build/jar_file_jni... build/jar_file_jni_generator.gypi:20: # 'includes': [ '../build/system_classes_jni_generator.gypi' ], On 2012/08/01 20:27:26, Yaron wrote: > update Done. https://chromiumcodereview.appspot.com/10832034/diff/14005/build/jar_file_jni... build/jar_file_jni_generator.gypi:35: '<(SHARED_INTERMEDIATE_DIR)/<(jni_gen_dir)/jni/<(RULE_INPUT_ROOT)_jni.h', On 2012/08/01 20:27:26, Yaron wrote: > I don't think this generates it with the right name. In jni_generator.gypi the > "inputs" are java files. Here it's a jar. You don't want the generated header to > have the jar name in it's path Done. https://chromiumcodereview.appspot.com/10832034/diff/14005/chrome/chrome_brow... File chrome/chrome_browser.gypi (right): https://chromiumcodereview.appspot.com/10832034/diff/14005/chrome/chrome_brow... chrome/chrome_browser.gypi:5496: 'android_sdk%': On 2012/08/01 20:27:26, Yaron wrote: > Why is |android_sdk| being set here? It's now being passed in from gyp. Please > remove and let me know if it doesn't work. Done.
lgtm
Hey Yaron, That solution still didn't work. I had to change it again. Ptal in the new uploaded patch. It should work now. On Thu, Aug 2, 2012 at 5:04 PM, <yfriedman@chromium.org> wrote: > lgtm > > > > https://chromiumcodereview.**appspot.com/10832034/<https://chromiumcodereview... >
https://chromiumcodereview.appspot.com/10832034/diff/15016/build/jar_file_jni... File build/jar_file_jni_generator.gypi (right): https://chromiumcodereview.appspot.com/10832034/diff/15016/build/jar_file_jni... build/jar_file_jni_generator.gypi:14: # 'input_jar_file': '<(android_sdk)/android.jar', Update https://chromiumcodereview.appspot.com/10832034/diff/15016/chrome/chrome_brow... File chrome/chrome_browser.gypi (right): https://chromiumcodereview.appspot.com/10832034/diff/15016/chrome/chrome_brow... chrome/chrome_browser.gypi:5498: '<(android_sdk)/android.jar', I think that sources should really be the set of class files as I can imagine a scenario of wanting to extract multiple classes from a single jar. Can you not apply the same transformations on <(RULE_INPUT_PATH)?
OK, done as discussed by chat. Ptal On Thu, Aug 2, 2012 at 5:17 PM, <yfriedman@chromium.org> wrote: > > https://chromiumcodereview.**appspot.com/10832034/diff/** > 15016/build/jar_file_jni_**generator.gypi<https://chromiumcodereview.appspot.com/10832034/diff/15016/build/jar_file_jni_generator.gypi> > File build/jar_file_jni_generator.**gypi (right): > > https://chromiumcodereview.**appspot.com/10832034/diff/** > 15016/build/jar_file_jni_**generator.gypi#newcode14<https://chromiumcodereview.appspot.com/10832034/diff/15016/build/jar_file_jni_generator.gypi#newcode14> > build/jar_file_jni_generator.**gypi:14: # 'input_jar_file': > '<(android_sdk)/android.jar', > Update > > https://chromiumcodereview.**appspot.com/10832034/diff/** > 15016/chrome/chrome_browser.**gypi<https://chromiumcodereview.appspot.com/10832034/diff/15016/chrome/chrome_browser.gypi> > File chrome/chrome_browser.gypi (right): > > https://chromiumcodereview.**appspot.com/10832034/diff/** > 15016/chrome/chrome_browser.**gypi#newcode5498<https://chromiumcodereview.appspot.com/10832034/diff/15016/chrome/chrome_browser.gypi#newcode5498> > chrome/chrome_browser.gypi:**5498: '<(android_sdk)/android.jar', > I think that sources should really be the set of class files as I can > imagine a scenario of wanting to extract multiple classes from a single > jar. Can you not apply the same transformations on <(RULE_INPUT_PATH)? > > https://chromiumcodereview.**appspot.com/10832034/<https://chromiumcodereview... >
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/felipeg@chromium.org/10832034/3024
Change committed as 149653 |
