Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1275813002:
Implemented typo recognition in Chrome for Android. (Closed)
|
Created:
5 years, 4 months ago by dylanking Modified:
5 years, 4 months ago CC:
chromium-reviews, groby+spellwatch_chromium.org, rlp+watch_chromium.org, rouslan+spellwatch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@4_remove_mac_redundancies Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplemented typo recognition in Chrome for Android.
IPCs from the renderer arrive at the message filter,
which then calls into the native SpellCheckerSessionBridge.
Native SpellCheckerSessionBridge handles caching of
requests that come in while another request is busy.
Requests are sent to SpellCheckerSessionBridge.java
through JNI in order to be spellchecked. This will also
be where suggestions are generated in the future.
Spellcheck typo results are then sent back to native,
which then sends them via IPC to the renderer.
Design Doc: https://docs.google.com/document/d/1JQscDy-Vr4roGHbD0vRSiB9Zbl8pLpsbBMMsHffr9LE/edit#
BUG=415302
Committed: https://crrev.com/82c859eb3f9b56c35846f7e44e4f3aad94075ec4
Cr-Commit-Position: refs/heads/master@{#343962}
Patch Set 1 #
Total comments: 20
Patch Set 2 : Update to implementation and style #Patch Set 3 : Changed implementation to hide android details from generic header #Patch Set 4 : Cleanup & Rebase #Patch Set 5 : More cleanup & rebase #Patch Set 6 : Removed unused variables from past implementation approach #
Total comments: 36
Patch Set 7 : Uploading for rouslan@ to take a look at forward declaration issue, addressed some comments #Patch Set 8 : Addressed the rest of rouslan@'s comments. #Patch Set 9 : Changed spellchecker to pull locale from system settings instead of always English #Patch Set 10 : Added a clarifying comment about Locale to the java file #
Total comments: 44
Patch Set 11 : Uploading for rouslan@ to take a look at spellchecker_session_bridge.cc crash #Patch Set 12 : Changed pending requests to be handled in native, other comment addressing #
Total comments: 20
Patch Set 13 : Rebase #Patch Set 14 : Registered with JNI, addressed newt@'s comments. #
Total comments: 2
Patch Set 15 : Now using scoped_ptr #
Total comments: 2
Patch Set 16 : Now using implicit bool conversion of scoped_ptr's #Patch Set 17 : #ifdef declaration in message filter to have mac compile #
Total comments: 29
Patch Set 18 : Several style changes #Messages
Total messages: 33 (6 generated)
dylanking@google.com changed reviewers: + rouslan@chromium.org
Still might use some cleanup. Note that the feature isn't being gated yet. Will land that before this.
https://codereview.chromium.org/1275813002/diff/1/chrome/browser/spellchecker... File chrome/browser/spellchecker/spellcheck_message_filter_platform.h (right): https://codereview.chromium.org/1275813002/diff/1/chrome/browser/spellchecker... chrome/browser/spellchecker/spellcheck_message_filter_platform.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. Let's not put anything java-specific or android-specific into this file, because Mac uses it too. Mac build is likely to break with these includes. If you need a header file for some JNI functions, create a spellchecker_session_bridge.h/cc for those functions. RegisterSpellCheckMessageFilterPlatform() should be in there, for example. Try to put as much code as possible into spellcheck_message_filter_platform_android.cc, especially the anonymous namespace. That will prevent leaking android implementation details to other platforms, like Mac. https://codereview.chromium.org/1275813002/diff/1/chrome/browser/spellchecker... chrome/browser/spellchecker/spellcheck_message_filter_platform.h:54: bool RegisterSpellCheckMessageFilterPlatform(JNIEnv* env); From what I've seen, RegisterStuff(JNIEnv*) functions are usually static and not part of a class. https://codereview.chromium.org/1275813002/diff/1/chrome/browser/spellchecker... File chrome/browser/spellchecker/spellcheck_message_filter_platform_android.cc (left): https://codereview.chromium.org/1275813002/diff/1/chrome/browser/spellchecker... chrome/browser/spellchecker/spellcheck_message_filter_platform_android.cc:28: // static It's Chrome convention to put // static above definitions of methods that are declared static in the header file. Let's keep this comment. https://codereview.chromium.org/1275813002/diff/1/chrome/browser/spellchecker... File chrome/browser/spellchecker/spellcheck_message_filter_platform_android.cc (right): https://codereview.chromium.org/1275813002/diff/1/chrome/browser/spellchecker... chrome/browser/spellchecker/spellcheck_message_filter_platform_android.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. clang-format the whole file, please. https://codereview.chromium.org/1275813002/diff/1/chrome/browser/spellchecker... chrome/browser/spellchecker/spellcheck_message_filter_platform_android.cc:42: IPC_MESSAGE_HANDLER(SpellCheckHostMsg_CheckSpelling, Only OnRequestTextCheck does any work, so let's not handle IPC messages other than SpellCheckHostMsg_RequestTextCheck. https://codereview.chromium.org/1275813002/diff/1/chrome/browser/spellchecker... File chrome/browser/spellchecker/spellcheck_platform_android.cc (right): https://codereview.chromium.org/1275813002/diff/1/chrome/browser/spellchecker... chrome/browser/spellchecker/spellcheck_platform_android.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. clang-format this whole file please. https://codereview.chromium.org/1275813002/diff/1/chrome/renderer/spellchecke... File chrome/renderer/spellchecker/spellcheck.h (right): https://codereview.chromium.org/1275813002/diff/1/chrome/renderer/spellchecke... chrome/renderer/spellchecker/spellcheck.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. Is this a bad merge? I don't think that you should be modifying this file.
drive-by https://codereview.chromium.org/1275813002/diff/1/chrome/browser/spellchecker... File chrome/browser/spellchecker/spellcheck_message_filter_platform_android.cc (right): https://codereview.chromium.org/1275813002/diff/1/chrome/browser/spellchecker... chrome/browser/spellchecker/spellcheck_message_filter_platform_android.cc:87: No empty line, please https://codereview.chromium.org/1275813002/diff/1/chrome/browser/spellchecker... chrome/browser/spellchecker/spellcheck_message_filter_platform_android.cc:100: bool SpellCheckMessageFilterPlatform::RegisterSpellCheckMessageFilterPlatform(JNIEnv* env) { It's not static in the header. https://codereview.chromium.org/1275813002/diff/1/chrome/browser/spellchecker... File chrome/browser/spellchecker/spellcheck_platform_android.cc (right): https://codereview.chromium.org/1275813002/diff/1/chrome/browser/spellchecker... chrome/browser/spellchecker/spellcheck_platform_android.cc:15: return true; Wrong base patchset?
Yeah, sorry guys, this wasn't stylistically ready for review. Was just uploaded so rouslan@ could check out the implementation details before I proceeded to finish cleaning up the code. Had a bit of a time-crunch and failed to communicate that properly when I uploaded the patch. Will clean this up today and address comments.
dylanking@google.com changed reviewers: + groby@chromium.org
Addressed several comments and also updated the implementation to hopefully not miss requests. Still need to address one comment by rouslan@, will do that on Monday. https://codereview.chromium.org/1275813002/diff/1/chrome/browser/spellchecker... File chrome/browser/spellchecker/spellcheck_message_filter_platform.h (right): https://codereview.chromium.org/1275813002/diff/1/chrome/browser/spellchecker... chrome/browser/spellchecker/spellcheck_message_filter_platform.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. Have to catch the last bus home, and am OOO tomorrow (August 7th). Will address this when I get back on Monday as I still have some questions as to how to go about this. All other comments for this patchset should be addressed, however. https://codereview.chromium.org/1275813002/diff/1/chrome/browser/spellchecker... chrome/browser/spellchecker/spellcheck_message_filter_platform.h:54: bool RegisterSpellCheckMessageFilterPlatform(JNIEnv* env); I think they are supposed to be public, static class methods. Changed to reflect that. https://codereview.chromium.org/1275813002/diff/1/chrome/browser/spellchecker... File chrome/browser/spellchecker/spellcheck_message_filter_platform_android.cc (left): https://codereview.chromium.org/1275813002/diff/1/chrome/browser/spellchecker... chrome/browser/spellchecker/spellcheck_message_filter_platform_android.cc:28: // static Done. https://codereview.chromium.org/1275813002/diff/1/chrome/browser/spellchecker... File chrome/browser/spellchecker/spellcheck_message_filter_platform_android.cc (right): https://codereview.chromium.org/1275813002/diff/1/chrome/browser/spellchecker... chrome/browser/spellchecker/spellcheck_message_filter_platform_android.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. Done. https://codereview.chromium.org/1275813002/diff/1/chrome/browser/spellchecker... chrome/browser/spellchecker/spellcheck_message_filter_platform_android.cc:42: IPC_MESSAGE_HANDLER(SpellCheckHostMsg_CheckSpelling, Done. https://codereview.chromium.org/1275813002/diff/1/chrome/browser/spellchecker... chrome/browser/spellchecker/spellcheck_message_filter_platform_android.cc:87: Done. https://codereview.chromium.org/1275813002/diff/1/chrome/browser/spellchecker... chrome/browser/spellchecker/spellcheck_message_filter_platform_android.cc:100: bool SpellCheckMessageFilterPlatform::RegisterSpellCheckMessageFilterPlatform(JNIEnv* env) { It is now :) https://codereview.chromium.org/1275813002/diff/1/chrome/browser/spellchecker... File chrome/browser/spellchecker/spellcheck_platform_android.cc (right): https://codereview.chromium.org/1275813002/diff/1/chrome/browser/spellchecker... chrome/browser/spellchecker/spellcheck_platform_android.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. Done. https://codereview.chromium.org/1275813002/diff/1/chrome/browser/spellchecker... chrome/browser/spellchecker/spellcheck_platform_android.cc:15: return true; We've decided to move a lot of the functionality that was previously in spellcheck_platform_android, including all of the JNI interfacing, into spellcheck_message_filter_platform_android. Initially I thought that this method would be a good place to gate the feature with the chrome switch, but we've decided that the renderer would be a better spot because that allows us to reduce the IPC overhead when the feature is off. https://codereview.chromium.org/1275813002/diff/1/chrome/renderer/spellchecke... File chrome/renderer/spellchecker/spellcheck.h (right): https://codereview.chromium.org/1275813002/diff/1/chrome/renderer/spellchecke... chrome/renderer/spellchecker/spellcheck.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. Agreed, sorry. Fixed.
On 2015/08/06 at 16:58:28, Rouslan wrote: > Let's not put anything java-specific or android-specific into this file, because Mac uses it too. Mac build is likely to break with these includes. If you need a header file for some JNI functions, create a spellchecker_session_bridge.h/cc for those functions. RegisterSpellCheckMessageFilterPlatform() should be in there, for example. Try to put as much code as possible into spellcheck_message_filter_platform_android.cc, especially the anonymous namespace. That will prevent leaking android implementation details to other platforms, like Mac. This comment now addressed and patch is ready for review. groby@,rouslan@ please review *.h/*.cc, and feel free to check out SpellCheckerSessionBridge.java if you'd like.
Looks promising! https://codereview.chromium.org/1275813002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/spellchecker/SpellCheckerSessionBridge.java (right): https://codereview.chromium.org/1275813002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/spellchecker/SpellCheckerSessionBridge.java:53: System.out.println("DYLANKING Setting Pending Text To: " + text); Remove the log. https://codereview.chromium.org/1275813002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/spellchecker/SpellCheckerSessionBridge.java:60: mSpellCheckerSession.getSentenceSuggestions(new TextInfo[] {new TextInfo(mActiveText)}, 5); What is 5? https://codereview.chromium.org/1275813002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/spellchecker/SpellCheckerSessionBridge.java:87: mIsPendingRequest = false; You're saving pending text here, but identifier and route_id are overwritten in C++. You should save all of that information in one place. I suggest that you keep all request-specific information together. You need to keep track of two requests: the current one and the pending one. https://codereview.chromium.org/1275813002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/spellchecker/SpellCheckerSessionBridge.java:96: private int[] convertListToArray(ArrayList<Integer> toConvert) { Do any of these work? https://docs.oracle.com/javase/8/docs/api/java/util/ArrayList.html#toArray-- https://docs.oracle.com/javase/8/docs/api/java/util/ArrayList.html#toArray-T:A- https://codereview.chromium.org/1275813002/diff/100001/chrome/browser/spellch... File chrome/browser/spellchecker/spellcheck_message_filter_platform.h (right): https://codereview.chromium.org/1275813002/diff/100001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_message_filter_platform.h:11: #include "chrome/browser/spellchecker/spellchecker_session_bridge_android.h" Forward declare SpellCheckerSessionBridge instead of including its header. https://codereview.chromium.org/1275813002/diff/100001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_message_filter_platform.h:55: scoped_ptr<SpellCheckerSessionBridge> impl_; Add a comment that this implementation detail is used for Android. https://codereview.chromium.org/1275813002/diff/100001/chrome/browser/spellch... File chrome/browser/spellchecker/spellcheck_message_filter_platform_android.cc (right): https://codereview.chromium.org/1275813002/diff/100001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_message_filter_platform_android.cc:9: #include "chrome/browser/spellchecker/spellcheck_platform.h" Unused includes. https://codereview.chromium.org/1275813002/diff/100001/chrome/browser/spellch... File chrome/browser/spellchecker/spellchecker_session_bridge_android.cc (right): https://codereview.chromium.org/1275813002/diff/100001/chrome/browser/spellch... chrome/browser/spellchecker/spellchecker_session_bridge_android.cc:9: #include "base/android/jni_string.h" Chromium style is to not repeat header's includes in the source file. https://codereview.chromium.org/1275813002/diff/100001/chrome/browser/spellch... chrome/browser/spellchecker/spellchecker_session_bridge_android.cc:16: : render_process_id_(render_process_id) { Will this work? SpellCheckerSessionBridge::SpellCheckerSessionBridge(int render_process_id) : render_process_id_(render_process_id), java_object_(Java_SpellCheckerSessionBridge_create( base::android::AttachCurrentThread(), reinterpret_cast<intptr_t>(this))) {} https://codereview.chromium.org/1275813002/diff/100001/chrome/browser/spellch... chrome/browser/spellchecker/spellchecker_session_bridge_android.cc:46: std::vector<SpellCheckResult> local_results; Define local_results right before you use it (above the for loop). https://codereview.chromium.org/1275813002/diff/100001/chrome/browser/spellch... chrome/browser/spellchecker/spellchecker_session_bridge_android.cc:46: std::vector<SpellCheckResult> local_results; Just "results". It's simpler. https://codereview.chromium.org/1275813002/diff/100001/chrome/browser/spellch... File chrome/browser/spellchecker/spellchecker_session_bridge_android.h (right): https://codereview.chromium.org/1275813002/diff/100001/chrome/browser/spellch... chrome/browser/spellchecker/spellchecker_session_bridge_android.h:12: class SpellCheckerSessionBridge { Some docs on class and methods would not hurt. https://codereview.chromium.org/1275813002/diff/100001/chrome/browser/spellch... chrome/browser/spellchecker/spellchecker_session_bridge_android.h:14: SpellCheckerSessionBridge(int render_process_id); Mark this constructor explicit to avoid implicit conversion from an int to this object. https://codereview.chromium.org/1275813002/diff/100001/chrome/browser/spellch... chrome/browser/spellchecker/spellchecker_session_bridge_android.h:14: SpellCheckerSessionBridge(int render_process_id); If you have a constructor, you should have a destructor. https://codereview.chromium.org/1275813002/diff/100001/chrome/browser/spellch... chrome/browser/spellchecker/spellchecker_session_bridge_android.h:18: static bool RegisterSpellCheckerSessionBridge(JNIEnv* env); Static methods should be right after constructor/destructor pair. https://codereview.chromium.org/1275813002/diff/100001/chrome/browser/spellch... chrome/browser/spellchecker/spellchecker_session_bridge_android.h:19: void GetSpellcheckInfo(JNIEnv* env, s/GetSpellcheckInfo/ProcessSpellcheckResults/ https://codereview.chromium.org/1275813002/diff/100001/chrome/browser/spellch... chrome/browser/spellchecker/spellchecker_session_bridge_android.h:27: int route_id_; What is the difference between render_process_id_ and route_id_? https://codereview.chromium.org/1275813002/diff/100001/chrome/browser/spellch... chrome/browser/spellchecker/spellchecker_session_bridge_android.h:30: }; DISALLOW_COPY_AND_ASSIGN(SpellCheckerSessionBridge); https://codereview.chromium.org/1275813002/diff/100001/chrome/browser/spellch... chrome/browser/spellchecker/spellchecker_session_bridge_android.h:32: #endif #endif // CHROME_BROWSER_SPELLCHECKER_SPELLCHECKER_SESSION_BRIDGE_H_
dylanking@google.com changed reviewers: - groby@chromium.org
https://codereview.chromium.org/1275813002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/spellchecker/SpellCheckerSessionBridge.java (right): https://codereview.chromium.org/1275813002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/spellchecker/SpellCheckerSessionBridge.java:53: System.out.println("DYLANKING Setting Pending Text To: " + text); Don't know how that snuck through. Deleted. https://codereview.chromium.org/1275813002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/spellchecker/SpellCheckerSessionBridge.java:60: mSpellCheckerSession.getSentenceSuggestions(new TextInfo[] {new TextInfo(mActiveText)}, 5); The second parameter to this function is the maximum number of suggestions per word that we want returned to us by the spellchecker when we send off the text for spellchecking. I picked 5 because that's the default number that the Android spellchecker seems to use. However, since we are not currently displaying or doing anything with these suggestions, I suppose I can change this to 0 for now. https://codereview.chromium.org/1275813002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/spellchecker/SpellCheckerSessionBridge.java:87: mIsPendingRequest = false; Great point. I've created an inner class to encapsulate this information and keep it in one place. https://codereview.chromium.org/1275813002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/spellchecker/SpellCheckerSessionBridge.java:96: private int[] convertListToArray(ArrayList<Integer> toConvert) { I think those will convert the list of Integer *objects* into an array of Integer objects, whereas I wanted an array of *primitive* ints to send back to native, as they are easier to use when it comes to passing the array to native. However, if you think it may be a performance gain to use one of the linked methods and send back a jobjectArray (and then work some JNI magic to extract each object's int value), instead of a jintArray, I can do that. https://codereview.chromium.org/1275813002/diff/100001/chrome/browser/spellch... File chrome/browser/spellchecker/spellcheck_message_filter_platform.h (right): https://codereview.chromium.org/1275813002/diff/100001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_message_filter_platform.h:55: scoped_ptr<SpellCheckerSessionBridge> impl_; Done. https://codereview.chromium.org/1275813002/diff/100001/chrome/browser/spellch... File chrome/browser/spellchecker/spellchecker_session_bridge_android.h (right): https://codereview.chromium.org/1275813002/diff/100001/chrome/browser/spellch... chrome/browser/spellchecker/spellchecker_session_bridge_android.h:14: SpellCheckerSessionBridge(int render_process_id); Done.
dylanking@google.com changed reviewers: + newt@chromium.org
+rouslan@ for owners review of *.cc/*.h +newt@ for owners review of SpellCheckerSessionBridge.java https://codereview.chromium.org/1275813002/diff/100001/chrome/browser/spellch... File chrome/browser/spellchecker/spellcheck_message_filter_platform.h (right): https://codereview.chromium.org/1275813002/diff/100001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_message_filter_platform.h:11: #include "chrome/browser/spellchecker/spellchecker_session_bridge_android.h" Done, after some struggle. https://codereview.chromium.org/1275813002/diff/100001/chrome/browser/spellch... File chrome/browser/spellchecker/spellcheck_message_filter_platform_android.cc (right): https://codereview.chromium.org/1275813002/diff/100001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_message_filter_platform_android.cc:9: #include "chrome/browser/spellchecker/spellcheck_platform.h" Deleted, thanks for catching these. https://codereview.chromium.org/1275813002/diff/100001/chrome/browser/spellch... File chrome/browser/spellchecker/spellchecker_session_bridge_android.cc (right): https://codereview.chromium.org/1275813002/diff/100001/chrome/browser/spellch... chrome/browser/spellchecker/spellchecker_session_bridge_android.cc:9: #include "base/android/jni_string.h" Shouldn't have let that slip through. Thanks. https://codereview.chromium.org/1275813002/diff/100001/chrome/browser/spellch... chrome/browser/spellchecker/spellchecker_session_bridge_android.cc:16: : render_process_id_(render_process_id) { Yes! Reset() was the only assignment operator I knew. Looks like it won't be necessary here because of our strict 1:1 Native:Java object relationship. https://codereview.chromium.org/1275813002/diff/100001/chrome/browser/spellch... chrome/browser/spellchecker/spellchecker_session_bridge_android.cc:46: std::vector<SpellCheckResult> local_results; http://nuserve.co.uk/blog/wp-content/uploads/2013/12/JT_Sean_Parker_blog-300x... Done! :P https://codereview.chromium.org/1275813002/diff/100001/chrome/browser/spellch... File chrome/browser/spellchecker/spellchecker_session_bridge_android.h (right): https://codereview.chromium.org/1275813002/diff/100001/chrome/browser/spellch... chrome/browser/spellchecker/spellchecker_session_bridge_android.h:12: class SpellCheckerSessionBridge { Agreed, done. https://codereview.chromium.org/1275813002/diff/100001/chrome/browser/spellch... chrome/browser/spellchecker/spellchecker_session_bridge_android.h:18: static bool RegisterSpellCheckerSessionBridge(JNIEnv* env); Done. https://codereview.chromium.org/1275813002/diff/100001/chrome/browser/spellch... chrome/browser/spellchecker/spellchecker_session_bridge_android.h:19: void GetSpellcheckInfo(JNIEnv* env, Done, though I capitalized the C in "Spellcheck" to be more consistent with surrounding code. https://codereview.chromium.org/1275813002/diff/100001/chrome/browser/spellch... chrome/browser/spellchecker/spellchecker_session_bridge_android.h:27: int route_id_; I'm not entirely sure. render_process_id seems to be associated with the MessageFilter, whereas a route_id is associated with a specific IPC SpellCheckMessage. I have encapsulated some logic so that the route_id's get directly forwarded to the Java side and not stored in the C++ side, along with the other information contained in each specific message. https://codereview.chromium.org/1275813002/diff/100001/chrome/browser/spellch... chrome/browser/spellchecker/spellchecker_session_bridge_android.h:30: }; Done. https://codereview.chromium.org/1275813002/diff/100001/chrome/browser/spellch... chrome/browser/spellchecker/spellchecker_session_bridge_android.h:32: #endif Done.
I looked at the Java and C++ sides of SpellCheckerSessionBridge. Overall structure looks good, just lots of little comments :) https://codereview.chromium.org/1275813002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/spellchecker/SpellCheckerSessionBridge.java (right): https://codereview.chromium.org/1275813002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/spellchecker/SpellCheckerSessionBridge.java:22: public class SpellCheckerSessionBridge implements SpellCheckerSession.SpellCheckerSessionListener { nit: I'd just import SpellCheckerSessionListener and replace "SpellCheckerSession.SpellCheckerSessionListener" with "SpellCheckerSessionListener". https://codereview.chromium.org/1275813002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/spellchecker/SpellCheckerSessionBridge.java:23: private class SpellingRequest { Make this static. Inner classes should always be static unless they have a compelling need to access their enclosing class. Otherwise, the inner class holds an invisible reference to the outer class which complicates reasoning about object lifetimes and can lead to memory leaks. https://codereview.chromium.org/1275813002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/spellchecker/SpellCheckerSessionBridge.java:48: private final long mNativeSpellCheckerSessionBridge; nit: I'd order this variable first since it's final https://codereview.chromium.org/1275813002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/spellchecker/SpellCheckerSessionBridge.java:52: public SpellCheckerSessionBridge(long nativeSpellCheckerSessionBridge) { make this private unless it needs to be public https://codereview.chromium.org/1275813002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/spellchecker/SpellCheckerSessionBridge.java:61: // the system language specified in settings. Note that this is not the language specified Is this simply the "default locale", i.e. Locale.getDefault(), i.e. the language used by Chrome's UI? If so, I'd use the term "default locale" somewhere in here. However, TextServicesManager's javadoc spends so many words trying to describe which locale is used that it seems like it must be more complicated. Looking at the source [1] for TextServicesManager also indicates that it's more complicated. I'd spend some more time figuring out which locale is really used, as this is something we'll want to understand well before we ship this feature. Especially for users who use multiple languages, this is an important detail to get right. [1] http://androidxref.com/5.0.0_r2/xref/frameworks/base/core/java/android/view/t... https://codereview.chromium.org/1275813002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/spellchecker/SpellCheckerSessionBridge.java:72: private void requestTextCheck(int routeId, int identifier, String text) { Depending on how often this is called while an active request is in progress, it might be worthwhile to check on the native side whether a spelling request is in flight before converting the text to a Java String and calling this method over JNI. It's something to consider at least, perhaps as a future improvement. https://codereview.chromium.org/1275813002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/spellchecker/SpellCheckerSessionBridge.java:94: boolean isWordCorrect = (currentSuggestion.getSuggestionsAttributes() This indentation is just too weird (and doesn't follow Java style). How about: boolean isWordCorrect = (currentSuggestion.getSuggestionsAttributes() & SuggestionsInfo.RESULT_ATTR_LOOKS_LIKE_TYPO) == 0; https://codereview.chromium.org/1275813002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/spellchecker/SpellCheckerSessionBridge.java:121: private int[] convertListToArray(ArrayList<Integer> toConvert) { naming suggestion: "list" and "array" might be clearer names, rather than "toConvert" and "toReturn" https://codereview.chromium.org/1275813002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/spellchecker/SpellCheckerSessionBridge.java:124: for (Integer number : toConvert) { When iterating over Collections, we usually avoid the for-each syntax because it results in extra object creations. Especially here, since you need to keep track of the index anyway, I'd just use a regular for loop. (When iterating over arrays, on the other hand, the for-each syntax is perfectly efficient.) https://codereview.chromium.org/1275813002/diff/180001/chrome/browser/spellch... File chrome/browser/spellchecker/spellchecker_session_bridge_android.cc (right): https://codereview.chromium.org/1275813002/diff/180001/chrome/browser/spellch... chrome/browser/spellchecker/spellchecker_session_bridge_android.cc:23: } newline https://codereview.chromium.org/1275813002/diff/180001/chrome/browser/spellch... chrome/browser/spellchecker/spellchecker_session_bridge_android.cc:33: // Receives the spellcheck results back from the Java side in the form of Put method-level comments in the .h file only. Implementation comments, of course, belong in the .cc file. Why? Duplicating comments leads to comment skew, where the comments slowly diverge until they conflict with each other and people get confused. https://codereview.chromium.org/1275813002/diff/180001/chrome/browser/spellch... chrome/browser/spellchecker/spellchecker_session_bridge_android.cc:52: for (unsigned int j = 0; j < offsets.size(); j++) { I believe this should be "size_t", not "unsigned int" https://codereview.chromium.org/1275813002/diff/180001/chrome/browser/spellch... chrome/browser/spellchecker/spellchecker_session_bridge_android.cc:62: (int)route_id, (int)identifier, nit: use static_cast<int>() instead of (int). However, we often don't bother converting jint to int since they're the same underlying type; so you could just remove the cast altogether. https://codereview.chromium.org/1275813002/diff/180001/chrome/browser/spellch... File chrome/browser/spellchecker/spellchecker_session_bridge_android.h (right): https://codereview.chromium.org/1275813002/diff/180001/chrome/browser/spellch... chrome/browser/spellchecker/spellchecker_session_bridge_android.h:9: #include "base/android/jni_array.h" remove unneeded imports (jni_array.h and jni_string.h appear unused). OTOH, you need imports for base::string16 and base::android::ScopedJavaGlobalRef
https://codereview.chromium.org/1275813002/diff/180001/chrome/browser/spellch... File chrome/browser/spellchecker/spellcheck_message_filter_platform_android.cc (right): https://codereview.chromium.org/1275813002/diff/180001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_message_filter_platform_android.cc:39: const std::vector<SpellCheckResult>& local_results) { NOTREACHED(); https://codereview.chromium.org/1275813002/diff/180001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_message_filter_platform_android.cc:47: bool* correct) { NOTREACHED(); https://codereview.chromium.org/1275813002/diff/180001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_message_filter_platform_android.cc:52: std::vector<base::string16>* suggestions) { NOTREACHED(); https://codereview.chromium.org/1275813002/diff/180001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_message_filter_platform_android.cc:55: void SpellCheckMessageFilterPlatform::OnShowSpellingPanel(bool show) { NOTREACHED(); https://codereview.chromium.org/1275813002/diff/180001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_message_filter_platform_android.cc:59: const base::string16& word) { NOTREACHED(); https://codereview.chromium.org/1275813002/diff/180001/chrome/browser/spellch... File chrome/browser/spellchecker/spellchecker_session_bridge_android.h (right): https://codereview.chromium.org/1275813002/diff/180001/chrome/browser/spellch... chrome/browser/spellchecker/spellchecker_session_bridge_android.h:22: static bool RegisterSpellCheckerSessionBridge(JNIEnv* env); Since you've put this function inside of a class, you can call it Register(). It's already obvious what you're registering. Your call to register becomes: SpellCheckerSessionBridge::Register(env); Instead of this: SpellCheckerSessionBridge::RegisterSpellCheckSessionBridge(env); https://codereview.chromium.org/1275813002/diff/180001/chrome/browser/spellch... chrome/browser/spellchecker/spellchecker_session_bridge_android.h:22: static bool RegisterSpellCheckerSessionBridge(JNIEnv* env); Where do you call this? https://codereview.chromium.org/1275813002/diff/180001/chrome/browser/spellch... chrome/browser/spellchecker/spellchecker_session_bridge_android.h:37: jintArray offsetArray, C++ parameters should_use_underscores_in_names.
https://codereview.chromium.org/1275813002/diff/180001/chrome/browser/spellch... File chrome/browser/spellchecker/spellchecker_session_bridge_android.h (right): https://codereview.chromium.org/1275813002/diff/180001/chrome/browser/spellch... chrome/browser/spellchecker/spellchecker_session_bridge_android.h:22: static bool RegisterSpellCheckerSessionBridge(JNIEnv* env); On 2015/08/12 17:05:54, Rouslan wrote: > Since you've put this function inside of a class, you can call it Register(). > It's already obvious what you're registering. Your call to register becomes: > > SpellCheckerSessionBridge::Register(env); > > Instead of this: > > SpellCheckerSessionBridge::RegisterSpellCheckSessionBridge(env); Or call it "RegisterJNI", which is clear and concise.
https://codereview.chromium.org/1275813002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/spellchecker/SpellCheckerSessionBridge.java (right): https://codereview.chromium.org/1275813002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/spellchecker/SpellCheckerSessionBridge.java:22: public class SpellCheckerSessionBridge implements SpellCheckerSession.SpellCheckerSessionListener { Agreed, done. https://codereview.chromium.org/1275813002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/spellchecker/SpellCheckerSessionBridge.java:23: private class SpellingRequest { Great point, thanks for the explanation as well. https://codereview.chromium.org/1275813002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/spellchecker/SpellCheckerSessionBridge.java:48: private final long mNativeSpellCheckerSessionBridge; Done. https://codereview.chromium.org/1275813002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/spellchecker/SpellCheckerSessionBridge.java:52: public SpellCheckerSessionBridge(long nativeSpellCheckerSessionBridge) { Done. https://codereview.chromium.org/1275813002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/spellchecker/SpellCheckerSessionBridge.java:61: // the system language specified in settings. Note that this is not the language specified Yes, I believe this is simply the language used by Chrome's UI. I believe that this combination of parameters (namely, the second being null and the last being true) will pull this value. "The locale specified in settings will be used". Let's talk more tomorrow about this (have to run). Going through that source code did, however, alert me that this method will return null if the user has turned spellchecking off in their system settings. I've added a check in requestTextCheck() for this to prevent crashes, but it may be ideal to recognize this somewhere higher up the callstack to avoid unnecessary IPC (if this kind of information is exposed to native code). https://codereview.chromium.org/1275813002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/spellchecker/SpellCheckerSessionBridge.java:72: private void requestTextCheck(int routeId, int identifier, String text) { Very good point, I'll change the approach around to handle all the pending requests on the native side in order to avoid unnecessary JNI overhead. https://codereview.chromium.org/1275813002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/spellchecker/SpellCheckerSessionBridge.java:94: boolean isWordCorrect = (currentSuggestion.getSuggestionsAttributes() Whoa, I blame 'git cl format'! Fixed ;) https://codereview.chromium.org/1275813002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/spellchecker/SpellCheckerSessionBridge.java:121: private int[] convertListToArray(ArrayList<Integer> toConvert) { Changed. https://codereview.chromium.org/1275813002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/spellchecker/SpellCheckerSessionBridge.java:124: for (Integer number : toConvert) { Not sure why I did foreach especially when I was keeping track of the index. Fixed! https://codereview.chromium.org/1275813002/diff/180001/chrome/browser/spellch... File chrome/browser/spellchecker/spellcheck_message_filter_platform_android.cc (right): https://codereview.chromium.org/1275813002/diff/180001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_message_filter_platform_android.cc:39: const std::vector<SpellCheckResult>& local_results) { Done. https://codereview.chromium.org/1275813002/diff/180001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_message_filter_platform_android.cc:47: bool* correct) { Done. https://codereview.chromium.org/1275813002/diff/180001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_message_filter_platform_android.cc:52: std::vector<base::string16>* suggestions) { Done. https://codereview.chromium.org/1275813002/diff/180001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_message_filter_platform_android.cc:55: void SpellCheckMessageFilterPlatform::OnShowSpellingPanel(bool show) { Done. https://codereview.chromium.org/1275813002/diff/180001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_message_filter_platform_android.cc:59: const base::string16& word) { Done. https://codereview.chromium.org/1275813002/diff/180001/chrome/browser/spellch... File chrome/browser/spellchecker/spellchecker_session_bridge_android.cc (right): https://codereview.chromium.org/1275813002/diff/180001/chrome/browser/spellch... chrome/browser/spellchecker/spellchecker_session_bridge_android.cc:23: } Thanks, done. https://codereview.chromium.org/1275813002/diff/180001/chrome/browser/spellch... chrome/browser/spellchecker/spellchecker_session_bridge_android.cc:33: // Receives the spellcheck results back from the Java side in the form of I had tried to elaborate a little bit more on the implementation details of the method here, as opposed to the comment in the .h file, but looking back on it there isn't much difference between what I say in the two comments! I'll take this out and just add a small comment inside the method at the part I was trying to elucidate. https://codereview.chromium.org/1275813002/diff/180001/chrome/browser/spellch... chrome/browser/spellchecker/spellchecker_session_bridge_android.cc:52: for (unsigned int j = 0; j < offsets.size(); j++) { Done, good catch. Thanks. https://codereview.chromium.org/1275813002/diff/180001/chrome/browser/spellch... chrome/browser/spellchecker/spellchecker_session_bridge_android.cc:62: (int)route_id, (int)identifier, Removed the casts altogether. https://codereview.chromium.org/1275813002/diff/180001/chrome/browser/spellch... File chrome/browser/spellchecker/spellchecker_session_bridge_android.h (right): https://codereview.chromium.org/1275813002/diff/180001/chrome/browser/spellch... chrome/browser/spellchecker/spellchecker_session_bridge_android.h:9: #include "base/android/jni_array.h" jni_array.h and jni_string.h are needed for the parameter types of ProcessSpellCheckResults. Imported scoped_java_ref and string16, thanks! https://codereview.chromium.org/1275813002/diff/180001/chrome/browser/spellch... chrome/browser/spellchecker/spellchecker_session_bridge_android.h:22: static bool RegisterSpellCheckerSessionBridge(JNIEnv* env); There's some information about this here, but I'm not 100% where this gets called. My JNI works fine. Will investigate further tomorrow (have to run for the day): https://code.google.com/p/chromium/codesearch#chromium/src/base/android/jni_g... It seems that the naming convention on the Register method is inconsistent across the codebase. Some classes do Register(), some do RegisterJNI(), some do RegisterFullClassName(). I'll change mine to RegisterJNI() as that's what the Chromium JNI Guidelines have and I believe it's the most descriptive without being too verbose. https://codereview.chromium.org/1275813002/diff/180001/chrome/browser/spellch... chrome/browser/spellchecker/spellchecker_session_bridge_android.h:37: jintArray offsetArray, Thanks, done.
NOTE: Bad upstream target was set with the latest patch, will fix. On 2015/08/13 at 02:10:03, dylanking wrote: > https://codereview.chromium.org/1275813002/diff/180001/chrome/android/java/sr... > File chrome/android/java/src/org/chromium/chrome/browser/spellchecker/SpellCheckerSessionBridge.java (right): > > https://codereview.chromium.org/1275813002/diff/180001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/spellchecker/SpellCheckerSessionBridge.java:22: public class SpellCheckerSessionBridge implements SpellCheckerSession.SpellCheckerSessionListener { > Agreed, done. > > https://codereview.chromium.org/1275813002/diff/180001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/spellchecker/SpellCheckerSessionBridge.java:23: private class SpellingRequest { > Great point, thanks for the explanation as well. > > https://codereview.chromium.org/1275813002/diff/180001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/spellchecker/SpellCheckerSessionBridge.java:48: private final long mNativeSpellCheckerSessionBridge; > Done. > > https://codereview.chromium.org/1275813002/diff/180001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/spellchecker/SpellCheckerSessionBridge.java:52: public SpellCheckerSessionBridge(long nativeSpellCheckerSessionBridge) { > Done. > > https://codereview.chromium.org/1275813002/diff/180001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/spellchecker/SpellCheckerSessionBridge.java:61: // the system language specified in settings. Note that this is not the language specified > Yes, I believe this is simply the language used by Chrome's UI. > > I believe that this combination of parameters (namely, the second being null and the last being true) will pull this value. "The locale specified in settings will be used". Let's talk more tomorrow about this (have to run). > > Going through that source code did, however, alert me that this method will return null if the user has turned spellchecking off in their system settings. I've added a check in requestTextCheck() for this to prevent crashes, but it may be ideal to recognize this somewhere higher up the callstack to avoid unnecessary IPC (if this kind of information is exposed to native code). > > https://codereview.chromium.org/1275813002/diff/180001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/spellchecker/SpellCheckerSessionBridge.java:72: private void requestTextCheck(int routeId, int identifier, String text) { > Very good point, I'll change the approach around to handle all the pending requests on the native side in order to avoid unnecessary JNI overhead. > > https://codereview.chromium.org/1275813002/diff/180001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/spellchecker/SpellCheckerSessionBridge.java:94: boolean isWordCorrect = (currentSuggestion.getSuggestionsAttributes() > Whoa, I blame 'git cl format'! Fixed ;) > > https://codereview.chromium.org/1275813002/diff/180001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/spellchecker/SpellCheckerSessionBridge.java:121: private int[] convertListToArray(ArrayList<Integer> toConvert) { > Changed. > > https://codereview.chromium.org/1275813002/diff/180001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/spellchecker/SpellCheckerSessionBridge.java:124: for (Integer number : toConvert) { > Not sure why I did foreach especially when I was keeping track of the index. Fixed! > > https://codereview.chromium.org/1275813002/diff/180001/chrome/browser/spellch... > File chrome/browser/spellchecker/spellcheck_message_filter_platform_android.cc (right): > > https://codereview.chromium.org/1275813002/diff/180001/chrome/browser/spellch... > chrome/browser/spellchecker/spellcheck_message_filter_platform_android.cc:39: const std::vector<SpellCheckResult>& local_results) { > Done. > > https://codereview.chromium.org/1275813002/diff/180001/chrome/browser/spellch... > chrome/browser/spellchecker/spellcheck_message_filter_platform_android.cc:47: bool* correct) { > Done. > > https://codereview.chromium.org/1275813002/diff/180001/chrome/browser/spellch... > chrome/browser/spellchecker/spellcheck_message_filter_platform_android.cc:52: std::vector<base::string16>* suggestions) { > Done. > > https://codereview.chromium.org/1275813002/diff/180001/chrome/browser/spellch... > chrome/browser/spellchecker/spellcheck_message_filter_platform_android.cc:55: void SpellCheckMessageFilterPlatform::OnShowSpellingPanel(bool show) { > Done. > > https://codereview.chromium.org/1275813002/diff/180001/chrome/browser/spellch... > chrome/browser/spellchecker/spellcheck_message_filter_platform_android.cc:59: const base::string16& word) { > Done. > > https://codereview.chromium.org/1275813002/diff/180001/chrome/browser/spellch... > File chrome/browser/spellchecker/spellchecker_session_bridge_android.cc (right): > > https://codereview.chromium.org/1275813002/diff/180001/chrome/browser/spellch... > chrome/browser/spellchecker/spellchecker_session_bridge_android.cc:23: } > Thanks, done. > > https://codereview.chromium.org/1275813002/diff/180001/chrome/browser/spellch... > chrome/browser/spellchecker/spellchecker_session_bridge_android.cc:33: // Receives the spellcheck results back from the Java side in the form of > I had tried to elaborate a little bit more on the implementation details of the method here, as opposed to the comment in the .h file, but looking back on it there isn't much difference between what I say in the two comments! I'll take this out and just add a small comment inside the method at the part I was trying to elucidate. > > https://codereview.chromium.org/1275813002/diff/180001/chrome/browser/spellch... > chrome/browser/spellchecker/spellchecker_session_bridge_android.cc:52: for (unsigned int j = 0; j < offsets.size(); j++) { > Done, good catch. Thanks. > > https://codereview.chromium.org/1275813002/diff/180001/chrome/browser/spellch... > chrome/browser/spellchecker/spellchecker_session_bridge_android.cc:62: (int)route_id, (int)identifier, > Removed the casts altogether. > > https://codereview.chromium.org/1275813002/diff/180001/chrome/browser/spellch... > File chrome/browser/spellchecker/spellchecker_session_bridge_android.h (right): > > https://codereview.chromium.org/1275813002/diff/180001/chrome/browser/spellch... > chrome/browser/spellchecker/spellchecker_session_bridge_android.h:9: #include "base/android/jni_array.h" > jni_array.h and jni_string.h are needed for the parameter types of ProcessSpellCheckResults. > > Imported scoped_java_ref and string16, thanks! > > https://codereview.chromium.org/1275813002/diff/180001/chrome/browser/spellch... > chrome/browser/spellchecker/spellchecker_session_bridge_android.h:22: static bool RegisterSpellCheckerSessionBridge(JNIEnv* env); > There's some information about this here, but I'm not 100% where this gets called. My JNI works fine. Will investigate further tomorrow (have to run for the day): > https://code.google.com/p/chromium/codesearch#chromium/src/base/android/jni_g... > > It seems that the naming convention on the Register method is inconsistent across the codebase. Some classes do Register(), some do RegisterJNI(), some do RegisterFullClassName(). > > I'll change mine to RegisterJNI() as that's what the Chromium JNI Guidelines have and I believe it's the most descriptive without being too verbose. > > https://codereview.chromium.org/1275813002/diff/180001/chrome/browser/spellch... > chrome/browser/spellchecker/spellchecker_session_bridge_android.h:37: jintArray offsetArray, > Thanks, done.
Also, please elaborate a bit in the commit message. The commit message should give a general overview of the CL. The current description leaves me hanging. https://codereview.chromium.org/1275813002/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/spellchecker/SpellCheckerSessionBridge.java (right): https://codereview.chromium.org/1275813002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/spellchecker/SpellCheckerSessionBridge.java:44: return new SpellCheckerSessionBridge(nativeSpellCheckerSessionBridge); If mSpellCheckerSession is null, then this method might as well return null, instead of returning a useless SpellCheckerSessionBridge object. To do this, you could change the SpellCheckerSessionBridge constructor to take a SpellCheckerSession as a parameter, then move most of the code from the constructor into this create() method. Then you could move the null check from requestTextCheck() into C++ code. https://codereview.chromium.org/1275813002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/spellchecker/SpellCheckerSessionBridge.java:63: boolean isWordCorrect = (currentSuggestion.getSuggestionsAttributes() Still weird indentation. If git cl format did this though, it's OK to leave it as is. https://codereview.chromium.org/1275813002/diff/220001/chrome/browser/spellch... File chrome/browser/spellchecker/spellchecker_session_bridge_android.cc (right): https://codereview.chromium.org/1275813002/diff/220001/chrome/browser/spellch... chrome/browser/spellchecker/spellchecker_session_bridge_android.cc:83: if (is_pending_request_) { slightly simpler way to write this: active_request_ = pending_request_.Pass(); if (active_request_) { // attach current thread // Java...requestTextCheck() } https://codereview.chromium.org/1275813002/diff/220001/chrome/browser/spellch... chrome/browser/spellchecker/spellchecker_session_bridge_android.cc:89: env, java_object_.obj(), base::android::ConvertUTF16ToJavaString( weird indentation https://codereview.chromium.org/1275813002/diff/220001/chrome/browser/spellch... File chrome/browser/spellchecker/spellchecker_session_bridge_android.h (right): https://codereview.chromium.org/1275813002/diff/220001/chrome/browser/spellch... chrome/browser/spellchecker/spellchecker_session_bridge_android.h:8: #include "base/android/jni_android.h" I still don't think these jni_*.h includes are needed. jni_array.h, jni_android.h, and jni_string.h provide only methods, such as ToJavaIntArray(), AttachCurrentThread(), and ConvertJavaStringToUTF16(). OTOH, you need <jni.h>, which defines all the JNI basics: JNIEnv, jobject, jintArray, etc. https://codereview.chromium.org/1275813002/diff/220001/chrome/browser/spellch... chrome/browser/spellchecker/spellchecker_session_bridge_android.h:40: struct SpellingRequest { Since this class is used only internally, it'd be nice to put its definition in the .cc file and just put a forward declaration here. I'm not sure if you can put the definition in an anonymous namespace in the .cc file, but if so, that's even better. https://codereview.chromium.org/1275813002/diff/220001/chrome/browser/spellch... chrome/browser/spellchecker/spellchecker_session_bridge_android.h:42: SpellingRequest() {} probably don't need this method either (once you start using scoped_ptrs as explained below) https://codereview.chromium.org/1275813002/diff/220001/chrome/browser/spellch... chrome/browser/spellchecker/spellchecker_session_bridge_android.h:45: void set_route_id(int r_id) { route_id = r_id; } I'd probably just remove these methods. Since this is a privately used struct, it's OK to read and write the fields directly. https://codereview.chromium.org/1275813002/diff/220001/chrome/browser/spellch... chrome/browser/spellchecker/spellchecker_session_bridge_android.h:55: bool is_active_request_; Instead of using a bool to remember whether active_spelling_request_ and pending_spelling_request_ are valid, used a scoped_ptr: scoped_ptr<SpellingRequest> active_; scoped_ptr<SpellingRequest> pending_; Then you can can write code like: active_.reset(new SpellingRequest(route, id, text)); and if (pending_.get()) { active_ = pending_.Pass(); } This is exactly the situation that scoped_ptrs are designed to handle. See the header comment in base/memory/scoped_ptr.h if you aren't familiar.
Will make the patch description more elaborate tomorrow! https://codereview.chromium.org/1275813002/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/spellchecker/SpellCheckerSessionBridge.java (right): https://codereview.chromium.org/1275813002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/spellchecker/SpellCheckerSessionBridge.java:44: return new SpellCheckerSessionBridge(nativeSpellCheckerSessionBridge); I tried this and got some "non-static object cannot be referenced from a static context" referring to mSpellCheckerSession as well as the "this" pointer that is used in newSpellCheckerSession. I could make a local SpellCheckerSession in create() and only set mSpellCheckerSession to it if it's not null; I would likely also have to create a separate listener. I think it may be best to leave it as is, at least for now. What do you think? https://codereview.chromium.org/1275813002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/spellchecker/SpellCheckerSessionBridge.java:63: boolean isWordCorrect = (currentSuggestion.getSuggestionsAttributes() Yeah I had changed it, but git cl format keeps changing it back. I'll leave it as is. https://codereview.chromium.org/1275813002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/spellchecker/SpellCheckerSessionBridge.java:63: boolean isWordCorrect = (currentSuggestion.getSuggestionsAttributes() Yeah, I think it was git cl format because I fixed it last time and it popped up again. https://codereview.chromium.org/1275813002/diff/220001/chrome/browser/spellch... File chrome/browser/spellchecker/spellchecker_session_bridge_android.cc (right): https://codereview.chromium.org/1275813002/diff/220001/chrome/browser/spellch... chrome/browser/spellchecker/spellchecker_session_bridge_android.cc:83: if (is_pending_request_) { Will definitely change to this if I end up using scoped_ptr's after getting rouslan@'s opinion. Thanks! https://codereview.chromium.org/1275813002/diff/220001/chrome/browser/spellch... chrome/browser/spellchecker/spellchecker_session_bridge_android.cc:89: env, java_object_.obj(), base::android::ConvertUTF16ToJavaString( Fixed, but I have a feeling this was git cl format again. We'll see if it reverts itself when I git cl format and upload the next patch. https://codereview.chromium.org/1275813002/diff/220001/chrome/browser/spellch... File chrome/browser/spellchecker/spellchecker_session_bridge_android.h (right): https://codereview.chromium.org/1275813002/diff/220001/chrome/browser/spellch... chrome/browser/spellchecker/spellchecker_session_bridge_android.h:8: #include "base/android/jni_android.h" Ah, I see. Changed, thanks! https://codereview.chromium.org/1275813002/diff/220001/chrome/browser/spellch... chrome/browser/spellchecker/spellchecker_session_bridge_android.h:40: struct SpellingRequest { I believe I need to define it here since SpellCheckerSessionBridge holds actual SpellingRequest objects, not pointers or references to SpellingRequest objects. See top answer here: http://stackoverflow.com/questions/553682/when-can-i-use-a-forward-declaration https://codereview.chromium.org/1275813002/diff/220001/chrome/browser/spellch... chrome/browser/spellchecker/spellchecker_session_bridge_android.h:42: SpellingRequest() {} Gotcha. https://codereview.chromium.org/1275813002/diff/220001/chrome/browser/spellch... chrome/browser/spellchecker/spellchecker_session_bridge_android.h:45: void set_route_id(int r_id) { route_id = r_id; } Right, meant to delete those earlier. Fixed. https://codereview.chromium.org/1275813002/diff/220001/chrome/browser/spellch... chrome/browser/spellchecker/spellchecker_session_bridge_android.h:55: bool is_active_request_; rouslan@ has advised me to use composed objects rather than scoped_ptr's where possible. However, since the use of composed member objects does necessitate the use of two extra booleans, it may make more sense to use scoped_ptr. I will see what he thinks. Thanks!
https://codereview.chromium.org/1275813002/diff/220001/chrome/browser/spellch... File chrome/browser/spellchecker/spellchecker_session_bridge_android.h (right): https://codereview.chromium.org/1275813002/diff/220001/chrome/browser/spellch... chrome/browser/spellchecker/spellchecker_session_bridge_android.h:55: bool is_active_request_; On 2015/08/14 02:57:03, dylanking wrote: > rouslan@ has advised me to use composed objects rather than scoped_ptr's where > possible. However, since the use of composed member objects does necessitate > the use of two extra booleans, it may make more sense to use scoped_ptr. I will > see what he thinks. Thanks! I think this is a good use case for scoped pointers. "active_ = pending_.Pass()" is more elegant than "if (is_pending_request_) { is_pending_request_ = false; active_ = pending_; }", because it automagically moves pending_ data to active_ and sets pending_ to nullptr. When I said that you should not use scoped_ptr if you can avoid it, I meant was primarily talking about the forward declarations. It's possible to forward-declare SpellingRequest in the header, if you use a scoped_ptr. Technically, this reduces header size, but it also makes the code less readable. That's the use case that we discourage.
Looks good. Just waiting on scoped_ptrs. https://codereview.chromium.org/1275813002/diff/250001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/spellchecker/SpellCheckerSessionBridge.java (right): https://codereview.chromium.org/1275813002/diff/250001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/spellchecker/SpellCheckerSessionBridge.java:43: private static SpellCheckerSessionBridge create(long nativeSpellCheckerSessionBridge) { Oh, I see the problem. You're passing "this" to newSpellCheckerSession(). How about this: private static SpellCheckerSessionBridge create(long nativeSpellCheckerSessionBridge) { SpellCheckerSessionBridge bridge = new SpellCheckerSessionBridge(nativeSpellCheckerSessionBridge); if (bridge.mSpellCheckerSession == null) return null; return bridge; } Then you can remove the null check from requestTextCheck() and instead null check on the C++ side.
https://codereview.chromium.org/1275813002/diff/250001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/spellchecker/SpellCheckerSessionBridge.java (right): https://codereview.chromium.org/1275813002/diff/250001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/spellchecker/SpellCheckerSessionBridge.java:43: private static SpellCheckerSessionBridge create(long nativeSpellCheckerSessionBridge) { Gotcha. This will help reduce/eliminate JNI calls when we can't spellcheck a user's language or if their spellchecker is turned off. Thanks!
lgtm from my end! https://codereview.chromium.org/1275813002/diff/270001/chrome/browser/spellch... File chrome/browser/spellchecker/spellchecker_session_bridge_android.cc (right): https://codereview.chromium.org/1275813002/diff/270001/chrome/browser/spellch... chrome/browser/spellchecker/spellchecker_session_bridge_android.cc:43: if (active_request_.get()) { I believe you can omit ".get()". scoped_ptr supports conversion to bool similar to raw pointers.
https://codereview.chromium.org/1275813002/diff/270001/chrome/browser/spellch... File chrome/browser/spellchecker/spellchecker_session_bridge_android.cc (right): https://codereview.chromium.org/1275813002/diff/270001/chrome/browser/spellch... chrome/browser/spellchecker/spellchecker_session_bridge_android.cc:43: if (active_request_.get()) { Cool, just verified that. Changed.
The first line of the description should be a single line at at most 64 chars ideally. 72 chars max, if you must. Don't use the phrase "red underline" in the description for 2 reasons: 1) The renderer can choose what to do with the results. It may not draw anything, it may draw green underlines, it may draw yellow highlight. 2) "Red underline" is the name of a spell checking web server. In retrospect, we've chosen that name poorly. Let's avoid any further confusion here, though. https://codereview.chromium.org/1275813002/diff/310001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/spellchecker/SpellCheckerSessionBridge.java (right): https://codereview.chromium.org/1275813002/diff/310001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/spellchecker/SpellCheckerSessionBridge.java:21: * JNI interface for native SpellCheckerSessionBridge to use Android's spellchecker. +1 space before *. https://codereview.chromium.org/1275813002/diff/310001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/spellchecker/SpellCheckerSessionBridge.java:25: private SpellCheckerSession mSpellCheckerSession; final https://codereview.chromium.org/1275813002/diff/310001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/spellchecker/SpellCheckerSessionBridge.java:27: private SpellCheckerSessionBridge(long nativeSpellCheckerSessionBridge) { Javadocs for all of the methods please? https://codereview.chromium.org/1275813002/diff/310001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/spellchecker/SpellCheckerSessionBridge.java:66: SuggestionsInfo currentSuggestion = result.getSuggestionsInfoAt(i); Inline "currentSuggestion", unless that contradicts some style. https://codereview.chromium.org/1275813002/diff/310001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/spellchecker/SpellCheckerSessionBridge.java:67: boolean isWordCorrect = (currentSuggestion.getSuggestionsAttributes() Inline "isWordCorrect", unless that contradicts some style. Shouldn't need round brackets around the bitwise AND operation. https://codereview.chromium.org/1275813002/diff/310001/chrome/browser/spellch... File chrome/browser/spellchecker/spellcheck_message_filter_platform.h (right): https://codereview.chromium.org/1275813002/diff/310001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_message_filter_platform.h:57: // Android-specific object used to poll the Android spellchecker for results. "poll" in software usually means something like this: while (true) { if (dataAvalable()) processData(); } You're not polling in this sense, which is confusing. Use the word "query" instead. No need "for results" either. It's clear why a spellchecker is queried. https://codereview.chromium.org/1275813002/diff/310001/chrome/browser/spellch... File chrome/browser/spellchecker/spellchecker_session_bridge_android.cc (right): https://codereview.chromium.org/1275813002/diff/310001/chrome/browser/spellch... chrome/browser/spellchecker/spellchecker_session_bridge_android.cc:14: SpellCheckerSessionBridge::SpellingRequest::SpellingRequest( Match the order of methods in the header and the source file. If I am reading the header and the source at the same time, it's jarring to scroll one window up and the second window down. :-\ This must come after "ProcessSpellCheckResults()". https://codereview.chromium.org/1275813002/diff/310001/chrome/browser/spellch... chrome/browser/spellchecker/spellchecker_session_bridge_android.cc:15: int new_route_id, Name your parameters the same in both header and source file. Don't worry, compilers and readers are smart enough to figure out what ": route_id(route_id)" means. https://codereview.chromium.org/1275813002/diff/310001/chrome/browser/spellch... chrome/browser/spellchecker/spellchecker_session_bridge_android.cc:36: if (java_object_.is_null()) { C++ style (unlike Java style) says do not put curly braces around a single line if statement. https://codereview.chromium.org/1275813002/diff/310001/chrome/browser/spellch... chrome/browser/spellchecker/spellchecker_session_bridge_android.cc:68: // sent to the renderer to be drawn as red underlines. It's obvious what this code is doing. Remove this comment. As a side note, what the renderer will do with the markers is its own business. :-) On a more serious note, the renderer might make the underlines gray or change from underline to highlight, for example. Also, "red underline" is the name of a web server that spellchecks for Chrome. In summary, let's not use "red underline" phrase in comments. https://codereview.chromium.org/1275813002/diff/310001/chrome/browser/spellch... chrome/browser/spellchecker/spellchecker_session_bridge_android.cc:86: // request and be run. It's clear what the code is doing. Remove this comment. https://codereview.chromium.org/1275813002/diff/310001/chrome/browser/spellch... File chrome/browser/spellchecker/spellchecker_session_bridge_android.h (right): https://codereview.chromium.org/1275813002/diff/310001/chrome/browser/spellch... chrome/browser/spellchecker/spellchecker_session_bridge_android.h:14: // Owned by the SpellCheckMessageFilterPlatform. This class is used to Ownership of this class is not something that this class controls. If ownership of this class will change, then no one will bother to update this comment. Please remove it. https://codereview.chromium.org/1275813002/diff/310001/chrome/browser/spellch... chrome/browser/spellchecker/spellchecker_session_bridge_android.h:18: // spellchecked, and finally processes those results and sends them to the You're using the Oxford comma in some places, but not in others. This can be jarring to some readers. Please pick one style and stick with it. https://codereview.chromium.org/1275813002/diff/310001/chrome/browser/spellch... chrome/browser/spellchecker/spellchecker_session_bridge_android.h:40: struct SpellingRequest { nit: You /could/ forward declare this to relieve the burden from the reader and save a few milliseconds during the compile time. Not too important, though. https://codereview.chromium.org/1275813002/diff/310001/chrome/browser/spellch... chrome/browser/spellchecker/spellchecker_session_bridge_android.h:42: ~SpellingRequest() {} Let's not inline destructors. Technically, inline destructors are OK for simple objects, but I have my doubts about simplicity of base::string16 and whatever else might creep into this object eventually.
https://codereview.chromium.org/1275813002/diff/310001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/spellchecker/SpellCheckerSessionBridge.java (right): https://codereview.chromium.org/1275813002/diff/310001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/spellchecker/SpellCheckerSessionBridge.java:21: * JNI interface for native SpellCheckerSessionBridge to use Android's spellchecker. Done. https://codereview.chromium.org/1275813002/diff/310001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/spellchecker/SpellCheckerSessionBridge.java:25: private SpellCheckerSession mSpellCheckerSession; Done. https://codereview.chromium.org/1275813002/diff/310001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/spellchecker/SpellCheckerSessionBridge.java:66: SuggestionsInfo currentSuggestion = result.getSuggestionsInfoAt(i); Done. https://codereview.chromium.org/1275813002/diff/310001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/spellchecker/SpellCheckerSessionBridge.java:67: boolean isWordCorrect = (currentSuggestion.getSuggestionsAttributes() Inlined. == has higher precedence than bitwise AND in Java, so I think I need to keep the parentheses to evaluate the AND before the equality operator. https://codereview.chromium.org/1275813002/diff/310001/chrome/browser/spellch... File chrome/browser/spellchecker/spellcheck_message_filter_platform.h (right): https://codereview.chromium.org/1275813002/diff/310001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_message_filter_platform.h:57: // Android-specific object used to poll the Android spellchecker for results. Agreed, done. https://codereview.chromium.org/1275813002/diff/310001/chrome/browser/spellch... File chrome/browser/spellchecker/spellchecker_session_bridge_android.cc (right): https://codereview.chromium.org/1275813002/diff/310001/chrome/browser/spellch... chrome/browser/spellchecker/spellchecker_session_bridge_android.cc:14: SpellCheckerSessionBridge::SpellingRequest::SpellingRequest( Good point, done. https://codereview.chromium.org/1275813002/diff/310001/chrome/browser/spellch... chrome/browser/spellchecker/spellchecker_session_bridge_android.cc:15: int new_route_id, Ah, I didn't know the compiler would be ok with that (I've since looked into the standard). I usually rely on the '_' at the end of member names, but IIRC struct members don't receive those. Anyway, changed! https://codereview.chromium.org/1275813002/diff/310001/chrome/browser/spellch... chrome/browser/spellchecker/spellchecker_session_bridge_android.cc:36: if (java_object_.is_null()) { Done. https://codereview.chromium.org/1275813002/diff/310001/chrome/browser/spellch... chrome/browser/spellchecker/spellchecker_session_bridge_android.cc:68: // sent to the renderer to be drawn as red underlines. Removed. https://codereview.chromium.org/1275813002/diff/310001/chrome/browser/spellch... chrome/browser/spellchecker/spellchecker_session_bridge_android.cc:86: // request and be run. Removed. https://codereview.chromium.org/1275813002/diff/310001/chrome/browser/spellch... File chrome/browser/spellchecker/spellchecker_session_bridge_android.h (right): https://codereview.chromium.org/1275813002/diff/310001/chrome/browser/spellch... chrome/browser/spellchecker/spellchecker_session_bridge_android.h:14: // Owned by the SpellCheckMessageFilterPlatform. This class is used to Done! Good point, thanks. https://codereview.chromium.org/1275813002/diff/310001/chrome/browser/spellch... chrome/browser/spellchecker/spellchecker_session_bridge_android.h:18: // spellchecked, and finally processes those results and sends them to the Reworded the comment a bit to avoid this. Thanks. https://codereview.chromium.org/1275813002/diff/310001/chrome/browser/spellch... chrome/browser/spellchecker/spellchecker_session_bridge_android.h:40: struct SpellingRequest { Let's leave it in here for now. https://codereview.chromium.org/1275813002/diff/310001/chrome/browser/spellch... chrome/browser/spellchecker/spellchecker_session_bridge_android.h:42: ~SpellingRequest() {} Alright, moved to the .cc file.
lgtm
The CQ bit was checked by dylanking@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from newt@chromium.org Link to the patchset: https://codereview.chromium.org/1275813002/#ps330001 (title: "Several style changes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1275813002/330001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1275813002/330001
Message was sent while issue was closed.
Committed patchset #18 (id:330001)
Message was sent while issue was closed.
Patchset 18 (id:??) landed as https://crrev.com/82c859eb3f9b56c35846f7e44e4f3aad94075ec4 Cr-Commit-Position: refs/heads/master@{#343962} |
