Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2608333004:
Clarify that NTPSnippetsBridge holds a Java SnippetsBridge. (Closed)
|
Created:
3 years, 11 months ago by Michael van Ouwerkerk Modified:
3 years, 11 months ago Reviewers:
Bernhard Bauer CC:
chromium-reviews, noyau+watch_chromium.org, ntp-dev+reviews_chromium.org, agrieve+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionClarify that NTPSnippetsBridge holds a Java SnippetsBridge.
BUG=616090
Review-Url: https://codereview.chromium.org/2608333004
Cr-Commit-Position: refs/heads/master@{#441698}
Committed: https://chromium.googlesource.com/chromium/src/+/bfb9198a9bfe0605b5dd72b65bf39ab8048464af
Patch Set 1 #
Total comments: 10
Patch Set 2 : Address review comments from bauerb. #
Total comments: 2
Patch Set 3 : Clean up null checks and asserts. #Patch Set 4 : Add null checks for the Java observer. #
Messages
Total messages: 27 (19 generated)
The CQ bit was checked by mvanouwerkerk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
mvanouwerkerk@chromium.org changed reviewers: + bauerb@chromium.org
Bernhard, could you take a look please?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2608333004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java (right): https://codereview.chromium.org/2608333004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java:192: * @param observer object to notify when snippets are received, or {@code null} if we want to I don't think we actually support stopping observing anymore. https://codereview.chromium.org/2608333004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java:268: if (mObserver != null) mObserver.onCategoryStatusChanged(category, newStatus); So, here we check whether the observer is null, above we assert that it's not. I think we should probably do one or the other consistently (AFAICT, right now the observer is set on the same turn of the event loop as when this object is created, so asserting would be fine here as well, but we could in principle be more lenient). (And in principle we could refactor things so the observer is passed in the constructor, but that would require a bigger refactoring of the NewTabPageManager. Oh hey! 😉) https://codereview.chromium.org/2608333004/diff/1/chrome/browser/android/ntp/... File chrome/browser/android/ntp/ntp_snippets_bridge.cc (right): https://codereview.chromium.org/2608333004/diff/1/chrome/browser/android/ntp/... chrome/browser/android/ntp/ntp_snippets_bridge.cc:406: if (bridge_.is_null()) { Hm, we now know that |bridge_| is non-null, right?
https://codereview.chromium.org/2608333004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java (right): https://codereview.chromium.org/2608333004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java:192: * @param observer object to notify when snippets are received, or {@code null} if we want to On 2017/01/04 17:27:34, Bernhard Bauer wrote: > I don't think we actually support stopping observing anymore. I'm not sure what you mean. The caller of this method (SectionList) could pass null to stop being notified, even though that currently never happens. https://codereview.chromium.org/2608333004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java:268: if (mObserver != null) mObserver.onCategoryStatusChanged(category, newStatus); On 2017/01/04 17:27:34, Bernhard Bauer wrote: > So, here we check whether the observer is null, above we assert that it's not. I > think we should probably do one or the other consistently (AFAICT, right now the > observer is set on the same turn of the event loop as when this object is > created, so asserting would be fine here as well, but we could in principle be > more lenient). > > (And in principle we could refactor things so the observer is passed in the > constructor, but that would require a bigger refactoring of the > NewTabPageManager. Oh hey! 😉) I think we can assert, as it is currently only set back to null from destroy() and this is called from native. Done. https://codereview.chromium.org/2608333004/diff/1/chrome/browser/android/ntp/... File chrome/browser/android/ntp/ntp_snippets_bridge.cc (right): https://codereview.chromium.org/2608333004/diff/1/chrome/browser/android/ntp/... chrome/browser/android/ntp/ntp_snippets_bridge.cc:406: if (bridge_.is_null()) { On 2017/01/04 17:27:34, Bernhard Bauer wrote: > Hm, we now know that |bridge_| is non-null, right? I think it could be, as it is reset in ContentSuggestionsServiceShutdown.
The CQ bit was checked by mvanouwerkerk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2608333004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java (right): https://codereview.chromium.org/2608333004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java:192: * @param observer object to notify when snippets are received, or {@code null} if we want to On 2017/01/05 11:35:40, Michael van Ouwerkerk wrote: > On 2017/01/04 17:27:34, Bernhard Bauer wrote: > > I don't think we actually support stopping observing anymore. > > I'm not sure what you mean. The caller of this method (SectionList) could pass > null to stop being notified, even though that currently never happens. Yes, but we won't actually stop sending all notifications, because e.g. onNewSuggestions asserts that the observer is non-null. And because that code path isn't actually exercised right now, no one noticed :) https://codereview.chromium.org/2608333004/diff/1/chrome/browser/android/ntp/... File chrome/browser/android/ntp/ntp_snippets_bridge.cc (right): https://codereview.chromium.org/2608333004/diff/1/chrome/browser/android/ntp/... chrome/browser/android/ntp/ntp_snippets_bridge.cc:406: if (bridge_.is_null()) { On 2017/01/05 11:35:40, Michael van Ouwerkerk wrote: > On 2017/01/04 17:27:34, Bernhard Bauer wrote: > > Hm, we now know that |bridge_| is non-null, right? > > I think it could be, as it is reset in ContentSuggestionsServiceShutdown. But in that case we shouldn't get called back anymore, right? (Also, does that even happen on Android? 😃) https://codereview.chromium.org/2608333004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java (right): https://codereview.chromium.org/2608333004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java:268: assert mNativeSnippetsBridge != 0; I'm not sure we absolutely need to assert this -- if the native object is destroyed, nothing could call us. I don't feel that strongly about it though.
https://codereview.chromium.org/2608333004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java (right): https://codereview.chromium.org/2608333004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java:192: * @param observer object to notify when snippets are received, or {@code null} if we want to On 2017/01/05 11:57:47, Bernhard Bauer wrote: > On 2017/01/05 11:35:40, Michael van Ouwerkerk wrote: > > On 2017/01/04 17:27:34, Bernhard Bauer wrote: > > > I don't think we actually support stopping observing anymore. > > > > I'm not sure what you mean. The caller of this method (SectionList) could pass > > null to stop being notified, even though that currently never happens. > > Yes, but we won't actually stop sending all notifications, because e.g. > onNewSuggestions asserts that the observer is non-null. And because that code > path isn't actually exercised right now, no one noticed :) Changed it to assert the argument is not null. https://codereview.chromium.org/2608333004/diff/1/chrome/browser/android/ntp/... File chrome/browser/android/ntp/ntp_snippets_bridge.cc (right): https://codereview.chromium.org/2608333004/diff/1/chrome/browser/android/ntp/... chrome/browser/android/ntp/ntp_snippets_bridge.cc:406: if (bridge_.is_null()) { On 2017/01/05 11:57:47, Bernhard Bauer wrote: > On 2017/01/05 11:35:40, Michael van Ouwerkerk wrote: > > On 2017/01/04 17:27:34, Bernhard Bauer wrote: > > > Hm, we now know that |bridge_| is non-null, right? > > > > I think it could be, as it is reset in ContentSuggestionsServiceShutdown. > > But in that case we shouldn't get called back anymore, right? > > (Also, does that even happen on Android? 😃) Done. https://codereview.chromium.org/2608333004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java (right): https://codereview.chromium.org/2608333004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java:268: assert mNativeSnippetsBridge != 0; On 2017/01/05 11:57:47, Bernhard Bauer wrote: > I'm not sure we absolutely need to assert this -- if the native object is > destroyed, nothing could call us. I don't feel that strongly about it though. Done.
The CQ bit was checked by mvanouwerkerk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by mvanouwerkerk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Clarify that NTPSnippetsBridge holds a Java SnippetsBridge. ========== to ========== Clarify that NTPSnippetsBridge holds a Java SnippetsBridge. BUG=616090 ==========
lgtm
The CQ bit was checked by mvanouwerkerk@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1483638599154940,
"parent_rev": "cb6b77978f8ac89cfe31e15b7dc427af746f71a1", "commit_rev":
"bfb9198a9bfe0605b5dd72b65bf39ab8048464af"}
Message was sent while issue was closed.
Description was changed from ========== Clarify that NTPSnippetsBridge holds a Java SnippetsBridge. BUG=616090 ========== to ========== Clarify that NTPSnippetsBridge holds a Java SnippetsBridge. BUG=616090 Review-Url: https://codereview.chromium.org/2608333004 Cr-Commit-Position: refs/heads/master@{#441698} Committed: https://chromium.googlesource.com/chromium/src/+/bfb9198a9bfe0605b5dd72b65bf3... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/bfb9198a9bfe0605b5dd72b65bf3... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
