Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1884203002:
NTP tiles: split methods into C++ and Java parts. (Closed)
|
Created:
4 years, 8 months ago by sfiera Modified:
4 years, 8 months ago Reviewers:
Marc Treib CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionNTP tiles: split methods into C++ and Java parts.
This should be basically all of the "code change" necessary to split out
the component, though there will be a lot more code movement. For now,
C++ and Java methods remain as overloads in the same class.
BUG=603026
Committed: https://crrev.com/1e7993c61536d7cd7ad7eeaa8e60ff07994c0c46
Cr-Commit-Position: refs/heads/master@{#387878}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #
Total comments: 38
Patch Set 4 : #Patch Set 5 : #
Total comments: 1
Patch Set 6 : #Patch Set 7 : #
Total comments: 2
Patch Set 8 : #
Messages
Total messages: 17 (4 generated)
sfiera@chromium.org changed reviewers: + treib@chromium.org
This is untested; I'm not sure how the unittest for this is supposed to be run. It doesn't appear to be referenced from any .gn file.
On 2016/04/14 16:15:34, sfiera wrote: > This is untested; I'm not sure how the unittest for this is supposed to be run. > It doesn't appear to be referenced from any .gn file. In some cases, the .gn files somehow re-use the file lists from .gyp/.gypi files; I've never investigated why only sometimes, or how exactly. Anyway, this should be in the regular unit_tests target. See also: https://chromium.googlesource.com/chromium/src/+/master/docs/android_test_ins...
https://codereview.chromium.org/1884203002/diff/40001/chrome/browser/android/... File chrome/browser/android/ntp/most_visited_sites.cc (right): https://codereview.chromium.org/1884203002/diff/40001/chrome/browser/android/... chrome/browser/android/ntp/most_visited_sites.cc:213: namespace { This should be merged into the anonymous namespace above. https://codereview.chromium.org/1884203002/diff/40001/chrome/browser/android/... chrome/browser/android/ntp/most_visited_sites.cc:218: JNIEnv* env, const base::android::JavaParamRef<jobject>& obj) nit: "base::android::" isn't required here. https://codereview.chromium.org/1884203002/diff/40001/chrome/browser/android/... chrome/browser/android/ntp/most_visited_sites.cc:221: void NotifyMostVisitedURLsObserver( I think this should be called "OnMostVisitedSitesAvailable" or something like that? https://codereview.chromium.org/1884203002/diff/40001/chrome/browser/android/... chrome/browser/android/ntp/most_visited_sites.cc:245: base::android::ScopedJavaGlobalRef<jobject> observer_; Also here: no "base::android::" required. https://codereview.chromium.org/1884203002/diff/40001/chrome/browser/android/... chrome/browser/android/ntp/most_visited_sites.cc:306: static void CallJavaWithBitmap( This should go into the anonymous namespace above. https://codereview.chromium.org/1884203002/diff/40001/chrome/browser/android/... chrome/browser/android/ntp/most_visited_sites.cc:318: j_callback->Reset(env, j_callback_obj); Not your doing, but do you see any reason why we construct a ScopedJavaGlobalRef and then immediately reset it?! https://codereview.chromium.org/1884203002/diff/40001/chrome/browser/android/... chrome/browser/android/ntp/most_visited_sites.cc:881: if (!observer_.get()) nit: I think ".get()" isn't required. https://codereview.chromium.org/1884203002/diff/40001/chrome/browser/android/... File chrome/browser/android/ntp/most_visited_sites.h (right): https://codereview.chromium.org/1884203002/diff/40001/chrome/browser/android/... chrome/browser/android/ntp/most_visited_sites.h:52: const std::vector<std::string>& large_icon_urls); I think these should be pure virtual? (i.e. add " = 0" before the semicolon) Or alternatively, add an empty implementation. https://codereview.chromium.org/1884203002/diff/40001/chrome/browser/android/... chrome/browser/android/ntp/most_visited_sites.h:90: // C++ methods All these can be private for now, right? https://codereview.chromium.org/1884203002/diff/40001/chrome/browser/android/... chrome/browser/android/ntp/most_visited_sites.h:97: void GetURLThumbnail(const GURL& url, const ThumbnailCallback& callback); Hm.. generally, method overloading is kinda frowned upon in Chromium. But if this is just a temporary state until the class is split in two, I guess that's okay. https://codereview.chromium.org/1884203002/diff/40001/chrome/browser/android/... chrome/browser/android/ntp/most_visited_sites.h:216: const base::Callback<void(bool, const SkBitmap*)>& callback, ThumbnailCallback? https://codereview.chromium.org/1884203002/diff/40001/chrome/browser/android/... chrome/browser/android/ntp/most_visited_sites.h:223: const base::Callback<void(bool, const SkBitmap*)>& callback, Here too https://codereview.chromium.org/1884203002/diff/40001/chrome/browser/android/... chrome/browser/android/ntp/most_visited_sites.h:241: std::unique_ptr<MostVisitedSitesObserver> observer_; This is a very uncommon pattern - the observed object usually doesn't own the observer. But I guess this is also only temporary, while the class essentially observes itself.
I ran most_visited_sites_unittest.cc, but looking at it, it doesn't seem that it
exercises much of the interface--just the stuff that was easy to test from C++
and none of the Java, which is what is actually changing in this CL.
Looking at the methods I want to test:
RecordTileTypeMetrics)
RecordOpenedMostVisitedItem)
Probably easy. Just need to get the histograms and check before and after.
AddOrRemoveBlacklistedUrl)
Tractable? Just need simple mocks for TopSites and SuggestionsService.
GetURLThumbnail)
Trickier. Does PostTaskAndReplyWithResult() work as expected under test? If
so, it might not be much harder than the previous test.
SetMostVisitedURLsObserver)
This test will probably be a lot of work compared to the above. Might not
want to block this CL behind it.
https://codereview.chromium.org/1884203002/diff/40001/chrome/browser/android/...
File chrome/browser/android/ntp/most_visited_sites.cc (right):
https://codereview.chromium.org/1884203002/diff/40001/chrome/browser/android/...
chrome/browser/android/ntp/most_visited_sites.cc:213: namespace {
On 2016/04/14 16:57:36, Marc Treib wrote:
> This should be merged into the anonymous namespace above.
Done.
https://codereview.chromium.org/1884203002/diff/40001/chrome/browser/android/...
chrome/browser/android/ntp/most_visited_sites.cc:218: JNIEnv* env, const
base::android::JavaParamRef<jobject>& obj)
On 2016/04/14 16:57:35, Marc Treib wrote:
> nit: "base::android::" isn't required here.
Done.
https://codereview.chromium.org/1884203002/diff/40001/chrome/browser/android/...
chrome/browser/android/ntp/most_visited_sites.cc:221: void
NotifyMostVisitedURLsObserver(
On 2016/04/14 16:57:36, Marc Treib wrote:
> I think this should be called "OnMostVisitedSitesAvailable" or something like
> that?
Done. (also below, OnPopularSitesAvailable => OnPopularURLsAvailable)
https://codereview.chromium.org/1884203002/diff/40001/chrome/browser/android/...
chrome/browser/android/ntp/most_visited_sites.cc:245:
base::android::ScopedJavaGlobalRef<jobject> observer_;
On 2016/04/14 16:57:36, Marc Treib wrote:
> Also here: no "base::android::" required.
Done.
https://codereview.chromium.org/1884203002/diff/40001/chrome/browser/android/...
chrome/browser/android/ntp/most_visited_sites.cc:306: static void
CallJavaWithBitmap(
On 2016/04/14 16:57:36, Marc Treib wrote:
> This should go into the anonymous namespace above.
Any reason? It's static already.
https://codereview.chromium.org/1884203002/diff/40001/chrome/browser/android/...
chrome/browser/android/ntp/most_visited_sites.cc:318: j_callback->Reset(env,
j_callback_obj);
On 2016/04/14 16:57:36, Marc Treib wrote:
> Not your doing, but do you see any reason why we construct a
ScopedJavaGlobalRef
> and then immediately reset it?!
That's not exactly what's happening. We're calling
j_callback->[ScopedJavaGlobalRef::]Reset(), not
j_callback.[unique_ptr::]reset().
I still don't know why. Maybe you can't base::Passed() a ScopedJavaGlobalRef<>?
https://codereview.chromium.org/1884203002/diff/40001/chrome/browser/android/...
chrome/browser/android/ntp/most_visited_sites.cc:881: if (!observer_.get())
On 2016/04/14 16:57:35, Marc Treib wrote:
> nit: I think ".get()" isn't required.
Correct you are.
https://codereview.chromium.org/1884203002/diff/40001/chrome/browser/android/...
File chrome/browser/android/ntp/most_visited_sites.h (right):
https://codereview.chromium.org/1884203002/diff/40001/chrome/browser/android/...
chrome/browser/android/ntp/most_visited_sites.h:52: const
std::vector<std::string>& large_icon_urls);
On 2016/04/14 16:57:36, Marc Treib wrote:
> I think these should be pure virtual? (i.e. add " = 0" before the semicolon)
> Or alternatively, add an empty implementation.
Pure virtual; I'd forgotten.
https://codereview.chromium.org/1884203002/diff/40001/chrome/browser/android/...
chrome/browser/android/ntp/most_visited_sites.h:90: // C++ methods
On 2016/04/14 16:57:36, Marc Treib wrote:
> All these can be private for now, right?
Could be. If I could choose, I'd rather make the Java methods private, since the
C++ functions describe the public interface and the Java methods are just glue.
But I have no idea how JNI works and if that's allowed.
https://codereview.chromium.org/1884203002/diff/40001/chrome/browser/android/...
chrome/browser/android/ntp/most_visited_sites.h:97: void GetURLThumbnail(const
GURL& url, const ThumbnailCallback& callback);
On 2016/04/14 16:57:36, Marc Treib wrote:
> Hm.. generally, method overloading is kinda frowned upon in Chromium. But if
> this is just a temporary state until the class is split in two, I guess that's
> okay.
I double-checked the style guide and the verdict is basically "prefer not to;
only if you can tell at the call site what the difference is" and I think that's
the case here :)
https://codereview.chromium.org/1884203002/diff/40001/chrome/browser/android/...
chrome/browser/android/ntp/most_visited_sites.h:216: const
base::Callback<void(bool, const SkBitmap*)>& callback,
On 2016/04/14 16:57:36, Marc Treib wrote:
> ThumbnailCallback?
Done.
https://codereview.chromium.org/1884203002/diff/40001/chrome/browser/android/...
chrome/browser/android/ntp/most_visited_sites.h:223: const
base::Callback<void(bool, const SkBitmap*)>& callback,
On 2016/04/14 16:57:36, Marc Treib wrote:
> Here too
Done. (and in the .cc file)
https://codereview.chromium.org/1884203002/diff/40001/chrome/browser/android/...
chrome/browser/android/ntp/most_visited_sites.h:241:
std::unique_ptr<MostVisitedSitesObserver> observer_;
On 2016/04/14 16:57:36, Marc Treib wrote:
> This is a very uncommon pattern - the observed object usually doesn't own the
> observer.
> But I guess this is also only temporary, while the class essentially observes
> itself.
Again, I could do with some more knowledge about JNI. Do you know how the
observer object from Java is going to be owned in that case?
I could split this into two variables, an unowned one that will belong with C++
and one for the Java bridge.
Some top-level comments before I look at the actual code. On 2016/04/15 13:36:00, sfiera wrote: > I ran most_visited_sites_unittest.cc, but looking at it, it doesn't seem that it > exercises much of the interface--just the stuff that was easy to test from C++ > and none of the Java, which is what is actually changing in this CL. > > Looking at the methods I want to test: > > RecordTileTypeMetrics) > RecordOpenedMostVisitedItem) > Probably easy. Just need to get the histograms and check before and after. Eh, those don't really need tests IMO. They just record metrics but have no real logic otherwise. If anything, we might want to test that these methods are called at the right times, but not the actual histograms. > AddOrRemoveBlacklistedUrl) > Tractable? Just need simple mocks for TopSites and SuggestionsService. I think SuggestionsService isn't easily mockable right now. That should certainly change at some point, but I wouldn't block this CL on it. > GetURLThumbnail) > Trickier. Does PostTaskAndReplyWithResult() work as expected under test? If > so, it might not be much harder than the previous test. Not automatically - the DB thread probably doesn't exist by default in unit tests. This kind of thing can be set up, but it's generally yucky, and it'd probably be more setup code than actual test code... > SetMostVisitedURLsObserver) > This test will probably be a lot of work compared to the above. Might not > want to block this CL behind it. I'm not sure what exactly would be hard about it, and what exactly you'd even want to test? In any case, not adding tests isn't really worse than the status quo, so I wouldn't block this CL on it anyway.
On 2016/04/15 14:11:00, Marc Treib wrote: > Some top-level comments before I look at the actual code. > > On 2016/04/15 13:36:00, sfiera wrote: > > I ran most_visited_sites_unittest.cc, but looking at it, it doesn't seem that > it > > exercises much of the interface--just the stuff that was easy to test from C++ > > and none of the Java, which is what is actually changing in this CL. > > > > Looking at the methods I want to test: > > > > RecordTileTypeMetrics) > > RecordOpenedMostVisitedItem) > > Probably easy. Just need to get the histograms and check before and after. > > Eh, those don't really need tests IMO. They just record metrics but have no real > logic otherwise. > If anything, we might want to test that these methods are called at the right > times, but not the actual histograms. The current Java test doesn't do any testing. Wouldn't be hard to add. Wouldn't be appropriate to add it in this CL :) > > AddOrRemoveBlacklistedUrl) > > Tractable? Just need simple mocks for TopSites and SuggestionsService. > > I think SuggestionsService isn't easily mockable right now. That should > certainly change at some point, but I wouldn't block this CL on it. > > > GetURLThumbnail) > > Trickier. Does PostTaskAndReplyWithResult() work as expected under test? > If > > so, it might not be much harder than the previous test. > > Not automatically - the DB thread probably doesn't exist by default in unit > tests. This kind of thing can be set up, but it's generally yucky, and it'd > probably be more setup code than actual test code... > > > SetMostVisitedURLsObserver) > > This test will probably be a lot of work compared to the above. Might not > > want to block this CL behind it. > > I'm not sure what exactly would be hard about it, and what exactly you'd even > want to test? > In any case, not adding tests isn't really worse than the status quo, so I > wouldn't block this CL on it anyway. Just generally more involved control flow. The other methods are call/return or call/callback. I don't really know enough about what's going on here to know what I'd want to test, which is why I'd want a test for it :)
Thanks! Mostly looks good, some last comments/questions below. https://codereview.chromium.org/1884203002/diff/40001/chrome/browser/android/... File chrome/browser/android/ntp/most_visited_sites.cc (right): https://codereview.chromium.org/1884203002/diff/40001/chrome/browser/android/... chrome/browser/android/ntp/most_visited_sites.cc:306: static void CallJavaWithBitmap( On 2016/04/15 13:35:59, sfiera wrote: > On 2016/04/14 16:57:36, Marc Treib wrote: > > This should go into the anonymous namespace above. > > Any reason? It's static already. Ah, the code style indeed allows either. But putting it in a namespace is a lot more common in Chromium; I don't think I've ever seen a static function. Putting it in the namespace above will also remove the need to forward-declare it ;) https://codereview.chromium.org/1884203002/diff/40001/chrome/browser/android/... chrome/browser/android/ntp/most_visited_sites.cc:318: j_callback->Reset(env, j_callback_obj); On 2016/04/15 13:35:59, sfiera wrote: > On 2016/04/14 16:57:36, Marc Treib wrote: > > Not your doing, but do you see any reason why we construct a > ScopedJavaGlobalRef > > and then immediately reset it?! > > That's not exactly what's happening. We're calling > j_callback->[ScopedJavaGlobalRef::]Reset(), not > j_callback.[unique_ptr::]reset(). Ah, good point, I misread that. ScopedJavaGlobalRef has a ctor that takes two args (and in fact just calls Reset) though, so I'd just move the args into the ctor above and remove the Reset line. > I still don't know why. Maybe you can't base::Passed() a ScopedJavaGlobalRef<>? base::Passed explicitly transfers ownership, which doesn't make sense for a ScopedJavaGlobalRef. It should be cheap to just copy it though? Anyway, I'd leave that unchanged. https://codereview.chromium.org/1884203002/diff/40001/chrome/browser/android/... File chrome/browser/android/ntp/most_visited_sites.h (right): https://codereview.chromium.org/1884203002/diff/40001/chrome/browser/android/... chrome/browser/android/ntp/most_visited_sites.h:90: // C++ methods On 2016/04/15 13:36:00, sfiera wrote: > On 2016/04/14 16:57:36, Marc Treib wrote: > > All these can be private for now, right? > > Could be. If I could choose, I'd rather make the Java methods private, since the > C++ functions describe the public interface and the Java methods are just glue. > But I have no idea how JNI works and if that's allowed. AFAIK JNI doesn't care about public/private, so probably everything could be private. Anyway, I guess the next CL will split this class in two (the bridge part and the logic part), so it doesn't really matter. https://codereview.chromium.org/1884203002/diff/40001/chrome/browser/android/... chrome/browser/android/ntp/most_visited_sites.h:97: void GetURLThumbnail(const GURL& url, const ThumbnailCallback& callback); On 2016/04/15 13:36:00, sfiera wrote: > On 2016/04/14 16:57:36, Marc Treib wrote: > > Hm.. generally, method overloading is kinda frowned upon in Chromium. But if > > this is just a temporary state until the class is split in two, I guess that's > > okay. > > I double-checked the style guide and the verdict is basically "prefer not to; > only if you can tell at the call site what the difference is" and I think that's > the case here :) Fair enough. And, again, it's a temporary state of affairs anyway. https://codereview.chromium.org/1884203002/diff/40001/chrome/browser/android/... chrome/browser/android/ntp/most_visited_sites.h:241: std::unique_ptr<MostVisitedSitesObserver> observer_; On 2016/04/15 13:36:00, sfiera wrote: > On 2016/04/14 16:57:36, Marc Treib wrote: > > This is a very uncommon pattern - the observed object usually doesn't own the > > observer. > > But I guess this is also only temporary, while the class essentially observes > > itself. Java objects always have shared ownership, AFAIK that remains true through JNI. > Again, I could do with some more knowledge about JNI. Do you know how the > observer object from Java is going to be owned in that case? > > I could split this into two variables, an unowned one that will belong with C++ > and one for the Java bridge. I guess when this class is split in two, something like that would happen anyway? Then we can also leave it as-is for now. I'd imagine the final state something like this: A "MostVisitedBridge" object, which has the same lifetime and ownership as the current MostVisitedSites, and which holds a Java ref to the Java observer object. This class would implement your MostVisitedSitesObserver. The logic would live in a new MostVisitedSites object, which is probably a ProfileKeyedService, and which has an ObserverList. The MostVisitedBridge would register/unregister itself as an (unowned) observer. Does that match what you have in mind? https://codereview.chromium.org/1884203002/diff/80001/chrome/browser/android/... File chrome/browser/android/ntp/most_visited_sites.cc (right): https://codereview.chromium.org/1884203002/diff/80001/chrome/browser/android/... chrome/browser/android/ntp/most_visited_sites.cc:170: void OnMostVisitedURLsAvailable( I kinda preferred "Sites" over "URLs" (also below for Popular) - it's not just URLs. But since the Java code uses "URLs"... oh well.
https://codereview.chromium.org/1884203002/diff/40001/chrome/browser/android/... File chrome/browser/android/ntp/most_visited_sites.cc (right): https://codereview.chromium.org/1884203002/diff/40001/chrome/browser/android/... chrome/browser/android/ntp/most_visited_sites.cc:306: static void CallJavaWithBitmap( On 2016/04/15 15:21:41, Marc Treib wrote: > On 2016/04/15 13:35:59, sfiera wrote: > > On 2016/04/14 16:57:36, Marc Treib wrote: > > > This should go into the anonymous namespace above. > > > > Any reason? It's static already. > > Ah, the code style indeed allows either. But putting it in a namespace is a lot > more common in Chromium; I don't think I've ever seen a static function. > Putting it in the namespace above will also remove the need to forward-declare > it ;) There's actually no need to forward-declare it either; I could have defined it up where I declared it. I was keeping the code in the original order for a clear diff. https://codereview.chromium.org/1884203002/diff/40001/chrome/browser/android/... chrome/browser/android/ntp/most_visited_sites.cc:318: j_callback->Reset(env, j_callback_obj); On 2016/04/15 15:21:41, Marc Treib wrote: > On 2016/04/15 13:35:59, sfiera wrote: > > On 2016/04/14 16:57:36, Marc Treib wrote: > > > Not your doing, but do you see any reason why we construct a > > ScopedJavaGlobalRef > > > and then immediately reset it?! > > > > That's not exactly what's happening. We're calling > > j_callback->[ScopedJavaGlobalRef::]Reset(), not > > j_callback.[unique_ptr::]reset(). > > Ah, good point, I misread that. > ScopedJavaGlobalRef has a ctor that takes two args (and in fact just calls > Reset) though, so I'd just move the args into the ctor above and remove the > Reset line. > > > I still don't know why. Maybe you can't base::Passed() a > ScopedJavaGlobalRef<>? > > base::Passed explicitly transfers ownership, which doesn't make sense for a > ScopedJavaGlobalRef. It should be cheap to just copy it though? Anyway, I'd > leave that unchanged. Acknowledged. https://codereview.chromium.org/1884203002/diff/40001/chrome/browser/android/... File chrome/browser/android/ntp/most_visited_sites.h (right): https://codereview.chromium.org/1884203002/diff/40001/chrome/browser/android/... chrome/browser/android/ntp/most_visited_sites.h:90: // C++ methods On 2016/04/15 15:21:41, Marc Treib wrote: > On 2016/04/15 13:36:00, sfiera wrote: > > On 2016/04/14 16:57:36, Marc Treib wrote: > > > All these can be private for now, right? > > > > Could be. If I could choose, I'd rather make the Java methods private, since > the > > C++ functions describe the public interface and the Java methods are just > glue. > > But I have no idea how JNI works and if that's allowed. > > AFAIK JNI doesn't care about public/private, so probably everything could be > private. > Anyway, I guess the next CL will split this class in two (the bridge part and > the logic part), so it doesn't really matter. After digging into the generated code a little, I think it does care, so let's just have overloads for a few days. https://codereview.chromium.org/1884203002/diff/40001/chrome/browser/android/... chrome/browser/android/ntp/most_visited_sites.h:241: std::unique_ptr<MostVisitedSitesObserver> observer_; On 2016/04/15 15:21:41, Marc Treib wrote: > On 2016/04/15 13:36:00, sfiera wrote: > > On 2016/04/14 16:57:36, Marc Treib wrote: > > > This is a very uncommon pattern - the observed object usually doesn't own > the > > > observer. > > > But I guess this is also only temporary, while the class essentially > observes > > > itself. > > Java objects always have shared ownership, AFAIK that remains true through JNI. > > > Again, I could do with some more knowledge about JNI. Do you know how the > > observer object from Java is going to be owned in that case? > > > > I could split this into two variables, an unowned one that will belong with > C++ > > and one for the Java bridge. > > I guess when this class is split in two, something like that would happen > anyway? Then we can also leave it as-is for now. > > I'd imagine the final state something like this: > A "MostVisitedBridge" object, which has the same lifetime and ownership as the > current MostVisitedSites, and which holds a Java ref to the Java observer > object. This class would implement your MostVisitedSitesObserver. > > The logic would live in a new MostVisitedSites object, which is probably a > ProfileKeyedService, and which has an ObserverList. The MostVisitedBridge would > register/unregister itself as an (unowned) observer. > > Does that match what you have in mind? I think so, roughly, though I would put the Java ref in an inner class and have the inner class implement MostVisitedSitesObserver.
LGTM, thanks for putting up with my nagging ;) https://codereview.chromium.org/1884203002/diff/40001/chrome/browser/android/... File chrome/browser/android/ntp/most_visited_sites.cc (right): https://codereview.chromium.org/1884203002/diff/40001/chrome/browser/android/... chrome/browser/android/ntp/most_visited_sites.cc:306: static void CallJavaWithBitmap( On 2016/04/15 16:09:06, sfiera wrote: > On 2016/04/15 15:21:41, Marc Treib wrote: > > On 2016/04/15 13:35:59, sfiera wrote: > > > On 2016/04/14 16:57:36, Marc Treib wrote: > > > > This should go into the anonymous namespace above. > > > > > > Any reason? It's static already. > > > > Ah, the code style indeed allows either. But putting it in a namespace is a > lot > > more common in Chromium; I don't think I've ever seen a static function. > > Putting it in the namespace above will also remove the need to forward-declare > > it ;) > > There's actually no need to forward-declare it either; I could have defined it > up where I declared it. I was keeping the code in the original order for a clear > diff. Acknowledged. Please do eventually move it into the anonymous namespace though. (Next CL is fine, if you want to keep this diff small.) https://codereview.chromium.org/1884203002/diff/40001/chrome/browser/android/... File chrome/browser/android/ntp/most_visited_sites.h (right): https://codereview.chromium.org/1884203002/diff/40001/chrome/browser/android/... chrome/browser/android/ntp/most_visited_sites.h:90: // C++ methods On 2016/04/15 16:09:06, sfiera wrote: > On 2016/04/15 15:21:41, Marc Treib wrote: > > On 2016/04/15 13:36:00, sfiera wrote: > > > On 2016/04/14 16:57:36, Marc Treib wrote: > > > > All these can be private for now, right? > > > > > > Could be. If I could choose, I'd rather make the Java methods private, since > > the > > > C++ functions describe the public interface and the Java methods are just > > glue. > > > But I have no idea how JNI works and if that's allowed. > > > > AFAIK JNI doesn't care about public/private, so probably everything could be > > private. > > Anyway, I guess the next CL will split this class in two (the bridge part and > > the logic part), so it doesn't really matter. > > After digging into the generated code a little, I think it does care, so let's > just have overloads for a few days. Acknowledged. https://codereview.chromium.org/1884203002/diff/40001/chrome/browser/android/... chrome/browser/android/ntp/most_visited_sites.h:241: std::unique_ptr<MostVisitedSitesObserver> observer_; On 2016/04/15 16:09:06, sfiera wrote: > On 2016/04/15 15:21:41, Marc Treib wrote: > > On 2016/04/15 13:36:00, sfiera wrote: > > > On 2016/04/14 16:57:36, Marc Treib wrote: > > > > This is a very uncommon pattern - the observed object usually doesn't own > > the > > > > observer. > > > > But I guess this is also only temporary, while the class essentially > > observes > > > > itself. > > > > Java objects always have shared ownership, AFAIK that remains true through > JNI. > > > > > Again, I could do with some more knowledge about JNI. Do you know how the > > > observer object from Java is going to be owned in that case? > > > > > > I could split this into two variables, an unowned one that will belong with > > C++ > > > and one for the Java bridge. > > > > I guess when this class is split in two, something like that would happen > > anyway? Then we can also leave it as-is for now. > > > > I'd imagine the final state something like this: > > A "MostVisitedBridge" object, which has the same lifetime and ownership as the > > current MostVisitedSites, and which holds a Java ref to the Java observer > > object. This class would implement your MostVisitedSitesObserver. > > > > The logic would live in a new MostVisitedSites object, which is probably a > > ProfileKeyedService, and which has an ObserverList. The MostVisitedBridge > would > > register/unregister itself as an (unowned) observer. > > > > Does that match what you have in mind? > > I think so, roughly, though I would put the Java ref in an inner class and have > the inner class implement MostVisitedSitesObserver. Sure, SGTM. https://codereview.chromium.org/1884203002/diff/120001/chrome/browser/android... File chrome/browser/android/ntp/most_visited_sites.cc (right): https://codereview.chromium.org/1884203002/diff/120001/chrome/browser/android... chrome/browser/android/ntp/most_visited_sites.cc:313: j_callback->Reset(env, j_callback_obj); Any reason you changed this back?
Submitting. https://codereview.chromium.org/1884203002/diff/120001/chrome/browser/android... File chrome/browser/android/ntp/most_visited_sites.cc (right): https://codereview.chromium.org/1884203002/diff/120001/chrome/browser/android... chrome/browser/android/ntp/most_visited_sites.cc:313: j_callback->Reset(env, j_callback_obj); On 2016/04/15 16:30:03, Marc Treib wrote: > Any reason you changed this back? Mistake.
The CQ bit was checked by sfiera@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org Link to the patchset: https://codereview.chromium.org/1884203002/#ps140001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1884203002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1884203002/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== NTP tiles: split methods into C++ and Java parts. This should be basically all of the "code change" necessary to split out the component, though there will be a lot more code movement. For now, C++ and Java methods remain as overloads in the same class. BUG=603026 ========== to ========== NTP tiles: split methods into C++ and Java parts. This should be basically all of the "code change" necessary to split out the component, though there will be a lot more code movement. For now, C++ and Java methods remain as overloads in the same class. BUG=603026 Committed: https://crrev.com/1e7993c61536d7cd7ad7eeaa8e60ff07994c0c46 Cr-Commit-Position: refs/heads/master@{#387878} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/1e7993c61536d7cd7ad7eeaa8e60ff07994c0c46 Cr-Commit-Position: refs/heads/master@{#387878} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
