Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1899683003:
NTP tiles: Split C++ and Java MostVisitedSites classes. (Closed)
DescriptionNTP tiles: Split C++ and Java MostVisitedSites classes.
Reorder methods to match header order.
BUG=603026
Committed: https://crrev.com/929502f3f5b44e06ca9964d1f1727e033de778ae
Cr-Commit-Position: refs/heads/master@{#388751}
Patch Set 1 #
Total comments: 22
Patch Set 2 : #
Total comments: 1
Patch Set 3 : #Patch Set 4 : Rebased #
Total comments: 11
Patch Set 5 : #
Total comments: 2
Patch Set 6 : #
Total comments: 2
Patch Set 7 : #
Messages
Total messages: 30 (8 generated)
sfiera@chromium.org changed reviewers: + treib@chromium.org
Open question for me: is it appropriate to take bare pointers as observers? In this CL, it's guaranteed that the observer will outlive the MostVisitedSites object, but in the general case it seems like weak pointers might be preferred as observers. Also, perhaps MostVisitedSitesObserver methods should take vector<Suggestion>. I can see why the current way is better for JNI, but it would probably be nicer to get the structs in C++.
On 2016/04/18 12:36:20, sfiera wrote: > Open question for me: is it appropriate to take bare pointers as observers? In > this CL, it's guaranteed that the observer will outlive the MostVisitedSites > object, but in the general case it seems like weak pointers might be preferred > as observers. Yes, raw pointer is fine here. It's considered the responsibility of the observer to un-register itself when it's destroyed. > Also, perhaps MostVisitedSitesObserver methods should take vector<Suggestion>. I > can see why the current way is better for JNI, but it would probably be nicer to > get the structs in C++. Yep, that sounds reasonable. (I'd probably leave it for another CL though.)
https://codereview.chromium.org/1899683003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/profiles/MostVisitedSites.java (right): https://codereview.chromium.org/1899683003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/profiles/MostVisitedSites.java:16: private long mNativeMostVisitedSitesJavaBridge; nit: I'd call this just mNativeMostVisitedSitesBridge https://codereview.chromium.org/1899683003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/profiles/MostVisitedSites.java:61: Bitmap thumbnail, boolean isLocalThumbnail); Why this change? (In Java, lines are allowed to have 100 chars) https://codereview.chromium.org/1899683003/diff/1/chrome/browser/android/ntp/... File chrome/browser/android/ntp/most_visited_sites.cc (right): https://codereview.chromium.org/1899683003/diff/1/chrome/browser/android/ntp/... chrome/browser/android/ntp/most_visited_sites.cc:223: observer_ = observer; DCHECK that observer_ wasn't set before? (Eventually, we'll probably want to support multiple observers, but that will require some more changes.) https://codereview.chromium.org/1899683003/diff/1/chrome/browser/android/ntp/... chrome/browser/android/ntp/most_visited_sites.cc:869: GURL url(ConvertJavaStringToUTF8(env, j_url)); nit: I'd inline both of these below. https://codereview.chromium.org/1899683003/diff/1/chrome/browser/android/ntp/... chrome/browser/android/ntp/most_visited_sites.cc:878: GURL url(ConvertJavaStringToUTF8(env, j_url)); Also here https://codereview.chromium.org/1899683003/diff/1/chrome/browser/android/ntp/... chrome/browser/android/ntp/most_visited_sites.cc:908: DCHECK_EQ(titles.size(), urls.size()); Not your doing, but it would be good to add another DCHECK for whitelist_icon_paths.size() https://codereview.chromium.org/1899683003/diff/1/chrome/browser/android/ntp/... File chrome/browser/android/ntp/most_visited_sites.h (right): https://codereview.chromium.org/1899683003/diff/1/chrome/browser/android/ntp/... chrome/browser/android/ntp/most_visited_sites.h:244: class MostVisitedSitesJavaBridge { Also here: just "MostVisitedSitesBridge" will do. The "Java" is implicit in the "Bridge". I'd also move this into a separate file. https://codereview.chromium.org/1899683003/diff/1/chrome/browser/android/ntp/... chrome/browser/android/ntp/most_visited_sites.h:303: MostVisitedSites mv_; nit: most_visited_?
https://codereview.chromium.org/1899683003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/profiles/MostVisitedSites.java (right): https://codereview.chromium.org/1899683003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/profiles/MostVisitedSites.java:16: private long mNativeMostVisitedSitesJavaBridge; On 2016/04/19 08:14:47, Marc Treib wrote: > nit: I'd call this just mNativeMostVisitedSitesBridge It has to match the type name. Since we're renaming the type too, that'll work. https://codereview.chromium.org/1899683003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/profiles/MostVisitedSites.java:61: Bitmap thumbnail, boolean isLocalThumbnail); On 2016/04/19 08:14:47, Marc Treib wrote: > Why this change? (In Java, lines are allowed to have 100 chars) I'd broken all the lines that were 100+, but this one was exactly 100. How do you run lint directly on Java in chromium? I wanted to do that instead of having git cl upload error out. https://codereview.chromium.org/1899683003/diff/1/chrome/browser/android/ntp/... File chrome/browser/android/ntp/most_visited_sites.cc (right): https://codereview.chromium.org/1899683003/diff/1/chrome/browser/android/ntp/... chrome/browser/android/ntp/most_visited_sites.cc:223: observer_ = observer; On 2016/04/19 08:14:47, Marc Treib wrote: > DCHECK that observer_ wasn't set before? > (Eventually, we'll probably want to support multiple observers, but that will > require some more changes.) Done. I looked at the standard observer stuff in base/ and it seems like the "int num_sites" is going to be a bit of an annoyance. Maybe in the future it will be enough to make it a constructor parameter? https://codereview.chromium.org/1899683003/diff/1/chrome/browser/android/ntp/... chrome/browser/android/ntp/most_visited_sites.cc:869: GURL url(ConvertJavaStringToUTF8(env, j_url)); On 2016/04/19 08:14:47, Marc Treib wrote: > nit: I'd inline both of these below. That pushes the line over 80 chars. If it's going to be three lines either way, I find this way clearer. Opinions? https://codereview.chromium.org/1899683003/diff/1/chrome/browser/android/ntp/... chrome/browser/android/ntp/most_visited_sites.cc:878: GURL url(ConvertJavaStringToUTF8(env, j_url)); On 2016/04/19 08:14:47, Marc Treib wrote: > Also here Also here https://codereview.chromium.org/1899683003/diff/1/chrome/browser/android/ntp/... chrome/browser/android/ntp/most_visited_sites.cc:908: DCHECK_EQ(titles.size(), urls.size()); On 2016/04/19 08:14:47, Marc Treib wrote: > Not your doing, but it would be good to add another DCHECK for > whitelist_icon_paths.size() Sure. (I'm also going to count this as a vote for changing the observer method to take a single vector<Suggestion> in a later CL) https://codereview.chromium.org/1899683003/diff/1/chrome/browser/android/ntp/... File chrome/browser/android/ntp/most_visited_sites.h (right): https://codereview.chromium.org/1899683003/diff/1/chrome/browser/android/ntp/... chrome/browser/android/ntp/most_visited_sites.h:244: class MostVisitedSitesJavaBridge { On 2016/04/19 08:14:47, Marc Treib wrote: > Also here: just "MostVisitedSitesBridge" will do. The "Java" is implicit in the > "Bridge". > I'd also move this into a separate file. Renamed. What's the end goal? Do we want "android/.../most_visited_sites_bridge.h" and "components/.../most_visited_sites.h"? I'd been assuming "android/.../most_visited_sites.h" and "components/.../most_visited_sites.h" (same name), but maybe having different names is more Chromium style? https://codereview.chromium.org/1899683003/diff/1/chrome/browser/android/ntp/... chrome/browser/android/ntp/most_visited_sites.h:303: MostVisitedSites mv_; On 2016/04/19 08:14:47, Marc Treib wrote: > nit: most_visited_? OK.
LGTM, thanks! I'd still move the Bridge class into separate files. Feel free to do that in this CL or in a later one. https://codereview.chromium.org/1899683003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/profiles/MostVisitedSites.java (right): https://codereview.chromium.org/1899683003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/profiles/MostVisitedSites.java:16: private long mNativeMostVisitedSitesJavaBridge; On 2016/04/19 12:15:13, sfiera wrote: > On 2016/04/19 08:14:47, Marc Treib wrote: > > nit: I'd call this just mNativeMostVisitedSitesBridge > > It has to match the type name. Since we're renaming the type too, that'll work. Today I learned! https://codereview.chromium.org/1899683003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/profiles/MostVisitedSites.java:61: Bitmap thumbnail, boolean isLocalThumbnail); On 2016/04/19 12:15:13, sfiera wrote: > On 2016/04/19 08:14:47, Marc Treib wrote: > > Why this change? (In Java, lines are allowed to have 100 chars) > > I'd broken all the lines that were 100+, but this one was exactly 100. > > How do you run lint directly on Java in chromium? I wanted to do that instead of > having git cl upload error out. git cl presubmit --upload (IIRC. Without the "--upload", it'll run the commit checks only, which are generally less restrictive.) I usually run "git cl format" which will auto-format everything. https://codereview.chromium.org/1899683003/diff/1/chrome/browser/android/ntp/... File chrome/browser/android/ntp/most_visited_sites.cc (right): https://codereview.chromium.org/1899683003/diff/1/chrome/browser/android/ntp/... chrome/browser/android/ntp/most_visited_sites.cc:223: observer_ = observer; On 2016/04/19 12:15:13, sfiera wrote: > On 2016/04/19 08:14:47, Marc Treib wrote: > > DCHECK that observer_ wasn't set before? > > (Eventually, we'll probably want to support multiple observers, but that will > > require some more changes.) > > Done. > > I looked at the standard observer stuff in base/ and it seems like the "int > num_sites" is going to be a bit of an annoyance. Maybe in the future it will be > enough to make it a constructor parameter? Yup, the "num_pages" is weird, and we should get rid of it at some point. Maybe just always setting it to some default maximum value will be good enough. https://codereview.chromium.org/1899683003/diff/1/chrome/browser/android/ntp/... chrome/browser/android/ntp/most_visited_sites.cc:869: GURL url(ConvertJavaStringToUTF8(env, j_url)); On 2016/04/19 12:15:13, sfiera wrote: > On 2016/04/19 08:14:47, Marc Treib wrote: > > nit: I'd inline both of these below. > > That pushes the line over 80 chars. If it's going to be three lines either way, > I find this way clearer. Opinions? I tend to prefer "one" (broken) line - I guess I've gotten used to broken lines. But it's a matter of preference, so it's also fine to leave as is. https://codereview.chromium.org/1899683003/diff/1/chrome/browser/android/ntp/... File chrome/browser/android/ntp/most_visited_sites.h (right): https://codereview.chromium.org/1899683003/diff/1/chrome/browser/android/ntp/... chrome/browser/android/ntp/most_visited_sites.h:244: class MostVisitedSitesJavaBridge { On 2016/04/19 12:15:13, sfiera wrote: > On 2016/04/19 08:14:47, Marc Treib wrote: > > Also here: just "MostVisitedSitesBridge" will do. The "Java" is implicit in > the > > "Bridge". > > I'd also move this into a separate file. > > Renamed. > > What's the end goal? Do we want "android/.../most_visited_sites_bridge.h" and > "components/.../most_visited_sites.h"? Yes, that would be my preference. > I'd been assuming "android/.../most_visited_sites.h" and > "components/.../most_visited_sites.h" (same name), but maybe having different > names is more Chromium style? Not sure if there's anything specific in the style guide about this, but I'd avoid classes with the same name - IMO that's bound to lead to confusion. And at least the "...Bridge" classes are common in Chromium. https://codereview.chromium.org/1899683003/diff/20001/chrome/browser/android/... File chrome/browser/android/ntp/most_visited_sites.cc (right): https://codereview.chromium.org/1899683003/diff/20001/chrome/browser/android/... chrome/browser/android/ntp/most_visited_sites.cc:223: DCHECK(observer_ == nullptr); nit: The common way to do this is "DCHECK(!observer_)"
Separated. I'll wait for tomorrow to commit. https://codereview.chromium.org/1899683003/diff/1/chrome/browser/android/ntp/... File chrome/browser/android/ntp/most_visited_sites.h (right): https://codereview.chromium.org/1899683003/diff/1/chrome/browser/android/ntp/... chrome/browser/android/ntp/most_visited_sites.h:244: class MostVisitedSitesJavaBridge { On 2016/04/19 13:20:54, Marc Treib wrote: > On 2016/04/19 12:15:13, sfiera wrote: > > On 2016/04/19 08:14:47, Marc Treib wrote: > > > Also here: just "MostVisitedSitesBridge" will do. The "Java" is implicit in > > the > > > "Bridge". > > > I'd also move this into a separate file. > > > > Renamed. > > > > What's the end goal? Do we want "android/.../most_visited_sites_bridge.h" and > > "components/.../most_visited_sites.h"? > > Yes, that would be my preference. > > > I'd been assuming "android/.../most_visited_sites.h" and > > "components/.../most_visited_sites.h" (same name), but maybe having different > > names is more Chromium style? > > Not sure if there's anything specific in the style guide about this, but I'd > avoid classes with the same name - IMO that's bound to lead to confusion. And at > least the "...Bridge" classes are common in Chromium. OK. If the bridge class is going to end up in a different file, now is the time to move it.
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/1899683003/#ps40001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1899683003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1899683003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
On 2016/04/20 07:53:07, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) If you haven't figured it out already: This needs a rebase. "git rebase-update" is probably what you want, see: http://commondatastorage.googleapis.com/chrome-infra-docs/flat/depot_tools/do... BTW, it's considered okay to commit stuff while you're not around, given that you can't really predict when it'll land anyway.
Ah, and I just noticed: You'll also need an L-G-T-M from an android owner. Probably bauerb@ is best.
sfiera@chromium.org changed reviewers: + bauerb@chromium.org
Needs JNI/Java approval.
https://codereview.chromium.org/1899683003/diff/60001/chrome/browser/android/... File chrome/browser/android/ntp/most_visited_sites.cc (right): https://codereview.chromium.org/1899683003/diff/60001/chrome/browser/android/... chrome/browser/android/ntp/most_visited_sites.cc:195: DCHECK(observer_ == nullptr); DCHECK(!observer_); https://codereview.chromium.org/1899683003/diff/60001/chrome/browser/android/... File chrome/browser/android/ntp/most_visited_sites.h (right): https://codereview.chromium.org/1899683003/diff/60001/chrome/browser/android/... chrome/browser/android/ntp/most_visited_sites.h:62: MostVisitedSitesObserver* observer, int num_sites); Add a comment about ownership of |observer|? Actually, is there a reason why the observer is not owned anymore? That would make things a bit easier, I think. https://codereview.chromium.org/1899683003/diff/60001/chrome/browser/android/... File chrome/browser/android/ntp/most_visited_sites_bridge.cc (right): https://codereview.chromium.org/1899683003/diff/60001/chrome/browser/android/... chrome/browser/android/ntp/most_visited_sites_bridge.cc:43: bool MostVisitedSitesBridge::Register(JNIEnv* env) { Nit: These don't really match the header order. https://codereview.chromium.org/1899683003/diff/60001/chrome/browser/android/... chrome/browser/android/ntp/most_visited_sites_bridge.cc:48: : most_visited_(profile) { } Nit: no space between the braces. https://codereview.chromium.org/1899683003/diff/60001/chrome/browser/android/... File chrome/browser/android/ntp/most_visited_sites_bridge.h (right): https://codereview.chromium.org/1899683003/diff/60001/chrome/browser/android/... chrome/browser/android/ntp/most_visited_sites_bridge.h:60: class Observer : public MostVisitedSitesObserver { You can forward-declare the class here, and move the definition to the .cc file.
https://codereview.chromium.org/1899683003/diff/60001/chrome/browser/android/... File chrome/browser/android/ntp/most_visited_sites.h (right): https://codereview.chromium.org/1899683003/diff/60001/chrome/browser/android/... chrome/browser/android/ntp/most_visited_sites.h:62: MostVisitedSitesObserver* observer, int num_sites); On 2016/04/20 13:35:11, Bernhard Bauer wrote: > Add a comment about ownership of |observer|? > > Actually, is there a reason why the observer is not owned anymore? That would > make things a bit easier, I think. That's kinda my fault - IMO the observed class shouldn't own the observer, that's the wrong way around. This is also a bit of an intermediate state, eventually we want to support multiple observers, and having this class own them just doesn't make sense.
https://codereview.chromium.org/1899683003/diff/60001/chrome/browser/android/... File chrome/browser/android/ntp/most_visited_sites.cc (right): https://codereview.chromium.org/1899683003/diff/60001/chrome/browser/android/... chrome/browser/android/ntp/most_visited_sites.cc:195: DCHECK(observer_ == nullptr); On 2016/04/20 13:35:11, Bernhard Bauer wrote: > DCHECK(!observer_); Done. https://codereview.chromium.org/1899683003/diff/60001/chrome/browser/android/... File chrome/browser/android/ntp/most_visited_sites_bridge.cc (right): https://codereview.chromium.org/1899683003/diff/60001/chrome/browser/android/... chrome/browser/android/ntp/most_visited_sites_bridge.cc:43: bool MostVisitedSitesBridge::Register(JNIEnv* env) { On 2016/04/20 13:35:11, Bernhard Bauer wrote: > Nit: These don't really match the header order. Done. https://codereview.chromium.org/1899683003/diff/60001/chrome/browser/android/... chrome/browser/android/ntp/most_visited_sites_bridge.cc:48: : most_visited_(profile) { } On 2016/04/20 13:35:11, Bernhard Bauer wrote: > Nit: no space between the braces. Done. https://codereview.chromium.org/1899683003/diff/60001/chrome/browser/android/... File chrome/browser/android/ntp/most_visited_sites_bridge.h (right): https://codereview.chromium.org/1899683003/diff/60001/chrome/browser/android/... chrome/browser/android/ntp/most_visited_sites_bridge.h:60: class Observer : public MostVisitedSitesObserver { On 2016/04/20 13:35:11, Bernhard Bauer wrote: > You can forward-declare the class here, and move the definition to the .cc file. Done.
https://codereview.chromium.org/1899683003/diff/60001/chrome/browser/android/... File chrome/browser/android/ntp/most_visited_sites.h (right): https://codereview.chromium.org/1899683003/diff/60001/chrome/browser/android/... chrome/browser/android/ntp/most_visited_sites.h:62: MostVisitedSitesObserver* observer, int num_sites); On 2016/04/20 13:41:19, Marc Treib wrote: > On 2016/04/20 13:35:11, Bernhard Bauer wrote: > > Add a comment about ownership of |observer|? > > > > Actually, is there a reason why the observer is not owned anymore? That would > > make things a bit easier, I think. > > That's kinda my fault - IMO the observed class shouldn't own the observer, > that's the wrong way around. > This is also a bit of an intermediate state, eventually we want to support > multiple observers, and having this class own them just doesn't make sense. OTOH, this does require being more careful about ownership, and if we want to support multiple observers, presumably we won't have MostVisitedSitesBridge directly contain MostVisitedSites anymore either, so we're going to have to change the whole ownership model there anyway. Given that, I would slightly lean towards ownership here (if it helps, you could think of this more as a delegate than an observer), and change back to raw pointers when we need it. I don't reeally feel strongly about it though. https://codereview.chromium.org/1899683003/diff/80001/chrome/browser/android/... File chrome/browser/android/ntp/most_visited_sites.cc (right): https://codereview.chromium.org/1899683003/diff/80001/chrome/browser/android/... chrome/browser/android/ntp/most_visited_sites.cc:194: MostVisitedSitesObserver* observer, int num_sites) { If we do go forward with this, we should also have comments stating that this method should not be called more than once, and likewise up the call stack for this method (i.e. in Java).
https://codereview.chromium.org/1899683003/diff/80001/chrome/browser/android/... File chrome/browser/android/ntp/most_visited_sites.cc (right): https://codereview.chromium.org/1899683003/diff/80001/chrome/browser/android/... chrome/browser/android/ntp/most_visited_sites.cc:194: MostVisitedSitesObserver* observer, int num_sites) { On 2016/04/20 14:22:51, Bernhard Bauer wrote: > If we do go forward with this, we should also have comments stating that this > method should not be called more than once, and likewise up the call stack for > this method (i.e. in Java). Might as well just remove the DCHECK(). That preserves the old interface; the current interface isn't permanent anyway.
I added a warning comment to the header and removed the DCHECK(), which means that the interface from Java is the same as before (setting a new observer will free and replace the old one). I don't think anything longer-term is yet merited. Is that alright for now?
LGTM with a comment added: https://codereview.chromium.org/1899683003/diff/100001/chrome/browser/android... File chrome/browser/android/ntp/most_visited_sites.h (right): https://codereview.chromium.org/1899683003/diff/100001/chrome/browser/android... chrome/browser/android/ntp/most_visited_sites.h:67: void SetMostVisitedURLsObserver( Can you add a comment here about ownership of the |observer|?
https://codereview.chromium.org/1899683003/diff/100001/chrome/browser/android... File chrome/browser/android/ntp/most_visited_sites.h (right): https://codereview.chromium.org/1899683003/diff/100001/chrome/browser/android... chrome/browser/android/ntp/most_visited_sites.h:67: void SetMostVisitedURLsObserver( On 2016/04/21 11:21:27, Bernhard Bauer wrote: > Can you add a comment here about ownership of the |observer|? Done.
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, bauerb@chromium.org Link to the patchset: https://codereview.chromium.org/1899683003/#ps120001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1899683003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1899683003/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== NTP tiles: Split C++ and Java MostVisitedSites classes. Reorder methods to match header order. BUG=603026 ========== to ========== NTP tiles: Split C++ and Java MostVisitedSites classes. Reorder methods to match header order. BUG=603026 Committed: https://crrev.com/929502f3f5b44e06ca9964d1f1727e033de778ae Cr-Commit-Position: refs/heads/master@{#388751} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/929502f3f5b44e06ca9964d1f1727e033de778ae Cr-Commit-Position: refs/heads/master@{#388751} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
