Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
19874002:
[Android] Expose foreign session sync related funtionalities through JNI. (Closed)
|
Created:
7 years, 5 months ago by Kibeom Kim (inactive) Modified:
7 years, 4 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
Description[Android] Expose foreign session sync related funtionalities through JNI.
Native NTP needs foreign session sync related functionalities. Expose them to Java side.
BUG=178925
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=214539
Patch Set 1 #Patch Set 2 : remove mistake comment #Patch Set 3 : added a blank line before " private:" #Patch Set 4 : some renamings #
Total comments: 36
Patch Set 5 : fixed patchest 1 comments #
Total comments: 22
Patch Set 6 : Fixed Patch Set 5's comments #
Total comments: 26
Patch Set 7 : Fixed Patch Set 6's comments. #Patch Set 8 : Fixed Patch Set 7's comments #
Total comments: 2
Patch Set 9 : Patch Set 8 findbug fix. #Patch Set 10 : Patch Set 9 clang build fix. #Patch Set 11 : Patch Set 10's clang build fix was incorrect, this is the correct one. #Patch Set 12 : Reverted comparison by Long object. #
Total comments: 1
Patch Set 13 : Changed tab_url's GURL comparison to .SchemeIs and .host() == ... #Patch Set 14 : moved "&&" to the upper line. #Patch Set 15 : moved "||" to the upper line. #Patch Set 16 : findbug fix (dead code removal) #
Messages
Total messages: 21 (0 generated)
note: it turned out that we can also just use FaviconHelper.java for getting favicons for this :)
note2: foreign_session_helper.cc is similar to chrome/browser/ui/webui/ntp/foreign_session_handler.cc
https://codereview.chromium.org/19874002/diff/7001/chrome/android/java/src/or... File chrome/android/java/src/org/chromium/chrome/browser/ForeignSessionHelper.java (right): https://codereview.chromium.org/19874002/diff/7001/chrome/android/java/src/or... chrome/android/java/src/org/chromium/chrome/browser/ForeignSessionHelper.java:38: public static class Session { for each of these, I would add a private constructor that takes all params and then make all the fields final. https://codereview.chromium.org/19874002/diff/7001/chrome/android/java/src/or... chrome/android/java/src/org/chromium/chrome/browser/ForeignSessionHelper.java:50: public static class Window { purely for accessing these outside of this class, I would call them ForeignSessionWindow/ForeignSessionTab to avoid confusion with these already overloaded terms. Or have them match the native names of SessionWindow, SessionTab, and SyncedSession https://codereview.chromium.org/19874002/diff/7001/chrome/android/java/src/or... chrome/android/java/src/org/chromium/chrome/browser/ForeignSessionHelper.java:84: List<Session> sessions, long timestamp, int sessionId) { Instead of taking the list of sessions, can you just pass in the one you need to add it to? I guess in pushSession, you can just have it return a Session object and the native side could just pass that jobject in here. https://codereview.chromium.org/19874002/diff/7001/chrome/android/java/src/or... chrome/android/java/src/org/chromium/chrome/browser/ForeignSessionHelper.java:92: Session session = sessions.get(sessions.size() - 1); Yeah, with the above change, you could get rid of all these asserts. https://codereview.chromium.org/19874002/diff/7001/chrome/android/java/src/or... chrome/android/java/src/org/chromium/chrome/browser/ForeignSessionHelper.java:99: List<Session> sessions, String url, String title, long timestamp, Same comment as above, pass in a Window instance instead. https://codereview.chromium.org/19874002/diff/7001/chrome/android/java/src/or... chrome/android/java/src/org/chromium/chrome/browser/ForeignSessionHelper.java:130: protected void finalize() { we shouldn't rely on finalize being called, we should add an explicit destroy and mange the lifecycle ourselves. https://codereview.chromium.org/19874002/diff/7001/chrome/android/java/src/or... chrome/android/java/src/org/chromium/chrome/browser/ForeignSessionHelper.java:151: * reason. align with "The list" above https://codereview.chromium.org/19874002/diff/7001/chrome/android/java/src/or... chrome/android/java/src/org/chromium/chrome/browser/ForeignSessionHelper.java:162: return (int) Math.max(Math.min(Integer.MAX_VALUE, timeDiff), Integer.MIN_VALUE); This is the way I've seen number compare's done...it seems easier to read than this implementation. return session1.modifiedTime < session2.modifiedTime ? -1 : (session1.modifiedTime == session2.modifiedTime ? 0 : 1); https://codereview.chromium.org/19874002/diff/7001/chrome/android/java/src/or... chrome/android/java/src/org/chromium/chrome/browser/ForeignSessionHelper.java:183: * Set the given session collapsed or uncollapsed in Pref. Pref = preferences. https://codereview.chromium.org/19874002/diff/7001/chrome/browser/android/for... File chrome/browser/android/foreign_session_helper.cc (right): https://codereview.chromium.org/19874002/diff/7001/chrome/browser/android/for... chrome/browser/android/foreign_session_helper.cc:43: profile_ = profile; can you use an initializer list here instead? https://codereview.chromium.org/19874002/diff/7001/chrome/browser/android/for... chrome/browser/android/foreign_session_helper.cc:44: callback_.Reset(); is this necessary? https://codereview.chromium.org/19874002/diff/7001/chrome/browser/android/for... chrome/browser/android/foreign_session_helper.cc:47: ->GetForProfile(profile); having the -> on the previous line seems much more common git g -e "^[ ]*->" | grep \.cc: | wc -l = 366 git g -e "->$" | grep \.cc: | wc -l = 2270 https://codereview.chromium.org/19874002/diff/7001/chrome/browser/android/for... chrome/browser/android/foreign_session_helper.cc:63: ->GetForProfile(profile_); same comment as above https://codereview.chromium.org/19874002/diff/7001/chrome/browser/android/for... chrome/browser/android/foreign_session_helper.cc:67: void ForeignSessionHelper::SetOnForeignSessionCallback(JNIEnv* env, jobject obj, I think if all the params don't fit on one line, then they all need to go on separate Same comment elsewhere https://codereview.chromium.org/19874002/diff/7001/chrome/browser/android/for... chrome/browser/android/foreign_session_helper.cc:98: if (associator == NULL) if (!associator) https://codereview.chromium.org/19874002/diff/7001/chrome/browser/android/for... chrome/browser/android/foreign_session_helper.cc:145: for (std::vector<SessionTab*>::iterator iit = window->tabs.begin(); Might consider having helper methods for the innerards of each of these for loops just to shrink down this function a bit. https://codereview.chromium.org/19874002/diff/7001/chrome/browser/android/for... File chrome/browser/android/foreign_session_helper.h (right): https://codereview.chromium.org/19874002/diff/7001/chrome/browser/android/for... chrome/browser/android/foreign_session_helper.h:30: const content::NotificationDetails& details); need an OVERRIDE here I suspect. https://codereview.chromium.org/19874002/diff/7001/chrome/browser/android/for... chrome/browser/android/foreign_session_helper.h:39: Profile* profile_; // weak as a tiny nit, normally there are two spaces between the ; and the //
Many Dones. https://codereview.chromium.org/19874002/diff/7001/chrome/android/java/src/or... File chrome/android/java/src/org/chromium/chrome/browser/ForeignSessionHelper.java (right): https://codereview.chromium.org/19874002/diff/7001/chrome/android/java/src/or... chrome/android/java/src/org/chromium/chrome/browser/ForeignSessionHelper.java:38: public static class Session { On 2013/07/24 23:48:20, Ted C wrote: > for each of these, I would add a private constructor that takes all params and > then make all the fields final. Done. https://codereview.chromium.org/19874002/diff/7001/chrome/android/java/src/or... chrome/android/java/src/org/chromium/chrome/browser/ForeignSessionHelper.java:50: public static class Window { On 2013/07/24 23:48:20, Ted C wrote: > purely for accessing these outside of this class, I would call them > ForeignSessionWindow/ForeignSessionTab to avoid confusion with these already > overloaded terms. Or have them match the native names of SessionWindow, > SessionTab, and SyncedSession Done. https://codereview.chromium.org/19874002/diff/7001/chrome/android/java/src/or... chrome/android/java/src/org/chromium/chrome/browser/ForeignSessionHelper.java:84: List<Session> sessions, long timestamp, int sessionId) { On 2013/07/24 23:48:20, Ted C wrote: > Instead of taking the list of sessions, can you just pass in the one you need to > add it to? > > I guess in pushSession, you can just have it return a Session object and the > native side could just pass that jobject in here. Nice suggestion. Done. https://codereview.chromium.org/19874002/diff/7001/chrome/android/java/src/or... chrome/android/java/src/org/chromium/chrome/browser/ForeignSessionHelper.java:92: Session session = sessions.get(sessions.size() - 1); On 2013/07/24 23:48:20, Ted C wrote: > Yeah, with the above change, you could get rid of all these asserts. Done. https://codereview.chromium.org/19874002/diff/7001/chrome/android/java/src/or... chrome/android/java/src/org/chromium/chrome/browser/ForeignSessionHelper.java:99: List<Session> sessions, String url, String title, long timestamp, On 2013/07/24 23:48:20, Ted C wrote: > Same comment as above, pass in a Window instance instead. Done. https://codereview.chromium.org/19874002/diff/7001/chrome/android/java/src/or... chrome/android/java/src/org/chromium/chrome/browser/ForeignSessionHelper.java:130: protected void finalize() { On 2013/07/24 23:48:20, Ted C wrote: > we shouldn't rely on finalize being called, we should add an explicit destroy > and mange the lifecycle ourselves. Done. https://codereview.chromium.org/19874002/diff/7001/chrome/android/java/src/or... chrome/android/java/src/org/chromium/chrome/browser/ForeignSessionHelper.java:151: * reason. On 2013/07/24 23:48:20, Ted C wrote: > align with "The list" above Done. https://codereview.chromium.org/19874002/diff/7001/chrome/android/java/src/or... chrome/android/java/src/org/chromium/chrome/browser/ForeignSessionHelper.java:162: return (int) Math.max(Math.min(Integer.MAX_VALUE, timeDiff), Integer.MIN_VALUE); On 2013/07/24 23:48:20, Ted C wrote: > This is the way I've seen number compare's done...it seems easier to read than > this implementation. > > return session1.modifiedTime < session2.modifiedTime ? -1 : > (session1.modifiedTime == session2.modifiedTime ? 0 : 1); Done. https://codereview.chromium.org/19874002/diff/7001/chrome/android/java/src/or... chrome/android/java/src/org/chromium/chrome/browser/ForeignSessionHelper.java:183: * Set the given session collapsed or uncollapsed in Pref. On 2013/07/24 23:48:20, Ted C wrote: > Pref = preferences. Done. https://codereview.chromium.org/19874002/diff/7001/chrome/browser/android/for... File chrome/browser/android/foreign_session_helper.cc (right): https://codereview.chromium.org/19874002/diff/7001/chrome/browser/android/for... chrome/browser/android/foreign_session_helper.cc:43: profile_ = profile; On 2013/07/24 23:48:20, Ted C wrote: > can you use an initializer list here instead? Done. https://codereview.chromium.org/19874002/diff/7001/chrome/browser/android/for... chrome/browser/android/foreign_session_helper.cc:44: callback_.Reset(); On 2013/07/24 23:48:20, Ted C wrote: > is this necessary? No :( Done. https://codereview.chromium.org/19874002/diff/7001/chrome/browser/android/for... chrome/browser/android/foreign_session_helper.cc:47: ->GetForProfile(profile); On 2013/07/24 23:48:20, Ted C wrote: > having the -> on the previous line seems much more common > > git g -e "^[ ]*->" | grep \.cc: | wc -l = 366 > git g -e "->$" | grep \.cc: | wc -l = 2270 Done. https://codereview.chromium.org/19874002/diff/7001/chrome/browser/android/for... chrome/browser/android/foreign_session_helper.cc:63: ->GetForProfile(profile_); On 2013/07/24 23:48:20, Ted C wrote: > same comment as above Done. https://codereview.chromium.org/19874002/diff/7001/chrome/browser/android/for... chrome/browser/android/foreign_session_helper.cc:67: void ForeignSessionHelper::SetOnForeignSessionCallback(JNIEnv* env, jobject obj, On 2013/07/24 23:48:20, Ted C wrote: > I think if all the params don't fit on one line, then they all need to go on > separate > > Same comment elsewhere Done. https://codereview.chromium.org/19874002/diff/7001/chrome/browser/android/for... chrome/browser/android/foreign_session_helper.cc:98: if (associator == NULL) On 2013/07/24 23:48:20, Ted C wrote: > if (!associator) Done. https://codereview.chromium.org/19874002/diff/7001/chrome/browser/android/for... chrome/browser/android/foreign_session_helper.cc:145: for (std::vector<SessionTab*>::iterator iit = window->tabs.begin(); On 2013/07/24 23:48:20, Ted C wrote: > Might consider having helper methods for the innerards of each of these for > loops just to shrink down this function a bit. Done. https://codereview.chromium.org/19874002/diff/7001/chrome/browser/android/for... File chrome/browser/android/foreign_session_helper.h (right): https://codereview.chromium.org/19874002/diff/7001/chrome/browser/android/for... chrome/browser/android/foreign_session_helper.h:30: const content::NotificationDetails& details); On 2013/07/24 23:48:20, Ted C wrote: > need an OVERRIDE here I suspect. Done. https://codereview.chromium.org/19874002/diff/7001/chrome/browser/android/for... chrome/browser/android/foreign_session_helper.h:39: Profile* profile_; // weak On 2013/07/24 23:48:20, Ted C wrote: > as a tiny nit, normally there are two spaces between the ; and the // Done.
https://codereview.chromium.org/19874002/diff/21001/chrome/browser/android/fo... File chrome/browser/android/foreign_session_helper.h (right): https://codereview.chromium.org/19874002/diff/21001/chrome/browser/android/fo... chrome/browser/android/foreign_session_helper.h:54: virtual ~ForeignSessionHelper() {} clang build complains having {} here. I'll move {} to .cc in the next patchset.
https://chromiumcodereview.appspot.com/19874002/diff/21001/chrome/android/jav... File chrome/android/java/src/org/chromium/chrome/browser/ForeignSessionHelper.java (right): https://chromiumcodereview.appspot.com/19874002/diff/21001/chrome/android/jav... chrome/android/java/src/org/chromium/chrome/browser/ForeignSessionHelper.java:49: public final String name; I would put the params above the constructor in all of these. Also instead of passing in empty ArrayLists to each of these, I would just assign one with the definition of the param. public final List<ForeignSessionWindow> windows = new ArrayList<ForeignSessionWindow>(); that way there is no way this could be null. https://chromiumcodereview.appspot.com/19874002/diff/21001/chrome/android/jav... chrome/android/java/src/org/chromium/chrome/browser/ForeignSessionHelper.java:122: public void destroy() { javadoc :-) https://chromiumcodereview.appspot.com/19874002/diff/21001/chrome/browser/and... File chrome/browser/android/foreign_session_helper.cc (right): https://chromiumcodereview.appspot.com/19874002/diff/21001/chrome/browser/and... chrome/browser/android/foreign_session_helper.cc:39: ->GetForProfile(profile_); -> on previous line https://chromiumcodereview.appspot.com/19874002/diff/21001/chrome/browser/and... chrome/browser/android/foreign_session_helper.cc:48: void ForeignSessionHelper::CopyTabsToJava( can these be helper methods in an empty namespace to avoid having them defined on the class? https://chromiumcodereview.appspot.com/19874002/diff/21001/chrome/browser/and... chrome/browser/android/foreign_session_helper.cc:61: const ::sessions::SerializedNavigationEntry& current_navigation = tab I would put tab on the following line. https://chromiumcodereview.appspot.com/19874002/diff/21001/chrome/browser/and... chrome/browser/android/foreign_session_helper.cc:205: jboolean ForeignSessionHelper::OpenForeignSessionTab(JNIEnv* env, jobject obj, same comment about params on their own lines https://chromiumcodereview.appspot.com/19874002/diff/21001/chrome/browser/and... File chrome/browser/android/foreign_session_helper.h (right): https://chromiumcodereview.appspot.com/19874002/diff/21001/chrome/browser/and... chrome/browser/android/foreign_session_helper.h:28: static bool RegisterForeignSessionHelper(JNIEnv* env); the ordering of the methods should be as close as possible in the header and the cc file. So I would move this to the bottom of the public section and adjust a bit. https://chromiumcodereview.appspot.com/19874002/diff/21001/chrome/browser/and... chrome/browser/android/foreign_session_helper.h:34: virtual void Observe(int type, const content::NotificationSource& source, I usually see all the overridden methods separated from the one's your adding to make it easier to see. Sometimes, there will be a comment like "// NotificationObserver implemenation" before them. https://chromiumcodereview.appspot.com/19874002/diff/21001/chrome/browser/and... chrome/browser/android/foreign_session_helper.h:34: virtual void Observe(int type, const content::NotificationSource& source, if all the params don't fit on one line, then each one gets it's own. https://chromiumcodereview.appspot.com/19874002/diff/21001/chrome/browser/and... chrome/browser/android/foreign_session_helper.h:47: browser_sync::SessionModelAssociator* GetSessionModelAssociator(); private methods above variables.
More Dones. https://chromiumcodereview.appspot.com/19874002/diff/21001/chrome/android/jav... File chrome/android/java/src/org/chromium/chrome/browser/ForeignSessionHelper.java (right): https://chromiumcodereview.appspot.com/19874002/diff/21001/chrome/android/jav... chrome/android/java/src/org/chromium/chrome/browser/ForeignSessionHelper.java:49: public final String name; On 2013/07/26 22:29:47, Ted C wrote: > I would put the params above the constructor in all of these. > > Also instead of passing in empty ArrayLists to each of these, I would just > assign one with the definition of the param. > > public final List<ForeignSessionWindow> windows = new > ArrayList<ForeignSessionWindow>(); > > that way there is no way this could be null. Done. https://chromiumcodereview.appspot.com/19874002/diff/21001/chrome/android/jav... chrome/android/java/src/org/chromium/chrome/browser/ForeignSessionHelper.java:122: public void destroy() { On 2013/07/26 22:29:47, Ted C wrote: > javadoc :-) Done. https://chromiumcodereview.appspot.com/19874002/diff/21001/chrome/browser/and... File chrome/browser/android/foreign_session_helper.cc (right): https://chromiumcodereview.appspot.com/19874002/diff/21001/chrome/browser/and... chrome/browser/android/foreign_session_helper.cc:39: ->GetForProfile(profile_); On 2013/07/26 22:29:47, Ted C wrote: > -> on previous line Done. https://chromiumcodereview.appspot.com/19874002/diff/21001/chrome/browser/and... chrome/browser/android/foreign_session_helper.cc:48: void ForeignSessionHelper::CopyTabsToJava( On 2013/07/26 22:29:47, Ted C wrote: > can these be helper methods in an empty namespace to avoid having them defined > on the class? Done. https://chromiumcodereview.appspot.com/19874002/diff/21001/chrome/browser/and... chrome/browser/android/foreign_session_helper.cc:61: const ::sessions::SerializedNavigationEntry& current_navigation = tab On 2013/07/26 22:29:47, Ted C wrote: > I would put tab on the following line. Done. https://chromiumcodereview.appspot.com/19874002/diff/21001/chrome/browser/and... chrome/browser/android/foreign_session_helper.cc:205: jboolean ForeignSessionHelper::OpenForeignSessionTab(JNIEnv* env, jobject obj, On 2013/07/26 22:29:47, Ted C wrote: > same comment about params on their own lines Done. https://chromiumcodereview.appspot.com/19874002/diff/21001/chrome/browser/and... File chrome/browser/android/foreign_session_helper.h (right): https://chromiumcodereview.appspot.com/19874002/diff/21001/chrome/browser/and... chrome/browser/android/foreign_session_helper.h:28: static bool RegisterForeignSessionHelper(JNIEnv* env); On 2013/07/26 22:29:47, Ted C wrote: > the ordering of the methods should be as close as possible in the header and the > cc file. So I would move this to the bottom of the public section and adjust a > bit. Done. https://chromiumcodereview.appspot.com/19874002/diff/21001/chrome/browser/and... chrome/browser/android/foreign_session_helper.h:34: virtual void Observe(int type, const content::NotificationSource& source, On 2013/07/26 22:29:47, Ted C wrote: > I usually see all the overridden methods separated from the one's your adding to > make it easier to see. > > Sometimes, there will be a comment like "// NotificationObserver implemenation" > before them. Done. https://chromiumcodereview.appspot.com/19874002/diff/21001/chrome/browser/and... chrome/browser/android/foreign_session_helper.h:34: virtual void Observe(int type, const content::NotificationSource& source, On 2013/07/26 22:29:47, Ted C wrote: > if all the params don't fit on one line, then each one gets it's own. Done. https://chromiumcodereview.appspot.com/19874002/diff/21001/chrome/browser/and... chrome/browser/android/foreign_session_helper.h:47: browser_sync::SessionModelAssociator* GetSessionModelAssociator(); On 2013/07/26 22:29:47, Ted C wrote: > private methods above variables. Done. https://chromiumcodereview.appspot.com/19874002/diff/21001/chrome/browser/and... chrome/browser/android/foreign_session_helper.h:54: virtual ~ForeignSessionHelper() {} On 2013/07/26 18:35:44, Kibeom Kim wrote: > clang build complains having {} here. I'll move {} to .cc in the next patchset. Done.
lgtm w/ couple last nits https://chromiumcodereview.appspot.com/19874002/diff/40001/chrome/browser/and... File chrome/browser/android/foreign_session_helper.cc (right): https://chromiumcodereview.appspot.com/19874002/diff/40001/chrome/browser/and... chrome/browser/android/foreign_session_helper.cc:275: ForeignSessionHelper::~ForeignSessionHelper() { one of the instances where the ordering shouldn't match. the destructor should be next to the constructor https://chromiumcodereview.appspot.com/19874002/diff/40001/chrome/browser/and... File chrome/browser/android/foreign_session_helper.h (right): https://chromiumcodereview.appspot.com/19874002/diff/40001/chrome/browser/and... chrome/browser/android/foreign_session_helper.h:51: virtual ~ForeignSessionHelper() OVERRIDE; I don't think you need OVERRIDE here.
2 more Dones. https://chromiumcodereview.appspot.com/19874002/diff/40001/chrome/browser/and... File chrome/browser/android/foreign_session_helper.cc (right): https://chromiumcodereview.appspot.com/19874002/diff/40001/chrome/browser/and... chrome/browser/android/foreign_session_helper.cc:275: ForeignSessionHelper::~ForeignSessionHelper() { On 2013/07/26 23:48:32, Ted C wrote: > one of the instances where the ordering shouldn't match. the destructor should > be next to the constructor Done. https://chromiumcodereview.appspot.com/19874002/diff/40001/chrome/browser/and... File chrome/browser/android/foreign_session_helper.h (right): https://chromiumcodereview.appspot.com/19874002/diff/40001/chrome/browser/and... chrome/browser/android/foreign_session_helper.h:51: virtual ~ForeignSessionHelper() OVERRIDE; On 2013/07/26 23:48:32, Ted C wrote: > I don't think you need OVERRIDE here. Done.
https://codereview.chromium.org/19874002/diff/40001/chrome/android/java/src/o... File chrome/android/java/src/org/chromium/chrome/browser/ForeignSessionHelper.java (right): https://codereview.chromium.org/19874002/diff/40001/chrome/android/java/src/o... chrome/android/java/src/org/chromium/chrome/browser/ForeignSessionHelper.java:16: * This is a helper class that provides foreign session sync related functionalities. I feel like this comment could be clearer? Something like: "This class exposes to Java information about sessions, windows, and tabs on the user's synced devices." https://codereview.chromium.org/19874002/diff/40001/chrome/android/java/src/o... chrome/android/java/src/org/chromium/chrome/browser/ForeignSessionHelper.java:24: public interface OnForeignSessionCallback { "OnForeignSessionCallback" makes me wonder "on what?". How about just "ForeignSessionCallback". https://codereview.chromium.org/19874002/diff/40001/chrome/android/java/src/o... chrome/android/java/src/org/chromium/chrome/browser/ForeignSessionHelper.java:157: long timeDiff = session2.modifiedTime - session1.modifiedTime; even simpler: return Long.compare(sessionA.modifiedTime, sessionB.modifiedTime); https://codereview.chromium.org/19874002/diff/40001/chrome/browser/android/fo... File chrome/browser/android/foreign_session_helper.cc (right): https://codereview.chromium.org/19874002/diff/40001/chrome/browser/android/fo... chrome/browser/android/foreign_session_helper.cc:10: don't need newlines between includes, except a single newline between <> style and "" style includes https://codereview.chromium.org/19874002/diff/40001/chrome/browser/android/fo... chrome/browser/android/foreign_session_helper.cc:54: for (std::vector<SessionTab*>::const_iterator iit = window->tabs.begin(); "iit" -> "it"? https://codereview.chromium.org/19874002/diff/40001/chrome/browser/android/fo... chrome/browser/android/foreign_session_helper.cc:61: int selected_index = std::min(tab.current_navigation_index, seems odd to check this against the upper boundary but not check for negative values. aren't we already guaranteed that current_navigation_index is a valid value anyway? I'd be fine with leaving out the check altogether. https://codereview.chromium.org/19874002/diff/40001/chrome/browser/android/fo... chrome/browser/android/foreign_session_helper.cc:86: if (window->tabs.empty()) { this check seems superfluous. the code will work just fine if you remove lines 86-89, and it's not this class's job to test that window->tabs isn't empty. https://codereview.chromium.org/19874002/diff/40001/chrome/browser/android/fo... chrome/browser/android/foreign_session_helper.cc:111: ProfileSyncService* service =ProfileSyncServiceFactory::GetInstance()-> space after "=" https://codereview.chromium.org/19874002/diff/40001/chrome/browser/android/fo... chrome/browser/android/foreign_session_helper.cc:177: DictionaryValue* current_collapsed_sessions = pref_update.Get(); wouldn't "old_collapsed_sessions" make sense? https://codereview.chromium.org/19874002/diff/40001/chrome/browser/android/fo... chrome/browser/android/foreign_session_helper.cc:214: content::RecordComputedAction("MobileNTPForeignSession"); this action is actually obsolete. I'd leave this out for now, and we can add UMA stats to the NTP later. (this probably isn't the right place to record stats anyway, since this isn't part of the NTP)
just some small nits. looks good overall!
https://codereview.chromium.org/19874002/diff/40001/chrome/android/java/src/o... File chrome/android/java/src/org/chromium/chrome/browser/ForeignSessionHelper.java (right): https://codereview.chromium.org/19874002/diff/40001/chrome/android/java/src/o... chrome/android/java/src/org/chromium/chrome/browser/ForeignSessionHelper.java:16: * This is a helper class that provides foreign session sync related functionalities. On 2013/07/27 00:24:51, newt wrote: > I feel like this comment could be clearer? Something like: "This class exposes > to Java information about sessions, windows, and tabs on the user's synced > devices." Done. https://codereview.chromium.org/19874002/diff/40001/chrome/android/java/src/o... chrome/android/java/src/org/chromium/chrome/browser/ForeignSessionHelper.java:24: public interface OnForeignSessionCallback { On 2013/07/27 00:24:51, newt wrote: > "OnForeignSessionCallback" makes me wonder "on what?". How about just > "ForeignSessionCallback". Done. https://codereview.chromium.org/19874002/diff/40001/chrome/android/java/src/o... chrome/android/java/src/org/chromium/chrome/browser/ForeignSessionHelper.java:157: long timeDiff = session2.modifiedTime - session1.modifiedTime; On 2013/07/27 00:24:51, newt wrote: > even simpler: > > return Long.compare(sessionA.modifiedTime, sessionB.modifiedTime); Done. https://codereview.chromium.org/19874002/diff/40001/chrome/browser/android/fo... File chrome/browser/android/foreign_session_helper.cc (right): https://codereview.chromium.org/19874002/diff/40001/chrome/browser/android/fo... chrome/browser/android/foreign_session_helper.cc:10: On 2013/07/27 00:24:51, newt wrote: > don't need newlines between includes, except a single newline between <> style > and "" style includes Done. https://codereview.chromium.org/19874002/diff/40001/chrome/browser/android/fo... chrome/browser/android/foreign_session_helper.cc:54: for (std::vector<SessionTab*>::const_iterator iit = window->tabs.begin(); On 2013/07/27 00:24:51, newt wrote: > "iit" -> "it"? iit was a mistake :( I chose tab_it. But let me know if you know a better naming :( https://codereview.chromium.org/19874002/diff/40001/chrome/browser/android/fo... chrome/browser/android/foreign_session_helper.cc:61: int selected_index = std::min(tab.current_navigation_index, On 2013/07/27 00:24:51, newt wrote: > seems odd to check this against the upper boundary but not check for negative > values. aren't we already guaranteed that current_navigation_index is a valid > value anyway? I'd be fine with leaving out the check altogether. Done. https://codereview.chromium.org/19874002/diff/40001/chrome/browser/android/fo... chrome/browser/android/foreign_session_helper.cc:86: if (window->tabs.empty()) { On 2013/07/27 00:24:51, newt wrote: > this check seems superfluous. the code will work just fine if you remove lines > 86-89, and it's not this class's job to test that window->tabs isn't empty. Done. https://codereview.chromium.org/19874002/diff/40001/chrome/browser/android/fo... chrome/browser/android/foreign_session_helper.cc:111: ProfileSyncService* service =ProfileSyncServiceFactory::GetInstance()-> On 2013/07/27 00:24:51, newt wrote: > space after "=" Done. https://codereview.chromium.org/19874002/diff/40001/chrome/browser/android/fo... chrome/browser/android/foreign_session_helper.cc:177: DictionaryValue* current_collapsed_sessions = pref_update.Get(); On 2013/07/27 00:24:51, newt wrote: > wouldn't "old_collapsed_sessions" make sense? Hmm. I chose pref_collapsed_sessions. Done. https://codereview.chromium.org/19874002/diff/40001/chrome/browser/android/fo... chrome/browser/android/foreign_session_helper.cc:214: content::RecordComputedAction("MobileNTPForeignSession"); On 2013/07/27 00:24:51, newt wrote: > this action is actually obsolete. I'd leave this out for now, and we can add UMA > stats to the NTP later. (this probably isn't the right place to record stats > anyway, since this isn't part of the NTP) Done.
https://chromiumcodereview.appspot.com/19874002/diff/40001/chrome/android/jav... File chrome/android/java/src/org/chromium/chrome/browser/ForeignSessionHelper.java (right): https://chromiumcodereview.appspot.com/19874002/diff/40001/chrome/android/jav... chrome/android/java/src/org/chromium/chrome/browser/ForeignSessionHelper.java:157: long timeDiff = session2.modifiedTime - session1.modifiedTime; On 2013/07/27 00:24:51, newt wrote: > even simpler: > > return Long.compare(sessionA.modifiedTime, sessionB.modifiedTime); This API isn't in Android as it was added to java in 1.7. If you end up using Long.java, it will create two Long objects for each comparison. Certainly not the most expensive thing in the world, but it isn't free.
https://chromiumcodereview.appspot.com/19874002/diff/40001/chrome/android/jav... File chrome/android/java/src/org/chromium/chrome/browser/ForeignSessionHelper.java (right): https://chromiumcodereview.appspot.com/19874002/diff/40001/chrome/android/jav... chrome/android/java/src/org/chromium/chrome/browser/ForeignSessionHelper.java:157: long timeDiff = session2.modifiedTime - session1.modifiedTime; On 2013/07/29 15:33:17, Ted C wrote: > On 2013/07/27 00:24:51, newt wrote: > > even simpler: > > > > return Long.compare(sessionA.modifiedTime, sessionB.modifiedTime); > > This API isn't in Android as it was added to java in 1.7. If you end up using > Long.java, it will create two Long objects for each comparison. Certainly not > the most expensive thing in the world, but it isn't free. Done.
@newt lgtm? :)
one last comment https://codereview.chromium.org/19874002/diff/59001/chrome/browser/android/fo... File chrome/browser/android/foreign_session_helper.cc (right): https://codereview.chromium.org/19874002/diff/59001/chrome/browser/android/fo... chrome/browser/android/foreign_session_helper.cc:62: if (tab_url == GURL(chrome::kChromeUINewTabURL)) is this the behavior we want? this will exclude chrome://newtab, but not chrome://newtab#most_visited This should ignore chrome-native://newtab as well. Aaand GURL() is an expensive constructor. you should move it outside the loop here, or get rid of it altogether (i.e. just check tab_url's scheme and host). https://codereview.chromium.org/19874002/diff/98001/chrome/android/java/src/o... File chrome/android/java/src/org/chromium/chrome/browser/ForeignSessionHelper.java (right): https://codereview.chromium.org/19874002/diff/98001/chrome/android/java/src/o... chrome/android/java/src/org/chromium/chrome/browser/ForeignSessionHelper.java:159: return lhs.modifiedTime < rhs.modifiedTime ? 1 : too bad Long.compare() doesn't exist in 1.6 :'(
https://codereview.chromium.org/19874002/diff/59001/chrome/browser/android/fo... File chrome/browser/android/foreign_session_helper.cc (right): https://codereview.chromium.org/19874002/diff/59001/chrome/browser/android/fo... chrome/browser/android/foreign_session_helper.cc:62: if (tab_url == GURL(chrome::kChromeUINewTabURL)) On 2013/07/29 22:16:39, newt wrote: > is this the behavior we want? this will exclude chrome://newtab, but not > chrome://newtab#most_visited > > This should ignore chrome-native://newtab as well. > > Aaand GURL() is an expensive constructor. you should move it outside the loop > here, or get rid of it altogether (i.e. just check tab_url's scheme and host). Done.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kkimlabs@chromium.org/19874002/121001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kkimlabs@chromium.org/19874002/144001
Message was sent while issue was closed.
Change committed as 214539 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
