Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1919823002:
Update MostVisitedSites observer interface. (Closed)
DescriptionUpdate MostVisitedSites observer interface.
Move MostVisitedSitesObserver to MostVisitedSites::Observer. Define its
methods in terms of MostVisitedSites::Suggestion and PopularSites::Site.
This is nicer than coordinated lists of strings.
The Java interface remains unchanged, because lists of strings are
easier to pass across the JNI barrier.
BUG=603026
Committed: https://crrev.com/8df7b38b9f5e4d19e7db9b62a2e4f2ae2a83c6eb
Cr-Commit-Position: refs/heads/master@{#389775}
Patch Set 1 #
Total comments: 14
Patch Set 2 : #Patch Set 3 : #
Total comments: 9
Patch Set 4 : #Patch Set 5 : iFix test compilation. #Patch Set 6 : iMove move to .cc. #
Total comments: 8
Messages
Total messages: 39 (13 generated)
sfiera@chromium.org changed reviewers: + treib@chromium.org
PopularSites_Site is a trick borrowed from the proto compiler, but I can revert that part and include the header from most_visited_sites.h if you'd prefer. I also didn't change the names of the observer methods, although you'd suggested s/URLs/Sites/ previously. Since the C++ and Java interfaces are diverging a little, we could, but I'm inclined to keep them as similar as JNI makes convenient.
On 2016/04/25 13:09:29, sfiera wrote: > PopularSites_Site is a trick borrowed from the proto compiler, but I can revert > that part and include the header from most_visited_sites.h if you'd prefer. Hm, I generally dislike changing code just to make it forward-declarable. Just including the header shouldn't hurt. That said, I don't really mind if PopularSites::Site is an inner class or not. I do dislike the name with the underscore though - IMO it should just be PopularSite, but then PopularSites would kinda need to be renamed to make it more distinguishable (maybe "PopularSitesService" would be better anyway..?) > I also didn't change the names of the observer methods, although you'd suggested > s/URLs/Sites/ previously. Since the C++ and Java interfaces are diverging a > little, we could, but I'm inclined to keep them as similar as JNI makes > convenient. Once this moves into a component, the "similar to JNI" argument becomes less relevant ;P But it doesn't make a big difference, it's fine to keep as is.
https://codereview.chromium.org/1919823002/diff/1/chrome/browser/android/ntp/... File chrome/browser/android/ntp/most_visited_sites.h (right): https://codereview.chromium.org/1919823002/diff/1/chrome/browser/android/ntp/... chrome/browser/android/ntp/most_visited_sites.h:50: class Suggestion; Any reason for not defining these inline here? Since they're part of the public interface, I'd find that easier to read. https://codereview.chromium.org/1919823002/diff/1/chrome/browser/android/ntp/... chrome/browser/android/ntp/most_visited_sites.h:53: using PopularSiteVector = std::vector<PopularSites_Site>; Why is one of these a vector of pointers, the other of instances? That seems inconsistent and surprising - copying should be similarly expensive for both. I'd prefer vector of instances, unless there's a good reason for pointers? Also, nit: Please use either singular or plural for both. https://codereview.chromium.org/1919823002/diff/1/chrome/browser/android/ntp/... chrome/browser/android/ntp/most_visited_sites.h:228: public: If you want to keep the public member variables, then this should remain a struct. https://codereview.chromium.org/1919823002/diff/1/chrome/browser/android/ntp/... chrome/browser/android/ntp/most_visited_sites.h:243: std::string GetSourceHistogramName() const; Since Suggestion is now part of the public interface, I'd move this into MostVisitedSites.
https://codereview.chromium.org/1919823002/diff/1/chrome/browser/android/ntp/... File chrome/browser/android/ntp/most_visited_sites.h (right): https://codereview.chromium.org/1919823002/diff/1/chrome/browser/android/ntp/... chrome/browser/android/ntp/most_visited_sites.h:50: class Suggestion; On 2016/04/25 13:58:38, Marc Treib wrote: > Any reason for not defining these inline here? Since they're part of the public > interface, I'd find that easier to read. I find out-of-line easier, though some csearching suggests in-line is far more common in Chrome. The one technical thing is that Suggestion has to be defined after MostVisitedSource. Maybe it's weird that a private enum is in a public class, though? https://codereview.chromium.org/1919823002/diff/1/chrome/browser/android/ntp/... chrome/browser/android/ntp/most_visited_sites.h:53: using PopularSiteVector = std::vector<PopularSites_Site>; On 2016/04/25 13:58:38, Marc Treib wrote: > Why is one of these a vector of pointers, the other of instances? That seems > inconsistent and surprising - copying should be similarly expensive for both. > I'd prefer vector of instances, unless there's a good reason for pointers? > > Also, nit: Please use either singular or plural for both. These were the types that MostVisitedSites and PopularSites already were using. Internally, MostVisitedSites wants pointers because when merging sources, it uses nulls to indicate where suggestions are still needed. It wouldn't be hard to make the public interface use instances, though. We'd really only need to change SaveNewNTPSuggestions() to use a more complicated swap. PopularSites's public interface is with the vector of instances; I didn't look into what it would take to change that. https://codereview.chromium.org/1919823002/diff/1/chrome/browser/android/ntp/... chrome/browser/android/ntp/most_visited_sites.h:228: public: On 2016/04/25 13:58:38, Marc Treib wrote: > If you want to keep the public member variables, then this should remain a > struct. Done.
Description was changed from ========== Update MostVisitedSites observer interface. Move MostVisitedSitesObserver to MostVisitedSites::Observer. Define its methods in terms of MostVisitedSites::Suggestion and PopularSites::Site. This is nicer than coordinated lists of strings. The Java interface remains unchanged, because lists of strings are easier to pass across the JNI barrier. PopularSites::Site is changed into a typedef for PopularSites_Site, because the latter name can be forward-declared. My assumption is that PopularSites will follow MostVisitedSites into the component, so we will be able to depend on its definition there. BUG=603026 ========== to ========== Update MostVisitedSites observer interface. Move MostVisitedSitesObserver to MostVisitedSites::Observer. Define its methods in terms of MostVisitedSites::Suggestion and PopularSites::Site. This is nicer than coordinated lists of strings. The Java interface remains unchanged, because lists of strings are easier to pass across the JNI barrier. BUG=603026 ==========
https://codereview.chromium.org/1919823002/diff/1/chrome/browser/android/ntp/... File chrome/browser/android/ntp/most_visited_sites.h (right): https://codereview.chromium.org/1919823002/diff/1/chrome/browser/android/ntp/... chrome/browser/android/ntp/most_visited_sites.h:50: class Suggestion; On 2016/04/25 16:26:08, sfiera wrote: > On 2016/04/25 13:58:38, Marc Treib wrote: > > Any reason for not defining these inline here? Since they're part of the > public > > interface, I'd find that easier to read. > > I find out-of-line easier, though some csearching suggests in-line is far more > common in Chrome. That's my impression as well, but I don't have numbers to back that up ;) > The one technical thing is that Suggestion has to be defined after > MostVisitedSource. Maybe it's weird that a private enum is in a public class, > though? Huh. I find it weird that that even works. I'd make the enum public then I guess. https://codereview.chromium.org/1919823002/diff/1/chrome/browser/android/ntp/... chrome/browser/android/ntp/most_visited_sites.h:53: using PopularSiteVector = std::vector<PopularSites_Site>; On 2016/04/25 16:26:08, sfiera wrote: > On 2016/04/25 13:58:38, Marc Treib wrote: > > Why is one of these a vector of pointers, the other of instances? That seems > > inconsistent and surprising - copying should be similarly expensive for both. > > I'd prefer vector of instances, unless there's a good reason for pointers? > > > > Also, nit: Please use either singular or plural for both. > > These were the types that MostVisitedSites and PopularSites already were using. > > Internally, MostVisitedSites wants pointers because when merging sources, it > uses nulls to indicate where suggestions are still needed. It wouldn't be hard > to make the public interface use instances, though. We'd really only need to > change SaveNewNTPSuggestions() to use a more complicated swap. Ah, I see. Much of that merging/order-persisting code is going to go away: crbug.com/601734 After that's removed, it should be less work to switch to instances. So it's okay to leave it as-is, but please add a TODO(treib) so I'll remember. Or, alternatively, feel free to take that part over too :) > PopularSites's public interface is with the vector of instances; I didn't look > into what it would take to change that.
https://codereview.chromium.org/1919823002/diff/1/chrome/browser/android/ntp/... File chrome/browser/android/ntp/most_visited_sites.h (right): https://codereview.chromium.org/1919823002/diff/1/chrome/browser/android/ntp/... chrome/browser/android/ntp/most_visited_sites.h:50: class Suggestion; On 2016/04/25 16:42:36, Marc Treib wrote: > On 2016/04/25 16:26:08, sfiera wrote: > > On 2016/04/25 13:58:38, Marc Treib wrote: > > > Any reason for not defining these inline here? Since they're part of the > > public > > > interface, I'd find that easier to read. > > > > I find out-of-line easier, though some csearching suggests in-line is far more > > common in Chrome. > > That's my impression as well, but I don't have numbers to back that up ;) My numbers say it's about 100:1 in favor of in-line. https://cs.corp.google.com/search/?q=package:clankium+file:h$+-file:third_par... https://cs.corp.google.com/search/?q=package:clankium+file:h$+-file:third_par... > > The one technical thing is that Suggestion has to be defined after > > MostVisitedSource. Maybe it's weird that a private enum is in a public class, > > though? > > Huh. I find it weird that that even works. I'd make the enum public then I > guess. Done and inlined. https://codereview.chromium.org/1919823002/diff/1/chrome/browser/android/ntp/... chrome/browser/android/ntp/most_visited_sites.h:53: using PopularSiteVector = std::vector<PopularSites_Site>; On 2016/04/25 16:42:36, Marc Treib wrote: > On 2016/04/25 16:26:08, sfiera wrote: > > On 2016/04/25 13:58:38, Marc Treib wrote: > > > Why is one of these a vector of pointers, the other of instances? That seems > > > inconsistent and surprising - copying should be similarly expensive for > both. > > > I'd prefer vector of instances, unless there's a good reason for pointers? > > > > > > Also, nit: Please use either singular or plural for both. > > > > These were the types that MostVisitedSites and PopularSites already were > using. > > > > Internally, MostVisitedSites wants pointers because when merging sources, it > > uses nulls to indicate where suggestions are still needed. It wouldn't be hard > > to make the public interface use instances, though. We'd really only need to > > change SaveNewNTPSuggestions() to use a more complicated swap. > > Ah, I see. > Much of that merging/order-persisting code is going to go away: crbug.com/601734 > After that's removed, it should be less work to switch to instances. So it's > okay to leave it as-is, but please add a TODO(treib) so I'll remember. Or, > alternatively, feel free to take that part over too :) > > > PopularSites's public interface is with the vector of instances; I didn't look > > into what it would take to change that. > Well, in that case, how about I go ahead and update the public interface to use instances, and add the TODO to stop using pointers internally? (I've made that change but that part is again easy to revert) https://codereview.chromium.org/1919823002/diff/1/chrome/browser/android/ntp/... chrome/browser/android/ntp/most_visited_sites.h:243: std::string GetSourceHistogramName() const; On 2016/04/25 13:58:38, Marc Treib wrote: > Since Suggestion is now part of the public interface, I'd move this into > MostVisitedSites. I removed it from the header, since it no longer needs private access to the class.
BTW: It's common practice to upload a new patch set when saying "Done" to a comment. When I get a mail with CL comments, I usually interpret that as "please take a look again", and then I'm confused if there's no new patch set. If you just want to discuss some comment, without another round of code review, it might make sense to mention that in the message. https://codereview.chromium.org/1919823002/diff/1/chrome/browser/android/ntp/... File chrome/browser/android/ntp/most_visited_sites.h (right): https://codereview.chromium.org/1919823002/diff/1/chrome/browser/android/ntp/... chrome/browser/android/ntp/most_visited_sites.h:53: using PopularSiteVector = std::vector<PopularSites_Site>; On 2016/04/26 09:18:17, sfiera wrote: > On 2016/04/25 16:42:36, Marc Treib wrote: > > On 2016/04/25 16:26:08, sfiera wrote: > > > On 2016/04/25 13:58:38, Marc Treib wrote: > > > > Why is one of these a vector of pointers, the other of instances? That > seems > > > > inconsistent and surprising - copying should be similarly expensive for > > both. > > > > I'd prefer vector of instances, unless there's a good reason for pointers? > > > > > > > > Also, nit: Please use either singular or plural for both. > > > > > > These were the types that MostVisitedSites and PopularSites already were > > using. > > > > > > Internally, MostVisitedSites wants pointers because when merging sources, it > > > uses nulls to indicate where suggestions are still needed. It wouldn't be > hard > > > to make the public interface use instances, though. We'd really only need to > > > change SaveNewNTPSuggestions() to use a more complicated swap. > > > > Ah, I see. > > Much of that merging/order-persisting code is going to go away: > crbug.com/601734 > > After that's removed, it should be less work to switch to instances. So it's > > okay to leave it as-is, but please add a TODO(treib) so I'll remember. Or, > > alternatively, feel free to take that part over too :) > > > > > PopularSites's public interface is with the vector of instances; I didn't > look > > > into what it would take to change that. > > > > Well, in that case, how about I go ahead and update the public interface to use > instances, and add the TODO to stop using pointers internally? Sure, SGTM. I just wanted to avoid unnecessary work in updating code that'll be deleted soon. > (I've made that change but that part is again easy to revert)
Oh, I just forgot to upload. Done. On Tue, Apr 26, 2016 at 11:37 AM <treib@chromium.org> wrote: > BTW: It's common practice to upload a new patch set when saying "Done" to a > comment. When I get a mail with CL comments, I usually interpret that as > "please > take a look again", and then I'm confused if there's no new patch set. > If you just want to discuss some comment, without another round of code > review, > it might make sense to mention that in the message. > > > > > https://codereview.chromium.org/1919823002/diff/1/chrome/browser/android/ntp/... > File chrome/browser/android/ntp/most_visited_sites.h (right): > > > https://codereview.chromium.org/1919823002/diff/1/chrome/browser/android/ntp/... > chrome/browser/android/ntp/most_visited_sites.h:53: using > PopularSiteVector = std::vector<PopularSites_Site>; > On 2016/04/26 09:18:17, sfiera wrote: > > On 2016/04/25 16:42:36, Marc Treib wrote: > > > On 2016/04/25 16:26:08, sfiera wrote: > > > > On 2016/04/25 13:58:38, Marc Treib wrote: > > > > > Why is one of these a vector of pointers, the other of > instances? That > > seems > > > > > inconsistent and surprising - copying should be similarly > expensive for > > > both. > > > > > I'd prefer vector of instances, unless there's a good reason for > pointers? > > > > > > > > > > Also, nit: Please use either singular or plural for both. > > > > > > > > These were the types that MostVisitedSites and PopularSites > already were > > > using. > > > > > > > > Internally, MostVisitedSites wants pointers because when merging > sources, it > > > > uses nulls to indicate where suggestions are still needed. It > wouldn't be > > hard > > > > to make the public interface use instances, though. We'd really > only need to > > > > change SaveNewNTPSuggestions() to use a more complicated swap. > > > > > > Ah, I see. > > > Much of that merging/order-persisting code is going to go away: > > crbug.com/601734 > > > After that's removed, it should be less work to switch to instances. > So it's > > > okay to leave it as-is, but please add a TODO(treib) so I'll > remember. Or, > > > alternatively, feel free to take that part over too :) > > > > > > > PopularSites's public interface is with the vector of instances; I > didn't > > look > > > > into what it would take to change that. > > > > > > > Well, in that case, how about I go ahead and update the public > interface to use > > instances, and add the TODO to stop using pointers internally? > > Sure, SGTM. > I just wanted to avoid unnecessary work in updating code that'll be > deleted soon. > > > > (I've made that change but that part is again easy to revert) > > https://codereview.chromium.org/1919823002/ > -- Chris Pickel <sfiera@google.com> Software Engineer, Google Germany GmbH Erika-Mann-Straße 33, 80636 München Geschäftsführer: Matthew Scott Sucherman, Paul Terence Manicle Registergericht und -nummer: Hamburg, HRB 86891 Sitz der Gesellschaft: Hamburg Diese E-Mail ist vertraulich. Falls sie diese fälschlicherweise erhalten haben sollten, leiten Sie diese bitte nicht an jemand anderes weiter, löschen Sie alle Kopien und Anhänge davon und lassen Sie mich bitte wissen, dass die E-Mail an die falsche Person gesendet wurde. This e-mail is confidential. If you received this communication by mistake, please don't forward it to anyone else, please erase all copies and attachments, and please let me know that it has gone to the wrong person. -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
LGTM, thanks! https://codereview.chromium.org/1919823002/diff/1/chrome/browser/android/ntp/... File chrome/browser/android/ntp/most_visited_sites.h (right): https://codereview.chromium.org/1919823002/diff/1/chrome/browser/android/ntp/... chrome/browser/android/ntp/most_visited_sites.h:50: class Suggestion; On 2016/04/26 09:18:17, sfiera wrote: > On 2016/04/25 16:42:36, Marc Treib wrote: > > On 2016/04/25 16:26:08, sfiera wrote: > > > On 2016/04/25 13:58:38, Marc Treib wrote: > > > > Any reason for not defining these inline here? Since they're part of the > > > public > > > > interface, I'd find that easier to read. > > > > > > I find out-of-line easier, though some csearching suggests in-line is far > more > > > common in Chrome. > > > > That's my impression as well, but I don't have numbers to back that up ;) > > My numbers say it's about 100:1 in favor of in-line. > https://cs.corp.google.com/search/?q=package:clankium+file:h$+-file:third_par... > https://cs.corp.google.com/search/?q=package:clankium+file:h$+-file:third_par... > > > > The one technical thing is that Suggestion has to be defined after > > > MostVisitedSource. Maybe it's weird that a private enum is in a public > class, > > > though? > > > > Huh. I find it weird that that even works. I'd make the enum public then I > > guess. > > Done and inlined. Thanks for checking! https://codereview.chromium.org/1919823002/diff/40001/chrome/browser/android/... File chrome/browser/android/ntp/most_visited_sites.cc (right): https://codereview.chromium.org/1919823002/diff/40001/chrome/browser/android/... chrome/browser/android/ntp/most_visited_sites.cc:592: std::swap(*merged_suggestions[i], current_suggestions_[i]); Ah, this took me a while to get :D I think something like this would also work: current_suggestions_.emplace_back(std::move(*merged_suggestions[i]); (without resizing current_suggestions_ first) which I'd find a bit easier to read. But since this is temporary anyway, it's fine. https://codereview.chromium.org/1919823002/diff/40001/chrome/browser/android/... chrome/browser/android/ntp/most_visited_sites.cc:593: } nit: no braces required https://codereview.chromium.org/1919823002/diff/40001/chrome/browser/android/... File chrome/browser/android/ntp/most_visited_sites.h (right): https://codereview.chromium.org/1919823002/diff/40001/chrome/browser/android/... chrome/browser/android/ntp/most_visited_sites.h:49: struct Suggestion; nit: I'd move the "using"s below the definition, and remove the forward declaration. https://codereview.chromium.org/1919823002/diff/40001/chrome/browser/android/... chrome/browser/android/ntp/most_visited_sites.h:81: Suggestion& operator=(Suggestion&&) = default; Nice! I wasn't aware this was allowed now.
https://codereview.chromium.org/1919823002/diff/40001/chrome/browser/android/... File chrome/browser/android/ntp/most_visited_sites.cc (right): https://codereview.chromium.org/1919823002/diff/40001/chrome/browser/android/... chrome/browser/android/ntp/most_visited_sites.cc:592: std::swap(*merged_suggestions[i], current_suggestions_[i]); On 2016/04/26 09:57:55, Marc Treib wrote: > Ah, this took me a while to get :D > I think something like this would also work: > current_suggestions_.emplace_back(std::move(*merged_suggestions[i]); > (without resizing current_suggestions_ first) which I'd find a bit easier to > read. But since this is temporary anyway, it's fine. Personally, I find std::swap() a lot easier to think about than std::move(), though of course that's in part because std::swap() has been around longer. I'll leave as-is; std::swap() is used this way two other places in this file. https://codereview.chromium.org/1919823002/diff/40001/chrome/browser/android/... chrome/browser/android/ntp/most_visited_sites.cc:593: } On 2016/04/26 09:57:55, Marc Treib wrote: > nit: no braces required Done. https://codereview.chromium.org/1919823002/diff/40001/chrome/browser/android/... File chrome/browser/android/ntp/most_visited_sites.h (right): https://codereview.chromium.org/1919823002/diff/40001/chrome/browser/android/... chrome/browser/android/ntp/most_visited_sites.h:49: struct Suggestion; On 2016/04/26 09:57:55, Marc Treib wrote: > nit: I'd move the "using"s below the definition, and remove the forward > declaration. The using-declarations are needed for the definition of Observer. I could use the order [Suggestion, using-decls, Observer] but splitting the big stuff with small stuff seemed less clear to me.
Still LGTM https://codereview.chromium.org/1919823002/diff/40001/chrome/browser/android/... File chrome/browser/android/ntp/most_visited_sites.cc (right): https://codereview.chromium.org/1919823002/diff/40001/chrome/browser/android/... chrome/browser/android/ntp/most_visited_sites.cc:592: std::swap(*merged_suggestions[i], current_suggestions_[i]); On 2016/04/26 10:17:30, sfiera wrote: > On 2016/04/26 09:57:55, Marc Treib wrote: > > Ah, this took me a while to get :D > > I think something like this would also work: > > current_suggestions_.emplace_back(std::move(*merged_suggestions[i]); > > (without resizing current_suggestions_ first) which I'd find a bit easier to > > read. But since this is temporary anyway, it's fine. > > Personally, I find std::swap() a lot easier to think about than std::move(), > though of course that's in part because std::swap() has been around longer. I'll > leave as-is; std::swap() is used this way two other places in this file. What tripped me up wasn't the swap per se, but rather the mix of (owning) pointer and instance. Though that would apply to the move version as well... aaanyway. It's fine. https://codereview.chromium.org/1919823002/diff/40001/chrome/browser/android/... File chrome/browser/android/ntp/most_visited_sites.h (right): https://codereview.chromium.org/1919823002/diff/40001/chrome/browser/android/... chrome/browser/android/ntp/most_visited_sites.h:49: struct Suggestion; On 2016/04/26 10:17:31, sfiera wrote: > On 2016/04/26 09:57:55, Marc Treib wrote: > > nit: I'd move the "using"s below the definition, and remove the forward > > declaration. > > The using-declarations are needed for the definition of Observer. I could use > the order [Suggestion, using-decls, Observer] but splitting the big stuff with > small stuff seemed less clear to me. Ah, good point! Carry on then.
The CQ bit was checked by sfiera@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1919823002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1919823002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_dbg_recipe on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
(hadn't updated unittest.cc for SuggestionsPtrVector)
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/1919823002/#ps80001 (title: "iFix test compilation.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1919823002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1919823002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_clang_dbg_recipe on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
This has been a recurring problem--I don't know what I have to do to check a patchset before sending it to the CQ. Do Chromium developers... ...have some way to figure out what's needed? ...just remember, and I haven't learned yet? ...send it to the CQ without really knowing? I'm not even sure how to reproduce the failure I see on the Android CQ. I think I'm building the same targets it is. Maybe it only fails in is_debug builds?
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/1919823002/#ps100001 (title: "iMove move to .cc.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1919823002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1919823002/100001
Message was sent while issue was closed.
Description was changed from ========== Update MostVisitedSites observer interface. Move MostVisitedSitesObserver to MostVisitedSites::Observer. Define its methods in terms of MostVisitedSites::Suggestion and PopularSites::Site. This is nicer than coordinated lists of strings. The Java interface remains unchanged, because lists of strings are easier to pass across the JNI barrier. BUG=603026 ========== to ========== Update MostVisitedSites observer interface. Move MostVisitedSitesObserver to MostVisitedSites::Observer. Define its methods in terms of MostVisitedSites::Suggestion and PopularSites::Site. This is nicer than coordinated lists of strings. The Java interface remains unchanged, because lists of strings are easier to pass across the JNI barrier. BUG=603026 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Update MostVisitedSites observer interface. Move MostVisitedSitesObserver to MostVisitedSites::Observer. Define its methods in terms of MostVisitedSites::Suggestion and PopularSites::Site. This is nicer than coordinated lists of strings. The Java interface remains unchanged, because lists of strings are easier to pass across the JNI barrier. BUG=603026 ========== to ========== Update MostVisitedSites observer interface. Move MostVisitedSitesObserver to MostVisitedSites::Observer. Define its methods in terms of MostVisitedSites::Suggestion and PopularSites::Site. This is nicer than coordinated lists of strings. The Java interface remains unchanged, because lists of strings are easier to pass across the JNI barrier. BUG=603026 Committed: https://crrev.com/8df7b38b9f5e4d19e7db9b62a2e4f2ae2a83c6eb Cr-Commit-Position: refs/heads/master@{#389775} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/8df7b38b9f5e4d19e7db9b62a2e4f2ae2a83c6eb Cr-Commit-Position: refs/heads/master@{#389775}
Message was sent while issue was closed.
On 2016/04/26 13:16:27, sfiera wrote: > This has been a recurring problem--I don't know what I have to do to check a > patchset before sending it to the CQ. Do Chromium developers... > ...have some way to figure out what's needed? > ...just remember, and I haven't learned yet? > ...send it to the CQ without really knowing? You can do a CQ dry run that doesn't actually commit it (although it could be that for a non-committer this requires an LGTM first, so that we don't allow non-committers to execute arbitrary code on trybots). > I'm not even sure how to reproduce the failure I see on the Android CQ. I think > I'm building the same targets it is. Maybe it only fails in is_debug builds?
Message was sent while issue was closed.
bauerb@chromium.org changed reviewers: + bauerb@chromium.org
Message was sent while issue was closed.
Post-commit drive-by review! :-D https://codereview.chromium.org/1919823002/diff/100001/chrome/browser/android... File chrome/browser/android/ntp/most_visited_sites.h (right): https://codereview.chromium.org/1919823002/diff/100001/chrome/browser/android... chrome/browser/android/ntp/most_visited_sites.h:59: virtual ~Observer() {} Is there a specific reason to include the destructor in the public interface? If you don't plan on someone owning an instance through this interface, you could make it protected. https://codereview.chromium.org/1919823002/diff/100001/chrome/browser/android... File chrome/browser/android/ntp/most_visited_sites_bridge.cc (right): https://codereview.chromium.org/1919823002/diff/100001/chrome/browser/android... chrome/browser/android/ntp/most_visited_sites_bridge.cc:71: urls.reserve(suggestions.size()); Why are we not doing that for |whitelist_icon_paths|? :) https://codereview.chromium.org/1919823002/diff/100001/chrome/browser/android... chrome/browser/android/ntp/most_visited_sites_bridge.cc:90: urls.emplace_back(site.url.spec()); Why emplace_back here, but push_back above?
Message was sent while issue was closed.
https://codereview.chromium.org/1919823002/diff/100001/chrome/browser/android... File chrome/browser/android/ntp/most_visited_sites.h (right): https://codereview.chromium.org/1919823002/diff/100001/chrome/browser/android... chrome/browser/android/ntp/most_visited_sites.h:59: virtual ~Observer() {} On 2016/04/26 14:29:58, Bernhard Bauer wrote: > Is there a specific reason to include the destructor in the public interface? If > you don't plan on someone owning an instance through this interface, you could > make it protected. Wouldn't that require subclasses to re-declare the the destructor as public, even if they didn't need one? (the sole existing subclass so far doesn't) https://codereview.chromium.org/1919823002/diff/100001/chrome/browser/android... File chrome/browser/android/ntp/most_visited_sites_bridge.cc (right): https://codereview.chromium.org/1919823002/diff/100001/chrome/browser/android... chrome/browser/android/ntp/most_visited_sites_bridge.cc:71: urls.reserve(suggestions.size()); On 2016/04/26 14:29:58, Bernhard Bauer wrote: > Why are we not doing that for |whitelist_icon_paths|? :) It looks like here I was just moving the old code from most_visited_sites.cc... https://codereview.chromium.org/1919823002/diff/100001/chrome/browser/android... chrome/browser/android/ntp/most_visited_sites_bridge.cc:90: urls.emplace_back(site.url.spec()); On 2016/04/26 14:29:58, Bernhard Bauer wrote: > Why emplace_back here, but push_back above? ...but here I wrote it myself. So, the inefficient, inconsistent code is the code that I didn't write :)
Message was sent while issue was closed.
Patchset #7 (id:120001) has been deleted
Message was sent while issue was closed.
Sorry, I forgot to mention this before: It's better to create a new CL for followup changes after a commit, so that the last patch set is going to reflect what was committed. https://codereview.chromium.org/1919823002/diff/100001/chrome/browser/android... File chrome/browser/android/ntp/most_visited_sites.h (right): https://codereview.chromium.org/1919823002/diff/100001/chrome/browser/android... chrome/browser/android/ntp/most_visited_sites.h:59: virtual ~Observer() {} On 2016/04/26 14:42:26, sfiera wrote: > On 2016/04/26 14:29:58, Bernhard Bauer wrote: > > Is there a specific reason to include the destructor in the public interface? > If > > you don't plan on someone owning an instance through this interface, you could > > make it protected. > > Wouldn't that require subclasses to re-declare the the destructor as public, > even if they didn't need one? (the sole existing subclass so far doesn't) I think that would happen automatically, as the subclass does get a destructor generated by the compiler (and I don't think it necessarily gets generated with exactly the same visibility). And this helps to clarify the interface (basically, no one is supposed to own a pure Observer, only a concrete subclass). https://codereview.chromium.org/1919823002/diff/100001/chrome/browser/android... File chrome/browser/android/ntp/most_visited_sites_bridge.cc (right): https://codereview.chromium.org/1919823002/diff/100001/chrome/browser/android... chrome/browser/android/ntp/most_visited_sites_bridge.cc:90: urls.emplace_back(site.url.spec()); On 2016/04/26 14:42:26, sfiera wrote: > On 2016/04/26 14:29:58, Bernhard Bauer wrote: > > Why emplace_back here, but push_back above? > > ...but here I wrote it myself. > > So, the inefficient, inconsistent code is the code that I didn't write :) :-D To be fair, the old code was written before emplace_back was allowed. Personally I don't feel that strongly, but it would be nice to be consistent (and if we can in the process improve the existing code, hey!)
Message was sent while issue was closed.
On 2016/04/26 14:50:49, Bernhard Bauer wrote: > Sorry, I forgot to mention this before: It's better to create a new CL for > followup changes after a commit, so that the last patch set is going to reflect > what was committed. Yep, that was unintentional. I reused the branch name and git cl upload overwrote the old one. Works fine if you git cl issue 0 first. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
