Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10073024:
Add NetworkChangeNotifier for Android. (Closed)
|
Created:
8 years, 8 months ago by Yaron Modified:
8 years, 8 months ago CC:
chromium-reviews, erikwright (departed), cbentzel+watch_chromium.org, darin-cc_chromium.org, brettw-cc_chromium.org, John Knottenbelt Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd NetworkChangeNotifier for Android.
The native notifier spawns a Java-side notifier that registers for
notifications from Android's ConnectivityManager.
As for the lack of tests, they're written at a higher-level downstream.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=133142
Patch Set 1 #
Total comments: 39
Patch Set 2 : #
Total comments: 9
Patch Set 3 : add net/android/OWNERS, remove namespace #
Total comments: 2
Patch Set 4 : make register public #
Total comments: 4
Patch Set 5 : rebase on base chagnes #
Messages
Total messages: 17 (0 generated)
jrg: base/android net/android/java Note that I'll probably have to update the paths in the ant file assuming your apk-testrunner lands first. Also, should I add an OWNERS file in net/android to match base/android? rsleevi: The rest. I used the factory method approach based on a suggestion from willchan when I originally wrote this. I assume you guys are moving over to that approach, but I can switch to a plain old static create function if you'd prefer. Some context: This is upstreaming existing code from our fork. It currently has no effect because ChromeBrowserMainPartsAndroid hasn't been upstreamed, but it compiles/links in both C++/java.
Yes add OWNERS http://codereview.chromium.org/10073024/diff/1/net/android/java/java.gyp File net/android/java/java.gyp (right): http://codereview.chromium.org/10073024/diff/1/net/android/java/java.gyp#newc... net/android/java/java.gyp:22: '<(DEPTH)/net/android/java/dist/lib/chromium_net.jar', let's put output in a standard location outside the source tree E.g. see <property name="out.dir" location="${PRODUCT_DIR}"/> in http://chromiumcodereview.appspot.com/10051021/diff/8004/base/android/java/ba... (and the gyp change that defines it) http://codereview.chromium.org/10073024/diff/1/net/android/java/java.gyp#newc... net/android/java/java.gyp:26: '-buildfile', see more stuff here see http://chromiumcodereview.appspot.com/10051021/diff/8004/base/android/java/ja...
yaron: While not trying to slow down the upstreaming work, I'm still a youthfully idealistic Chromie and have some concerns with the approach. I'm more than happy to help however I can, so if you have any questions, please don't hesitate to ask. Since we're still very early on in the Java-upstreaming parts, I've love to come up with a robust solution that survives any tooling changes, while still giving you guys the flexibility you need. I'd like to work out the details before too much gets upstreamed, as then we're fighting both inertia and likely tooling. http://codereview.chromium.org/10073024/diff/1/net/android/java/java.gyp File net/android/java/java.gyp (right): http://codereview.chromium.org/10073024/diff/1/net/android/java/java.gyp#newc... net/android/java/java.gyp:15: '<(DEPTH)/net/android/java/net.xml', In Chromium code, we really, really try to minimize the use of '<(DEPTH)'. The exception being the WebKit code, as there's Chromium-In-WebKit (over in the WK tree) as much as there is WebKit-in-Chromium (as it is in the Chromium tree). Can you explain why this is necessary? http://codereview.chromium.org/10073024/diff/1/net/android/java/java.gyp#newc... net/android/java/java.gyp:16: '<!@(find <(DEPTH)/net/android/java -name "*.java")' This is really not ideal. The request for this has come up several times on gyp-developer, and this has consistently been dismissed as not consistent with the syntax GYP tries to promote (even though yes, it does work). I'd really like to push back on this before it becomes wide-spread, as I raised it when base/android/java/java.gyp landed Note also that base/android/java/java.gyp makes some implicit assumptions about where subcommands are run that are not guaranteed to hold. I see you've avoided this by using <(DEPTH) (which is good), but base/android/... will need to be fixed at some point. http://codereview.chromium.org/10073024/diff/1/net/android/java/java.gyp#newc... net/android/java/java.gyp:22: '<(DEPTH)/net/android/java/dist/lib/chromium_net.jar', Dropping files into the source tree = please avoid at all costs. See comments on net.xml http://codereview.chromium.org/10073024/diff/1/net/android/java/java.gyp#newc... net/android/java/java.gyp:27: '<(DEPTH)/net/android/java/net.xml', Rather than repeat this (shared with src/base/android/java/java.gyp), consider using an 'java.gypi' target combined with 'variables' to set the appropriate outputs as needed. 'java.gypi' can then defer to 'ant'. An example of how to do this can be seen with 'protoc.gypi' http://codereview.chromium.org/10073024/diff/1/net/android/java/net.xml File net/android/java/net.xml (right): http://codereview.chromium.org/10073024/diff/1/net/android/java/net.xml#newcode3 net/android/java/net.xml:3: building net java source code with ant nit? Building net/ java source code with ant. http://codereview.chromium.org/10073024/diff/1/net/android/java/net.xml#newco... net/android/java/net.xml:13: <property name="dist" location="dist"/> Does this mean your generated java files are being dumped into the source tree? I don't see where you properly map it as the other builders/generators do (eg: out/Debug) If you're assuming the generator runs from within that sub directory, that's also a guarantee that isn't part of GYPs design and is subject to ongoing discussions. http://codereview.chromium.org/10073024/diff/1/net/android/java/net.xml#newco... net/android/java/net.xml:38: makefiles are generated. --> Is there a bug (public or private) for this? It would be good to track publicly, as this is being upstreamed. http://codereview.chromium.org/10073024/diff/1/net/android/network_change_not... File net/android/network_change_notifier.cc (right): http://codereview.chromium.org/10073024/diff/1/net/android/network_change_not... net/android/network_change_notifier.cc:11: #include "jni/network_change_notifier_jni.h" This seems like a cheap hack to avoid the "don't use using within headers". If this header cannot compile without the definition, this header should be fixed. http://codereview.chromium.org/10073024/diff/1/net/android/network_change_not... net/android/network_change_notifier.cc:16: nit: delete this whitespace http://codereview.chromium.org/10073024/diff/1/net/android/network_change_not... net/android/network_change_notifier.cc:19: CHECK(java_network_change_notifier_android_object_.is_null()); Why CHECK here? You're in a constructor - how is this not guaranteed? http://codereview.chromium.org/10073024/diff/1/net/android/network_change_not... net/android/network_change_notifier.cc:24: CHECK(!java_network_change_notifier_android_object_.is_null()); Unnecessary CHECK? This is also guaranteed. http://codereview.chromium.org/10073024/diff/1/net/android/network_change_not... net/android/network_change_notifier.cc:28: java_network_change_notifier_android_object_.Reset(); Isn't this unnecessary and guaranteed by the destructor of |java_network_change_notifier_android_object_| ? http://codereview.chromium.org/10073024/diff/1/net/android/network_change_not... net/android/network_change_notifier.cc:37: CHECK(!java_network_change_notifier_android_object_.is_null()); Do you really need to CHECK here? Would a DCHECK suffice? Are both superflous and the user will end up crashing on a null-deref when they actually try to use it, accomplishing the same thing? http://codereview.chromium.org/10073024/diff/1/net/android/network_change_not... net/android/network_change_notifier.cc:45: CHECK(!java_network_change_notifier_android_object_.is_null()); And here http://codereview.chromium.org/10073024/diff/1/net/android/network_change_not... File net/android/network_change_notifier.h (right): http://codereview.chromium.org/10073024/diff/1/net/android/network_change_not... net/android/network_change_notifier.h:32: java_network_change_notifier_android_object_; nit: shorten the variable name? Certainly, "_object" seems superfluous? http://codereview.chromium.org/10073024/diff/1/net/android/network_change_not... net/android/network_change_notifier.h:37: bool RegisterNetworkChangeNotifier(JNIEnv* env); Any reason to have this free floating? Can you not just make it a static member on NCN? net::android::NetworkChangeNotifier::Register(JNIEnv* env) ? http://codereview.chromium.org/10073024/diff/1/net/net.gyp File net/net.gyp (right): http://codereview.chromium.org/10073024/diff/1/net/net.gyp#newcode1815 net/net.gyp:1815: '<@(_outputs)', Please consider using jni_generator.gypi, as mentioned elsewhere.
Re-based on top of other patch. We're still missing net.xml being auto-generated but Peter is workign on that seperately http://codereview.chromium.org/10073024/diff/1/net/android/java/java.gyp File net/android/java/java.gyp (right): http://codereview.chromium.org/10073024/diff/1/net/android/java/java.gyp#newc... net/android/java/java.gyp:15: '<(DEPTH)/net/android/java/net.xml', On 2012/04/13 22:37:58, Ryan Sleevi wrote: > In Chromium code, we really, really try to minimize the use of '<(DEPTH)'. The > exception being the WebKit code, as there's Chromium-In-WebKit (over in the WK > tree) as much as there is WebKit-in-Chromium (as it is in the Chromium tree). > > Can you explain why this is necessary? It's not. I didn't realize it was frowned upon. http://codereview.chromium.org/10073024/diff/1/net/android/java/java.gyp#newc... net/android/java/java.gyp:16: '<!@(find <(DEPTH)/net/android/java -name "*.java")' On 2012/04/13 22:37:58, Ryan Sleevi wrote: > This is really not ideal. The request for this has come up several times on > gyp-developer, and this has consistently been dismissed as not consistent with > the syntax GYP tries to promote (even though yes, it does work). > > I'd really like to push back on this before it becomes wide-spread, as I raised > it when base/android/java/java.gyp landed > > Note also that base/android/java/java.gyp makes some implicit assumptions about > where subcommands are run that are not guaranteed to hold. I see you've avoided > this by using <(DEPTH) (which is good), but base/android/... will need to be > fixed at some point. This was discussed in the spin-off bug http://codereview.chromium.org/10073024/diff/1/net/android/java/java.gyp#newc... net/android/java/java.gyp:22: '<(DEPTH)/net/android/java/dist/lib/chromium_net.jar', On 2012/04/13 22:37:58, Ryan Sleevi wrote: > Dropping files into the source tree = please avoid at all costs. See comments on > net.xml Yep, I agree but was again following base for consistency and figured they'd all be fixed together. John's fixing this in http://codereview.chromium.org/10051021 and I figured he'd land first and I'd update accordingly but I'll make the changes now. http://codereview.chromium.org/10073024/diff/1/net/android/java/java.gyp#newc... net/android/java/java.gyp:22: '<(DEPTH)/net/android/java/dist/lib/chromium_net.jar', On 2012/04/13 22:37:30, John Grabowski wrote: > let's put output in a standard location outside the source tree > E.g. see > <property name="out.dir" location="${PRODUCT_DIR}"/> > in > http://chromiumcodereview.appspot.com/10051021/diff/8004/base/android/java/ba... > (and the gyp change that defines it) Done. http://codereview.chromium.org/10073024/diff/1/net/android/java/java.gyp#newc... net/android/java/java.gyp:26: '-buildfile', On 2012/04/13 22:37:30, John Grabowski wrote: > see more stuff here > see > http://chromiumcodereview.appspot.com/10051021/diff/8004/base/android/java/ja... Done. http://codereview.chromium.org/10073024/diff/1/net/android/java/java.gyp#newc... net/android/java/java.gyp:27: '<(DEPTH)/net/android/java/net.xml', On 2012/04/13 22:37:58, Ryan Sleevi wrote: > Rather than repeat this (shared with src/base/android/java/java.gyp), consider > using an 'java.gypi' target combined with 'variables' to set the appropriate > outputs as needed. > > 'java.gypi' can then defer to 'ant'. > > An example of how to do this can be seen with 'protoc.gypi' Done. http://codereview.chromium.org/10073024/diff/1/net/android/java/net.xml File net/android/java/net.xml (right): http://codereview.chromium.org/10073024/diff/1/net/android/java/net.xml#newcode3 net/android/java/net.xml:3: building net java source code with ant On 2012/04/13 22:37:58, Ryan Sleevi wrote: > nit? > Building net/ java source code with ant. Done. http://codereview.chromium.org/10073024/diff/1/net/android/java/net.xml#newco... net/android/java/net.xml:13: <property name="dist" location="dist"/> On 2012/04/13 22:37:58, Ryan Sleevi wrote: > Does this mean your generated java files are being dumped into the source tree? > I don't see where you properly map it as the other builders/generators do (eg: > out/Debug) > > If you're assuming the generator runs from within that sub directory, that's > also a guarantee that isn't part of GYPs design and is subject to ongoing > discussions. Done. http://codereview.chromium.org/10073024/diff/1/net/android/java/net.xml#newco... net/android/java/net.xml:38: makefiles are generated. --> On 2012/04/13 22:37:58, Ryan Sleevi wrote: > Is there a bug (public or private) for this? > > It would be good to track publicly, as this is being upstreamed. Done. http://codereview.chromium.org/10073024/diff/1/net/android/network_change_not... File net/android/network_change_notifier.cc (right): http://codereview.chromium.org/10073024/diff/1/net/android/network_change_not... net/android/network_change_notifier.cc:11: #include "jni/network_change_notifier_jni.h" On 2012/04/13 22:37:58, Ryan Sleevi wrote: > This seems like a cheap hack to avoid the "don't use using within headers". If > this header cannot compile without the definition, this header should be fixed. Hmm. So the header is auto-generated by the jni_generator and assumes that everything is in global scope. Not sure how to incorporate namespaces in the auto-generated code. http://codereview.chromium.org/10073024/diff/1/net/android/network_change_not... net/android/network_change_notifier.cc:16: On 2012/04/13 22:37:58, Ryan Sleevi wrote: > nit: delete this whitespace Done. http://codereview.chromium.org/10073024/diff/1/net/android/network_change_not... net/android/network_change_notifier.cc:19: CHECK(java_network_change_notifier_android_object_.is_null()); On 2012/04/13 22:37:58, Ryan Sleevi wrote: > Why CHECK here? You're in a constructor - how is this not guaranteed? Done. http://codereview.chromium.org/10073024/diff/1/net/android/network_change_not... net/android/network_change_notifier.cc:24: CHECK(!java_network_change_notifier_android_object_.is_null()); On 2012/04/13 22:37:58, Ryan Sleevi wrote: > Unnecessary CHECK? This is also guaranteed. Done. http://codereview.chromium.org/10073024/diff/1/net/android/network_change_not... net/android/network_change_notifier.cc:28: java_network_change_notifier_android_object_.Reset(); On 2012/04/13 22:37:58, Ryan Sleevi wrote: > Isn't this unnecessary and guaranteed by the destructor of > |java_network_change_notifier_android_object_| ? Done. http://codereview.chromium.org/10073024/diff/1/net/android/network_change_not... net/android/network_change_notifier.cc:37: CHECK(!java_network_change_notifier_android_object_.is_null()); On 2012/04/13 22:37:58, Ryan Sleevi wrote: > Do you really need to CHECK here? Would a DCHECK suffice? Are both superflous > and the user will end up crashing on a null-deref when they actually try to use > it, accomplishing the same thing? Done. http://codereview.chromium.org/10073024/diff/1/net/android/network_change_not... net/android/network_change_notifier.cc:45: CHECK(!java_network_change_notifier_android_object_.is_null()); On 2012/04/13 22:37:58, Ryan Sleevi wrote: > And here Done. http://codereview.chromium.org/10073024/diff/1/net/android/network_change_not... File net/android/network_change_notifier.h (right): http://codereview.chromium.org/10073024/diff/1/net/android/network_change_not... net/android/network_change_notifier.h:32: java_network_change_notifier_android_object_; On 2012/04/13 22:37:58, Ryan Sleevi wrote: > nit: shorten the variable name? > > Certainly, "_object" seems superfluous? Done. http://codereview.chromium.org/10073024/diff/1/net/android/network_change_not... net/android/network_change_notifier.h:37: bool RegisterNetworkChangeNotifier(JNIEnv* env); On 2012/04/13 22:37:58, Ryan Sleevi wrote: > Any reason to have this free floating? > > Can you not just make it a static member on NCN? > > net::android::NetworkChangeNotifier::Register(JNIEnv* env) ? This style is consistent with downstream but that doesn't mean it's right... I tend to agree with you so I'll do that here and change everyone downstream. I think the issue is that some of these registrations are just static helpers and not an object but I can keep those as Register* http://codereview.chromium.org/10073024/diff/1/net/net.gyp File net/net.gyp (right): http://codereview.chromium.org/10073024/diff/1/net/net.gyp#newcode1815 net/net.gyp:1815: '<@(_outputs)', On 2012/04/13 22:37:58, Ryan Sleevi wrote: > Please consider using jni_generator.gypi, as mentioned elsewhere. Done. http://codereview.chromium.org/10073024/diff/5001/net/net.gyp File net/net.gyp (right): http://codereview.chromium.org/10073024/diff/5001/net/net.gyp#newcode1816 net/net.gyp:1816: 'java_in_dir': '../net/android/java', The "../net" prefix is needed otherwise the find picks up some files from base.
"using" namespace bit seems like it should be fixed, but I realize it may need to be a separate fix. Few comments below, trying to understand, but I think otherwise everything looks good to me. http://codereview.chromium.org/10073024/diff/1/net/android/network_change_not... File net/android/network_change_notifier.cc (right): http://codereview.chromium.org/10073024/diff/1/net/android/network_change_not... net/android/network_change_notifier.cc:11: #include "jni/network_change_notifier_jni.h" On 2012/04/17 16:18:03, Yaron wrote: > On 2012/04/13 22:37:58, Ryan Sleevi wrote: > > This seems like a cheap hack to avoid the "don't use using within headers". If > > this header cannot compile without the definition, this header should be > fixed. > > Hmm. So the header is auto-generated by the jni_generator and assumes that > everything is in global scope. Not sure how to incorporate namespaces in the > auto-generated code. Update the jni generator? ;) http://codereview.chromium.org/10073024/diff/5001/net/android/java/net.xml File net/android/java/net.xml (right): http://codereview.chromium.org/10073024/diff/5001/net/android/java/net.xml#ne... net/android/java/net.xml:44: <path location="../../../base/android/java/dist/lib/chromium_base.jar"/> Does this need to be updated now that it's been rebased? http://codereview.chromium.org/10073024/diff/5001/net/net.gyp File net/net.gyp (right): http://codereview.chromium.org/10073024/diff/5001/net/net.gyp#newcode1816 net/net.gyp:1816: 'java_in_dir': '../net/android/java', On 2012/04/17 16:18:04, Yaron wrote: > The "../net" prefix is needed otherwise the find picks up some files from base. Really? 'java_in_dir': '.' should be relativized to 'src/net', so 'android/java' should be relativized to 'src/net/android/java' and, by the time passed to find (line 40 build/java.gypi) should be fully expanded...
http://codereview.chromium.org/10073024/diff/5001/net/android/java/net.xml File net/android/java/net.xml (right): http://codereview.chromium.org/10073024/diff/5001/net/android/java/net.xml#ne... net/android/java/net.xml:44: <path location="../../../base/android/java/dist/lib/chromium_base.jar"/> On 2012/04/17 17:56:06, Ryan Sleevi wrote: > Does this need to be updated now that it's been rebased? No, it requires jrg@s patch. If this lands first, he'll have to update this one. http://codereview.chromium.org/10073024/diff/5001/net/net.gyp File net/net.gyp (right): http://codereview.chromium.org/10073024/diff/5001/net/net.gyp#newcode1816 net/net.gyp:1816: 'java_in_dir': '../net/android/java', On 2012/04/17 17:56:06, Ryan Sleevi wrote: > On 2012/04/17 16:18:04, Yaron wrote: > > The "../net" prefix is needed otherwise the find picks up some files from > base. > > Really? > > 'java_in_dir': '.' should be relativized to 'src/net', so 'android/java' should > be relativized to 'src/net/android/java' and, by the time passed to find (line > 40 build/java.gypi) should be fully expanded... > Yes. If I leave it as "android/java", I see the following in net/net_java.target.mk: ### Rules for action "ant_net": quiet_cmd_net_java_ant_net = ACTION Building net java sources. $@ cmd_net_java_ant_net = LD_LIBRARY_PATH=$(builddir)/lib.host:$(builddir)/lib.target:$$LD_LIBRARY_PATH; export LD_LIBRARY_PATH; cd net; mkdir -p $( builddir); ant "-DPRODUCT_DIR=$(builddir)" -buildfile android/java/net.xml $(builddir)/chromium_net.jar: obj := $(abs_obj) $(builddir)/chromium_net.jar: builddir := $(abs_builddir) $(builddir)/chromium_net.jar: TOOLSET := $(TOOLSET) $(builddir)/chromium_net.jar: net/android/java/net.xml net/android/java/org/chromium/base/PathUtils.java net/android/java/org/chromium/base/Calle dByNative.java net/android/java/org/chromium/base/BuildInfo.java net/android/java/org/chromium/base/SystemMessageHandler.java net/android/java/or g/chromium/base/ActivityStatus.java FORCE_DO_CMD $(call do_cmd,net_java_ant_net)
http://codereview.chromium.org/10073024/diff/5001/net/net.gyp File net/net.gyp (right): http://codereview.chromium.org/10073024/diff/5001/net/net.gyp#newcode1816 net/net.gyp:1816: 'java_in_dir': '../net/android/java', On 2012/04/17 22:47:38, Yaron wrote: > On 2012/04/17 17:56:06, Ryan Sleevi wrote: > > On 2012/04/17 16:18:04, Yaron wrote: > > > The "../net" prefix is needed otherwise the find picks up some files from > > base. > > > > Really? > > > > 'java_in_dir': '.' should be relativized to 'src/net', so 'android/java' > should > > be relativized to 'src/net/android/java' and, by the time passed to find (line > > 40 build/java.gypi) should be fully expanded... > > > > Yes. If I leave it as "android/java", I see the following in > net/net_java.target.mk: > > ### Rules for action "ant_net": > > quiet_cmd_net_java_ant_net = ACTION Building net java sources. $@ > > cmd_net_java_ant_net = > LD_LIBRARY_PATH=$(builddir)/lib.host:$(builddir)/lib.target:$$LD_LIBRARY_PATH; > export LD_LIBRARY_PATH; cd net; mkdir -p $( > builddir); ant "-DPRODUCT_DIR=$(builddir)" -buildfile android/java/net.xml > > > $(builddir)/chromium_net.jar: obj := $(abs_obj) > > $(builddir)/chromium_net.jar: builddir := $(abs_builddir) > > $(builddir)/chromium_net.jar: TOOLSET := $(TOOLSET) > > $(builddir)/chromium_net.jar: net/android/java/net.xml > net/android/java/org/chromium/base/PathUtils.java > net/android/java/org/chromium/base/Calle > dByNative.java net/android/java/org/chromium/base/BuildInfo.java > net/android/java/org/chromium/base/SystemMessageHandler.java net/android/java/or > g/chromium/base/ActivityStatus.java FORCE_DO_CMD > > $(call do_cmd,net_java_ant_net) Maybe I'm blind, but what's not correct there? I don't see any files from base picked up - just net/android/java/org/chromium/base/[blah].java
http://codereview.chromium.org/10073024/diff/5001/net/net.gyp File net/net.gyp (right): http://codereview.chromium.org/10073024/diff/5001/net/net.gyp#newcode1816 net/net.gyp:1816: 'java_in_dir': '../net/android/java', On 2012/04/17 22:54:42, Ryan Sleevi wrote: > On 2012/04/17 22:47:38, Yaron wrote: > > On 2012/04/17 17:56:06, Ryan Sleevi wrote: > > > On 2012/04/17 16:18:04, Yaron wrote: > > > > The "../net" prefix is needed otherwise the find picks up some files from > > > base. > > > > > > Really? > > > > > > 'java_in_dir': '.' should be relativized to 'src/net', so 'android/java' > > should > > > be relativized to 'src/net/android/java' and, by the time passed to find > (line > > > 40 build/java.gypi) should be fully expanded... > > > > > > > Yes. If I leave it as "android/java", I see the following in > > net/net_java.target.mk: > > > > ### Rules for action "ant_net": > > > > > quiet_cmd_net_java_ant_net = ACTION Building net java sources. $@ > > > > > cmd_net_java_ant_net = > > LD_LIBRARY_PATH=$(builddir)/lib.host:$(builddir)/lib.target:$$LD_LIBRARY_PATH; > > export LD_LIBRARY_PATH; cd net; mkdir -p $( > > builddir); ant "-DPRODUCT_DIR=$(builddir)" -buildfile android/java/net.xml > > > > > > > $(builddir)/chromium_net.jar: obj := $(abs_obj) > > > > > $(builddir)/chromium_net.jar: builddir := $(abs_builddir) > > > > > $(builddir)/chromium_net.jar: TOOLSET := $(TOOLSET) > > > > > $(builddir)/chromium_net.jar: net/android/java/net.xml > > net/android/java/org/chromium/base/PathUtils.java > > net/android/java/org/chromium/base/Calle > > dByNative.java net/android/java/org/chromium/base/BuildInfo.java > > net/android/java/org/chromium/base/SystemMessageHandler.java > net/android/java/or > > g/chromium/base/ActivityStatus.java FORCE_DO_CMD > > > > > $(call do_cmd,net_java_ant_net) > > Maybe I'm blind, but what's not correct there? I don't see any files from base > picked up - just net/android/java/org/chromium/base/[blah].java net/android/java/org/chromium/base/PathUtils.java should be: base/android/java/org/chromium/base/PathUtils.java same as for other base java files
http://codereview.chromium.org/10073024/diff/5001/net/android/java/org/chromi... File net/android/java/org/chromium/net/NetworkChangeNotifier.java (right): http://codereview.chromium.org/10073024/diff/5001/net/android/java/org/chromi... net/android/java/org/chromium/net/NetworkChangeNotifier.java:127: private native void nativeNotifyObservers(int nativeNetworkChangeNotifier); You can specify the class and namespace of nativeNetworkChangeNotifier in a comment afterwards, so that the JNI generator uses the correct namespace. e.g. private native void nativeNotifyObservers(int nativeNetworkChangeNotifier /* net::android::NetworkChangeNotifier */) Maybe this will solve your problem with the "using net::android::NetworkChangeNotifier" before the #include in the cc file.
http://codereview.chromium.org/10073024/diff/5001/net/android/java/org/chromi... File net/android/java/org/chromium/net/NetworkChangeNotifier.java (right): http://codereview.chromium.org/10073024/diff/5001/net/android/java/org/chromi... net/android/java/org/chromium/net/NetworkChangeNotifier.java:127: private native void nativeNotifyObservers(int nativeNetworkChangeNotifier); On 2012/04/18 14:31:38, John Knottenbelt wrote: > You can specify the class and namespace of nativeNetworkChangeNotifier in a > comment afterwards, so that the JNI generator uses the correct namespace. > > e.g. > > private native void nativeNotifyObservers(int nativeNetworkChangeNotifier /* > net::android::NetworkChangeNotifier */) > > Maybe this will solve your problem with the "using > net::android::NetworkChangeNotifier" before the #include in the cc file. Done. Thanks John!
LGTM http://codereview.chromium.org/10073024/diff/14001/net/android/java/net.xml File net/android/java/net.xml (right): http://codereview.chromium.org/10073024/diff/14001/net/android/java/net.xml#n... net/android/java/net.xml:13: <property name="out.dir" location="${PRODUCT_DIR}"/> Per discussions with tedchoc, we are probably going to make this be ${PRODUCT_DIR}/lib.java http://codereview.chromium.org/10073024/diff/16001/net/android/java/net.xml File net/android/java/net.xml (right): http://codereview.chromium.org/10073024/diff/16001/net/android/java/net.xml#n... net/android/java/net.xml:13: <property name="out.dir" location="${PRODUCT_DIR}"/> ditto comment http://codereview.chromium.org/10073024/diff/16001/net/android/java/org/chrom... File net/android/java/org/chromium/net/NetworkChangeNotifier.java (right): http://codereview.chromium.org/10073024/diff/16001/net/android/java/org/chrom... net/android/java/org/chromium/net/NetworkChangeNotifier.java:30: private boolean mRegistered; Chrome standard is to add a comment per member. We should consider applying this to Java as well. http://codereview.chromium.org/10073024/diff/16001/net/net.gyp File net/net.gyp (right): http://codereview.chromium.org/10073024/diff/16001/net/net.gyp#newcode993 net/net.gyp:993: 'android/network_library.cc', can we add them back in now?
Thanks John. rsleevi: final look? Ted or I will look into fixing the package issue where I needed to specify ../net/android/java. I think the current plan is to go back to our original approach of specifying the full package instead of the relative path, but one of us will give the current approach another try. Regardless, I think that's somewhat orthogonal and would like to land this first net/android code. http://codereview.chromium.org/10073024/diff/14001/net/android/java/net.xml File net/android/java/net.xml (right): http://codereview.chromium.org/10073024/diff/14001/net/android/java/net.xml#n... net/android/java/net.xml:13: <property name="out.dir" location="${PRODUCT_DIR}"/> On 2012/04/19 22:35:41, John Grabowski wrote: > Per discussions with tedchoc, we are probably going to make this be > ${PRODUCT_DIR}/lib.java Done. http://codereview.chromium.org/10073024/diff/16001/net/net.gyp File net/net.gyp (right): http://codereview.chromium.org/10073024/diff/16001/net/net.gyp#newcode993 net/net.gyp:993: 'android/network_library.cc', On 2012/04/19 22:35:41, John Grabowski wrote: > can we add them back in now? It won't compile because it uses old JNI (i.e. it's a zombie patch). I'll fix it up separately.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yfriedman@chromium.org/10073024/19002
Try job failure for 10073024-19002 on linux_clang for step "update". http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan... Step "update" is always a major failure. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yfriedman@chromium.org/10073024/19002
Change committed as 133142 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
