Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1294603003:
[BackgroundSync] Trigger Background Sync events when Chrome is backgrounded on Android (Closed)
|
Created:
5 years, 4 months ago by iclelland Modified:
5 years, 2 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, jkarlin+watch_chromium.org, maniscalco+watch_chromium.org, maxbogue+watch_chromium.org, plaree+watch_chromium.org, pvalenzuela+watch_chromium.org, tim+watch_chromium.org, zea+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[BackgroundSync] Trigger Background Sync events when Chrome is backgrounded on Android
BUG=511129
Committed: https://crrev.com/a5fb97d6fb9c3831994aadebea4664e2acc3c356
Cr-Commit-Position: refs/heads/master@{#351093}
Patch Set 1 #Patch Set 2 : Remove code for triggering the sync manager from the paused/stopped states #Patch Set 3 : Move BackgroundSyncNetworkObserverAndroid destructor to satisfy the compiler #
Total comments: 10
Patch Set 4 : Addressing review comments #Patch Set 5 : Rebase #Patch Set 6 : Update browser tests to trigger network observer directly #
Total comments: 11
Patch Set 7 : Rebase #Patch Set 8 : Hold JNI references in a dedicated class on UI thread #
Total comments: 7
Patch Set 9 : Fix weakptr threading issue #Patch Set 10 : Addressing review comments #
Total comments: 14
Patch Set 11 : Rebase #Patch Set 12 : Switch to single autodetect, register native observers, change observer creation pattern #
Total comments: 28
Patch Set 13 : Make BSNOA::Observer destructor private #Patch Set 14 : Addressing review comments from PS#12 #Patch Set 15 : Better "ignore real network state" mechanism for tests #
Total comments: 7
Patch Set 16 : Stop caching JNI environment #Patch Set 17 : Fix Android unit tests (Isolate tests from real network status) #Patch Set 18 : Rebase (Post-Blink-Chromium-Merge) #
Total comments: 28
Patch Set 19 : Addressing review comments #
Total comments: 2
Patch Set 20 : Register JNI methods in component unittests; stop swallowing errors #
Total comments: 2
Patch Set 21 : Rebase post-revert #Patch Set 22 : Don't crash in embedded webview; check permissions before creating NCNAD #Patch Set 23 : Don't register observers with nonexistant Autodetector #Messages
Total messages: 60 (9 generated)
iclelland@chromium.org changed reviewers: + jkarlin@chromium.org
jkarlin, PTAL -- compare and contrast with cr/1282993005/ Thanks!
I really like where this is going. There is one piece missing though. If the BackgroundSyncNetworkObserver.java created its own NetworkChangeNotifierAutoDetect with alwaysWatchForChanges set to true then we wouldn't need the special-case triggering code, which would make the logic the same for Android and Desktop on C++ side of things.
On 2015/08/18 18:04:40, jkarlin wrote: > I really like where this is going. There is one piece missing though. If the > BackgroundSyncNetworkObserver.java created its own > NetworkChangeNotifierAutoDetect with alwaysWatchForChanges set to true then we > wouldn't need the special-case triggering code, which would make the logic the > same for Android and Desktop on C++ side of things. As per our off-line discussion: - BackgroundSyncNetworkObserver.java already does this, in this CL - The code in background_sync_launcher_android which associates callbacks with each registration is likely not necessary for this implementation, and so I'll remove that. - We can couple this with using the GCM Network Manager (in a separate CL) to handle the Chrome-is-completely-shut-down case.
On 2015/08/27 17:42:25, iclelland wrote: > On 2015/08/18 18:04:40, jkarlin wrote: > > I really like where this is going. There is one piece missing though. If the > > BackgroundSyncNetworkObserver.java created its own > > NetworkChangeNotifierAutoDetect with alwaysWatchForChanges set to true then we > > wouldn't need the special-case triggering code, which would make the logic the > > same for Android and Desktop on C++ side of things. > > As per our off-line discussion: > - BackgroundSyncNetworkObserver.java already does this, in this CL > - The code in background_sync_launcher_android which associates callbacks with > each registration is likely not necessary for this implementation, and so I'll > remove that. > - We can couple this with using the GCM Network Manager (in a separate CL) to > handle the Chrome-is-completely-shut-down case. jkarlin, PTAL again, thanks!
Looking very good! Some nits. The browsertests are going to need some work in the Android case. The tests simulate sending network change events via the normal NCN. But the observer is no longer listening to the normal NCN. You'll need to instead trigger the BackgroundSyncNetworkObserver to send the events instead when on Android. https://codereview.chromium.org/1294603003/diff/40001/content/browser/android... File content/browser/android/background_sync_network_observer_android.cc (right): https://codereview.chromium.org/1294603003/diff/40001/content/browser/android... content/browser/android/background_sync_network_observer_android.cc:36: BackgroundSyncNetworkObserverAndroid::~BackgroundSyncNetworkObserverAndroid() { Since this is empty, prefer = default in the header file. https://codereview.chromium.org/1294603003/diff/40001/content/browser/android... File content/browser/android/background_sync_network_observer_android.h (right): https://codereview.chromium.org/1294603003/diff/40001/content/browser/android... content/browser/android/background_sync_network_observer_android.h:15: // BackgroundSyncNetworkObserver which always uses the actual connectivity state "always uses the actual connectivity state" sounds like the other observer doesn't use the actual connectivity state. Perhaps rephrase to, "backed by a NetworkChangeNotifier that listens for network events even when the browser is paused, unlike the standard NetworkChangeNotifier". On that note, we should probably not listen for connectivity events while paused if there aren't any background syncs pending. Please add a bug/todo for that. https://codereview.chromium.org/1294603003/diff/40001/content/browser/android... content/browser/android/background_sync_network_observer_android.h:23: // called when the network connection changes asynchronously via PostMessage. can probably remove 'asynchronously' without losing anything https://codereview.chromium.org/1294603003/diff/40001/content/browser/android... content/browser/android/background_sync_network_observer_android.h:33: // by this class and calls the |network_changed_callback|. calls |network_changed_callback| provided to the constructor https://codereview.chromium.org/1294603003/diff/40001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/BackgroundSyncNetworkObserver.java (right): https://codereview.chromium.org/1294603003/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/BackgroundSyncNetworkObserver.java:18: * actually online, for the purpose of triggering background sync events, without having to trigger remove "actually". The last bit doesn't make much sense to me "without having to trigger an online event while Chrome is backgrounded. Perhaps: The purpose of this class is to listen for and forward network connectivity events to the BackgroundSyncNetworkObserver even when the application is paused. The standard NetworkChangeNotifier does not listen for connectivity events when the application is paused.
Comments mostly addressed; I'm updating the browser tests (in the next patch) https://codereview.chromium.org/1294603003/diff/40001/content/browser/android... File content/browser/android/background_sync_network_observer_android.cc (right): https://codereview.chromium.org/1294603003/diff/40001/content/browser/android... content/browser/android/background_sync_network_observer_android.cc:36: BackgroundSyncNetworkObserverAndroid::~BackgroundSyncNetworkObserverAndroid() { On 2015/08/28 01:19:48, jkarlin_(out til Sep 2nd) wrote: > Since this is empty, prefer = default in the header file. Done. https://codereview.chromium.org/1294603003/diff/40001/content/browser/android... File content/browser/android/background_sync_network_observer_android.h (right): https://codereview.chromium.org/1294603003/diff/40001/content/browser/android... content/browser/android/background_sync_network_observer_android.h:15: // BackgroundSyncNetworkObserver which always uses the actual connectivity state On 2015/08/28 01:19:48, jkarlin_(out til Sep 2nd) wrote: > "always uses the actual connectivity state" sounds like the other observer > doesn't use the actual connectivity state. Perhaps rephrase to, "backed by a > NetworkChangeNotifier that listens for network events even when the browser is > paused, unlike the standard NetworkChangeNotifier". > > On that note, we should probably not listen for connectivity events while paused > if there aren't any background syncs pending. Please add a bug/todo for that. Done. https://codereview.chromium.org/1294603003/diff/40001/content/browser/android... content/browser/android/background_sync_network_observer_android.h:23: // called when the network connection changes asynchronously via PostMessage. On 2015/08/28 01:19:48, jkarlin_(out til Sep 2nd) wrote: > can probably remove 'asynchronously' without losing anything Done. https://codereview.chromium.org/1294603003/diff/40001/content/browser/android... content/browser/android/background_sync_network_observer_android.h:33: // by this class and calls the |network_changed_callback|. On 2015/08/28 01:19:48, jkarlin_(out til Sep 2nd) wrote: > calls |network_changed_callback| provided to the constructor Done. https://codereview.chromium.org/1294603003/diff/40001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/BackgroundSyncNetworkObserver.java (right): https://codereview.chromium.org/1294603003/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/BackgroundSyncNetworkObserver.java:18: * actually online, for the purpose of triggering background sync events, without having to trigger On 2015/08/28 01:19:48, jkarlin_(out til Sep 2nd) wrote: > remove "actually". The last bit doesn't make much sense to me "without having to > trigger an online event while Chrome is backgrounded. > > Perhaps: > > The purpose of this class is to listen for and forward network connectivity > events to the BackgroundSyncNetworkObserver even when the application is paused. > The standard NetworkChangeNotifier does not listen for connectivity events when > the application is paused. Done.
Are you still planning to add tests to this?
On 2015/09/08 15:08:30, jkarlin wrote: > Are you still planning to add tests to this? Tests added, (and then added again, and then greatly simplified, and then merged with the non-android tests.) jkarlin, PTAL, thanks!
Patchset #5 (id:80001) has been deleted
Almost there. https://codereview.chromium.org/1294603003/diff/120001/content/browser/androi... File content/browser/android/background_sync_network_observer_android.cc (right): https://codereview.chromium.org/1294603003/diff/120001/content/browser/androi... content/browser/android/background_sync_network_observer_android.cc:21: DCHECK_CURRENTLY_ON(BrowserThread::IO); I'm pretty tired so I may be wrong but I believe this class needs to live on the UI thread to talk to Java code. https://codereview.chromium.org/1294603003/diff/120001/content/browser/androi... content/browser/android/background_sync_network_observer_android.cc:43: DCHECK_CURRENTLY_ON(BrowserThread::IO); I believe this will be called on UI? Probably explains the purple android bot. https://codereview.chromium.org/1294603003/diff/120001/content/browser/backgr... File content/browser/background_sync/background_sync_browsertest.cc (right): https://codereview.chromium.org/1294603003/diff/120001/content/browser/backgr... content/browser/background_sync/background_sync_browsertest.cc:157: void SetOnlineOnIOThread(BackgroundSyncContext* sync_context, bool online); const scoped_refptr<BackgroundSyncContext>& https://codereview.chromium.org/1294603003/diff/120001/content/browser/backgr... content/browser/background_sync/background_sync_browsertest.cc:179: if (shell_) { What happens if there isn't a shell? Perhaps ASSERT_TRUE(shell_)?
https://codereview.chromium.org/1294603003/diff/120001/content/browser/androi... File content/browser/android/background_sync_network_observer_android.cc (right): https://codereview.chromium.org/1294603003/diff/120001/content/browser/androi... content/browser/android/background_sync_network_observer_android.cc:21: DCHECK_CURRENTLY_ON(BrowserThread::IO); On 2015/09/11 00:05:23, jkarlin wrote: > I'm pretty tired so I may be wrong but I believe this class needs to live on the > UI thread to talk to Java code. > Well... JNI is usable from any thread, and this class is created on the IO thread (it's parent class constructor has a similar DCHECK) so we need to stay on IO at least long enough to call RemoveNetworkChangeObserver. But perhaps you're right in that the Java-side objects should be created on the UI thread. The NetworkChangeNotifierAutodetect.Observer notes that notifications always happen on the main thread. https://codereview.chromium.org/1294603003/diff/120001/content/browser/androi... content/browser/android/background_sync_network_observer_android.cc:43: DCHECK_CURRENTLY_ON(BrowserThread::IO); On 2015/09/11 00:05:23, jkarlin wrote: > I believe this will be called on UI? > > Probably explains the purple android bot. Apparently you weren't the only tired one. I fixed this on the *other* CL. I'll backport that here. https://codereview.chromium.org/1294603003/diff/120001/content/browser/backgr... File content/browser/background_sync/background_sync_browsertest.cc (right): https://codereview.chromium.org/1294603003/diff/120001/content/browser/backgr... content/browser/background_sync/background_sync_browsertest.cc:179: if (shell_) { On 2015/09/11 00:05:23, jkarlin wrote: > What happens if there isn't a shell? > It would just not try to get the sync context from it -- basically turning this into a NOOP -- this was a guard from earlier, when SetOnline was called before SetIncognito in the setup methods. Not needed anymore, but I didn't actually change this into an assertion. > Perhaps ASSERT_TRUE(shell_)? Yes, definitely.
https://codereview.chromium.org/1294603003/diff/120001/content/browser/androi... File content/browser/android/background_sync_network_observer_android.cc (right): https://codereview.chromium.org/1294603003/diff/120001/content/browser/androi... content/browser/android/background_sync_network_observer_android.cc:21: DCHECK_CURRENTLY_ON(BrowserThread::IO); On 2015/09/11 14:42:15, iclelland wrote: > On 2015/09/11 00:05:23, jkarlin wrote: > > I'm pretty tired so I may be wrong but I believe this class needs to live on > the > > UI thread to talk to Java code. > > > > Well... JNI is usable from any thread, and this class is created on the IO > thread (it's parent class constructor has a similar DCHECK) so we need to stay > on IO at least long enough to call RemoveNetworkChangeObserver. Good point. This class needs to be on the IO thread. > > But perhaps you're right in that the Java-side objects should be created on the > UI thread. The NetworkChangeNotifierAutodetect.Observer notes that notifications > always happen on the main thread. I'd like the java side of this to all be on one thread. Ideally every class lives on one thread. So perhaps there should be a new class that does the communication with Java and it lives on the IO thread.
https://codereview.chromium.org/1294603003/diff/120001/content/browser/androi... File content/browser/android/background_sync_network_observer_android.cc (right): https://codereview.chromium.org/1294603003/diff/120001/content/browser/androi... content/browser/android/background_sync_network_observer_android.cc:21: DCHECK_CURRENTLY_ON(BrowserThread::IO); On 2015/09/11 15:16:06, jkarlin wrote: > On 2015/09/11 14:42:15, iclelland wrote: > > On 2015/09/11 00:05:23, jkarlin wrote: > > > I'm pretty tired so I may be wrong but I believe this class needs to live on > > the > > > UI thread to talk to Java code. > > > > > > > Well... JNI is usable from any thread, and this class is created on the IO > > thread (it's parent class constructor has a similar DCHECK) so we need to stay > > on IO at least long enough to call RemoveNetworkChangeObserver. > > Good point. This class needs to be on the IO thread. > > > > > But perhaps you're right in that the Java-side objects should be created on > the > > UI thread. The NetworkChangeNotifierAutodetect.Observer notes that > notifications > > always happen on the main thread. > > I'd like the java side of this to all be on one thread. Ideally every class > lives on one thread. So perhaps there should be a new class that does the > communication with Java and it lives on the IO thread. > Sorry, I meant the class that talks to Java should be on the UI thread of course.
I've added a new class, just for the purpose of holding the Java references and communicating over JNI, which lives on the UI thread. This particular code is crashing on me right now, with a weak pointer check on the wrong thread, which I'm tracking down, but please feel free to take a look at the general design. https://codereview.chromium.org/1294603003/diff/120001/content/browser/backgr... File content/browser/background_sync/background_sync_browsertest.cc (right): https://codereview.chromium.org/1294603003/diff/120001/content/browser/backgr... content/browser/background_sync/background_sync_browsertest.cc:157: void SetOnlineOnIOThread(BackgroundSyncContext* sync_context, bool online); On 2015/09/11 00:05:23, jkarlin wrote: > const scoped_refptr<BackgroundSyncContext>& Done. https://codereview.chromium.org/1294603003/diff/120001/content/browser/backgr... content/browser/background_sync/background_sync_browsertest.cc:179: if (shell_) { On 2015/09/11 14:42:15, iclelland wrote: > On 2015/09/11 00:05:23, jkarlin wrote: > > What happens if there isn't a shell? > > > It would just not try to get the sync context from it -- basically turning this > into a NOOP -- this was a guard from earlier, when SetOnline was called before > SetIncognito in the setup methods. Not needed anymore, but I didn't actually > change this into an assertion. > > > Perhaps ASSERT_TRUE(shell_)? > > Yes, definitely. Done.
https://codereview.chromium.org/1294603003/diff/160001/content/browser/androi... File content/browser/android/background_sync_network_observer_android.cc (right): https://codereview.chromium.org/1294603003/diff/160001/content/browser/androi... content/browser/android/background_sync_network_observer_android.cc:19: : callback_(callback) { DCHECK on UI thread https://codereview.chromium.org/1294603003/diff/160001/content/browser/androi... content/browser/android/background_sync_network_observer_android.cc:33: jint new_connection_type) { DCHECK on UI thread https://codereview.chromium.org/1294603003/diff/160001/content/browser/androi... content/browser/android/background_sync_network_observer_android.cc:54: weak_ptr_factory_.GetWeakPtr(), You can't use a weakptr on a different thread. Here is a common pattern: Make BSNOA::Observer inherit from RefCountedThreadSafe and use the parameters to make sure its destructor is called on the UI thread if the destructor isn't thread safe. Create the BSNOA::Observer on the IO thread.
https://codereview.chromium.org/1294603003/diff/160001/content/browser/androi... File content/browser/android/background_sync_network_observer_android.cc (right): https://codereview.chromium.org/1294603003/diff/160001/content/browser/androi... content/browser/android/background_sync_network_observer_android.cc:54: weak_ptr_factory_.GetWeakPtr(), On 2015/09/16 17:39:44, jkarlin wrote: > You can't use a weakptr on a different thread. Right -- I didn't think I was, by doing it this way. I thought this code, which ran on the IO thread, safely encapsulated the weakptr in the callback until it was used to post back to the IO thread... ... and now I see the issue :) Thanks. > > Here is a common pattern: > > Make BSNOA::Observer inherit from RefCountedThreadSafe and use the parameters to > make sure its destructor is called on the UI thread if the destructor isn't > thread safe. Already done, see the .h file. > > Create the BSNOA::Observer on the IO thread. That I will try. Thanks!
https://codereview.chromium.org/1294603003/diff/160001/content/browser/androi... File content/browser/android/background_sync_network_observer_android.cc (right): https://codereview.chromium.org/1294603003/diff/160001/content/browser/androi... content/browser/android/background_sync_network_observer_android.cc:19: : callback_(callback) { On 2015/09/16 17:39:44, jkarlin wrote: > DCHECK on UI thread Done. Well, actually checks for IO thread now, since I'm constructing it there. https://codereview.chromium.org/1294603003/diff/160001/content/browser/androi... content/browser/android/background_sync_network_observer_android.cc:33: jint new_connection_type) { On 2015/09/16 17:39:44, jkarlin wrote: > DCHECK on UI thread Done. See the corresponding change to the Java BackgroundSyncNetworkObserver constructor to comply with this. https://codereview.chromium.org/1294603003/diff/160001/content/browser/androi... content/browser/android/background_sync_network_observer_android.cc:54: weak_ptr_factory_.GetWeakPtr(), On 2015/09/16 17:47:38, iclelland wrote: > On 2015/09/16 17:39:44, jkarlin wrote: > > You can't use a weakptr on a different thread. > Right -- I didn't think I was, by doing it this way. I thought this code, which > ran on the IO thread, safely encapsulated the weakptr in the callback until it > was used to post back to the IO thread... > > ... and now I see the issue :) Thanks. > > > > > Here is a common pattern: > > > > Make BSNOA::Observer inherit from RefCountedThreadSafe and use the parameters > to > > make sure its destructor is called on the UI thread if the destructor isn't > > thread safe. > Already done, see the .h file. > > > > > Create the BSNOA::Observer on the IO thread. > That I will try. Thanks! Done. https://codereview.chromium.org/1294603003/diff/200001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/BackgroundSyncNetworkObserver.java (right): https://codereview.chromium.org/1294603003/diff/200001/content/public/android... content/public/android/java/src/org/chromium/content/browser/BackgroundSyncNetworkObserver.java:33: ThreadUtils.runOnUiThread(new Runnable() { This change, made necessary by the DCHECK_CURRENTLY_ON(UI) in BackgroundSyncNetworkObserverAndroid::Observer::NotifyConnectionTypeChanged, brings back the race that breaks the Incognito test. (Because this runs on construction, we always notify the browser of the current *actual* connectivity state, before we have a chance to override it in the test, and sometimes that means that this online notification even happens *after* the test override.)
iclelland@chromium.org changed reviewers: + yfriedman@chromium.org
+r yfriedman -- can you take a look at the code under /content/*/android/ ? Especially the JNI code, to make sure that I haven't done anything *too* dumb here :) Thanks!
https://codereview.chromium.org/1294603003/diff/200001/content/browser/androi... File content/browser/android/background_sync_network_observer_android.cc (right): https://codereview.chromium.org/1294603003/diff/200001/content/browser/androi... content/browser/android/background_sync_network_observer_android.cc:20: DCHECK_CURRENTLY_ON(BrowserThread::IO); It seems like the code here and below in this function should be on the UI thread. If you had a Create function and an Init function, you could have something like: scoped_refptr<BSNOA::Observer::Observer> Create() { DCHECK_CURRENTLY_ON(BrowserThread::IO); scoped_refptr<BSNOA::Observer::Observer> observer(new BSNOA::Observer::Observer()); BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, base::Bind(&BSNOA::Observer::Init, observer); return observer; } And then the JNI code would all be called on the UI thread. What do you think?
https://codereview.chromium.org/1294603003/diff/200001/content/browser/androi... File content/browser/android/background_sync_network_observer_android.h (right): https://codereview.chromium.org/1294603003/diff/200001/content/browser/androi... content/browser/android/background_sync_network_observer_android.h:55: base::android::ScopedJavaLocalRef<jobject> observer_; Nit: rename j_observer_ (to help distinguish between this and the outer observer_). Also I think it should be a ScopedJavaGlobalRef if it's a member. https://codereview.chromium.org/1294603003/diff/200001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/BackgroundSyncNetworkObserver.java (right): https://codereview.chromium.org/1294603003/diff/200001/content/public/android... content/public/android/java/src/org/chromium/content/browser/BackgroundSyncNetworkObserver.java:19: * BackgroundSyncNetworkObserverAndroid even when the application is paused. The standard I take it this works to have multiple registered NetworkChangeNotifierAutoDetect instances with the same intent filter? Both of them get them the notification? https://codereview.chromium.org/1294603003/diff/200001/content/public/android... content/public/android/java/src/org/chromium/content/browser/BackgroundSyncNetworkObserver.java:25: public class BackgroundSyncNetworkObserver implements NetworkChangeNotifierAutoDetect.Observer { nit: you can make this class and non-interface overrides private. Access modifiers are just compile-time checks and we do JNI this way to indicate that it's meant purely as a helper for native code. https://codereview.chromium.org/1294603003/diff/200001/content/public/android... content/public/android/java/src/org/chromium/content/browser/BackgroundSyncNetworkObserver.java:31: mNotifier = new NetworkChangeNotifierAutoDetect(this, ctx, true); Hmm. You register this observer but never unregister it. If the native counterpart were deleted, this object would still live because NetworkChangeNotifierAutoDetect is still alive and it registers as a BroadcastReceiver. If a notification were then to come through to the broadcast receiver, you'd crash as soon as it calls into native. Is this possible? You'd want to have @CalledByNative destroy method called from native destructor to remedy this. https://codereview.chromium.org/1294603003/diff/200001/content/public/android... content/public/android/java/src/org/chromium/content/browser/BackgroundSyncNetworkObserver.java:36: onConnectionTypeChanged(mNotifier.getCurrentConnectionType( nit: looks overidented (should be 4)
yfriedman -- thanks for the review! PTAL at the updates to the design. jkarlin, PTAL, thanks! https://codereview.chromium.org/1294603003/diff/200001/content/browser/androi... File content/browser/android/background_sync_network_observer_android.cc (right): https://codereview.chromium.org/1294603003/diff/200001/content/browser/androi... content/browser/android/background_sync_network_observer_android.cc:20: DCHECK_CURRENTLY_ON(BrowserThread::IO); On 2015/09/16 20:13:26, jkarlin wrote: > It seems like the code here and below in this function should be on the UI > thread. > > If you had a Create function and an Init function, you could have something > like: > > scoped_refptr<BSNOA::Observer::Observer> Create() { > DCHECK_CURRENTLY_ON(BrowserThread::IO); > scoped_refptr<BSNOA::Observer::Observer> observer(new > BSNOA::Observer::Observer()); > BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, > base::Bind(&BSNOA::Observer::Init, observer); > return observer; > } > > And then the JNI code would all be called on the UI thread. What do you think? That's a great idea -- thanks for that! I've added a static Create, and made the constructor private for this now. Also, the Create method takes the callback parameter, and passes it through to the constructor. Done. https://codereview.chromium.org/1294603003/diff/200001/content/browser/androi... File content/browser/android/background_sync_network_observer_android.h (right): https://codereview.chromium.org/1294603003/diff/200001/content/browser/androi... content/browser/android/background_sync_network_observer_android.h:55: base::android::ScopedJavaLocalRef<jobject> observer_; On 2015/09/16 22:04:36, Yaron wrote: > Nit: rename j_observer_ (to help distinguish between this and the outer > observer_). Also I think it should be a ScopedJavaGlobalRef if it's a member. Done. https://codereview.chromium.org/1294603003/diff/200001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/BackgroundSyncNetworkObserver.java (right): https://codereview.chromium.org/1294603003/diff/200001/content/public/android... content/public/android/java/src/org/chromium/content/browser/BackgroundSyncNetworkObserver.java:19: * BackgroundSyncNetworkObserverAndroid even when the application is paused. The standard On 2015/09/16 22:04:36, Yaron wrote: > I take it this works to have multiple registered NetworkChangeNotifierAutoDetect > instances with the same intent filter? Both of them get them the notification? It appears to, but it's not very clean. I'll see if I can make this a singleton instead. https://codereview.chromium.org/1294603003/diff/200001/content/public/android... content/public/android/java/src/org/chromium/content/browser/BackgroundSyncNetworkObserver.java:25: public class BackgroundSyncNetworkObserver implements NetworkChangeNotifierAutoDetect.Observer { On 2015/09/16 22:04:36, Yaron wrote: > nit: you can make this class and non-interface overrides private. Access > modifiers are just compile-time checks and we do JNI this way to indicate that > it's meant purely as a helper for native code. Thanks, done. https://codereview.chromium.org/1294603003/diff/200001/content/public/android... content/public/android/java/src/org/chromium/content/browser/BackgroundSyncNetworkObserver.java:31: mNotifier = new NetworkChangeNotifierAutoDetect(this, ctx, true); On 2015/09/16 22:04:36, Yaron wrote: > Hmm. You register this observer but never unregister it. If the native > counterpart were deleted, this object would still live because > NetworkChangeNotifierAutoDetect is still alive and it registers as a > BroadcastReceiver. If a notification were then to come through to the broadcast > receiver, you'd crash as soon as it calls into native. Is this possible? You'd > want to have @CalledByNative destroy method called from native destructor to > remedy this. Thanks -- I forgot about the additional reference that will keep this alive. Switching to a singleton NCNAD means I'll need to unregister observers in any case, so I'll add that. https://codereview.chromium.org/1294603003/diff/200001/content/public/android... content/public/android/java/src/org/chromium/content/browser/BackgroundSyncNetworkObserver.java:36: onConnectionTypeChanged(mNotifier.getCurrentConnectionType( On 2015/09/16 22:04:36, Yaron wrote: > nit: looks overidented (should be 4) Done.
Looks like there is a check failure on android in some unittests. https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_... https://codereview.chromium.org/1294603003/diff/240001/content/browser/androi... File content/browser/android/background_sync_network_observer_android.cc (right): https://codereview.chromium.org/1294603003/diff/240001/content/browser/androi... content/browser/android/background_sync_network_observer_android.cc:45: env_, j_observer_.obj(), reinterpret_cast<jlong>(this)); DCHECK_CURRENTLY_ON(BrowserThread::UI); https://codereview.chromium.org/1294603003/diff/240001/content/browser/androi... content/browser/android/background_sync_network_observer_android.cc:63: : callback_(callback) {} DCHECK_CURRENTLY_ON(BrowserThread::IO); https://codereview.chromium.org/1294603003/diff/240001/content/browser/androi... content/browser/android/background_sync_network_observer_android.cc:80: void BackgroundSyncNetworkObserverAndroid::DisableNotificationForTesting() { Looks like you call this on UI. Please rename to DisableNotificationForTestingOnUIThread and add a dcheck. https://codereview.chromium.org/1294603003/diff/240001/content/browser/androi... File content/browser/android/background_sync_network_observer_android.h (right): https://codereview.chromium.org/1294603003/diff/240001/content/browser/androi... content/browser/android/background_sync_network_observer_android.h:32: static void DisableNotificationForTesting(); Since this is public please add a comment https://codereview.chromium.org/1294603003/diff/240001/content/browser/androi... content/browser/android/background_sync_network_observer_android.h:51: // constructor, with the new connection type. append "on the IO thread" to the comment. https://codereview.chromium.org/1294603003/diff/240001/content/browser/androi... content/browser/android/background_sync_network_observer_android.h:61: base::Callback<void(net::NetworkChangeNotifier::ConnectionType)> callback_; Add comment that callback_ is to be run on the IO thread. https://codereview.chromium.org/1294603003/diff/240001/content/browser/androi... content/browser/android/background_sync_network_observer_android.h:63: JNIEnv* env_; DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/1294603003/diff/240001/content/browser/backgr... File content/browser/background_sync/background_sync_browsertest.cc (right): https://codereview.chromium.org/1294603003/diff/240001/content/browser/backgr... content/browser/background_sync/background_sync_browsertest.cc:117: BackgroundSyncNetworkObserverAndroid::DisableNotificationForTesting(); What if you added IgnoreNetworkChangeNotifierForTesting() to BackgroundSyncNetworkObserver? Then you could get rid of the ifdefs in this file and the SetTestNtoificationsOnly(true) line above. You could implement it by simply not calling the callback from BSNO when notifications come in. Alternatively, and I like this a bit more, you could make a MockBackgroundSyncNetworkObserver and call BackgroundSyncManager->SetNetworkObserver(mock) and just change the network as necessary via the mock observer. Then the BackgroundSyncNetworkObservers wouldn't need any ForTesting functions at all. The observers would need a SetCallback function though so that the BSM could change the mock's callback in SetNetworkObserver. https://codereview.chromium.org/1294603003/diff/240001/content/browser/backgr... content/browser/background_sync/background_sync_browsertest.cc:127: BackgroundSyncContext* GetSyncContextFromShell(Shell* shell) { Can you use this new function in OneShotPending? https://codereview.chromium.org/1294603003/diff/240001/content/browser/backgr... content/browser/background_sync/background_sync_browsertest.cc:168: void SetOnline(bool online); Please add comment noting that this runs asynchronously on the IO thread but we don't need to wait for it to complete before running a background sync operation since those also run on the IO thread. https://codereview.chromium.org/1294603003/diff/240001/content/browser/backgr... File content/browser/background_sync/background_sync_manager.h (right): https://codereview.chromium.org/1294603003/diff/240001/content/browser/backgr... content/browser/background_sync/background_sync_manager.h:334: Delete new line https://codereview.chromium.org/1294603003/diff/240001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/BackgroundSyncNetworkObserver.java (right): https://codereview.chromium.org/1294603003/diff/240001/content/public/android... content/public/android/java/src/org/chromium/content/browser/BackgroundSyncNetworkObserver.java:27: * This class should live on the main thread. s/should live/lives/ https://codereview.chromium.org/1294603003/diff/240001/content/public/android... content/public/android/java/src/org/chromium/content/browser/BackgroundSyncNetworkObserver.java:34: // The singleton instance. There should only be one BackgroundSyncNetworkObserver. The second sentence of the comment is redundant. https://codereview.chromium.org/1294603003/diff/240001/content/public/android... content/public/android/java/src/org/chromium/content/browser/BackgroundSyncNetworkObserver.java:67: ThreadUtils.runOnUiThread(new Runnable() { This shouldn't be necessary, we should be on the UI thread. Please add ThreadUtils.assertOnUiThread(); to all of the methods.
https://codereview.chromium.org/1294603003/diff/240001/content/browser/androi... File content/browser/android/background_sync_network_observer_android.cc (right): https://codereview.chromium.org/1294603003/diff/240001/content/browser/androi... content/browser/android/background_sync_network_observer_android.cc:45: env_, j_observer_.obj(), reinterpret_cast<jlong>(this)); On 2015/09/19 00:16:06, jkarlin wrote: > DCHECK_CURRENTLY_ON(BrowserThread::UI); I figured that DeleteOnUIThread<> made that redundant, but done. https://codereview.chromium.org/1294603003/diff/240001/content/browser/androi... content/browser/android/background_sync_network_observer_android.cc:63: : callback_(callback) {} On 2015/09/19 00:16:06, jkarlin wrote: > DCHECK_CURRENTLY_ON(BrowserThread::IO); Done. https://codereview.chromium.org/1294603003/diff/240001/content/browser/androi... content/browser/android/background_sync_network_observer_android.cc:80: void BackgroundSyncNetworkObserverAndroid::DisableNotificationForTesting() { On 2015/09/19 00:16:06, jkarlin wrote: > Looks like you call this on UI. Please rename to > DisableNotificationForTestingOnUIThread and add a dcheck. Renamed. It's too early to check here, though -- I think that SetUp is called before any thread message loops exist, so I can't actually DCHECK_CURRENTLY_ON anything. I added a check on the Java side to ensure that it is actually on the UI thread there. https://codereview.chromium.org/1294603003/diff/240001/content/browser/androi... File content/browser/android/background_sync_network_observer_android.h (right): https://codereview.chromium.org/1294603003/diff/240001/content/browser/androi... content/browser/android/background_sync_network_observer_android.h:32: static void DisableNotificationForTesting(); On 2015/09/19 00:16:07, jkarlin wrote: > Since this is public please add a comment Done. https://codereview.chromium.org/1294603003/diff/240001/content/browser/androi... content/browser/android/background_sync_network_observer_android.h:51: // constructor, with the new connection type. On 2015/09/19 00:16:07, jkarlin wrote: > append "on the IO thread" to the comment. Done. https://codereview.chromium.org/1294603003/diff/240001/content/browser/androi... content/browser/android/background_sync_network_observer_android.h:61: base::Callback<void(net::NetworkChangeNotifier::ConnectionType)> callback_; On 2015/09/19 00:16:07, jkarlin wrote: > Add comment that callback_ is to be run on the IO thread. Done. https://codereview.chromium.org/1294603003/diff/240001/content/browser/androi... content/browser/android/background_sync_network_observer_android.h:63: JNIEnv* env_; On 2015/09/19 00:16:07, jkarlin wrote: > DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/1294603003/diff/240001/content/browser/backgr... File content/browser/background_sync/background_sync_browsertest.cc (right): https://codereview.chromium.org/1294603003/diff/240001/content/browser/backgr... content/browser/background_sync/background_sync_browsertest.cc:117: BackgroundSyncNetworkObserverAndroid::DisableNotificationForTesting(); On 2015/09/19 00:16:07, jkarlin wrote: > What if you added IgnoreNetworkChangeNotifierForTesting() to > BackgroundSyncNetworkObserver? Then you could get rid of the ifdefs in this file > and the SetTestNtoificationsOnly(true) line above. You could implement it by > simply not calling the callback from BSNO when notifications come in. I'll try that approach; it might be cleaner than passing Disable..() up over JNI to the Java object. It'll have to be a static method, so that all observers (including those not yet created) are affected, though. > > Alternatively, and I like this a bit more, you could make a > MockBackgroundSyncNetworkObserver and call > BackgroundSyncManager->SetNetworkObserver(mock) and just change the network as > necessary via the mock observer. Then the BackgroundSyncNetworkObservers > wouldn't need any ForTesting functions at all. The observers would need a > SetCallback function though so that the BSM could change the mock's callback in > SetNetworkObserver. This is actually what I wanted to do, but it seemed more complicated than it really should be, just for testing. I'd have to set the BSM behaviour before it gets constructed. The BSM creates the observer in its constructor, and on Android, on creation, the Java BSNO needs to send the *actual* connectivity state down to the BSM. This is fine in the real world, but in testing, if we've set the status to offline in a regular tab, and then create a new incognito tab, the actual online state will trigger the BSM code, and fire events that we don't want to happen yet. We'd need to interfere with the construction process, with something like an ObserverFactory injected into the BackgroundSyncContext, and it started to look like a lot more complexity than was warranted for a test method. https://codereview.chromium.org/1294603003/diff/240001/content/browser/backgr... content/browser/background_sync/background_sync_browsertest.cc:127: BackgroundSyncContext* GetSyncContextFromShell(Shell* shell) { On 2015/09/19 00:16:07, jkarlin wrote: > Can you use this new function in OneShotPending? I could, but we need the storage partition separately in OneShotPending as well (to get the ServiceWorkerContext), so it doesn't really seem to make anything clearer. https://codereview.chromium.org/1294603003/diff/240001/content/browser/backgr... File content/browser/background_sync/background_sync_manager.h (right): https://codereview.chromium.org/1294603003/diff/240001/content/browser/backgr... content/browser/background_sync/background_sync_manager.h:334: On 2015/09/19 00:16:07, jkarlin wrote: > Delete new line Done. https://codereview.chromium.org/1294603003/diff/240001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/BackgroundSyncNetworkObserver.java (right): https://codereview.chromium.org/1294603003/diff/240001/content/public/android... content/public/android/java/src/org/chromium/content/browser/BackgroundSyncNetworkObserver.java:27: * This class should live on the main thread. On 2015/09/19 00:16:07, jkarlin wrote: > s/should live/lives/ Done. https://codereview.chromium.org/1294603003/diff/240001/content/public/android... content/public/android/java/src/org/chromium/content/browser/BackgroundSyncNetworkObserver.java:34: // The singleton instance. There should only be one BackgroundSyncNetworkObserver. On 2015/09/19 00:16:07, jkarlin wrote: > The second sentence of the comment is redundant. Done. Done. https://codereview.chromium.org/1294603003/diff/240001/content/public/android... content/public/android/java/src/org/chromium/content/browser/BackgroundSyncNetworkObserver.java:67: ThreadUtils.runOnUiThread(new Runnable() { On 2015/09/19 00:16:07, jkarlin wrote: > This shouldn't be necessary, we should be on the UI thread. Please add > ThreadUtils.assertOnUiThread(); to all of the methods. Done.
https://codereview.chromium.org/1294603003/diff/240001/content/browser/backgr... File content/browser/background_sync/background_sync_browsertest.cc (right): https://codereview.chromium.org/1294603003/diff/240001/content/browser/backgr... content/browser/background_sync/background_sync_browsertest.cc:117: BackgroundSyncNetworkObserverAndroid::DisableNotificationForTesting(); On 2015/09/21 18:49:30, iclelland wrote: > On 2015/09/19 00:16:07, jkarlin wrote: > > What if you added IgnoreNetworkChangeNotifierForTesting() to > > BackgroundSyncNetworkObserver? Then you could get rid of the ifdefs in this > file > > and the SetTestNtoificationsOnly(true) line above. You could implement it by > > simply not calling the callback from BSNO when notifications come in. > > I'll try that approach; it might be cleaner than passing Disable..() up over JNI > to the Java object. It'll have to be a static method, so that all observers > (including those not yet created) are affected, though. This is done now, in the latest patch set.
lgtm assuming I'm on the right track with my question. Sorry for the slow review - busy times. I'll be OOO tomorrow but happy to discuss on Thursday if my understanding isn't right https://codereview.chromium.org/1294603003/diff/200001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/BackgroundSyncNetworkObserver.java (right): https://codereview.chromium.org/1294603003/diff/200001/content/public/android... content/public/android/java/src/org/chromium/content/browser/BackgroundSyncNetworkObserver.java:19: * BackgroundSyncNetworkObserverAndroid even when the application is paused. The standard On 2015/09/18 15:46:28, iclelland wrote: > On 2015/09/16 22:04:36, Yaron wrote: > > I take it this works to have multiple registered > NetworkChangeNotifierAutoDetect > > instances with the same intent filter? Both of them get them the notification? > > It appears to, but it's not very clean. I'll see if I can make this a singleton > instead. Well you also aren't the only one to own an instance. NetworkChangeNotifier also has one but I guess you've already verified it works. That was actually what I was referring to. How do you get multiple observers? It seems like BackgroundSyncManager constructor instantiates this so you'd have one per instance of that. Oh you can have a BackgroundSyncManager per profile? I suppose that's possible on Android with incognito. https://codereview.chromium.org/1294603003/diff/300001/content/browser/androi... File content/browser/android/background_sync_network_observer_android.h (right): https://codereview.chromium.org/1294603003/diff/300001/content/browser/androi... content/browser/android/background_sync_network_observer_android.h:67: JNIEnv* env_; Hmm, we rarely cache this. I believe base::android::AttachCurrentThread is negligible overhead, especially since your thread would've done it so it's just caching at a lower layer.
lgtm assuming I'm on the right track with my question. Sorry for the slow review - busy times. I'll be OOO tomorrow but happy to discuss on Thursday if my understanding isn't right
Looking good! Note that the Android tests are still failing. https://codereview.chromium.org/1294603003/diff/300001/content/browser/backgr... File content/browser/background_sync/background_sync_network_observer.cc (right): https://codereview.chromium.org/1294603003/diff/300001/content/browser/backgr... content/browser/background_sync/background_sync_network_observer.cc:63: if (ignore_network_change_notifier_) No need for the new function (NotifyManagerIfNetworkChanged) but not a strong preference. https://codereview.chromium.org/1294603003/diff/300001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/BackgroundSyncNetworkObserver.java (right): https://codereview.chromium.org/1294603003/diff/300001/content/public/android... content/public/android/java/src/org/chromium/content/browser/BackgroundSyncNetworkObserver.java:60: mNotifier = new NetworkChangeNotifierAutoDetect(this, mContext, true); true /* always listen */ https://codereview.chromium.org/1294603003/diff/360001/content/browser/androi... File content/browser/android/background_sync_network_observer_android.h (right): https://codereview.chromium.org/1294603003/diff/360001/content/browser/androi... content/browser/android/background_sync_network_observer_android.h:35: class Observer : public base::RefCountedThreadSafe< Can this class declaration be moved to the source file? https://codereview.chromium.org/1294603003/diff/360001/content/browser/backgr... File content/browser/background_sync/background_sync_browsertest.cc (right): https://codereview.chromium.org/1294603003/diff/360001/content/browser/backgr... content/browser/background_sync/background_sync_browsertest.cc:112: #endif Can you try deleting the above chromeos code? I think it should work. https://codereview.chromium.org/1294603003/diff/360001/content/browser/backgr... File content/browser/background_sync/background_sync_manager_unittest.cc (right): https://codereview.chromium.org/1294603003/diff/360001/content/browser/backgr... content/browser/background_sync/background_sync_manager_unittest.cc:257: // Restore the network observer functionality for subsequent tests I don't believe this is necessary? Each test runs independently. https://codereview.chromium.org/1294603003/diff/360001/content/browser/backgr... content/browser/background_sync/background_sync_manager_unittest.cc:318: network_observer->NotifyManagerIfNetworkChanged(connection_type); The name "NotifyManagerIfNetworkChanged" doesn't seem to fit here. I'd expect more of a "SetNetworkTypeForTesting()" or something. https://codereview.chromium.org/1294603003/diff/360001/content/browser/backgr... File content/browser/background_sync/background_sync_network_observer.h (right): https://codereview.chromium.org/1294603003/diff/360001/content/browser/backgr... content/browser/background_sync/background_sync_network_observer.h:37: void NotifyManagerIfNetworkChanged( This would be private if not for tests. Make this private and either friend the tests or add a ForTesting function. https://codereview.chromium.org/1294603003/diff/360001/content/browser/backgr... content/browser/background_sync/background_sync_network_observer.h:50: static bool ignore_network_change_notifier_; I think you can initialize this to false here. https://codereview.chromium.org/1294603003/diff/360001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/BackgroundSyncNetworkObserver.java (right): https://codereview.chromium.org/1294603003/diff/360001/content/public/android... content/public/android/java/src/org/chromium/content/browser/BackgroundSyncNetworkObserver.java:17: import java.util.List; insert newline https://codereview.chromium.org/1294603003/diff/360001/content/public/android... content/public/android/java/src/org/chromium/content/browser/BackgroundSyncNetworkObserver.java:67: // This may fail in unit tests, if the JNI native methods have not been registered. Rather than add try/catch and a comment to production code for the test, let's just perform the registration for the applicable unit tests. https://codereview.chromium.org/1294603003/diff/360001/content/public/android... content/public/android/java/src/org/chromium/content/browser/BackgroundSyncNetworkObserver.java:91: // This may fail in unit tests, if the JNI native methods have not been registered. Ditto. https://codereview.chromium.org/1294603003/diff/360001/net/android/java/src/o... File net/android/java/src/org/chromium/net/NetworkChangeNotifier.java (right): https://codereview.chromium.org/1294603003/diff/360001/net/android/java/src/o... net/android/java/src/org/chromium/net/NetworkChangeNotifier.java:84: // This may fail in unit tests, if the JNI native methods have not been registered. Ditto
https://codereview.chromium.org/1294603003/diff/300001/content/browser/backgr... File content/browser/background_sync/background_sync_network_observer.cc (right): https://codereview.chromium.org/1294603003/diff/300001/content/browser/backgr... content/browser/background_sync/background_sync_network_observer.cc:63: if (ignore_network_change_notifier_) On 2015/09/24 17:51:30, jkarlin wrote: > No need for the new function (NotifyManagerIfNetworkChanged) but not a strong > preference. Please disregard this comment.
https://codereview.chromium.org/1294603003/diff/360001/content/browser/backgr... File content/browser/background_sync/background_sync_browsertest.cc (right): https://codereview.chromium.org/1294603003/diff/360001/content/browser/backgr... content/browser/background_sync/background_sync_browsertest.cc:32: #if defined(OS_ANDROID) Remove this block
Android tests may have been failing due to manifest permissions in the unit_tests APK. I've updated that. I'll look deeper into the best way to do the JNI registration in unit tests, but other issues have been addressed. https://codereview.chromium.org/1294603003/diff/300001/content/browser/androi... File content/browser/android/background_sync_network_observer_android.h (right): https://codereview.chromium.org/1294603003/diff/300001/content/browser/androi... content/browser/android/background_sync_network_observer_android.h:67: JNIEnv* env_; On 2015/09/22 21:20:35, Yaron (OOO 9.23) wrote: > Hmm, we rarely cache this. I believe base::android::AttachCurrentThread is > negligible overhead, especially since your thread would've done it so it's just > caching at a lower layer. I cached it so that the same pointer was used on destruction, in case that actually mattered. It doesn't seem to (I've removed that now, and everything is still working as expected; all tests are still passing), so done. Thanks. https://codereview.chromium.org/1294603003/diff/300001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/BackgroundSyncNetworkObserver.java (right): https://codereview.chromium.org/1294603003/diff/300001/content/public/android... content/public/android/java/src/org/chromium/content/browser/BackgroundSyncNetworkObserver.java:60: mNotifier = new NetworkChangeNotifierAutoDetect(this, mContext, true); On 2015/09/24 17:51:30, jkarlin wrote: > true /* always listen */ Done. https://codereview.chromium.org/1294603003/diff/360001/content/browser/androi... File content/browser/android/background_sync_network_observer_android.h (right): https://codereview.chromium.org/1294603003/diff/360001/content/browser/androi... content/browser/android/background_sync_network_observer_android.h:35: class Observer : public base::RefCountedThreadSafe< On 2015/09/24 17:51:30, jkarlin wrote: > Can this class declaration be moved to the source file? I don't think so -- because it does the JNI, it needs to be visible to browser_jni_registrar.cc. I can either leave it here, as a nested class, or I can promote it to a non-nested class, but it will still have to be declared in a header file. https://codereview.chromium.org/1294603003/diff/360001/content/browser/backgr... File content/browser/background_sync/background_sync_browsertest.cc (right): https://codereview.chromium.org/1294603003/diff/360001/content/browser/backgr... content/browser/background_sync/background_sync_browsertest.cc:32: #if defined(OS_ANDROID) On 2015/09/24 17:55:19, jkarlin wrote: > Remove this block Done. https://codereview.chromium.org/1294603003/diff/360001/content/browser/backgr... content/browser/background_sync/background_sync_browsertest.cc:112: #endif On 2015/09/24 17:51:30, jkarlin wrote: > Can you try deleting the above chromeos code? I think it should work. Done. https://codereview.chromium.org/1294603003/diff/360001/content/browser/backgr... File content/browser/background_sync/background_sync_manager_unittest.cc (right): https://codereview.chromium.org/1294603003/diff/360001/content/browser/backgr... content/browser/background_sync/background_sync_manager_unittest.cc:257: // Restore the network observer functionality for subsequent tests On 2015/09/24 17:51:30, jkarlin wrote: > I don't believe this is necessary? Each test runs independently. They're not completely isolated, and this is a process-wide global setting. In content_unittests_apk, they are run in blocks of 256 tests at a time, and not resetting this was causing the BackgroundSyncNetworkObserver unit tests to fail if they ran after these ones. This makes these tests cleaner, by undoing the modifications that SetUp makes to the environment. https://codereview.chromium.org/1294603003/diff/360001/content/browser/backgr... content/browser/background_sync/background_sync_manager_unittest.cc:318: network_observer->NotifyManagerIfNetworkChanged(connection_type); On 2015/09/24 17:51:30, jkarlin wrote: > The name "NotifyManagerIfNetworkChanged" doesn't seem to fit here. I'd expect > more of a "SetNetworkTypeForTesting()" or something. This code is actually calling the BSNO method which triggers its observers if the network state has changed. It's the same method that is called by the network_observer object's OnNetworkChanged(), except that OnNetworkChanged can be stopped by using SetIgnoreNetworkChangeNotifierForTests, and this method always calls down to the observers. https://codereview.chromium.org/1294603003/diff/360001/content/browser/backgr... File content/browser/background_sync/background_sync_network_observer.h (right): https://codereview.chromium.org/1294603003/diff/360001/content/browser/backgr... content/browser/background_sync/background_sync_network_observer.h:37: void NotifyManagerIfNetworkChanged( On 2015/09/24 17:51:30, jkarlin wrote: > This would be private if not for tests. Make this private and either friend the > tests or add a ForTesting function. Done. I've friended the test classes (and moved BackgroundSyncBrowserTest into content:: to be able to reference it.) https://codereview.chromium.org/1294603003/diff/360001/content/browser/backgr... content/browser/background_sync/background_sync_network_observer.h:50: static bool ignore_network_change_notifier_; On 2015/09/24 17:51:30, jkarlin wrote: > I think you can initialize this to false here. Nah.... "error: ISO C++ forbids in-class initialization of non-const static member". It can be declared here, but because it needs storage, it needs to be defined in an implementation file. https://codereview.chromium.org/1294603003/diff/360001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/BackgroundSyncNetworkObserver.java (right): https://codereview.chromium.org/1294603003/diff/360001/content/public/android... content/public/android/java/src/org/chromium/content/browser/BackgroundSyncNetworkObserver.java:17: import java.util.List; On 2015/09/24 17:51:30, jkarlin wrote: > insert newline Done. https://codereview.chromium.org/1294603003/diff/360001/content/public/android... content/public/android/java/src/org/chromium/content/browser/BackgroundSyncNetworkObserver.java:67: // This may fail in unit tests, if the JNI native methods have not been registered. On 2015/09/24 17:51:30, jkarlin wrote: > Rather than add try/catch and a comment to production code for the test, let's > just perform the registration for the applicable unit tests. Acknowledged. https://codereview.chromium.org/1294603003/diff/360001/content/public/android... content/public/android/java/src/org/chromium/content/browser/BackgroundSyncNetworkObserver.java:91: // This may fail in unit tests, if the JNI native methods have not been registered. On 2015/09/24 17:51:30, jkarlin wrote: > Ditto. Acknowledged. https://codereview.chromium.org/1294603003/diff/360001/net/android/java/src/o... File net/android/java/src/org/chromium/net/NetworkChangeNotifier.java (right): https://codereview.chromium.org/1294603003/diff/360001/net/android/java/src/o... net/android/java/src/org/chromium/net/NetworkChangeNotifier.java:84: // This may fail in unit tests, if the JNI native methods have not been registered. On 2015/09/24 17:51:30, jkarlin wrote: > Ditto Acknowledged.
lgtm provided the try/catches are removed https://codereview.chromium.org/1294603003/diff/360001/content/browser/androi... File content/browser/android/background_sync_network_observer_android.h (right): https://codereview.chromium.org/1294603003/diff/360001/content/browser/androi... content/browser/android/background_sync_network_observer_android.h:35: class Observer : public base::RefCountedThreadSafe< On 2015/09/24 20:37:19, iclelland wrote: > On 2015/09/24 17:51:30, jkarlin wrote: > > Can this class declaration be moved to the source file? > > I don't think so -- because it does the JNI, it needs to be visible to > browser_jni_registrar.cc. I can either leave it here, as a nested class, or I > can promote it to a non-nested class, but it will still have to be declared in a > header file. Acknowledged. https://codereview.chromium.org/1294603003/diff/360001/content/browser/backgr... File content/browser/background_sync/background_sync_manager_unittest.cc (right): https://codereview.chromium.org/1294603003/diff/360001/content/browser/backgr... content/browser/background_sync/background_sync_manager_unittest.cc:257: // Restore the network observer functionality for subsequent tests On 2015/09/24 20:37:19, iclelland wrote: > On 2015/09/24 17:51:30, jkarlin wrote: > > I don't believe this is necessary? Each test runs independently. > > They're not completely isolated, and this is a process-wide global setting. In > content_unittests_apk, they are run in blocks of 256 tests at a time, and not > resetting this was causing the BackgroundSyncNetworkObserver unit tests to fail > if they ran after these ones. This makes these tests cleaner, by undoing the > modifications that SetUp makes to the environment. Acknowledged. https://codereview.chromium.org/1294603003/diff/360001/content/browser/backgr... content/browser/background_sync/background_sync_manager_unittest.cc:318: network_observer->NotifyManagerIfNetworkChanged(connection_type); On 2015/09/24 20:37:19, iclelland wrote: > On 2015/09/24 17:51:30, jkarlin wrote: > > The name "NotifyManagerIfNetworkChanged" doesn't seem to fit here. I'd expect > > more of a "SetNetworkTypeForTesting()" or something. > > This code is actually calling the BSNO method which triggers its observers if > the network state has changed. It's the same method that is called by the > network_observer object's OnNetworkChanged(), except that OnNetworkChanged can > be stopped by using SetIgnoreNetworkChangeNotifierForTests, and this method > always calls down to the observers. I realize that, but the name seems strange from this context. But not a strong preference on my part. https://codereview.chromium.org/1294603003/diff/360001/content/browser/backgr... File content/browser/background_sync/background_sync_network_observer.h (right): https://codereview.chromium.org/1294603003/diff/360001/content/browser/backgr... content/browser/background_sync/background_sync_network_observer.h:50: static bool ignore_network_change_notifier_; On 2015/09/24 20:37:19, iclelland wrote: > On 2015/09/24 17:51:30, jkarlin wrote: > > I think you can initialize this to false here. > > Nah.... "error: ISO C++ forbids in-class initialization of non-const static > member". It can be declared here, but because it needs storage, it needs to be > defined in an implementation file. Bah, that's right.
https://codereview.chromium.org/1294603003/diff/360001/content/browser/backgr... File content/browser/background_sync/background_sync_manager_unittest.cc (right): https://codereview.chromium.org/1294603003/diff/360001/content/browser/backgr... content/browser/background_sync/background_sync_manager_unittest.cc:318: network_observer->NotifyManagerIfNetworkChanged(connection_type); On 2015/09/24 23:33:12, jkarlin wrote: > On 2015/09/24 20:37:19, iclelland wrote: > > On 2015/09/24 17:51:30, jkarlin wrote: > > > The name "NotifyManagerIfNetworkChanged" doesn't seem to fit here. I'd > expect > > > more of a "SetNetworkTypeForTesting()" or something. > > > > This code is actually calling the BSNO method which triggers its observers if > > the network state has changed. It's the same method that is called by the > > network_observer object's OnNetworkChanged(), except that OnNetworkChanged can > > be stopped by using SetIgnoreNetworkChangeNotifierForTests, and this method > > always calls down to the observers. > > I realize that, but the name seems strange from this context. But not a strong > preference on my part. Would something like OnNetworkChangedImpl or OnNetworkChangedInternal be clearer here? It signifies that we're not calling the public interface, we're directly calling the method that does the work (skipping some of the checks for testing)
https://codereview.chromium.org/1294603003/diff/360001/content/browser/backgr... File content/browser/background_sync/background_sync_manager_unittest.cc (right): https://codereview.chromium.org/1294603003/diff/360001/content/browser/backgr... content/browser/background_sync/background_sync_manager_unittest.cc:318: network_observer->NotifyManagerIfNetworkChanged(connection_type); On 2015/09/25 13:05:01, iclelland wrote: > On 2015/09/24 23:33:12, jkarlin wrote: > > On 2015/09/24 20:37:19, iclelland wrote: > > > On 2015/09/24 17:51:30, jkarlin wrote: > > > > The name "NotifyManagerIfNetworkChanged" doesn't seem to fit here. I'd > > expect > > > > more of a "SetNetworkTypeForTesting()" or something. > > > > > > This code is actually calling the BSNO method which triggers its observers > if > > > the network state has changed. It's the same method that is called by the > > > network_observer object's OnNetworkChanged(), except that OnNetworkChanged > can > > > be stopped by using SetIgnoreNetworkChangeNotifierForTests, and this method > > > always calls down to the observers. > > > > I realize that, but the name seems strange from this context. But not a strong > > preference on my part. > > Would something like OnNetworkChangedImpl or OnNetworkChangedInternal be clearer > here? It signifies that we're not calling the public interface, we're directly > calling the method that does the work (skipping some of the checks for testing) Yes, OnNetworkChangedImpl would do nicely.
https://codereview.chromium.org/1294603003/diff/300001/content/browser/androi... File content/browser/android/background_sync_network_observer_android.h (right): https://codereview.chromium.org/1294603003/diff/300001/content/browser/androi... content/browser/android/background_sync_network_observer_android.h:67: JNIEnv* env_; On 2015/09/24 20:37:19, iclelland wrote: > On 2015/09/22 21:20:35, Yaron (OOO 9.23) wrote: > > Hmm, we rarely cache this. I believe base::android::AttachCurrentThread is > > negligible overhead, especially since your thread would've done it so it's > just > > caching at a lower layer. > > I cached it so that the same pointer was used on destruction, in case that > actually mattered. It doesn't. global refs can be used across threads, and you just need the result of an AttachCurrentThread env. It doesn't seem to (I've removed that now, and everything is > still working as expected; all tests are still passing), so done. Thanks. https://codereview.chromium.org/1294603003/diff/380001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/BackgroundSyncNetworkObserver.java (right): https://codereview.chromium.org/1294603003/diff/380001/content/public/android... content/public/android/java/src/org/chromium/content/browser/BackgroundSyncNetworkObserver.java:69: // This may fail in unit tests, if the JNI native methods have not been registered. This really shouldn't happen. Are you able to repro locally? I can help investigate. Which test suites were failing to have registrations? Any test that builds on top of content layer should've had a call to the library loader and done all java registratoins from content and below
https://codereview.chromium.org/1294603003/diff/380001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/BackgroundSyncNetworkObserver.java (right): https://codereview.chromium.org/1294603003/diff/380001/content/public/android... content/public/android/java/src/org/chromium/content/browser/BackgroundSyncNetworkObserver.java:69: // This may fail in unit tests, if the JNI native methods have not been registered. On 2015/09/25 18:43:02, Yaron wrote: > This really shouldn't happen. Are you able to repro locally? I can help > investigate. Which test suites were failing to have registrations? Any test that > builds on top of content layer should've had a call to the library loader and > done all java registratoins from content and below So, this has been solved. The components unit tests did not register either the content::browser or net JNI methods. Adding those (as well as adding ACCESS_NETWORK_STATE to unit_tests_apk) has allowed all of the tests to pass on Android.
slgtm though you'll need an owner for the components/ changes
iclelland@chromium.org changed reviewers: + jochen@chromium.org
On 2015/09/28 11:42:43, jkarlin wrote: > slgtm though you'll need an owner for the components/ changes Thanks. jochen, can you PTAL at the changes to components/*?
lgtm https://codereview.chromium.org/1294603003/diff/400001/components/test/run_al... File components/test/run_all_unittests.cc (right): https://codereview.chromium.org/1294603003/diff/400001/components/test/run_al... components/test/run_all_unittests.cc:29: #include "content/browser/android/browser_jni_registrar.h" sorry about you having to deal with this - it's kind of just luck that it worked until now.
lgtm
https://codereview.chromium.org/1294603003/diff/400001/components/test/run_al... File components/test/run_all_unittests.cc (right): https://codereview.chromium.org/1294603003/diff/400001/components/test/run_al... components/test/run_all_unittests.cc:29: #include "content/browser/android/browser_jni_registrar.h" On 2015/09/28 14:19:23, Yaron wrote: > sorry about you having to deal with this - it's kind of just luck that it worked > until now. Well, at least I got to learn something new :) Thanks for the help untangling it.
The CQ bit was checked by iclelland@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jkarlin@chromium.org Link to the patchset: https://codereview.chromium.org/1294603003/#ps400001 (title: "Register JNI methods in component unittests; stop swallowing errors")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1294603003/400001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1294603003/400001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
iclelland@chromium.org changed reviewers: + mmenke@chromium.org
+r mmenke -- Can I get a review for the change in components/test/DEPS? (See the change to components/test/run_all_unittests.cc for context) Thanks!
On 2015/09/28 17:54:16, iclelland wrote: > +r mmenke -- Can I get a review for the change in components/test/DEPS? > > (See the change to components/test/run_all_unittests.cc for context) > > Thanks! components/test/DEPS LGTM.
The CQ bit was checked by iclelland@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1294603003/400001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1294603003/400001
Message was sent while issue was closed.
Committed patchset #20 (id:400001)
Message was sent while issue was closed.
Patchset 20 (id:??) landed as https://crrev.com/a5fb97d6fb9c3831994aadebea4664e2acc3c356 Cr-Commit-Position: refs/heads/master@{#351093}
Message was sent while issue was closed.
A revert of this CL (patchset #20 id:400001) has been created in https://codereview.chromium.org/1376563003/ by iclelland@chromium.org. The reason for reverting is: This patch breaks Android System WebView embedding, by assuming that ACCESS_NETWORK_STATE is available in all cases. NetworkChangeNotifierAutoDetect should not be created automatically by a WebView. Need to fix this before relanding this patch..
On 2015/09/29 19:44:36, iclelland wrote: > A revert of this CL (patchset #20 id:400001) has been created in > https://codereview.chromium.org/1376563003/ by mailto:iclelland@chromium.org. > > The reason for reverting is: This patch breaks Android System WebView embedding, > by assuming that ACCESS_NETWORK_STATE is available in all cases. > > NetworkChangeNotifierAutoDetect should not be created automatically by a > WebView. Need to fix this before relanding this patch.. jkarlin - can you PTAL at the proposed changes (PS #22,23) which should isolate this code from crashing if embedded in an app which does not have the ACCESS_NETWORK_STATE permission.
On 2015/10/01 02:33:01, iclelland wrote: > On 2015/09/29 19:44:36, iclelland wrote: > > A revert of this CL (patchset #20 id:400001) has been created in > > https://codereview.chromium.org/1376563003/ by mailto:iclelland@chromium.org. > > > > The reason for reverting is: This patch breaks Android System WebView > embedding, > > by assuming that ACCESS_NETWORK_STATE is available in all cases. > > > > NetworkChangeNotifierAutoDetect should not be created automatically by a > > WebView. Need to fix this before relanding this patch.. > > jkarlin - can you PTAL at the proposed changes (PS #22,23) which should isolate > this code from crashing if embedded in an app which does not have the > ACCESS_NETWORK_STATE permission. The usual approach to relanding a CL in chrome is to make a new CL with the first PS being the landed (but broken) patch and the fixes after that. Also, include links to the original land and its revert in the new CL's description. Thanks!
On 2015/10/01 14:05:56, jkarlin wrote: > On 2015/10/01 02:33:01, iclelland wrote: > > On 2015/09/29 19:44:36, iclelland wrote: > > > A revert of this CL (patchset #20 id:400001) has been created in > > > https://codereview.chromium.org/1376563003/ by > mailto:iclelland@chromium.org. > > > > > > The reason for reverting is: This patch breaks Android System WebView > > embedding, > > > by assuming that ACCESS_NETWORK_STATE is available in all cases. > > > > > > NetworkChangeNotifierAutoDetect should not be created automatically by a > > > WebView. Need to fix this before relanding this patch.. > > > > jkarlin - can you PTAL at the proposed changes (PS #22,23) which should > isolate > > this code from crashing if embedded in an app which does not have the > > ACCESS_NETWORK_STATE permission. > > The usual approach to relanding a CL in chrome is to make a new CL with the > first PS being the landed (but broken) patch and the fixes after that. Also, > include links to the original land and its revert in the new CL's description. > Thanks! Done -- https://codereview.chromium.org/1383663004/
Message was sent while issue was closed.
Just FYI: I'm closing this issue again. |
