Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2896143002:
Move batchedit related logics into InputConnection (Closed)
|
Created:
3 years, 7 months ago by Changwan Ryu Modified:
3 years, 6 months ago Reviewers:
Ted C CC:
chromium-reviews, jdonnelly+watch_chromium.org, agrieve+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionMove batchedit related logics into InputConnection
This helps reduce the number of API calls between View related logic
and autocomplete logic, and thus makes the split easier.
Also removes mention of URL bar from comments in
AutocompleteEditText.java.
BUG=722908
Review-Url: https://codereview.chromium.org/2896143002
Cr-Commit-Position: refs/heads/master@{#478173}
Committed: https://chromium.googlesource.com/chromium/src/+/a5770d08b1a9c1c7157934f512714195396057fe
Patch Set 1 #Patch Set 2 : rebased #Patch Set 3 : test fixed in another CL and rebased #
Total comments: 2
Messages
Total messages: 23 (16 generated)
The CQ bit was checked by changwan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by changwan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by changwan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
changwan@chromium.org changed reviewers: + tedchoc@chromium.org
PTAL, I'm thinking of splitting view and the model as we discussed offline, and I think this helps because I can have batch edit related logic easily contained in the InputConnection and model. Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2896143002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/omnibox/AutocompleteEditText.java (right): https://codereview.chromium.org/2896143002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omnibox/AutocompleteEditText.java:46: private int mBatchEditNestCount; The thing I struggle with is that this moves more state/shared variables between the input connection and this class. It seems like that will make the split even harder. Is it possible to move more of these into the connection? Should we work towards making that a static class that doesn't muck with the internals? I guess I'm wondering where you think the split should end up happening as I think that would be good to get an agreement on before we start moving a ton of code.
https://codereview.chromium.org/2896143002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/omnibox/AutocompleteEditText.java (right): https://codereview.chromium.org/2896143002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omnibox/AutocompleteEditText.java:46: private int mBatchEditNestCount; Hmm.. I didn’t really think much about how to split InputConnection and the model so far. I’m planning to move most of the logic here into AutocompleteEditTextModel and AutocompleteEditTextModelBase (interface), and add a new model (SpannableAutocompleteEditTextModel) after that. And the only thing I had in mind was to split between View and model where I just wanted to have as few number of methods called between them as possible. I’m not planning to invest refactoring efforts in the old model, but I’ll try to make sure that the new model has static InputConnection. Is this ok?
Another rationale I found for moving this logic into InputConnection is due to #2 Span approach of the design doc. I need to call the methods before super.endBatchEdit() is called, and this cannot be easily achieved with View#onEndBatchEdit().
lgtm
The CQ bit was checked by changwan@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1496969899175860,
"parent_rev": "685631bcd8b90950498e80f74009660ea5f75010", "commit_rev":
"a5770d08b1a9c1c7157934f512714195396057fe"}
Message was sent while issue was closed.
Description was changed from ========== Move batchedit related logics into InputConnection This helps reduce the number of API calls between View related logic and autocomplete logic, and thus makes the split easier. Also removes mention of URL bar from comments in AutocompleteEditText.java. BUG=722908 ========== to ========== Move batchedit related logics into InputConnection This helps reduce the number of API calls between View related logic and autocomplete logic, and thus makes the split easier. Also removes mention of URL bar from comments in AutocompleteEditText.java. BUG=722908 Review-Url: https://codereview.chromium.org/2896143002 Cr-Commit-Position: refs/heads/master@{#478173} Committed: https://chromium.googlesource.com/chromium/src/+/a5770d08b1a9c1c7157934f51271... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/a5770d08b1a9c1c7157934f51271... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
