Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2407303005:
Let embedder provide select action mode (Closed)
|
Created:
4 years, 2 months ago by Jinsuk Kim Modified:
4 years, 1 month ago CC:
agrieve+watch_chromium.org, android-webview-reviews_chromium.org, chromium-reviews, darin-cc_chromium.org, donnd+watch_chromium.org, jam, jochen+watch_chromium.org, mdjones+watch_chromium.org, mlamouri+watch-content_chromium.org, Peter Beverloo Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionLet embedder provide select action mode
This CL tries to simplify selection action mode
configuration being done through ContentViewClient that
will be gone after refactoring. Now Embedders provide with
a customized ActionMode.Callback. This effectively
moves the relevant implementation details from
ContentViewClient to the callback.
SelectionPopupController pulls out all the select
action mode-related methods and variables from
ContentViewCore and encapsulates them. This also helps
break up ContentViewCore.
BUG=623783
Committed: https://crrev.com/1eef196331dcff4093afabf4d4b5507fd84dfa94
Cr-Commit-Position: refs/heads/master@{#431299}
Patch Set 1 #
Total comments: 16
Patch Set 2 : WebActionMode #
Total comments: 4
Patch Set 3 : onSelectionEvent #Patch Set 4 : fix tests #
Total comments: 19
Patch Set 5 : fix tests #Patch Set 6 : WebActionModeHandler #Patch Set 7 : Inheriting ActionMode.Callback #
Total comments: 35
Patch Set 8 : addressed comments #Patch Set 9 : Move FloatingPaste into WebActionMode #
Total comments: 20
Patch Set 10 : addressed comments #
Total comments: 12
Patch Set 11 : comments/bug fixes #
Total comments: 30
Patch Set 12 : more comments addressed #
Total comments: 26
Patch Set 13 : more comments addressed #
Total comments: 39
Patch Set 14 : content_public/ActionModeCallbackHelper #
Total comments: 14
Patch Set 15 : SelectionPopupController #Patch Set 16 : static sanitizeQuery #
Total comments: 6
Patch Set 17 : addressed comments chrome/webview #
Total comments: 4
Patch Set 18 : mTab.isIncognito #Patch Set 19 : fixing tests #
Total comments: 2
Messages
Total messages: 130 (59 generated)
The CQ bit was checked by jinsukkim@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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by jinsukkim@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 checked by jinsukkim@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...
Patchset #5 (id:80001) has been deleted
Patchset #4 (id:60001) has been deleted
Patchset #3 (id:40001) has been deleted
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Description was changed from ========== WIP: Embedder provide select action mode BUG=623783 ========== to ========== Embedder provide select action mode This CL tries to simplify selection action mode configuration being done through ContentViewClient that will be gone after refactoring. Now Embedders provide with a customized WebActionMode and the associated WebActionModeHandler implementation by inheriting WebActionModeManager introduced here. This effectively moves the relevant implementation details from ContentViewClient to the action mode handler. WebActionModeManager pulls out all the select action mode-related methods and variables from ContentViewCore and encapsulates them. This also helps break up ContentViewCore. BUG=623783 ==========
Description was changed from ========== Embedder provide select action mode This CL tries to simplify selection action mode configuration being done through ContentViewClient that will be gone after refactoring. Now Embedders provide with a customized WebActionMode and the associated WebActionModeHandler implementation by inheriting WebActionModeManager introduced here. This effectively moves the relevant implementation details from ContentViewClient to the action mode handler. WebActionModeManager pulls out all the select action mode-related methods and variables from ContentViewCore and encapsulates them. This also helps break up ContentViewCore. BUG=623783 ========== to ========== Embedder provide select action mode This CL tries to simplify selection action mode configuration being done through ContentViewClient that will be gone after refactoring. Now Embedders provide with a customized WebActionMode and the associated WebActionModeHandler implementation by inheriting WebActionModeManager introduced here. This effectively moves the relevant implementation details from ContentViewClient to the action mode handler. WebActionModeManager pulls out all the select action mode- related methods and variables from ContentViewCore and encapsulates them. This also helps break up ContentViewCore. BUG=623783 ==========
Description was changed from ========== Embedder provide select action mode This CL tries to simplify selection action mode configuration being done through ContentViewClient that will be gone after refactoring. Now Embedders provide with a customized WebActionMode and the associated WebActionModeHandler implementation by inheriting WebActionModeManager introduced here. This effectively moves the relevant implementation details from ContentViewClient to the action mode handler. WebActionModeManager pulls out all the select action mode- related methods and variables from ContentViewCore and encapsulates them. This also helps break up ContentViewCore. BUG=623783 ========== to ========== Let embedder provide select action mode This CL tries to simplify selection action mode configuration being done through ContentViewClient that will be gone after refactoring. Now Embedders provide with a customized WebActionMode and the associated WebActionModeHandler implementation by inheriting WebActionModeManager introduced here. This effectively moves the relevant implementation details from ContentViewClient to the action mode handler. WebActionModeManager pulls out all the select action mode- related methods and variables from ContentViewCore and encapsulates them. This also helps break up ContentViewCore. BUG=623783 ==========
jinsukkim@chromium.org changed reviewers: + boliu@chromium.org
The intention is also to group all the stuff related with select action mode under WebActionManager. There are some rough edges but the basic idea is in place. I'll add missing Javadocs, etc, soon. Note that I removed the interface methods onContextualActionBarShown/Hidden, since the default is no-op, and needed only by Chrome. Moved them to ChromeWebActionModeActionHandler.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I'll make sure to review this by monday your time, so you have something to do. Hopefully you didn't plan on working on the weekend :p
I didn't look at chrome, and only skimmed android_webview. Read content more thoroughly. I generally like the direction. A few high level comments. But I haven't thought all of them through so maybe not all of them are practical. * Embedder overrides two classes. That seems odd. Is it possible to override only one class? I'm not sure if that's entirely possible (without duplicating a lot of code), but should check. * Specifically for WebActionModeManager, is it possible to do composition over inheritance? So embedder overrides a pure interface, but is passed a default/shared implementation object that it can call for most things. If that's too much boilerplate, then marking everything not meant to be overridden in WebActionModeManager as final is also ok. * Looks like WebActionMode and WebActionModeCallback are implementation details in content. Given the amount of implementation details that's spread among 4 classes already, which makes it super hard to read. I wonder if it's possible to do away with those two abstractions altogether, ie just fold WebActionMode/WebActionModeCallback into Manager/Handler, and then simplify as needed. * Can CVC not hold mActionMode at all? Instead of exposing the individual states in Manager, CVC can just forward the required calls, like onSelectionEvent, into Manager? https://codereview.chromium.org/2407303005/diff/100001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2407303005/diff/100001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1851: if (left == 0 && top == 0 && right == 0 && bottom == 0) { these two things does the exact same thing, no need for the branch https://codereview.chromium.org/2407303005/diff/100001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/WebActionModeHandler.java (right): https://codereview.chromium.org/2407303005/diff/100001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionModeHandler.java:24: * Base implementation of {@link WebActionModeCallback.ActionHandler} that has common bikesed: then maybe WebActionModeHandler*Base*? https://codereview.chromium.org/2407303005/diff/100001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionModeHandler.java:198: protected String sanitizeQuery(String query, int maxLength) { static? https://codereview.chromium.org/2407303005/diff/100001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionModeHandler.java:229: public static class EmptyHandler implements WebActionModeCallback.ActionHandler { private? https://codereview.chromium.org/2407303005/diff/100001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/WebActionModeManager.java (right): https://codereview.chromium.org/2407303005/diff/100001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionModeManager.java:26: public class WebActionModeManager { Should this also be the class that deals with ActionMode, so the CVC doesn't have to? https://codereview.chromium.org/2407303005/diff/100001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionModeManager.java:61: public WebActionMode create(WebContents webContents, ViewGroup containerView, s/ViewGroup/View/ everywhere https://codereview.chromium.org/2407303005/diff/100001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionModeManager.java:68: protected WebActionMode createInternal(WebContents webContents, ViewGroup containerView, If this method is not meant to be overridden, mark it final? https://codereview.chromium.org/2407303005/diff/100001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionModeManager.java:132: public void setUnselectAllOnDismiss(boolean unselectAll) { again, methods that are not meant to be overridden should be marked final
Appreciating the review in time.... On 2016/10/16 22:22:31, boliu wrote: > I didn't look at chrome, and only skimmed android_webview. Read content more > thoroughly. I generally like the direction. > > A few high level comments. But I haven't thought all of them through so maybe > not all of them are practical. > > * Embedder overrides two classes. That seems odd. Is it possible to override > only one class? I'm not sure if that's entirely possible (without duplicating a > lot of code), but should check. > I merged WebActionModeManager/WebActionMode and let WebActionMode directly implement ActionMode.Callback so WebActionModeCallbac/ActionHandler won't be necessary. In this way embedders only needs to override WebActionMode only. > * Specifically for WebActionModeManager, is it possible to do composition over > inheritance? So embedder overrides a pure interface, but is passed a > default/shared implementation object that it can call for most things. If that's > too much boilerplate, then marking everything not meant to be overridden in > WebActionModeManager as final is also ok. > Tried a few options and ended up with the approach above. Let me know what you think. > * Looks like WebActionMode and WebActionModeCallback are implementation details > in content. Given the amount of implementation details that's spread among 4 > classes already, which makes it super hard to read. I wonder if it's possible to > do away with those two abstractions altogether, ie just fold > WebActionMode/WebActionModeCallback into Manager/Handler, and then simplify as > needed. > What I did tries to address this feedback as well. > * Can CVC not hold mActionMode at all? Instead of exposing the individual states > in Manager, CVC can just forward the required calls, like onSelectionEvent, into > Manager? Now mActionMode CVC holds is of type WebActionMode as Manager is merged together with it. Moving onSelectionEvent entirely sounds plausible. Will try it in the next patch.
With manager class gone, I chose to replace the method |create()| that creates a new instance of WebActionMode with |initialize()| so that WebActionMode will be ready only when the method is called, like how CVC works. Please see if the overall change (merging of WebActionMode* classes) looks okay first. I'll take care of the other issues such as: - Null-checking against |mWebActionMode| in CVC needs updating, since it now plays the role of WebActionModeManager as well, which is not supposed to be null. - public/private scope of methods, final attr for some public methods are not thoroughly checked. https://codereview.chromium.org/2407303005/diff/100001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2407303005/diff/100001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1851: if (left == 0 && top == 0 && right == 0 && bottom == 0) { On 2016/10/16 22:22:30, boliu wrote: > these two things does the exact same thing, no need for the branch Done. https://codereview.chromium.org/2407303005/diff/100001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/WebActionModeHandler.java (right): https://codereview.chromium.org/2407303005/diff/100001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionModeHandler.java:24: * Base implementation of {@link WebActionModeCallback.ActionHandler} that has common On 2016/10/16 22:22:31, boliu wrote: > bikesed: then maybe WebActionModeHandler*Base*? You're not bikeshedding 'cause I also thought about it. Now the class is gone. https://codereview.chromium.org/2407303005/diff/100001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionModeHandler.java:198: protected String sanitizeQuery(String query, int maxLength) { On 2016/10/16 22:22:30, boliu wrote: > static? Done in the new class it was moved to. (WebActionMode) https://codereview.chromium.org/2407303005/diff/100001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionModeHandler.java:229: public static class EmptyHandler implements WebActionModeCallback.ActionHandler { On 2016/10/16 22:22:30, boliu wrote: > private? ditto https://codereview.chromium.org/2407303005/diff/100001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/WebActionModeManager.java (right): https://codereview.chromium.org/2407303005/diff/100001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionModeManager.java:26: public class WebActionModeManager { On 2016/10/16 22:22:31, boliu wrote: > Should this also be the class that deals with ActionMode, so the CVC doesn't > have to? Merged with WebActionMode in the new patch, and now CVC has a reference to WebActionMode instead (mActionMode). https://codereview.chromium.org/2407303005/diff/100001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionModeManager.java:61: public WebActionMode create(WebContents webContents, ViewGroup containerView, On 2016/10/16 22:22:31, boliu wrote: > s/ViewGroup/View/ everywhere Done. https://codereview.chromium.org/2407303005/diff/100001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionModeManager.java:68: protected WebActionMode createInternal(WebContents webContents, ViewGroup containerView, On 2016/10/16 22:22:31, boliu wrote: > If this method is not meant to be overridden, mark it final? Done (there are some still left unmarked. Will do them all soon). https://codereview.chromium.org/2407303005/diff/100001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionModeManager.java:132: public void setUnselectAllOnDismiss(boolean unselectAll) { On 2016/10/16 22:22:31, boliu wrote: > again, methods that are not meant to be overridden should be marked final ditto
sorry, busy these days, two high level comments https://codereview.chromium.org/2407303005/diff/120001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/WebActionMode.java (right): https://codereview.chromium.org/2407303005/diff/120001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:85: private boolean mEditable; next step is probably simplify some of these states, like can we get rid of one of mEditable vs mFocusedNodeEditable? https://codereview.chromium.org/2407303005/diff/120001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:151: public final void showActionMode(boolean allowFallback) { methods such as this one is only called by ContentViewCore, right? then can we mark them as package visible I think we need to make a clear distinction which methods are allowed to be overridden by embedder, and which methods shouldn't be visible at all
Patchset #3 (id:140001) has been deleted
Also moved onSelectionEvent and PastePopupMenu that are closely related to the events. Will look into broken tests. https://codereview.chromium.org/2407303005/diff/120001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/WebActionMode.java (right): https://codereview.chromium.org/2407303005/diff/120001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:85: private boolean mEditable; On 2016/10/18 04:04:12, boliu wrote: > next step is probably simplify some of these states, like can we get rid of one > of mEditable vs mFocusedNodeEditable? While I could get rid of a few duplicated methods, mEditable may have to stay. It caches the old value for ActionMode.Callback callbacks to compare the current value against it. Renamed it to mEditableCached to make its meaning clear. Same for mIsPasswordType/mIsInsertion. https://codereview.chromium.org/2407303005/diff/120001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:151: public final void showActionMode(boolean allowFallback) { On 2016/10/18 04:04:12, boliu wrote: > methods such as this one is only called by ContentViewCore, right? then can we > mark them as package visible > > I think we need to make a clear distinction which methods are allowed to be > overridden by embedder, and which methods shouldn't be visible at all Makes sense. Done.
Description was changed from ========== Let embedder provide select action mode This CL tries to simplify selection action mode configuration being done through ContentViewClient that will be gone after refactoring. Now Embedders provide with a customized WebActionMode and the associated WebActionModeHandler implementation by inheriting WebActionModeManager introduced here. This effectively moves the relevant implementation details from ContentViewClient to the action mode handler. WebActionModeManager pulls out all the select action mode- related methods and variables from ContentViewCore and encapsulates them. This also helps break up ContentViewCore. BUG=623783 ========== to ========== Let embedder provide select action mode This CL tries to simplify selection action mode configuration being done through ContentViewClient that will be gone after refactoring. Now Embedders provide with a customized WebActionMode. This effectively moves the relevant implementation details from ContentViewClient to the action mode handler. WebActionMode pulls out all the select action mode- related methods and variables from ContentViewCore and encapsulates them. This also helps break up ContentViewCore. BUG=623783 ==========
Patchset #4 (id:180001) has been deleted
I'll have more time tomorrow to give this a more thorough look. I generally like unifying everything action mode under one file, but I was kinda hoping that one file could be simplified a bit more :p https://codereview.chromium.org/2407303005/diff/200001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/FloatingWebActionModeCallback.java (right): https://codereview.chromium.org/2407303005/diff/200001/content/public/android... content/public/android/java/src/org/chromium/content/browser/FloatingWebActionModeCallback.java:21: private final WebActionMode mActionMode; ActionMode.Callback? https://codereview.chromium.org/2407303005/diff/200001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/WebActionMode.java (right): https://codereview.chromium.org/2407303005/diff/200001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:53: public class WebActionMode implements ActionMode.Callback { sorry for the back and forth.. but I have like another idea, but it's sort of going back and unsplitting this a bit Let's not make this class inherit ActionMode.Callback. Embedder directly implement ActionMode.Callback, but the implementation are passed a reference to this class. So this class can still contain all the state and default implementations. It's just embedders are composing this class rather than subclassing it. So the classic "composition over inheritance" rule of thumb. Does that work? I think FloatingWebActionModeCallback should still work as is?
https://codereview.chromium.org/2407303005/diff/200001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/FloatingWebActionModeCallback.java (right): https://codereview.chromium.org/2407303005/diff/200001/content/public/android... content/public/android/java/src/org/chromium/content/browser/FloatingWebActionModeCallback.java:21: private final WebActionMode mActionMode; On 2016/10/19 03:45:55, boliu wrote: > ActionMode.Callback? ActionMode.Callback doesn't have |onGetContentRect| which is a new addition to Callback2. Previously WebActionModeCallback was used here to bridge the call to WebActionModeCallback.ActionHandler. WebActionMode is taking the role in this CL. https://codereview.chromium.org/2407303005/diff/200001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/WebActionMode.java (right): https://codereview.chromium.org/2407303005/diff/200001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:53: public class WebActionMode implements ActionMode.Callback { On 2016/10/19 03:45:55, boliu wrote: > sorry for the back and forth.. but I have like another idea, but it's sort of > going back and unsplitting this a bit > > Let's not make this class inherit ActionMode.Callback. Embedder directly > implement ActionMode.Callback, but the implementation are passed a reference to > this class. So this class can still contain all the state and default > implementations. It's just embedders are composing this class rather than > subclassing it. So the classic "composition over inheritance" rule of thumb. > > Does that work? I think FloatingWebActionModeCallback should still work as is? Kinda get your point. I *think* your suggestion works in this way: class WebActionModeHandler { /* new class */ public copy() public processText() public search()... etc. } class ChromeWebActionModeHandler { /* renamed from ChromeWebActionMode */ @Override processText() { // chrome-specific task for processing text } ... } class WebActionMode (still) implements ActionMode.Callback { WebActionModeHander mHandler; @override onCreationActionMode() // ActionMode.Callback ... void setHandler(WebActionModeHandler handler) { mHandler = handler } void workworkwork() { mHandler.processText(); } ... } Is this understanding correct? It will mostly work - one problem would be that WebActionMode not only uses WebActionModeHandler but also the handler uses WebActionMode now. Then it wouldn't go along well with the idea of embedder providing handler only, since WebActionMode is not being exposed outside CVC. They need to cross-reference each other or a delegate needs to be set when a handler is set to the mode, which doesn't look nice. OTH, Embedder could directly implement ActionMode.Callback like you said, but I"m afraid it would end up having too much duplicated code. WebActionMode is there to absorb those common parts. It would be better to keep them somewhere (preferably in WebActionMode like they are now).
> It will mostly work - one problem would be that WebActionMode not only uses > WebActionModeHandler but also the handler uses WebActionMode now. Then it > wouldn't go along well with the idea of embedder providing handler only, since > WebActionMode is not being exposed outside CVC. They need to cross-reference > each other or a delegate needs to be set when a handler is set to the mode, > which doesn't look nice. Actually I'm wrong... it'll be possible by moving nearly the all stuff from WebActionMode to WebActionHandler, leaving the class as it used to be, and letting it forward calls from CVC to handler by composition.
On 2016/10/19 05:41:53, Jinsuk Kim wrote: > https://codereview.chromium.org/2407303005/diff/200001/content/public/android... > File > content/public/android/java/src/org/chromium/content/browser/FloatingWebActionModeCallback.java > (right): > > https://codereview.chromium.org/2407303005/diff/200001/content/public/android... > content/public/android/java/src/org/chromium/content/browser/FloatingWebActionModeCallback.java:21: > private final WebActionMode mActionMode; > On 2016/10/19 03:45:55, boliu wrote: > > ActionMode.Callback? > > ActionMode.Callback doesn't have |onGetContentRect| which is a new addition to > Callback2. Previously WebActionModeCallback was used here to bridge the call to > WebActionModeCallback.ActionHandler. WebActionMode is taking the role in this > CL. > > https://codereview.chromium.org/2407303005/diff/200001/content/public/android... > File > content/public/android/java/src/org/chromium/content/browser/WebActionMode.java > (right): > > https://codereview.chromium.org/2407303005/diff/200001/content/public/android... > content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:53: > public class WebActionMode implements ActionMode.Callback { > On 2016/10/19 03:45:55, boliu wrote: > > sorry for the back and forth.. but I have like another idea, but it's sort of > > going back and unsplitting this a bit > > > > Let's not make this class inherit ActionMode.Callback. Embedder directly > > implement ActionMode.Callback, but the implementation are passed a reference > to > > this class. So this class can still contain all the state and default > > implementations. It's just embedders are composing this class rather than > > subclassing it. So the classic "composition over inheritance" rule of thumb. > > > > Does that work? I think FloatingWebActionModeCallback should still work as is? > > Kinda get your point. I *think* your suggestion works in this way: > > class WebActionModeHandler { /* new class */ > public copy() > public processText() > public search()... etc. > } > > class ChromeWebActionModeHandler { /* renamed from ChromeWebActionMode */ > @Override processText() { > // chrome-specific task for processing text > } > ... > } > > class WebActionMode (still) implements ActionMode.Callback { > WebActionModeHander mHandler; > > @override onCreationActionMode() // ActionMode.Callback > ... > > void setHandler(WebActionModeHandler handler) { mHandler = handler } > > void workworkwork() { > mHandler.processText(); > } > ... > } > > Is this understanding correct? Not quite. So going to back to state of things in PS4. WebActionMode would *not* inherit from anything. CVC still calls into WebActionMode (through package visible APIs), and WebActionMode holds an ActionMode.Callback, implemented by the embedder. The in embedder, ChromeWebActionMode directly implements ActionMode.Callback. ChromeWebActionMode is also given a reference to WebActionMode (perhaps under a different interface, not sure..). Then WebActionMode provides the default implementation if ChromeWebActionMode doesn't need specialization, and provide information about the action mode, like if it's floating, or it's text or whatnot. Basically composition over inheritance
On 2016/10/19 21:33:05, boliu wrote: > On 2016/10/19 05:41:53, Jinsuk Kim wrote: > > > https://codereview.chromium.org/2407303005/diff/200001/content/public/android... > > File > > > content/public/android/java/src/org/chromium/content/browser/FloatingWebActionModeCallback.java > > (right): > > > > > https://codereview.chromium.org/2407303005/diff/200001/content/public/android... > > > content/public/android/java/src/org/chromium/content/browser/FloatingWebActionModeCallback.java:21: > > private final WebActionMode mActionMode; > > On 2016/10/19 03:45:55, boliu wrote: > > > ActionMode.Callback? > > > > ActionMode.Callback doesn't have |onGetContentRect| which is a new addition to > > Callback2. Previously WebActionModeCallback was used here to bridge the call > to > > WebActionModeCallback.ActionHandler. WebActionMode is taking the role in this > > CL. > > > > > https://codereview.chromium.org/2407303005/diff/200001/content/public/android... > > File > > > content/public/android/java/src/org/chromium/content/browser/WebActionMode.java > > (right): > > > > > https://codereview.chromium.org/2407303005/diff/200001/content/public/android... > > > content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:53: > > public class WebActionMode implements ActionMode.Callback { > > On 2016/10/19 03:45:55, boliu wrote: > > > sorry for the back and forth.. but I have like another idea, but it's sort > of > > > going back and unsplitting this a bit > > > > > > Let's not make this class inherit ActionMode.Callback. Embedder directly > > > implement ActionMode.Callback, but the implementation are passed a reference > > to > > > this class. So this class can still contain all the state and default > > > implementations. It's just embedders are composing this class rather than > > > subclassing it. So the classic "composition over inheritance" rule of thumb. > > > > > > Does that work? I think FloatingWebActionModeCallback should still work as > is? > > > > Kinda get your point. I *think* your suggestion works in this way: > > > > class WebActionModeHandler { /* new class */ > > public copy() > > public processText() > > public search()... etc. > > } > > > > class ChromeWebActionModeHandler { /* renamed from ChromeWebActionMode */ > > @Override processText() { > > // chrome-specific task for processing text > > } > > ... > > } > > > > class WebActionMode (still) implements ActionMode.Callback { > > WebActionModeHander mHandler; > > > > @override onCreationActionMode() // ActionMode.Callback > > ... > > > > void setHandler(WebActionModeHandler handler) { mHandler = handler } > > > > void workworkwork() { > > mHandler.processText(); > > } > > ... > > } > > > > Is this understanding correct? > > Not quite. > > So going to back to state of things in PS4. WebActionMode would *not* inherit > from anything. Well, would *not* inherit from ActionMode.Callback I think. > CVC still calls into WebActionMode (through package visible > APIs), and WebActionMode holds an ActionMode.Callback, implemented by the > embedder. > > The in embedder, ChromeWebActionMode directly implements ActionMode.Callback. > ChromeWebActionMode is also given a reference to WebActionMode (perhaps under a > different interface, not sure..). Then WebActionMode provides the default > implementation if ChromeWebActionMode doesn't need specialization, and provide > information about the action mode, like if it's floating, or it's text or > whatnot. > > Basically composition over inheritance
so I'm still reviewing PS4, since it looks like PS6 went in the wrong direction.. Definitely did not finish reading the whole thing line by line but left a bunch of more concrete comments :) https://codereview.chromium.org/2407303005/diff/200001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2407303005/diff/200001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1305: if (unselectAllOnDismiss() && mWebContents != null) mWebContents.dismissTextHandles(); this should just be mActionMode.hidePopups(), and then can remove a bunch of cruft, eg no longer need to expose "unselectAllOnDismiss" value there are probably more cases like this: hasSelection https://codereview.chromium.org/2407303005/diff/200001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2011: boolean actionModeConfigurationChanged = all of this code can just move to WebActionMode, then this becomes mActionMode.updateImeAdapter(...), or maybe pick a more sensible name than updateImeAdapter, but you get the idea then I think a lot of state don't need to exposed by WebActionMode https://codereview.chromium.org/2407303005/diff/200001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/WebActionMode.java (right): https://codereview.chromium.org/2407303005/diff/200001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:85: private View mView; this match CVC mContainerView, which *can change*, so need to handle that here as well also mView should never be allowed to be null (after initialization), so hopefully all the null checks aren't necessary https://codereview.chromium.org/2407303005/diff/200001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:89: private boolean mHidden; mHidden always equals mDraggingSelection || touchScrollInProgress, so can be dropped entirely https://codereview.chromium.org/2407303005/diff/200001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:98: private boolean mIsDestroyed; this is equivalent to mActionMode != null https://codereview.chromium.org/2407303005/diff/200001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:111: private boolean mEditableCached; so looks like this *cached variables exist purely to detect changes, to decide whether the menu needs to be rebuilt? we can collapse the 3 into a single bool mNeedsPrepare or something like that? https://codereview.chromium.org/2407303005/diff/200001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:287: public void onDestroy() { hmm... "hideActionMode", to match "showActionMode"? onDestroy in other places generally means this object is not coming back, and that's not the case here (I hope this is not supposed to be an @Override method..)
Description was changed from ========== Let embedder provide select action mode This CL tries to simplify selection action mode configuration being done through ContentViewClient that will be gone after refactoring. Now Embedders provide with a customized WebActionMode. This effectively moves the relevant implementation details from ContentViewClient to the action mode handler. WebActionMode pulls out all the select action mode- related methods and variables from ContentViewCore and encapsulates them. This also helps break up ContentViewCore. BUG=623783 ========== to ========== Let embedder provide select action mode This CL tries to simplify selection action mode configuration being done through ContentViewClient that will be gone after refactoring. Now Embedders provide with a customized ActionMode.Callback. This effectively moves the relevant implementation details from ContentViewClient to the callback. WebActionMode pulls out all the select action mode- related methods and variables from ContentViewCore and encapsulates them. This also helps break up ContentViewCore. BUG=623783 ==========
Patchset #7 (id:260001) has been deleted
Patchset #7 (id:280001) has been deleted
Patchset #7 (id:300001) has been deleted
PTAL https://codereview.chromium.org/2407303005/diff/200001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2407303005/diff/200001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1305: if (unselectAllOnDismiss() && mWebContents != null) mWebContents.dismissTextHandles(); On 2016/10/19 22:55:19, boliu wrote: > this should just be mActionMode.hidePopups(), and then can remove a bunch of > cruft, eg no longer need to expose "unselectAllOnDismiss" value > > there are probably more cases like this: > hasSelection Done. hasSelection/isInsertion are used by tests. https://codereview.chromium.org/2407303005/diff/200001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2011: boolean actionModeConfigurationChanged = On 2016/10/19 22:55:19, boliu wrote: > all of this code can just move to WebActionMode, then this becomes > mActionMode.updateImeAdapter(...), or maybe pick a more sensible name than > updateImeAdapter, but you get the idea > > then I think a lot of state don't need to exposed by WebActionMode Done. Removed isFocusedNodePassword but isFocusedNodeEditable is also used by other classes including tests. https://codereview.chromium.org/2407303005/diff/200001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/WebActionMode.java (right): https://codereview.chromium.org/2407303005/diff/200001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:85: private View mView; On 2016/10/19 22:55:19, boliu wrote: > this match CVC mContainerView, which *can change*, so need to handle that here > as well > > also mView should never be allowed to be null (after initialization), so > hopefully all the null checks aren't necessary Done. https://codereview.chromium.org/2407303005/diff/200001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:89: private boolean mHidden; On 2016/10/19 22:55:19, boliu wrote: > mHidden always equals mDraggingSelection || touchScrollInProgress, so can be > dropped entirely It is being referenced in invalidation-related methods. To remove it I think I need to have mTouchScrollInProgress instead. Using mHidden would be handy though. https://codereview.chromium.org/2407303005/diff/200001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:98: private boolean mIsDestroyed; On 2016/10/19 22:55:19, boliu wrote: > this is equivalent to mActionMode != null Done. https://codereview.chromium.org/2407303005/diff/200001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:111: private boolean mEditableCached; On 2016/10/19 22:55:19, boliu wrote: > so looks like this *cached variables exist purely to detect changes, to decide > whether the menu needs to be rebuilt? > > we can collapse the 3 into a single bool mNeedsPrepare or something like that? I'm not clear about how it can be done. Would you elaborate (with code snippet if possible)? https://codereview.chromium.org/2407303005/diff/200001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:287: public void onDestroy() { On 2016/10/19 22:55:19, boliu wrote: > hmm... "hideActionMode", to match "showActionMode"? onDestroy in other places > generally means this object is not coming back, and that's not the case here > > (I hope this is not supposed to be an @Override method..) Removed the method.
https://codereview.chromium.org/2407303005/diff/200001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/WebActionMode.java (right): https://codereview.chromium.org/2407303005/diff/200001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:111: private boolean mEditableCached; On 2016/10/20 10:44:10, Jinsuk Kim wrote: > On 2016/10/19 22:55:19, boliu wrote: > > so looks like this *cached variables exist purely to detect changes, to decide > > whether the menu needs to be rebuilt? > > > > we can collapse the 3 into a single bool mNeedsPrepare or something like that? > > I'm not clear about how it can be done. Would you elaborate (with code snippet > if possible)? Instead of *Cached, have a mNeedsPrepare Each time any of the state is modified, set mNeedsPrepare to true, then in onPrepareActionMode, if mNeedsPrepare is true, do things, and then set it back to false
https://codereview.chromium.org/2407303005/diff/320001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2407303005/diff/320001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1278: private void hidePopupsAndClearSelection() { this method should live on WebActionMode, ditto for hidePopupsAndPreserveSelection, and then remove setUnselectAllOnDismiss https://codereview.chromium.org/2407303005/diff/320001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1815: mActionMode.initialize(mWebContents, mRenderCoordinates); * I think CVC has all the constructor params, so can we have CVC create WebActionMode as well? * Can we just fold the initialize method into the constructor? It doesn't look that involved to me to warrant doing it lazily * maybe I think you can directly pass in the callback into CVC.initialize as well, and fold setCallback into a single constructor as well https://codereview.chromium.org/2407303005/diff/320001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/WebActionMode.java (right): https://codereview.chromium.org/2407303005/diff/320001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:100: private boolean mEditableCached; reminder the *Cached variables can just be collasped to a single boolean https://codereview.chromium.org/2407303005/diff/320001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:110: protected int mActionModeType; nothing uses this, delete? https://codereview.chromium.org/2407303005/diff/320001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:115: private String mLastSelectedText; this can continue living on CVC, nothing here actually uses it, and CVC already knows about hasSelection https://codereview.chromium.org/2407303005/diff/320001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:149: mView = view; a few things to clean up before releasing the old view: * mView.removeCallbacks * destroyPastePopup (I think?) * onDestroyActionMode (I think?) https://codereview.chromium.org/2407303005/diff/320001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:196: public boolean isEmpty() { should just do this check at the beginning of showActionMode and just early out, looks CVC is already set up to handle showActionMode failing https://codereview.chromium.org/2407303005/diff/320001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:204: public interface MenuChecker { can the embedder just pass in a static list of allowed menu items. also embedder doesn't need to be involved with dynamically checking for package at all (checkAllowedForShareAndSearchMenu) https://codereview.chromium.org/2407303005/diff/320001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:530: public void onDestroyActionMode(ActionMode mode) { why does this take a parameter? https://codereview.chromium.org/2407303005/diff/320001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:863: if (mPastePopupMenu != null) return mPastePopupMenu.isShowing(); nit: return mPastePopupMenu != null && mPastePopupMenu.isShowing(); https://codereview.chromium.org/2407303005/diff/320001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:928: public static ActionMode.Callback defaultCallback(final WebActionMode actionMode) { only Shell.java calls this, just move this there? https://codereview.chromium.org/2407303005/diff/320001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:955: public static WebActionMode empty(Context context) { createEmpty https://codereview.chromium.org/2407303005/diff/320001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:984: private static final EmptyActionCallback EMPTY_CALLBACK = new EmptyActionCallback(); private static finals are usually at the top of the class https://codereview.chromium.org/2407303005/diff/320001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/FloatingPastePopupMenu.java (right): https://codereview.chromium.org/2407303005/diff/320001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/FloatingPastePopupMenu.java:125: mActionMode = actionMode; I'm really confused here. WebActionMode creates this class, but then it goes on to create a *new* WebActionMode?
Patchset #8 (id:340001) has been deleted
Patchset #8 (id:360001) has been deleted
Thanks again for sparing extra time for reviewing. https://codereview.chromium.org/2407303005/diff/320001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2407303005/diff/320001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1278: private void hidePopupsAndClearSelection() { On 2016/10/23 19:56:48, boliu wrote: > this method should live on WebActionMode, ditto for > hidePopupsAndPreserveSelection, and then remove setUnselectAllOnDismiss I moved PastePopup stuff back to CVC (see my comment in FloatingPastePopupMenu). Then I think this method better stay. https://codereview.chromium.org/2407303005/diff/320001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1815: mActionMode.initialize(mWebContents, mRenderCoordinates); On 2016/10/23 19:56:48, boliu wrote: > * I think CVC has all the constructor params, so can we have CVC create > WebActionMode as well? Mm... *ActionModeCallback and WebActionMode has a reference to each other. I'm creating WebActionMode, passing it to *ActionModeCallback ctr, and using setCallback() next. If WebActionMode is to be created inside CVC, it should be the other way around - *ActionModeCallback needs to implement a method like setWebActionMode which the callbacks should somehow inherit from a new interface/class. But I take it that you prefer them directly inheriting ActionMode.Callback? If *ActionModeCallback inherits from, say, WebActionModeCallback that defines setWebActionMode() interface, it would begin to look somewhat like PS6. And the action callback for FloatingPastePopupMenu inherits not from ActionMode.Callback but from ActionMode.Callback2. With the new interface, it's not clear to me how the callback can fit in. > * Can we just fold the initialize method into the constructor? It doesn't look > that involved to me to warrant doing it lazily I guess you meant CVC.initialize()? Though not involved, that's how the unnecessary initialization of select action was avoided before the patch, which I tried to keep as is. Let me know if you really want this - will move it. > * maybe I think you can directly pass in the callback into CVC.initialize as > well, and fold setCallback into a single constructor as well https://codereview.chromium.org/2407303005/diff/320001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/WebActionMode.java (right): https://codereview.chromium.org/2407303005/diff/320001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:100: private boolean mEditableCached; On 2016/10/23 19:56:48, boliu wrote: > reminder the *Cached variables can just be collasped to a single boolean Done. https://codereview.chromium.org/2407303005/diff/320001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:110: protected int mActionModeType; On 2016/10/23 19:56:48, boliu wrote: > nothing uses this, delete? Done. https://codereview.chromium.org/2407303005/diff/320001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:115: private String mLastSelectedText; On 2016/10/23 19:56:48, boliu wrote: > this can continue living on CVC, nothing here actually uses it, and CVC already > knows about hasSelection share()/search()/processText() are using it via getSelectedText()...? https://codereview.chromium.org/2407303005/diff/320001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:149: mView = view; On 2016/10/23 19:56:48, boliu wrote: > a few things to clean up before releasing the old view: > > * mView.removeCallbacks > * destroyPastePopup (I think?) > * onDestroyActionMode (I think?) I think it is CVC has been doing necessary clean-up already. Or are you asking to handle them now because it is not done right? - Removing callbacks should be handled by call sites and whoever added runnables before entering here. Or maybe I just do view.getHandler().removeCallbacksAndMessage(null) here? - Paste popup is already being destroyed in CVC. - Action mode is already being destroyed via hideSelectActionMode. Changed the method name to clarify this. https://codereview.chromium.org/2407303005/diff/320001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:196: public boolean isEmpty() { On 2016/10/23 19:56:49, boliu wrote: > should just do this check at the beginning of showActionMode and just early out, > looks CVC is already set up to handle showActionMode failing I'm already calling this at the beginning of CVC.showSelectActionMode(). Do you mean to say it should be added here too? https://codereview.chromium.org/2407303005/diff/320001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:204: public interface MenuChecker { On 2016/10/23 19:56:49, boliu wrote: > can the embedder just pass in a static list of allowed menu items. also embedder > doesn't need to be involved with dynamically checking for package at all > (checkAllowedForShareAndSearchMenu) Done. https://codereview.chromium.org/2407303005/diff/320001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:530: public void onDestroyActionMode(ActionMode mode) { On 2016/10/23 19:56:48, boliu wrote: > why does this take a parameter? Removed it. https://codereview.chromium.org/2407303005/diff/320001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:863: if (mPastePopupMenu != null) return mPastePopupMenu.isShowing(); On 2016/10/23 19:56:48, boliu wrote: > nit: return mPastePopupMenu != null && mPastePopupMenu.isShowing(); Done. (moved back to CVC) https://codereview.chromium.org/2407303005/diff/320001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:928: public static ActionMode.Callback defaultCallback(final WebActionMode actionMode) { On 2016/10/23 19:56:48, boliu wrote: > only Shell.java calls this, just move this there? Done. https://codereview.chromium.org/2407303005/diff/320001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:955: public static WebActionMode empty(Context context) { On 2016/10/23 19:56:49, boliu wrote: > createEmpty Done. https://codereview.chromium.org/2407303005/diff/320001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:984: private static final EmptyActionCallback EMPTY_CALLBACK = new EmptyActionCallback(); On 2016/10/23 19:56:48, boliu wrote: > private static finals are usually at the top of the class Done. https://codereview.chromium.org/2407303005/diff/320001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/FloatingPastePopupMenu.java (right): https://codereview.chromium.org/2407303005/diff/320001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/FloatingPastePopupMenu.java:125: mActionMode = actionMode; On 2016/10/23 19:56:49, boliu wrote: > I'm really confused here. WebActionMode creates this class, but then it goes on > to create a *new* WebActionMode? I was confused too :( On second thought, I think it is better to move PastePopup stuff back to CVC. SelectAction and PastePopup are using their own Android.view.ActionMode, and it doesn't look right for PastePopup to reside inside WebActionMode and create its own WebActionMode in this class again. Also, Pedro is planning to unify PastePopup and SelectAction https://crrev.com/2390633003/ which is I think is nice. I'd rather leave the PastePopup stuff here and see it go away.
https://codereview.chromium.org/2407303005/diff/320001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2407303005/diff/320001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1815: mActionMode.initialize(mWebContents, mRenderCoordinates); On 2016/10/24 06:05:59, Jinsuk Kim wrote: > On 2016/10/23 19:56:48, boliu wrote: > > * I think CVC has all the constructor params, so can we have CVC create > > WebActionMode as well? > > Mm... *ActionModeCallback and WebActionMode has a reference to each other. That highly suggests there should be an interface between them. ie Callback in the embedder probably should talk to WebActionMode through a public *interface*. I've been meaning to bring that up, but there had been other issues first :) > I'm > creating WebActionMode, passing it to *ActionModeCallback ctr, and using > setCallback() next. If WebActionMode is to be created inside CVC, it should be > the other way around - *ActionModeCallback needs to implement a method like > setWebActionMode which the callbacks should somehow inherit from a new > interface/class. But I take it that you prefer them directly inheriting > ActionMode.Callback? If *ActionModeCallback inherits from, say, > WebActionModeCallback that defines setWebActionMode() interface, it would begin > to look somewhat like PS6. Wait.. you are discussing WebActionMode.setCallback here, and looks like it probably can't go away. But I was saying WebActionMode constructor and WebActionMode.initialize can be merged together. Sorry I wasn't clear. Let's say there's a public interface already, then I think CVC can create WebActionMode, return that as whatever the public interface is called, then embedder can use that to create the callback and then call setCallback. > > And the action callback for FloatingPastePopupMenu inherits not from > ActionMode.Callback but from ActionMode.Callback2. With the new interface, it's > not clear to me how the callback can fit in. I think I need to wrap my head around FloatingPastePopupMenu and what it does. I'll have an answer later.. > > > * Can we just fold the initialize method into the constructor? It doesn't look > > that involved to me to warrant doing it lazily > > I guess you meant CVC.initialize()? No I meant WebActionMode.initialize. Sorry I wasn't clear. > Though not involved, that's how the > unnecessary initialization of select action was avoided before the patch, which > I tried to keep as is. Let me know if you really want this - will move it. > > > * maybe I think you can directly pass in the callback into CVC.initialize as > > well, and fold setCallback into a single constructor as well Ok setCallback can't move I guess. https://codereview.chromium.org/2407303005/diff/320001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/WebActionMode.java (right): https://codereview.chromium.org/2407303005/diff/320001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:115: private String mLastSelectedText; On 2016/10/24 06:05:59, Jinsuk Kim wrote: > On 2016/10/23 19:56:48, boliu wrote: > > this can continue living on CVC, nothing here actually uses it, and CVC > already > > knows about hasSelection > > share()/search()/processText() are using it via getSelectedText()...? Ohh they call the method... ok. meh https://codereview.chromium.org/2407303005/diff/320001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:149: mView = view; On 2016/10/24 06:05:59, Jinsuk Kim wrote: > On 2016/10/23 19:56:48, boliu wrote: > > a few things to clean up before releasing the old view: > > > > * mView.removeCallbacks > > * destroyPastePopup (I think?) > > * onDestroyActionMode (I think?) > > I think it is CVC has been doing necessary clean-up already. Or are you asking > to handle them now because it is not done right? Yeah move all that handling here and out of CVC. > > - Removing callbacks should be handled by call sites and whoever added runnables > before entering here. Or maybe I just do > view.getHandler().removeCallbacksAndMessage(null) here? wait, doesn't that remove *all* of them? > - Paste popup is already being destroyed in CVC. > - Action mode is already being destroyed via hideSelectActionMode. Changed the > method name to clarify this. Yeah move that logic from CVC to here instead. https://codereview.chromium.org/2407303005/diff/320001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:196: public boolean isEmpty() { On 2016/10/24 06:05:59, Jinsuk Kim wrote: > On 2016/10/23 19:56:49, boliu wrote: > > should just do this check at the beginning of showActionMode and just early > out, > > looks CVC is already set up to handle showActionMode failing > > I'm already calling this at the beginning of CVC.showSelectActionMode(). Do you > mean to say it should be added here too? I mean it's WebActionMode's responsibility for this check. So yeah, *move* the check to WebActionMode.showActionMode, then don't expose this method to CVC at all.
Ok, re [Floating|Legacy]PastePopupMenu, I talked to Ted. Here's the rough summary: * Before M, LegacyPastePopupMenu does not use an action mode, and we just create a pop up ourselves. * After M, FloatingPastePopupMenu just re-uses the floating action mode UI, so it creates an action mode * A "regular" action mode, ie one created by CVC.startActionMode, and the PastePopupMenu are never active at the same time. If one starts, the other one is dismissed. This is just a fact of life on M, since there is only one active ActionMode per app (I think). This is to be the case before M too, even though there is no technical reason why that's the case. So FloatingPastePopupMenu is almost like another client of [Web]ActionMode. Let's do this, move the creation of both PastePopupMenu to WebActionMode. So there are two entry points from CVC, WebActionMode.startActionMode, and WebActionMode.startPastePopup, something like that. For FloatingPastePopupMenu, WebActionMode can call startActionMode, and then pass the ActionMode to FloatingPastePopupMenu. LegacyPastePopup doesn't need an ActionMode, so if we are running pre-M device, there is no need to have an ActionMode for startPastePopup, and we'll need to keep track of that.. So then overall to CVC, PastePopupMenu is almost like where CVC is the "embedder" of the WebActionMode. Does that make sense?
Patchset #9 (id:400001) has been deleted
https://codereview.chromium.org/2407303005/diff/320001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2407303005/diff/320001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1815: mActionMode.initialize(mWebContents, mRenderCoordinates); On 2016/10/24 18:09:38, boliu wrote: > On 2016/10/24 06:05:59, Jinsuk Kim wrote: > > On 2016/10/23 19:56:48, boliu wrote: > > > * I think CVC has all the constructor params, so can we have CVC create > > > WebActionMode as well? > > > > Mm... *ActionModeCallback and WebActionMode has a reference to each other. > > That highly suggests there should be an interface between them. ie Callback in > the embedder probably should talk to WebActionMode through a public *interface*. > I've been meaning to bring that up, but there had been other issues first :) > Added WebActionModeDelegate, and had WebActionMode be its own delegate for convenience's sake. > > I'm > > creating WebActionMode, passing it to *ActionModeCallback ctr, and using > > setCallback() next. If WebActionMode is to be created inside CVC, it should be > > the other way around - *ActionModeCallback needs to implement a method like > > setWebActionMode which the callbacks should somehow inherit from a new > > interface/class. But I take it that you prefer them directly inheriting > > ActionMode.Callback? If *ActionModeCallback inherits from, say, > > WebActionModeCallback that defines setWebActionMode() interface, it would > begin > > to look somewhat like PS6. > > Wait.. you are discussing WebActionMode.setCallback here, and looks like it > probably can't go away. But I was saying WebActionMode constructor and > WebActionMode.initialize can be merged together. Sorry I wasn't clear. > OK I take that you do prefer no lazy initialization. CVC gets the all the necessary info for WebActionMode at CVC.initialize() - so that's where WAM can be initialized, not in CVC ctr. > Let's say there's a public interface already, then I think CVC can create > WebActionMode, return that as whatever the public interface is called, then > embedder can use that to create the callback and then call setCallback. > Oh sorry somehow I was getting a wrong message that WebActionMode should not be exposed outside CVC. All goes straightforward and easy if it is accessible via CVC.getActionMode() (or getActionModeDelegate()). See the new patch for how it is done. it doesn't even need setCallback() since the callback can be passed through WAM.initialize() if it the initialization is done in CVC.initialize (i.e. not done lazily) > > > > And the action callback for FloatingPastePopupMenu inherits not from > > ActionMode.Callback but from ActionMode.Callback2. With the new interface, > it's > > not clear to me how the callback can fit in. > > I think I need to wrap my head around FloatingPastePopupMenu and what it does. > I'll have an answer later.. > > > > > > * Can we just fold the initialize method into the constructor? It doesn't > look > > > that involved to me to warrant doing it lazily > > > > I guess you meant CVC.initialize()? > > No I meant WebActionMode.initialize. Sorry I wasn't clear. > > > Though not involved, that's how the > > unnecessary initialization of select action was avoided before the patch, > which > > I tried to keep as is. Let me know if you really want this - will move it. > > > > > * maybe I think you can directly pass in the callback into CVC.initialize as > > > well, and fold setCallback into a single constructor as well > > Ok setCallback can't move I guess. https://codereview.chromium.org/2407303005/diff/320001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/WebActionMode.java (right): https://codereview.chromium.org/2407303005/diff/320001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:149: mView = view; On 2016/10/24 18:09:38, boliu wrote: > On 2016/10/24 06:05:59, Jinsuk Kim wrote: > > On 2016/10/23 19:56:48, boliu wrote: > > > a few things to clean up before releasing the old view: > > > > > > * mView.removeCallbacks > > > * destroyPastePopup (I think?) > > > * onDestroyActionMode (I think?) > > > > I think it is CVC has been doing necessary clean-up already. Or are you asking > > to handle them now because it is not done right? > > Yeah move all that handling here and out of CVC. > > > > > - Removing callbacks should be handled by call sites and whoever added > runnables > > before entering here. Or maybe I just do > > view.getHandler().removeCallbacksAndMessage(null) here? > > wait, doesn't that remove *all* of them? Yes it will. Then I don't know how to figure out which callbacks to remove. I still think it's acceptable to leave the job to call sites. > > > - Paste popup is already being destroyed in CVC. > > - Action mode is already being destroyed via hideSelectActionMode. Changed the > > method name to clarify this. > > Yeah move that logic from CVC to here instead. Moved the clean-up stuff that can be done in here. https://codereview.chromium.org/2407303005/diff/320001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:196: public boolean isEmpty() { On 2016/10/24 18:09:38, boliu wrote: > On 2016/10/24 06:05:59, Jinsuk Kim wrote: > > On 2016/10/23 19:56:49, boliu wrote: > > > should just do this check at the beginning of showActionMode and just early > > out, > > > looks CVC is already set up to handle showActionMode failing > > > > I'm already calling this at the beginning of CVC.showSelectActionMode(). Do > you > > mean to say it should be added here too? > > I mean it's WebActionMode's responsibility for this check. So yeah, *move* the > check to WebActionMode.showActionMode, then don't expose this method to CVC at > all. Done.
https://codereview.chromium.org/2407303005/diff/420001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (left): https://codereview.chromium.org/2407303005/diff/420001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2528: if (!mHasInsertion || !canPaste()) return; this line changed on trunk, probably should rebase this CL now https://codereview.chromium.org/2407303005/diff/420001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2407303005/diff/420001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:467: mActionMode = WebActionMode.EMPTY; I think this is probably overkill. CVC is not considered initialized until CVC.initialize is called, and before that, things are expected to be broken. This allows a bunch of things to be removed from WebActionMode, like the private contructor https://codereview.chromium.org/2407303005/diff/420001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:505: public WebActionMode getActionMode() { can you rename this to getWebActionModeForTesting? https://codereview.chromium.org/2407303005/diff/420001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:612: * @param actionMode WebActionMode instance that handles select action mode. comment out of date now https://codereview.chromium.org/2407303005/diff/420001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:663: destroyPastePopup(); I'll ask yusufo later why this isn't going through the full attach/detach path, I mean there is no reason why we don't also dismiss regular action mode here, for example.. but this is ok for now https://codereview.chromium.org/2407303005/diff/420001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:714: destroyPastePopup(); this should be handled inside WebActionMode.setContainerView https://codereview.chromium.org/2407303005/diff/420001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1832: if (!mActionMode.isValid()) return; move this check into WebActionMode https://codereview.chromium.org/2407303005/diff/420001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2180: public boolean isPastePopupShowing() { can you double check all these VisibleForTesting methods that's just forwarding the call to mActionMode, if tests can just call getWebActionMode directly, so the methods on CVC can be removed? https://codereview.chromium.org/2407303005/diff/420001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/WebActionModeDelegate.java (right): https://codereview.chromium.org/2407303005/diff/420001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionModeDelegate.java:19: public interface WebActionModeDelegate { we can discuss naming this later, but this is definitely wrong :p I'm thinking maybe ActionModeCallbackHelper, because it's helping embedder implement the callback https://codereview.chromium.org/2407303005/diff/420001/content/shell/android/... File content/shell/android/java/src/org/chromium/content_shell/Shell.java (right): https://codereview.chromium.org/2407303005/diff/420001/content/shell/android/... content/shell/android/java/src/org/chromium/content_shell/Shell.java:300: webContents, mWindow, defaultActionCallback(mContentViewCore.getActionMode())); getActionModeDelegate (basically the interface version)
https://codereview.chromium.org/2407303005/diff/420001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (left): https://codereview.chromium.org/2407303005/diff/420001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2528: if (!mHasInsertion || !canPaste()) return; On 2016/10/26 00:20:57, boliu wrote: > this line changed on trunk, probably should rebase this CL now Rebased. https://codereview.chromium.org/2407303005/diff/420001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2407303005/diff/420001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:467: mActionMode = WebActionMode.EMPTY; On 2016/10/26 00:20:57, boliu wrote: > I think this is probably overkill. CVC is not considered initialized until > CVC.initialize is called, and before that, things are expected to be broken. > This allows a bunch of things to be removed from WebActionMode, like the private > contructor Done. https://codereview.chromium.org/2407303005/diff/420001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:505: public WebActionMode getActionMode() { On 2016/10/26 00:20:57, boliu wrote: > can you rename this to getWebActionModeForTesting? Done. https://codereview.chromium.org/2407303005/diff/420001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:612: * @param actionMode WebActionMode instance that handles select action mode. On 2016/10/26 00:20:57, boliu wrote: > comment out of date now Done. https://codereview.chromium.org/2407303005/diff/420001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:663: destroyPastePopup(); On 2016/10/26 00:20:57, boliu wrote: > I'll ask yusufo later why this isn't going through the full attach/detach path, > I mean there is no reason why we don't also dismiss regular action mode here, > for example.. > > but this is ok for now Acknowledged. https://codereview.chromium.org/2407303005/diff/420001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:714: destroyPastePopup(); On 2016/10/26 00:20:57, boliu wrote: > this should be handled inside WebActionMode.setContainerView Done. https://codereview.chromium.org/2407303005/diff/420001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1832: if (!mActionMode.isValid()) return; On 2016/10/26 00:20:57, boliu wrote: > move this check into WebActionMode Can be removed here safely as canHide() in WAM.hide() already was checking isValid(). https://codereview.chromium.org/2407303005/diff/420001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2180: public boolean isPastePopupShowing() { On 2016/10/26 00:20:57, boliu wrote: > can you double check all these VisibleForTesting methods that's just forwarding > the call to mActionMode, if tests can just call getWebActionMode directly, so > the methods on CVC can be removed? Yes it can. Done. https://codereview.chromium.org/2407303005/diff/420001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/WebActionModeDelegate.java (right): https://codereview.chromium.org/2407303005/diff/420001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionModeDelegate.java:19: public interface WebActionModeDelegate { On 2016/10/26 00:20:57, boliu wrote: > we can discuss naming this later, but this is definitely wrong :p > > I'm thinking maybe ActionModeCallbackHelper, because it's helping embedder > implement the callback Sounds better. Renamed. https://codereview.chromium.org/2407303005/diff/420001/content/shell/android/... File content/shell/android/java/src/org/chromium/content_shell/Shell.java (right): https://codereview.chromium.org/2407303005/diff/420001/content/shell/android/... content/shell/android/java/src/org/chromium/content_shell/Shell.java:300: webContents, mWindow, defaultActionCallback(mContentViewCore.getActionMode())); On 2016/10/26 00:20:57, boliu wrote: > getActionModeDelegate (basically the interface version) Done.
Overall this CL is a great improvement, but I had one concern. I think that |WebActionMode| shouldn't be responsible for keeping track of the selection state (the mEditable, mIsInsertion, mIsPasswordType, etc. fields). Ideally, all of the selection state would be passed to |WebActionMode| at |showActionMode| and would never be modified. It would also be easier for me to implement my context menu CL if the selection state fields stayed in CVC.
On 2016/10/26 21:41:54, amaralp wrote: > Overall this CL is a great improvement, but I had one concern. I think that > |WebActionMode| shouldn't be responsible for keeping track of the selection > state (the mEditable, mIsInsertion, mIsPasswordType, etc. fields). Ideally, > all of the selection state would be passed to |WebActionMode| at > |showActionMode| and would never be modified. Except they are not actually static. And WebActionMode needs to respond to these states changing. > It would also be easier > for me to implement my context menu CL if the selection state fields stayed > in CVC. The overall goal here is to move things out of CVC. And the idea of ActionMode and context menu are pretty close. Perhaps they should be the same thing.. got a CL?
boliu@chromium.org changed reviewers: + amaralp@chromium.org, changwan@chromium.org
I think the overall structure is ok. Next step of review is actually looking at details to make sure is still correct, and there are no unintended behavior changes etc. Can probably try manual testing and see if anything is broken :) +amaralp,changwan Overall summary is action mode and paste pop up logic is "mostly" moved from CVC to WebActionMode. Now all the state like is editable etc is in there instead of in CVC. WAM is a long lived object, and is now responsible for creating ActionMode now. The the content API for action mode is a bit different now, but not too relevant to this discussion. So amaralp already brought up this will impact context menu, although I'd like to see the use case. I guess this may affect ime as well? Any thoughts here? I feel like we are creating a new object to manage all these state, so maybe WebActionMode isn't the best name :p
Currently select popup and action mode logic is split between /browser/ and /browser/input/. I think it would make sense to move the relevant logic to /browser/input/. This includes ContentViewCoreSelectionTest.java - how about /browser/input/SelectionTest.java, although it’s not well maintained. https://codereview.chromium.org/2407303005/diff/440001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2407303005/diff/440001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1300: private void setUnselectAllOnDismiss(boolean flag) { I don't think this method is needed. You can directly call mActionMode.setUnselectAllOnDismiss(flag). https://codereview.chromium.org/2407303005/diff/440001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1312: if (hasSelection()) showSelectActionMode(true); Do you need to check !mActionMode.isValid() as well? Otherwise it will invalidate once more. Not sure if it's intended... https://codereview.chromium.org/2407303005/diff/440001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/WebActionMode.java (right): https://codereview.chromium.org/2407303005/diff/440001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:55: public class WebActionMode implements ActionModeCallbackHelper { WebActionMode used to have the same lifecycle as ActionMode, but now controls a lot of things without being destroyed. It even contains paste popup when it’s managing selection popup. Would it make sense to rename it as ActionModeManager? Or even better, we can name it as SelectPastePopupManager and hide the term ‘ActionMode’ from ContentViewCore and other places. We can also rename a method like showSelectActionMode to be showSelectPopup(). Having both ActionMode and PopupMenu was confusing anyways. Or SelectPasteManager that oversees selection status as well. Another way to structure this would be wrap 'select' ActionMode in another place and have SelectPastePopupManager control both 'select' and 'paste' action modes if it makes sense. (I think they should be merged at one point though...) Also, instead of passing ActionMode.Callback / ActionMode.Callback2 around, how about passing a SelectionPopupCallback (renamed from ActionModeCallbackHelper)? SelectPastePopupManager is in complete control over starting action mode, so it should be able to instantiate ActionMode.Callback / ActionMode.Callback2 as necessary (anonymous class or inner class.) Then this class doesn't need to implement a callback. Also, EmptyCallback can be replaced by null or an empty object. Is it possible to change this way? https://codereview.chromium.org/2407303005/diff/440001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:364: public void hide(boolean hide) { We should probably change this name to hideSelectPopup(), as this class is not all about Select ActionMode any more.
On 2016/10/26 21:41:54, amaralp wrote: > Overall this CL is a great improvement, but I had one concern. I think that > |WebActionMode| shouldn't be responsible for keeping track of the selection > state (the mEditable, mIsInsertion, mIsPasswordType, etc. fields). Ideally, > all of the selection state would be passed to |WebActionMode| at > |showActionMode| and would never be modified. It would also be easier > for me to implement my context menu CL if the selection state fields stayed > in CVC. What you prefer may work, since WebActionMode needs to start ActionMode with a snapshot of those states and won't expect them to change while it's on. Only thing I'm not clear about is the way ActionMode.Calback#onPrepareActionMode handles them now - looks like it expects them to change, but I don't know when that can happen. In the end all the states and functionalities managed by CVC will have to find a new place. I'm hoping the new class I'm adding can be the one for all the action mode (including paste popup)-based context menu. Is it possible to take it into account for your change as well?
Did manual tests and fixes some bugs. Works fine now. - ActionModeCallbackHelper passed to the constructor of embedders' Callback impl is changed to 'provider' of the helper so that the helper instance can be retrieved in a lazy manner - the helper was not ready at the constructor. - set WebActionMode.mIsInsertion flag correctly - Fixed FloatingWebActionModeCallback so it forwards to calls to the ActionMode.Callback instance, not directly to ActionModeCallbackHelper. https://codereview.chromium.org/2407303005/diff/440001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2407303005/diff/440001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1300: private void setUnselectAllOnDismiss(boolean flag) { On 2016/10/27 05:09:36, Changwan Ryu wrote: > I don't think this method is needed. You can directly call > mActionMode.setUnselectAllOnDismiss(flag). Done. https://codereview.chromium.org/2407303005/diff/440001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1312: if (hasSelection()) showSelectActionMode(true); On 2016/10/27 05:09:36, Changwan Ryu wrote: > Do you need to check !mActionMode.isValid() as well? Otherwise it will > invalidate once more. Not sure if it's intended... Done. https://codereview.chromium.org/2407303005/diff/440001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/WebActionMode.java (right): https://codereview.chromium.org/2407303005/diff/440001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:55: public class WebActionMode implements ActionModeCallbackHelper { On 2016/10/27 05:09:37, Changwan Ryu wrote: > WebActionMode used to have the same lifecycle as ActionMode, but now controls a > lot of things without being destroyed. It even contains paste popup when it’s > managing selection popup. Would it make sense to rename it as ActionModeManager? > Or even better, we can name it as SelectPastePopupManager and hide the term > ‘ActionMode’ from ContentViewCore and other places. We can also rename a method > like showSelectActionMode to be showSelectPopup(). Having both ActionMode and > PopupMenu was confusing anyways. Or SelectPasteManager that oversees selection > status as well. Name suggestion is welcome. Unfortunately the name SelectPopup is already being used for dropdown list popup for HTML <select> form. Maybe ContextMenu something? > > Another way to structure this would be wrap 'select' ActionMode in another place > and have SelectPastePopupManager control both 'select' and 'paste' action modes > if it makes sense. (I think they should be merged at one point though...) > > Also, instead of passing ActionMode.Callback / ActionMode.Callback2 around, how > about passing a SelectionPopupCallback (renamed from ActionModeCallbackHelper)? > SelectPastePopupManager is in complete control over starting action mode, so it > should be able to instantiate ActionMode.Callback / ActionMode.Callback2 as > necessary (anonymous class or inner class.) > > Then this class doesn't need to implement a callback. > Early iterations of the CL has approaches somewhat similar to your suggestion but it grew into WebActionMode that has most of the features while embedder comes with an implementation of ActionMode.Callback configuring action mode to their requirements. I wouldn't say it's the best but it surely simplifies the configuration a lot. So I think it's worth giving it a go. > Also, EmptyCallback can be replaced by null or an empty object. > > Is it possible to change this way? https://codereview.chromium.org/2407303005/diff/440001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:364: public void hide(boolean hide) { On 2016/10/27 05:09:36, Changwan Ryu wrote: > We should probably change this name to hideSelectPopup(), as this class is not > all about Select ActionMode any more. Done.
On 2016/10/27 06:36:25, Jinsuk Kim wrote: > On 2016/10/26 21:41:54, amaralp wrote: > > Overall this CL is a great improvement, but I had one concern. I think that > > |WebActionMode| shouldn't be responsible for keeping track of the selection > > state (the mEditable, mIsInsertion, mIsPasswordType, etc. fields). Ideally, > > all of the selection state would be passed to |WebActionMode| at > > |showActionMode| and would never be modified. It would also be easier > > for me to implement my context menu CL if the selection state fields stayed > > in CVC. > > What you prefer may work, since WebActionMode needs to start ActionMode with a > snapshot of those states and won't expect them to change while it's on. Only > thing I'm not clear about is the way ActionMode.Calback#onPrepareActionMode > handles them now - looks like it expects them to change, but I don't know when > that can happen. > > In the end all the states and functionalities managed by CVC will have to find a > new place. I'm hoping the new class I'm adding can be the one for all the action > mode (including paste popup)-based context menu. Is it possible to take it into > account for your change as well? Or just land the CL first - I can take care of the rest :)
https://codereview.chromium.org/2407303005/diff/440001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/WebActionMode.java (right): https://codereview.chromium.org/2407303005/diff/440001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:55: public class WebActionMode implements ActionModeCallbackHelper { On 2016/10/27 11:44:59, Jinsuk Kim wrote: > On 2016/10/27 05:09:37, Changwan Ryu wrote: > > WebActionMode used to have the same lifecycle as ActionMode, but now controls > a > > lot of things without being destroyed. It even contains paste popup when it’s > > managing selection popup. Would it make sense to rename it as > ActionModeManager? > > Or even better, we can name it as SelectPastePopupManager and hide the term > > ‘ActionMode’ from ContentViewCore and other places. We can also rename a > method > > like showSelectActionMode to be showSelectPopup(). Having both ActionMode and > > PopupMenu was confusing anyways. Or SelectPasteManager that oversees selection > > status as well. > > Name suggestion is welcome. Unfortunately the name SelectPopup is already being > used for dropdown list popup for HTML <select> form. Maybe ContextMenu > something? > > > > > Another way to structure this would be wrap 'select' ActionMode in another > place > > and have SelectPastePopupManager control both 'select' and 'paste' action > modes > > if it makes sense. (I think they should be merged at one point though...) > > > > Also, instead of passing ActionMode.Callback / ActionMode.Callback2 around, > how > > about passing a SelectionPopupCallback (renamed from > ActionModeCallbackHelper)? > > SelectPastePopupManager is in complete control over starting action mode, so > it > > should be able to instantiate ActionMode.Callback / ActionMode.Callback2 as > > necessary (anonymous class or inner class.) > > > > Then this class doesn't need to implement a callback. > > I'm not sure that's simpler, at least in my head.. In your suggestion, do you see SelectionPopupCallback as a pure interface, or has default implementations that embedder can override/reuse? The ActionModeCallbackHelper class exists mostly to reduce code duplication for multiple embedders (and also pass information up to embedder). If we didn't care about code duplication, we'd probably just pass up a (const?) state object instead. > > Early iterations of the CL has approaches somewhat similar to your suggestion > but it grew into WebActionMode that has most of the features while embedder > comes with an implementation of ActionMode.Callback configuring action mode to > their requirements. I wouldn't say it's the best but it surely simplifies the > configuration a lot. So I think it's worth giving it a go. > > > Also, EmptyCallback can be replaced by null or an empty object. > > > > Is it possible to change this way? >
On 2016/10/27 at 06:36:25, jinsukkim wrote: > On 2016/10/26 21:41:54, amaralp wrote: > > Overall this CL is a great improvement, but I had one concern. I think that > > |WebActionMode| shouldn't be responsible for keeping track of the selection > > state (the mEditable, mIsInsertion, mIsPasswordType, etc. fields). Ideally, > > all of the selection state would be passed to |WebActionMode| at > > |showActionMode| and would never be modified. It would also be easier > > for me to implement my context menu CL if the selection state fields stayed > > in CVC. > > What you prefer may work, since WebActionMode needs to start ActionMode with a snapshot of those states and won't expect them to change while it's on. Only thing I'm not clear about is the way ActionMode.Calback#onPrepareActionMode handles them now - looks like it expects them to change, but I don't know when that can happen. > > In the end all the states and functionalities managed by CVC will have to find a new place. I'm hoping the new class I'm adding can be the one for all the action mode (including paste popup)-based context menu. Is it possible to take it into account for your change as well? I'm also not sure if ActionMode.Calback#onPrepareActionMode or ActionMode#invalidate are necessary. Eventually I'd like to have a wrapper class around select action mode and paste popup and abstract them to act more like a traditional desktop context menu. In particular I'd like the all of the selection state to be set during creation. I think your CL is a going in the right direction and I would be able to build upon it.
I was thinking that we could move more of the selection logic out of CVC. Here are some suggestions. https://codereview.chromium.org/2407303005/diff/460001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2407303005/diff/460001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1882: private void onSelectionEvent( Have this just call mActionMode.onSelectionEvent and have WebActionMode do all of this logic. This would mean that mContextualSearchClient would have to also be moved to WebActionMode. Eventually I think we'd want to make a native class to go along with WebActionMode.java and move the corresponding content_view_core_impl.cc code into it. https://codereview.chromium.org/2407303005/diff/460001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2171: private void onSelectionChanged(String text) { Move this to WebActionMode https://codereview.chromium.org/2407303005/diff/460001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/WebActionMode.java (right): https://codereview.chromium.org/2407303005/diff/460001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:55: public class WebActionMode implements ActionModeCallbackHelper { Might want to rename this to something more general (SelectionController?) https://codereview.chromium.org/2407303005/diff/460001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:182: public boolean isValid() { Rename to isActionModeValid. https://codereview.chromium.org/2407303005/diff/460001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:326: public void invalidate() { Rename to invalidateActionMode
Also bikeshedding name of WebActionMode.. I can't think of anything better than SelectPastePopupManager at the moment. But feels a bit too specific :p I think "ContextMenuManager" may not work, because context menu is actually a different thing, it's the menu that pops up when you long click a link. But maybe one day ContextMenu will live here in the future as well, which case "SelectPastePopupManager" doesn't work either.., barrrrrr https://codereview.chromium.org/2407303005/diff/440001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/WebActionMode.java (right): https://codereview.chromium.org/2407303005/diff/440001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:55: public class WebActionMode implements ActionModeCallbackHelper { On 2016/10/27 23:23:47, boliu wrote: > On 2016/10/27 11:44:59, Jinsuk Kim wrote: > > On 2016/10/27 05:09:37, Changwan Ryu wrote: > > > WebActionMode used to have the same lifecycle as ActionMode, but now > controls > > a > > > lot of things without being destroyed. It even contains paste popup when > it’s > > > managing selection popup. Would it make sense to rename it as > > ActionModeManager? > > > Or even better, we can name it as SelectPastePopupManager and hide the term > > > ‘ActionMode’ from ContentViewCore and other places. We can also rename a > > method > > > like showSelectActionMode to be showSelectPopup(). Having both ActionMode > and > > > PopupMenu was confusing anyways. Or SelectPasteManager that oversees > selection > > > status as well. > > > > Name suggestion is welcome. Unfortunately the name SelectPopup is already > being > > used for dropdown list popup for HTML <select> form. Maybe ContextMenu > > something? > > > > > > > > Another way to structure this would be wrap 'select' ActionMode in another > > place > > > and have SelectPastePopupManager control both 'select' and 'paste' action > > modes > > > if it makes sense. (I think they should be merged at one point though...) > > > > > > Also, instead of passing ActionMode.Callback / ActionMode.Callback2 around, > > how > > > about passing a SelectionPopupCallback (renamed from > > ActionModeCallbackHelper)? > > > SelectPastePopupManager is in complete control over starting action mode, so > > it > > > should be able to instantiate ActionMode.Callback / ActionMode.Callback2 as > > > necessary (anonymous class or inner class.) > > > > > > Then this class doesn't need to implement a callback. > > > > > I'm not sure that's simpler, at least in my head.. > > In your suggestion, do you see SelectionPopupCallback as a pure interface, or > has default implementations that embedder can override/reuse? > > The ActionModeCallbackHelper class exists mostly to reduce code duplication for > multiple embedders (and also pass information up to embedder). If we didn't care > about code duplication, we'd probably just pass up a (const?) state object > instead. Sorry, that's probably was not a complete train of thought.. So even if we have SelectionPopupCallback as you suggest, we'll still need something to share code. If SelectionPopupCallback is a class and can have default implementations that embedder override, then it's very close what we had before, and the problem with that is * splitting logic even more, between WAM and SelectionPopupCallback, which usually also requires passing state back and forth * it uses inheritance, which imo is worse than composition If SelectionPopupCallback is a pure interface, then then embedder probably still needs something else to do what ActionModeCallbackHelper current does, so that just seems like making things more complicated? > > > > > Early iterations of the CL has approaches somewhat similar to your suggestion > > but it grew into WebActionMode that has most of the features while embedder > > comes with an implementation of ActionMode.Callback configuring action mode to > > their requirements. I wouldn't say it's the best but it surely simplifies the > > configuration a lot. So I think it's worth giving it a go. > > > > > Also, EmptyCallback can be replaced by null or an empty object. > > > > > > Is it possible to change this way? > > >
https://codereview.chromium.org/2407303005/diff/440001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/WebActionMode.java (right): https://codereview.chromium.org/2407303005/diff/440001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:55: public class WebActionMode implements ActionModeCallbackHelper { On 2016/10/28 01:06:13, boliu wrote: > On 2016/10/27 23:23:47, boliu wrote: > > On 2016/10/27 11:44:59, Jinsuk Kim wrote: > > > On 2016/10/27 05:09:37, Changwan Ryu wrote: > > > > WebActionMode used to have the same lifecycle as ActionMode, but now > > controls > > > a > > > > lot of things without being destroyed. It even contains paste popup when > > it’s > > > > managing selection popup. Would it make sense to rename it as > > > ActionModeManager? > > > > Or even better, we can name it as SelectPastePopupManager and hide the > term > > > > ‘ActionMode’ from ContentViewCore and other places. We can also rename a > > > method > > > > like showSelectActionMode to be showSelectPopup(). Having both ActionMode > > and > > > > PopupMenu was confusing anyways. Or SelectPasteManager that oversees > > selection > > > > status as well. > > > > > > Name suggestion is welcome. Unfortunately the name SelectPopup is already > > being > > > used for dropdown list popup for HTML <select> form. Maybe ContextMenu > > > something? > > > > > > > > > > > Another way to structure this would be wrap 'select' ActionMode in another > > > place > > > > and have SelectPastePopupManager control both 'select' and 'paste' action > > > modes > > > > if it makes sense. (I think they should be merged at one point though...) > > > > > > > > Also, instead of passing ActionMode.Callback / ActionMode.Callback2 > around, > > > how > > > > about passing a SelectionPopupCallback (renamed from > > > ActionModeCallbackHelper)? > > > > SelectPastePopupManager is in complete control over starting action mode, > so > > > it > > > > should be able to instantiate ActionMode.Callback / ActionMode.Callback2 > as > > > > necessary (anonymous class or inner class.) > > > > > > > > Then this class doesn't need to implement a callback. > > > > > > > > I'm not sure that's simpler, at least in my head.. > > > > In your suggestion, do you see SelectionPopupCallback as a pure interface, or > > has default implementations that embedder can override/reuse? > > > > The ActionModeCallbackHelper class exists mostly to reduce code duplication > for > > multiple embedders (and also pass information up to embedder). If we didn't > care > > about code duplication, we'd probably just pass up a (const?) state object > > instead. > > Sorry, that's probably was not a complete train of thought.. > > So even if we have SelectionPopupCallback as you suggest, we'll still need > something to share code. If SelectionPopupCallback is a class and can have > default implementations that embedder override, then it's very close what we had > before, and the problem with that is > * splitting logic even more, between WAM and SelectionPopupCallback, which > usually also requires passing state back and forth > * it uses inheritance, which imo is worse than composition > > If SelectionPopupCallback is a pure interface, then then embedder probably still > needs something else to do what ActionModeCallbackHelper current does, so that > just seems like making things more complicated? Hmm... What I had in mind was mostly a passive object that does not require state lookup / change back and forth. I haven't thought through this too much myself, I have to depend on Jinsuk to think of good APIs here if it's possible at all. :) Since the only implementer of ActionMode.Callback is ChromeActionModeCallback, I don’t think there is much difference in code size. But I think it would be a cleaner design if a user of SelectionPopup / PastePopup doesn’t have to know about the additional embedder. Also, ActionMode itself is quite complicated as there are two interfaces and we’re already in control of starting and destroying it, I feel that we’re exposing a part of additional implementation detail that a user of ActionMode doesn’t have to know. Also this removes the need of HelperProvider in the new patch. > > > > > > > > > Early iterations of the CL has approaches somewhat similar to your > suggestion > > > but it grew into WebActionMode that has most of the features while embedder > > > comes with an implementation of ActionMode.Callback configuring action mode > to > > > their requirements. I wouldn't say it's the best but it surely simplifies > the > > > configuration a lot. So I think it's worth giving it a go. > > > > > > > Also, EmptyCallback can be replaced by null or an empty object. > > > > > > > > Is it possible to change this way? > > > > > >
https://codereview.chromium.org/2407303005/diff/440001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/WebActionMode.java (right): https://codereview.chromium.org/2407303005/diff/440001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:55: public class WebActionMode implements ActionModeCallbackHelper { On 2016/10/28 01:29:43, Changwan Ryu wrote: > On 2016/10/28 01:06:13, boliu wrote: > > On 2016/10/27 23:23:47, boliu wrote: > > > On 2016/10/27 11:44:59, Jinsuk Kim wrote: > > > > On 2016/10/27 05:09:37, Changwan Ryu wrote: > > > > > WebActionMode used to have the same lifecycle as ActionMode, but now > > > controls > > > > a > > > > > lot of things without being destroyed. It even contains paste popup when > > > it’s > > > > > managing selection popup. Would it make sense to rename it as > > > > ActionModeManager? > > > > > Or even better, we can name it as SelectPastePopupManager and hide the > > term > > > > > ‘ActionMode’ from ContentViewCore and other places. We can also rename a > > > > method > > > > > like showSelectActionMode to be showSelectPopup(). Having both > ActionMode > > > and > > > > > PopupMenu was confusing anyways. Or SelectPasteManager that oversees > > > selection > > > > > status as well. > > > > > > > > Name suggestion is welcome. Unfortunately the name SelectPopup is already > > > being > > > > used for dropdown list popup for HTML <select> form. Maybe ContextMenu > > > > something? > > > > > > > > > > > > > > Another way to structure this would be wrap 'select' ActionMode in > another > > > > place > > > > > and have SelectPastePopupManager control both 'select' and 'paste' > action > > > > modes > > > > > if it makes sense. (I think they should be merged at one point > though...) > > > > > > > > > > Also, instead of passing ActionMode.Callback / ActionMode.Callback2 > > around, > > > > how > > > > > about passing a SelectionPopupCallback (renamed from > > > > ActionModeCallbackHelper)? > > > > > SelectPastePopupManager is in complete control over starting action > mode, > > so > > > > it > > > > > should be able to instantiate ActionMode.Callback / ActionMode.Callback2 > > as > > > > > necessary (anonymous class or inner class.) > > > > > > > > > > Then this class doesn't need to implement a callback. > > > > > > > > > > > I'm not sure that's simpler, at least in my head.. > > > > > > In your suggestion, do you see SelectionPopupCallback as a pure interface, > or > > > has default implementations that embedder can override/reuse? > > > > > > The ActionModeCallbackHelper class exists mostly to reduce code duplication > > for > > > multiple embedders (and also pass information up to embedder). If we didn't > > care > > > about code duplication, we'd probably just pass up a (const?) state object > > > instead. > > > > Sorry, that's probably was not a complete train of thought.. > > > > So even if we have SelectionPopupCallback as you suggest, we'll still need > > something to share code. If SelectionPopupCallback is a class and can have > > default implementations that embedder override, then it's very close what we > had > > before, and the problem with that is > > * splitting logic even more, between WAM and SelectionPopupCallback, which > > usually also requires passing state back and forth > > * it uses inheritance, which imo is worse than composition > > > > If SelectionPopupCallback is a pure interface, then then embedder probably > still > > needs something else to do what ActionModeCallbackHelper current does, so that > > just seems like making things more complicated? > > Hmm... What I had in mind was mostly a passive object that does not require > state lookup / change back and forth. I haven't thought through this too much > myself, I have to depend on Jinsuk to think of good APIs here if it's possible > at all. :) > > Since the only implementer of ActionMode.Callback is ChromeActionModeCallback, > I don’t think there is much difference in code size. AwActionModeCallback in webview? :p > But I think it would be a > cleaner design if a user of SelectionPopup / PastePopup doesn’t have to know > about the additional embedder. > Also, ActionMode itself is quite complicated as there are two interfaces and > we’re already in control of starting and destroying it, I feel that we’re > exposing a part of additional implementation detail that a user of ActionMode > doesn’t have to know. Also this removes the need of HelperProvider in the new > patch. > > > > > > > > > > > > > > Early iterations of the CL has approaches somewhat similar to your > > suggestion > > > > but it grew into WebActionMode that has most of the features while > embedder > > > > comes with an implementation of ActionMode.Callback configuring action > mode > > to > > > > their requirements. I wouldn't say it's the best but it surely simplifies > > the > > > > configuration a lot. So I think it's worth giving it a go. > > > > > > > > > Also, EmptyCallback can be replaced by null or an empty object. > > > > > > > > > > Is it possible to change this way? > > > > > > > > > >
On 2016/10/28 01:06:14, boliu wrote: > Also bikeshedding name of WebActionMode.. I can't think of anything better than > SelectPastePopupManager at the moment. But feels a bit too specific :p > > I think "ContextMenuManager" may not work, because context menu is actually a > different thing, it's the menu that pops up when you long click a link. But > maybe one day ContextMenu will live here in the future as well, which case > "SelectPastePopupManager" doesn't work either.., barrrrrr > amaralp: SelectionController changwan: ActionModeManager << SelectPastePopupManager boliu: SelectPastePopupManager I'd like to see a word that can represent both select and paste - SelectPastePopupManager sounds a bit too verbose and descriptive to me. SelectPopup still in CVC keep getting on my nerves but trying to set it aside for now....h ow about InputPopupManager or EditPopupManager? Let's get one more round of responses and stop bikeshedding :) Give me your final suggestion or just stick to your last one. Thanks.
On 2016/10/28 04:54:18, Jinsuk Kim wrote: > On 2016/10/28 01:06:14, boliu wrote: > > Also bikeshedding name of WebActionMode.. I can't think of anything better > than > > SelectPastePopupManager at the moment. But feels a bit too specific :p > > > > I think "ContextMenuManager" may not work, because context menu is actually a > > different thing, it's the menu that pops up when you long click a link. But > > maybe one day ContextMenu will live here in the future as well, which case > > "SelectPastePopupManager" doesn't work either.., barrrrrr > > > > amaralp: SelectionController > changwan: ActionModeManager << SelectPastePopupManager > boliu: SelectPastePopupManager > > I'd like to see a word that can represent both select and paste - > SelectPastePopupManager sounds a bit too verbose and descriptive to me. > SelectPopup still in CVC keep getting on my nerves but trying to set it aside > for now....h ow about InputPopupManager or EditPopupManager? > Let's get one more round of responses and stop bikeshedding :) Give me your > final suggestion or just stick to your last one. Thanks. Hmm... Let me change my suggestion. How about naming it SelectionPopupController and making it handle onSelectEvent and related logic as amaralp@ suggested. The reason I prefer it over SelectionController is that the class is more handling popup than actually controlling the selection. (Also note that there is ui/views/selection_controller.h, which does 'control' selection from touch events.) And manager may be too broad in this context. INSERTION_HANDLE_* events are considered SelectionEventType in many places, so it may not be understandable if SelectionPopupController also handles paste popup scenario. Not sure if it can happen in this CL, but I hope that mContextualSearchClient can be refactored into observing SelectionPopupController.
On 2016/10/28 06:25:34, Changwan Ryu wrote: > On 2016/10/28 04:54:18, Jinsuk Kim wrote: > > On 2016/10/28 01:06:14, boliu wrote: > > > Also bikeshedding name of WebActionMode.. I can't think of anything better > > than > > > SelectPastePopupManager at the moment. But feels a bit too specific :p > > > > > > I think "ContextMenuManager" may not work, because context menu is actually > a > > > different thing, it's the menu that pops up when you long click a link. But > > > maybe one day ContextMenu will live here in the future as well, which case > > > "SelectPastePopupManager" doesn't work either.., barrrrrr > > > > > > > amaralp: SelectionController > > changwan: ActionModeManager << SelectPastePopupManager > > boliu: SelectPastePopupManager > > > > I'd like to see a word that can represent both select and paste - > > SelectPastePopupManager sounds a bit too verbose and descriptive to me. > > SelectPopup still in CVC keep getting on my nerves but trying to set it aside > > for now....h ow about InputPopupManager or EditPopupManager? > > Let's get one more round of responses and stop bikeshedding :) Give me your > > final suggestion or just stick to your last one. Thanks. > > Hmm... Let me change my suggestion. > How about naming it SelectionPopupController and making it handle onSelectEvent > and related logic as amaralp@ suggested. > > The reason I prefer it over SelectionController is that the class is more > handling popup than actually controlling the selection. > (Also note that there is ui/views/selection_controller.h, which does 'control' > selection from touch events.) > And manager may be too broad in this context. > > INSERTION_HANDLE_* events are considered SelectionEventType in many places, so > it may not be understandable if SelectionPopupController > also handles paste popup scenario. Sorry for the typo. I mean "it may *be* understandable"... > > Not sure if it can happen in this CL, but I hope that mContextualSearchClient > can be refactored into observing SelectionPopupController.
Only looked at the new code in CVC this round. Basically more suggestions to move a bunch more stuff to WAM Next step is then verify all the new code in WAM actually still do the same thing. And all the embedders code still does the same thing etc etc. Hopefully this CL is coming together :p https://codereview.chromium.org/2407303005/diff/460001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2407303005/diff/460001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1305: private void hidePopups() { nit: this is actually super hard to read, hidePopup is here to implement the two methods above, which only differs by setUnselectAllOnDismiss. so how about this: add WebActionMode.destroyActionModeAndUnselect, and .destroyActionModeKeepSelection, then can remove setUnselectAllOnDismiss from WebActionMode Then I think duplicating the other 3 lines is ok. So just remove this method and inline the other calls. And bigger picture.. Here are the "pop ups" that I already see, in this CL: action mode paste selection handle pop up zoomer aka disambiguation pop up not sure of context menu should be here too There are many "dismiss" here in CVC that different combinations of these things, but I think probably all the callsites actually intends to dismiss all of them. It's just things got added incrementally and people didn't pay attention. I think it's out of scope for this CL to clean all of them up, but definitely should do that at some point, and hopefully unify all the code paths. https://codereview.chromium.org/2407303005/diff/460001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1313: if (hasSelection() && mActionMode.isValid()) showSelectActionMode(true); logic here can be moved to WebActionMode https://codereview.chromium.org/2407303005/diff/460001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1813: private WebActionMode.HelperProvider mActionModeHelper = new WebActionMode.HelperProvider() { uhh, this is way too overkill imo, we can just add a CVC/WAM.setActionModeCallback, so that the callback doesn't need to be provided on WAM construction. https://codereview.chromium.org/2407303005/diff/460001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1830: mActionMode.showActionMode(allowFallback); I think logic up in this method up to this line can be moved to WAM https://codereview.chromium.org/2407303005/diff/460001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1868: public boolean hasSelection() { this should be redundant, since test can just get the WebActionMode directly? can you double check all the VisibleForTesting methods here that just calls into mActionMode and see if that's the case? https://codereview.chromium.org/2407303005/diff/460001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1887: case SelectionEventType.SELECTION_HANDLES_SHOWN: this case can move to WAM https://codereview.chromium.org/2407303005/diff/460001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1896: mDraggingSelection = true; looks like mDraggingSelection is only used for WAM, so can be moved to WAM entirely as well. then WAM.hide can become setTouchScrollInProgress, and I think this entire switch block here can be removed https://codereview.chromium.org/2407303005/diff/460001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2024: if (!focusedNodeEditable) hidePastePopup(); this line can move to WAM.updateSelectionState https://codereview.chromium.org/2407303005/diff/460001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2651: public void onReceivedProcessTextResult(int resultCode, Intent data) { can leave this as a TODO for follow up, but I think this call should move to ActionModeCallbackHelper
https://codereview.chromium.org/2407303005/diff/460001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2407303005/diff/460001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1305: private void hidePopups() { On 2016/10/30 19:57:16, boliu wrote: > nit: this is actually super hard to read, hidePopup is here to implement the two > methods above, which only differs by setUnselectAllOnDismiss. so how about this: > > add WebActionMode.destroyActionModeAndUnselect, and > .destroyActionModeKeepSelection, then can remove setUnselectAllOnDismiss from > WebActionMode > > Then I think duplicating the other 3 lines is ok. So just remove this method and > inline the other calls. > > > And bigger picture.. Here are the "pop ups" that I already see, in this CL: > action mode > paste > selection handle > pop up zoomer aka disambiguation pop up > not sure of context menu should be here too > > There are many "dismiss" here in CVC that different combinations of these > things, but I think probably all the callsites actually intends to dismiss all > of them. It's just things got added incrementally and people didn't pay > attention. I think it's out of scope for this CL to clean all of them up, but > definitely should do that at some point, and hopefully unify all the code paths. Done. https://codereview.chromium.org/2407303005/diff/460001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1313: if (hasSelection() && mActionMode.isValid()) showSelectActionMode(true); On 2016/10/30 19:57:16, boliu wrote: > logic here can be moved to WebActionMode Done. https://codereview.chromium.org/2407303005/diff/460001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1813: private WebActionMode.HelperProvider mActionModeHelper = new WebActionMode.HelperProvider() { On 2016/10/30 19:57:16, boliu wrote: > uhh, this is way too overkill imo, we can just add a > CVC/WAM.setActionModeCallback, so that the callback doesn't need to be provided > on WAM construction. *ActionModeCallback's constructor is the only place the helper can be passed in. What I needed is not CVC/WAM.setActionModeCallback but *ActionModeCallback.setActionModeCallbackHelper. But *ActionModeCallback don't have such method as they inherit directly from ActionMode.Callback. An alternative I could think of (but preferred the helper provider to) is using the embedder instance to get the helper. Let me know what you think. https://codereview.chromium.org/2407303005/diff/460001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1830: mActionMode.showActionMode(allowFallback); On 2016/10/30 19:57:16, boliu wrote: > I think logic up in this method up to this line can be moved to WAM Done. https://codereview.chromium.org/2407303005/diff/460001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1868: public boolean hasSelection() { On 2016/10/30 19:57:16, boliu wrote: > this should be redundant, since test can just get the WebActionMode directly? > > can you double check all the VisibleForTesting methods here that just calls into > mActionMode and see if that's the case? Yes - remopved hasSelection/hasInsertion. https://codereview.chromium.org/2407303005/diff/460001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1882: private void onSelectionEvent( On 2016/10/28 00:04:26, amaralp wrote: > Have this just call mActionMode.onSelectionEvent and have WebActionMode do all > of this logic. This would mean that mContextualSearchClient would have to also > be moved to WebActionMode. > > Eventually I think we'd want to make a native class to go along with > WebActionMode.java and move the corresponding content_view_core_impl.cc code > into it. Done. https://codereview.chromium.org/2407303005/diff/460001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1887: case SelectionEventType.SELECTION_HANDLES_SHOWN: On 2016/10/30 19:57:16, boliu wrote: > this case can move to WAM Done. https://codereview.chromium.org/2407303005/diff/460001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1896: mDraggingSelection = true; On 2016/10/30 19:57:16, boliu wrote: > looks like mDraggingSelection is only used for WAM, so can be moved to WAM > entirely as well. > > then WAM.hide can become setTouchScrollInProgress, and I think this entire > switch block here can be removed Done. touchScrollInProgress is used in hide() only, so kept the method names and moved the comment there to make it clear how these flags are used to update the visibility. https://codereview.chromium.org/2407303005/diff/460001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2024: if (!focusedNodeEditable) hidePastePopup(); On 2016/10/30 19:57:16, boliu wrote: > this line can move to WAM.updateSelectionState Done. https://codereview.chromium.org/2407303005/diff/460001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2171: private void onSelectionChanged(String text) { On 2016/10/28 00:04:26, amaralp wrote: > Move this to WebActionMode Done. https://codereview.chromium.org/2407303005/diff/460001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2651: public void onReceivedProcessTextResult(int resultCode, Intent data) { On 2016/10/30 19:57:16, boliu wrote: > can leave this as a TODO for follow up, but I think this call should move to > ActionModeCallbackHelper Done. (I mean - added the method to the interface definition) https://codereview.chromium.org/2407303005/diff/460001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/WebActionMode.java (right): https://codereview.chromium.org/2407303005/diff/460001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:55: public class WebActionMode implements ActionModeCallbackHelper { On 2016/10/28 00:04:26, amaralp wrote: > Might want to rename this to something more general (SelectionController?) I'm waiting for another round of suggestion. https://codereview.chromium.org/2407303005/diff/460001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:182: public boolean isValid() { On 2016/10/28 00:04:26, amaralp wrote: > Rename to isActionModeValid. Done. https://codereview.chromium.org/2407303005/diff/460001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:326: public void invalidate() { On 2016/10/28 00:04:26, amaralp wrote: > Rename to invalidateActionMode Done.
On 2016/10/27 05:09:37, Changwan Ryu wrote: > Currently select popup and action mode logic is split between /browser/ and > /browser/input/. I think it would make sense to move the relevant logic to > /browser/input/. > > This includes ContentViewCoreSelectionTest.java - how about > /browser/input/SelectionTest.java, although it’s not well maintained. Oh sorry I forgot to answer this - SelectionTest is for select popup dropdown test. So I'm not sure if it's better to move the tests there. Let me move around classes in a followup CL if you're okay. I think I have to modify many other classes just for import path updates, and makes the cl much bigger, and harder to see what's the key change made in the CL.
https://codereview.chromium.org/2407303005/diff/460001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2407303005/diff/460001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1813: private WebActionMode.HelperProvider mActionModeHelper = new WebActionMode.HelperProvider() { On 2016/10/31 02:28:55, Jinsuk Kim wrote: > On 2016/10/30 19:57:16, boliu wrote: > > uhh, this is way too overkill imo, we can just add a > > CVC/WAM.setActionModeCallback, so that the callback doesn't need to be > provided > > on WAM construction. > > *ActionModeCallback's constructor is the only place the helper can be passed in. > What I needed is not CVC/WAM.setActionModeCallback but > *ActionModeCallback.setActionModeCallbackHelper. But *ActionModeCallback don't > have such method as they inherit directly from ActionMode.Callback. > > An alternative I could think of (but preferred the helper provider to) is using > the embedder instance to get the helper. Let me know what you think. This should be ok in the embedder, right? cvc = new CVC(..) cvc.initialize(...) // do not pass in ActionModeCallback here fooActionModeCallback = new FooActionModeCallback(cvc.getHelper()) cvc.setActionModeCallback(fooActionModeCallback)
https://codereview.chromium.org/2407303005/diff/480001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/FloatingWebActionModeCallback.java (right): https://codereview.chromium.org/2407303005/diff/480001/content/public/android... content/public/android/java/src/org/chromium/content/browser/FloatingWebActionModeCallback.java:20: public class FloatingWebActionModeCallback extends ActionMode.Callback2 { dunno where to put this.. but WebActionModeCallback.java can be removed, afaict? https://codereview.chromium.org/2407303005/diff/480001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/WebActionMode.java (right): https://codereview.chromium.org/2407303005/diff/480001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:5: package org.chromium.content.browser; remove all imports of this class outside of content https://codereview.chromium.org/2407303005/diff/480001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:60: public static final int MAX_SEARCH_QUERY_LENGTH = 1000; this should be in the interface (I think it may need to be promoted to an abstract class to have static things?) nothing outside of content should need to import this class https://codereview.chromium.org/2407303005/diff/480001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:66: public static final int MAX_SHARE_QUERY_LENGTH = 100000; private https://codereview.chromium.org/2407303005/diff/480001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:70: public static final int MENU_ITEM_SHARE = 1 << 0; all private here https://codereview.chromium.org/2407303005/diff/480001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:86: private static boolean sFloatingActionModeCreationFailed; this was not static before, any reason why was made static? https://codereview.chromium.org/2407303005/diff/480001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:211: return false; this should return true? https://codereview.chromium.org/2407303005/diff/480001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:266: final float topControlsShownPix = mRenderCoordinates.getContentOffsetYPix(); this variable this got renamed to browserControlsShownPix on trunk, also probably rebase again? https://codereview.chromium.org/2407303005/diff/480001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:316: public int getType() { package visible https://codereview.chromium.org/2407303005/diff/480001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:333: public void invalidateActionMode() { private https://codereview.chromium.org/2407303005/diff/480001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:349: I read up to this far..
Patchset #13 (id:500001) has been deleted
Patchset #13 (id:520001) has been deleted
https://codereview.chromium.org/2407303005/diff/460001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2407303005/diff/460001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1813: private WebActionMode.HelperProvider mActionModeHelper = new WebActionMode.HelperProvider() { On 2016/10/31 21:14:28, boliu wrote: > On 2016/10/31 02:28:55, Jinsuk Kim wrote: > > On 2016/10/30 19:57:16, boliu wrote: > > > uhh, this is way too overkill imo, we can just add a > > > CVC/WAM.setActionModeCallback, so that the callback doesn't need to be > > provided > > > on WAM construction. > > > > *ActionModeCallback's constructor is the only place the helper can be passed > in. > > What I needed is not CVC/WAM.setActionModeCallback but > > *ActionModeCallback.setActionModeCallbackHelper. But *ActionModeCallback don't > > have such method as they inherit directly from ActionMode.Callback. > > > > An alternative I could think of (but preferred the helper provider to) is > using > > the embedder instance to get the helper. Let me know what you think. > > This should be ok in the embedder, right? > > cvc = new CVC(..) > cvc.initialize(...) // do not pass in ActionModeCallback here > fooActionModeCallback = new FooActionModeCallback(cvc.getHelper()) > cvc.setActionModeCallback(fooActionModeCallback) Oh that would work. I guess in my mind I wanted to keep the action mode callback as final in WebActionMode, which didn't let me see in this direction. Between CVC.initialize() and CVC.setActionModeCallback(), I set the action mode callback to a default (empty) callback by default - just don't want to leave it null. https://codereview.chromium.org/2407303005/diff/480001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/FloatingWebActionModeCallback.java (right): https://codereview.chromium.org/2407303005/diff/480001/content/public/android... content/public/android/java/src/org/chromium/content/browser/FloatingWebActionModeCallback.java:20: public class FloatingWebActionModeCallback extends ActionMode.Callback2 { On 2016/11/01 00:37:36, boliu wrote: > dunno where to put this.. but WebActionModeCallback.java can be removed, afaict? Yes - removed. And renamed this class a bit. https://codereview.chromium.org/2407303005/diff/480001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/WebActionMode.java (right): https://codereview.chromium.org/2407303005/diff/480001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:5: package org.chromium.content.browser; On 2016/11/01 00:37:36, boliu wrote: > remove all imports of this class outside of content Done. https://codereview.chromium.org/2407303005/diff/480001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:60: public static final int MAX_SEARCH_QUERY_LENGTH = 1000; On 2016/11/01 00:37:36, boliu wrote: > this should be in the interface (I think it may need to be promoted to an > abstract class to have static things?) > > nothing outside of content should need to import this class Done. https://codereview.chromium.org/2407303005/diff/480001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:66: public static final int MAX_SHARE_QUERY_LENGTH = 100000; On 2016/11/01 00:37:36, boliu wrote: > private Done. https://codereview.chromium.org/2407303005/diff/480001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:70: public static final int MENU_ITEM_SHARE = 1 << 0; On 2016/11/01 00:37:36, boliu wrote: > all private here Moved to the abstract class ActionModeCallbackHelper but kept them public - they are used in AwActionModeCallback. https://codereview.chromium.org/2407303005/diff/480001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:86: private static boolean sFloatingActionModeCreationFailed; On 2016/11/01 00:37:36, boliu wrote: > this was not static before, any reason why was made static? Previously this flag was being set for every CVC. It doesn't have to be. The success/failure depends on underlying Android platform version. So only the result of the very first invocation of startActionMode(), when failed, sets this and the rest can just make use of it to decide which type of action mode (or action mode/popup for pastepoup) to start. https://codereview.chromium.org/2407303005/diff/480001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:211: return false; On 2016/11/01 00:37:36, boliu wrote: > this should return true? CVC.showSelectActionMode() uses this value to determine if it should return right away or check the result of startActionMode() and act accordingly (i.e. clear selection if failed). It returns false if the action mode was already on. That's how it worked before. https://codereview.chromium.org/2407303005/diff/480001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:266: final float topControlsShownPix = mRenderCoordinates.getContentOffsetYPix(); On 2016/11/01 00:37:36, boliu wrote: > this variable this got renamed to browserControlsShownPix on trunk, also > probably rebase again? Thanks. rebased. https://codereview.chromium.org/2407303005/diff/480001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:316: public int getType() { On 2016/11/01 00:37:36, boliu wrote: > package visible In fact it's not used any more. Removed. https://codereview.chromium.org/2407303005/diff/480001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:333: public void invalidateActionMode() { On 2016/11/01 00:37:36, boliu wrote: > private Done. https://codereview.chromium.org/2407303005/diff/480001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:349: On 2016/11/01 00:37:36, boliu wrote: > I read up to this far.. Acknowledged.
https://codereview.chromium.org/2407303005/diff/480001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/WebActionMode.java (right): https://codereview.chromium.org/2407303005/diff/480001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:86: private static boolean sFloatingActionModeCreationFailed; On 2016/11/01 04:43:56, Jinsuk Kim wrote: > On 2016/11/01 00:37:36, boliu wrote: > > this was not static before, any reason why was made static? > > Previously this flag was being set for every CVC. It doesn't have to be. The > success/failure depends on underlying Android platform version. So only the > result of the very first invocation of startActionMode(), when failed, sets this > and the rest can just make use of it to decide which type of action mode (or > action mode/popup for pastepoup) to start. Did you test that? What if failure is due to some other random breakage? Probably shouldn't insert subtle behavior changes in a gigantic refactor CL. If you want to make this change, pull it out into a separate CL, before or after this refactor. https://codereview.chromium.org/2407303005/diff/480001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:211: return false; On 2016/11/01 04:43:56, Jinsuk Kim wrote: > On 2016/11/01 00:37:36, boliu wrote: > > this should return true? > > CVC.showSelectActionMode() uses this value to determine if it should return > right away or check the result of startActionMode() and act accordingly (i.e. > clear selection if failed). It returns false if the action mode was already on. > That's how it worked before. You sure? This is meant to return whether showActionMode succeeded or not. This case here is a success case And CVC code can just do if (!mWAM.showActionMode) clearSelection https://codereview.chromium.org/2407303005/diff/540001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActionModeCallback.java (right): https://codereview.chromium.org/2407303005/diff/540001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeActionModeCallback.java:55: int groupId = item.getGroupId(); findbugs identified this as not needed https://codereview.chromium.org/2407303005/diff/540001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ActionModeCallbackHelper.java (right): https://codereview.chromium.org/2407303005/diff/540001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ActionModeCallbackHelper.java:24: public abstract class ActionModeCallbackHelper { this should be under content_public https://codereview.chromium.org/2407303005/diff/540001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ActionModeCallbackHelper.java:49: public static boolean supportsFloatingActionMode() { this is still meant to be an interface. Holding static constants is ok, but holding implementation is not. You can keep the implementation on WebActionMode, but just forward the calls from here to WebActionMode https://codereview.chromium.org/2407303005/diff/540001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/WebActionMode.java (right): https://codereview.chromium.org/2407303005/diff/540001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:294: if (isActionModeValid()) mActionMode.finish(); should mActionMode be set to null at end of this? https://codereview.chromium.org/2407303005/diff/540001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:322: public void invalidateContentRect() { private https://codereview.chromium.org/2407303005/diff/540001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:336: public void onWindowFocusChanged(boolean hasWindowFocus) { package? https://codereview.chromium.org/2407303005/diff/540001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:338: if (isActionModeValid()) mActionMode.onWindowFocusChanged(hasWindowFocus); nit: can write the two conditions together in a single if statement https://codereview.chromium.org/2407303005/diff/540001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:367: private void hideTemporarily(long duration) { hideActionModeTemporarily I guess? https://codereview.chromium.org/2407303005/diff/540001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:374: private boolean canHide() { canHideActionMode https://codereview.chromium.org/2407303005/diff/540001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:396: mNeedsPrepare = false; this can move to createActionMenu, one less line of duplication https://codereview.chromium.org/2407303005/diff/540001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:397: return true; this always returns true, should just drop the return value? https://codereview.chromium.org/2407303005/diff/540001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:402: if (mNeedsPrepare) { nit: prefer early out: if (!mNeedsPrepare) return false; then the rest of the body don't need to be indented https://codereview.chromium.org/2407303005/diff/540001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:493: private Intent createProcessTextIntent() { static https://codereview.chromium.org/2407303005/diff/540001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:573: public void selectAll() { private https://codereview.chromium.org/2407303005/diff/540001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:587: public void cut() { I think these 3 methods should be package visible and @VisibleForTesting? https://codereview.chromium.org/2407303005/diff/540001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:630: RecordUserAction.record("MobileActionMode.ProcessTextIntent"); I think this needs to be noted that the usage counter here is potentially different, if embedder no longer calls this method and just handles process text on its own chrome doesn't do that right? and webview doesn't have uma yet, so maybe should just be ok..? https://codereview.chromium.org/2407303005/diff/540001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:657: RecordUserAction.record("MobileActionMode.WebSearch"); ditto about usage counter, does chrome override this one? https://codereview.chromium.org/2407303005/diff/540001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:754: if (isSelectionEditable()) { this block to unselect looks new like new code in this CL (I didn't find where it came from) why are we dismissing selection here? Also now this is duplicated code with CVC.clearSelection, can we have CVC call this instead? https://codereview.chromium.org/2407303005/diff/540001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:860: void destroyActionModeAndUnselect() { I meant these two methods actually do destroy action mode (and unselect here)
Patchset #14 (id:560001) has been deleted
Any more feedback on the new name of WebActionMode? I'm thinking of SelectionPopupController( as suggested by Changwan) if you're okay. I found that showActionMode(boolean allowFeeback) is always called with true. Maybe remove the parameter unless it is there for a reason? https://codereview.chromium.org/2407303005/diff/480001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/WebActionMode.java (right): https://codereview.chromium.org/2407303005/diff/480001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:86: private static boolean sFloatingActionModeCreationFailed; On 2016/11/01 22:24:32, boliu wrote: > On 2016/11/01 04:43:56, Jinsuk Kim wrote: > > On 2016/11/01 00:37:36, boliu wrote: > > > this was not static before, any reason why was made static? > > > > Previously this flag was being set for every CVC. It doesn't have to be. The > > success/failure depends on underlying Android platform version. So only the > > result of the very first invocation of startActionMode(), when failed, sets > this > > and the rest can just make use of it to decide which type of action mode (or > > action mode/popup for pastepoup) to start. > > Did you test that? What if failure is due to some other random breakage? > Probably shouldn't insert subtle behavior changes in a gigantic refactor CL. If > you want to make this change, pull it out into a separate CL, before or after > this refactor. Done. Please note that the flag is already shared by ActionMode instances created within a CVC i.e. if floating action mode creation fails somehow by a random breakage then what you worry about would happen already, affecting the action mode instances being created afterwards. I just had extended it to be shared across CVCs but reverted. Maybe worth revisiting and look into what can make floating action mode creation fail other than Android platform not supporting it. https://codereview.chromium.org/2407303005/diff/480001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:211: return false; On 2016/11/01 22:24:32, boliu wrote: > On 2016/11/01 04:43:56, Jinsuk Kim wrote: > > On 2016/11/01 00:37:36, boliu wrote: > > > this should return true? > > > > CVC.showSelectActionMode() uses this value to determine if it should return > > right away or check the result of startActionMode() and act accordingly (i.e. > > clear selection if failed). It returns false if the action mode was already > on. > > That's how it worked before. > > You sure? This is meant to return whether showActionMode succeeded or not. This > case here is a success case > > And CVC code can just do > > if (!mWAM.showActionMode) > clearSelection This is what was being done before change: if (mActionMode.isValid()) { mActionMode.invalidate(); return; } I added the return type so that true means Android.view.View.startActionMode() was invoked. The intention is that the caller (CVC or WAM) then should check the result (mActionMode being null or not) and clear selection only if it fails. With false returned, you don't have to care about anything but simply get short-circuited. But I agree that return value indicating the success of action mode creation (or it being already on) is more intuitive. I added isEmpty() check in clearSelection() to be defensive, since it also causes the method to return false. Moved clearSelection() to WAM to avoid code duplication. restoreSelectionPopupsIfNecessary also should have handled the call this way. Fixed it now. https://codereview.chromium.org/2407303005/diff/540001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActionModeCallback.java (right): https://codereview.chromium.org/2407303005/diff/540001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeActionModeCallback.java:55: int groupId = item.getGroupId(); On 2016/11/01 22:24:32, boliu wrote: > findbugs identified this as not needed Done. https://codereview.chromium.org/2407303005/diff/540001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ActionModeCallbackHelper.java (right): https://codereview.chromium.org/2407303005/diff/540001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ActionModeCallbackHelper.java:24: public abstract class ActionModeCallbackHelper { On 2016/11/01 22:24:32, boliu wrote: > this should be under content_public Done. https://codereview.chromium.org/2407303005/diff/540001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ActionModeCallbackHelper.java:49: public static boolean supportsFloatingActionMode() { On 2016/11/01 22:24:32, boliu wrote: > this is still meant to be an interface. Holding static constants is ok, but > holding implementation is not. You can keep the implementation on WebActionMode, > but just forward the calls from here to WebActionMode done. https://codereview.chromium.org/2407303005/diff/540001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/WebActionMode.java (right): https://codereview.chromium.org/2407303005/diff/540001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:294: if (isActionModeValid()) mActionMode.finish(); On 2016/11/01 22:24:32, boliu wrote: > should mActionMode be set to null at end of this? ActionMode calls destroyActionMode in response, and mActionMode is set to null there. https://codereview.chromium.org/2407303005/diff/540001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:322: public void invalidateContentRect() { On 2016/11/01 22:24:32, boliu wrote: > private Used by FloatingPastePopupMenu. https://codereview.chromium.org/2407303005/diff/540001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:336: public void onWindowFocusChanged(boolean hasWindowFocus) { On 2016/11/01 22:24:32, boliu wrote: > package? Done. https://codereview.chromium.org/2407303005/diff/540001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:338: if (isActionModeValid()) mActionMode.onWindowFocusChanged(hasWindowFocus); On 2016/11/01 22:24:32, boliu wrote: > nit: can write the two conditions together in a single if statement Done. https://codereview.chromium.org/2407303005/diff/540001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:367: private void hideTemporarily(long duration) { On 2016/11/01 22:24:32, boliu wrote: > hideActionModeTemporarily I guess? Done. https://codereview.chromium.org/2407303005/diff/540001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:374: private boolean canHide() { On 2016/11/01 22:24:32, boliu wrote: > canHideActionMode Done. https://codereview.chromium.org/2407303005/diff/540001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:396: mNeedsPrepare = false; On 2016/11/01 22:24:33, boliu wrote: > this can move to createActionMenu, one less line of duplication Done. https://codereview.chromium.org/2407303005/diff/540001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:397: return true; On 2016/11/01 22:24:32, boliu wrote: > this always returns true, should just drop the return value? Done. https://codereview.chromium.org/2407303005/diff/540001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:402: if (mNeedsPrepare) { On 2016/11/01 22:24:32, boliu wrote: > nit: prefer early out: > > if (!mNeedsPrepare) return false; > > then the rest of the body don't need to be indented Done. https://codereview.chromium.org/2407303005/diff/540001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:493: private Intent createProcessTextIntent() { On 2016/11/01 22:24:32, boliu wrote: > static Done. https://codereview.chromium.org/2407303005/diff/540001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:573: public void selectAll() { On 2016/11/01 22:24:32, boliu wrote: > private Use by a test. Made package visible like the methods below. https://codereview.chromium.org/2407303005/diff/540001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:587: public void cut() { On 2016/11/01 22:24:32, boliu wrote: > I think these 3 methods should be package visible and @VisibleForTesting? Done. https://codereview.chromium.org/2407303005/diff/540001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:630: RecordUserAction.record("MobileActionMode.ProcessTextIntent"); On 2016/11/01 22:24:32, boliu wrote: > I think this needs to be noted that the usage counter here is potentially > different, if embedder no longer calls this method and just handles process text > on its own > > chrome doesn't do that right? and webview doesn't have uma yet, so maybe should > just be ok..? Chrome does use this method. https://codereview.chromium.org/2407303005/diff/540001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:657: RecordUserAction.record("MobileActionMode.WebSearch"); On 2016/11/01 22:24:33, boliu wrote: > ditto about usage counter, does chrome override this one? Chrome has its own search() and usage counter is handled there too. https://codereview.chromium.org/2407303005/diff/540001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:754: if (isSelectionEditable()) { On 2016/11/01 22:24:32, boliu wrote: > this block to unselect looks new like new code in this CL (I didn't find where > it came from) > > why are we dismissing selection here? > > Also now this is duplicated code with CVC.clearSelection, can we have CVC call > this instead? This is copied over from CVC to do CVC.clearSelection() as comments suggested all selections events be handled here, have CVC.onSelectionEvent simply call it and do nothing else there. Moved clearSelection body to WAM and called it. https://codereview.chromium.org/2407303005/diff/540001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:860: void destroyActionModeAndUnselect() { On 2016/11/01 22:24:32, boliu wrote: > I meant these two methods actually do destroy action mode (and unselect here) Oops missed it. Added finishActionMode().
On 2016/11/02 03:48:44, Jinsuk Kim wrote: > Any more feedback on the new name of WebActionMode? I'm thinking of > SelectionPopupController( as suggested by Changwan) if you're okay. sgtm. have a comment summarizing roughly what the class does > > I found that showActionMode(boolean allowFeeback) is always called with true. > Maybe remove the parameter unless it is there for a reason? yes please https://codereview.chromium.org/2407303005/diff/540001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/WebActionMode.java (right): https://codereview.chromium.org/2407303005/diff/540001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:754: if (isSelectionEditable()) { On 2016/11/02 03:48:43, Jinsuk Kim wrote: > On 2016/11/01 22:24:32, boliu wrote: > > this block to unselect looks new like new code in this CL (I didn't find where > > it came from) > > > > why are we dismissing selection here? > > > > Also now this is duplicated code with CVC.clearSelection, can we have CVC call > > this instead? > > This is copied over from CVC to do CVC.clearSelection() as comments suggested > all selections events be handled here, have CVC.onSelectionEvent simply call it > and do nothing else there. Ahh, so this was near the end of CVC.showSelectActionMode before this refactor? Cool > > Moved clearSelection body to WAM and called it.
looked through rest of content today :) https://codereview.chromium.org/2407303005/diff/580001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/FloatingActionModeCallback.java (right): https://codereview.chromium.org/2407303005/diff/580001/content/public/android... content/public/android/java/src/org/chromium/content/browser/FloatingActionModeCallback.java:21: public class FloatingActionModeCallback extends ActionMode.Callback2 { package https://codereview.chromium.org/2407303005/diff/580001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/FloatingPastePopupMenu.java (right): https://codereview.chromium.org/2407303005/diff/580001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/FloatingPastePopupMenu.java:110: ActionMode actionMode = mParent.startActionMode( hmm, I thought we agreed earlier that FloatingPastePopupMenu would *not* create its own actionmode? Just make it behave like a regular action mode, and just make this class be the "embedder"? maybe that's too big a change for this CL.. but there's so much code duplication here, definitely the next step https://codereview.chromium.org/2407303005/diff/580001/content/public/android... File content/public/android/java/src/org/chromium/content_public/browser/ActionModeCallbackHelper.java (right): https://codereview.chromium.org/2407303005/diff/580001/content/public/android... content/public/android/java/src/org/chromium/content_public/browser/ActionModeCallbackHelper.java:22: private static final String TAG = "cr.ActionModeHelper"; not needed? https://codereview.chromium.org/2407303005/diff/580001/content/public/android... content/public/android/java/src/org/chromium/content_public/browser/ActionModeCallbackHelper.java:41: public abstract String sanitizeQuery(String query, int maxLength); static methods should remain static, just import WebActionMode here and call the static methods on WebActionMode. I know it's one more level of indirection required, but not a big deal https://codereview.chromium.org/2407303005/diff/580001/content/public/android... File content/public/android/java/src/org/chromium/content_public/browser/WebContents.java (right): https://codereview.chromium.org/2407303005/diff/580001/content/public/android... content/public/android/java/src/org/chromium/content_public/browser/WebContents.java:373: public void dismissTextHandles(); this doesn't need to be on the public interface
https://codereview.chromium.org/2407303005/diff/580001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/FloatingActionModeCallback.java (right): https://codereview.chromium.org/2407303005/diff/580001/content/public/android... content/public/android/java/src/org/chromium/content/browser/FloatingActionModeCallback.java:21: public class FloatingActionModeCallback extends ActionMode.Callback2 { On 2016/11/02 22:13:40, boliu wrote: > package Done. https://codereview.chromium.org/2407303005/diff/580001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/FloatingPastePopupMenu.java (right): https://codereview.chromium.org/2407303005/diff/580001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/FloatingPastePopupMenu.java:110: ActionMode actionMode = mParent.startActionMode( On 2016/11/02 22:13:40, boliu wrote: > hmm, I thought we agreed earlier that FloatingPastePopupMenu would *not* create > its own actionmode? Just make it behave like a regular action mode, and just > make this class be the "embedder"? > > maybe that's too big a change for this CL.. but there's so much code duplication > here, definitely the next step Sorry I forgot to tell you clearly why I left it almost intact (I had tried it but kind of backed off after doing a bad job making it better) The duplication (LegacyPastePopupMenu here and in WAM) mainly comes from the fact that for action mode creation is as good as 'show', while popup has creation and show split. That's why this popup creation is delayed to public show() method and the constructor does almost nothing wrt action mode. I'll visit this in a follow-up CL to see if the code can be deduped. https://codereview.chromium.org/2407303005/diff/580001/content/public/android... File content/public/android/java/src/org/chromium/content_public/browser/ActionModeCallbackHelper.java (right): https://codereview.chromium.org/2407303005/diff/580001/content/public/android... content/public/android/java/src/org/chromium/content_public/browser/ActionModeCallbackHelper.java:22: private static final String TAG = "cr.ActionModeHelper"; On 2016/11/02 22:13:40, boliu wrote: > not needed? Done. https://codereview.chromium.org/2407303005/diff/580001/content/public/android... content/public/android/java/src/org/chromium/content_public/browser/ActionModeCallbackHelper.java:41: public abstract String sanitizeQuery(String query, int maxLength); On 2016/11/02 22:13:40, boliu wrote: > static methods should remain static, just import WebActionMode here and call the > static methods on WebActionMode. I know it's one more level of indirection > required, but not a big deal Importing a class in org.chromium.content.browser doesn't seem desirable to me - at least I don't see any classes in content_public do this. can we give it a break and have an implementation body here... this is a quite independent utility method whose presence in an abstract class can be justified I think. https://codereview.chromium.org/2407303005/diff/580001/content/public/android... File content/public/android/java/src/org/chromium/content_public/browser/WebContents.java (right): https://codereview.chromium.org/2407303005/diff/580001/content/public/android... content/public/android/java/src/org/chromium/content_public/browser/WebContents.java:373: public void dismissTextHandles(); On 2016/11/02 22:13:40, boliu wrote: > this doesn't need to be on the public interface Removed public and moved below the declaration of copy/cut/selectAll() I think that's a better location.
https://codereview.chromium.org/2407303005/diff/580001/content/public/android... File content/public/android/java/src/org/chromium/content_public/browser/ActionModeCallbackHelper.java (right): https://codereview.chromium.org/2407303005/diff/580001/content/public/android... content/public/android/java/src/org/chromium/content_public/browser/ActionModeCallbackHelper.java:41: public abstract String sanitizeQuery(String query, int maxLength); On 2016/11/03 01:21:58, Jinsuk Kim wrote: > On 2016/11/02 22:13:40, boliu wrote: > > static methods should remain static, just import WebActionMode here and call > the > > static methods on WebActionMode. I know it's one more level of indirection > > required, but not a big deal > > Importing a class in org.chromium.content.browser doesn't seem desirable to me - > at least I don't see any classes in content_public do this. See my explanation here: https://codereview.chromium.org/2461853002/#msg5 > can we give it a > break and have an implementation body here... this is a quite independent > utility method whose presence in an abstract class can be justified I think. Yes the method can and should be in the abstract class, but imo the implementation should not be here. See fromWebContents here: https://codereview.chromium.org/2453623003/diff/220001/content/public/android... Probably the content owners should sit down and make a decision, but this is my decision for the moment https://codereview.chromium.org/2407303005/diff/580001/content/public/android... File content/public/android/java/src/org/chromium/content_public/browser/WebContents.java (right): https://codereview.chromium.org/2407303005/diff/580001/content/public/android... content/public/android/java/src/org/chromium/content_public/browser/WebContents.java:373: public void dismissTextHandles(); On 2016/11/03 01:21:58, Jinsuk Kim wrote: > On 2016/11/02 22:13:40, boliu wrote: > > this doesn't need to be on the public interface > > Removed public and moved below the declaration of copy/cut/selectAll() I think > that's a better location. Ohh... existing code is so so wrong. First, not putting "public" on a method in an interface doesn't make it package visible. Methods in an interface are by default public. I don't even now how you make it package visible... Also, in native code, the pattern is: content implementation code is allowed (and encouraged) to down cast to Impl type. In that case the methods don't need to be on the interface at all. And I think java should follow that pattern in general as well. But I guess existing code is already wrong, and this is just making it a little bit more wrong, so whatever in this CL.. should fix all of this in a separate CL later.
Description was changed from ========== Let embedder provide select action mode This CL tries to simplify selection action mode configuration being done through ContentViewClient that will be gone after refactoring. Now Embedders provide with a customized ActionMode.Callback. This effectively moves the relevant implementation details from ContentViewClient to the callback. WebActionMode pulls out all the select action mode- related methods and variables from ContentViewCore and encapsulates them. This also helps break up ContentViewCore. BUG=623783 ========== to ========== Let embedder provide select action mode This CL tries to simplify selection action mode configuration being done through ContentViewClient that will be gone after refactoring. Now Embedders provide with a customized ActionMode.Callback. This effectively moves the relevant implementation details from ContentViewClient to the callback. SelectionPopupController pulls out all the select action mode-related methods and variables from ContentViewCore and encapsulates them. This also helps break up ContentViewCore. BUG=623783 ==========
https://codereview.chromium.org/2407303005/diff/580001/content/public/android... File content/public/android/java/src/org/chromium/content_public/browser/ActionModeCallbackHelper.java (right): https://codereview.chromium.org/2407303005/diff/580001/content/public/android... content/public/android/java/src/org/chromium/content_public/browser/ActionModeCallbackHelper.java:41: public abstract String sanitizeQuery(String query, int maxLength); On 2016/11/03 04:47:53, boliu wrote: > On 2016/11/03 01:21:58, Jinsuk Kim wrote: > > On 2016/11/02 22:13:40, boliu wrote: > > > static methods should remain static, just import WebActionMode here and call > > the > > > static methods on WebActionMode. I know it's one more level of indirection > > > required, but not a big deal > > > > Importing a class in org.chromium.content.browser doesn't seem desirable to me > - > > at least I don't see any classes in content_public do this. > > See my explanation here: https://codereview.chromium.org/2461853002/#msg5 > > > can we give it a > > break and have an implementation body here... this is a quite independent > > utility method whose presence in an abstract class can be justified I think. > > Yes the method can and should be in the abstract class, but imo the > implementation should not be here. See fromWebContents here: > https://codereview.chromium.org/2453623003/diff/220001/content/public/android... > > Probably the content owners should sit down and make a decision, but this is my > decision for the moment Done. https://codereview.chromium.org/2407303005/diff/580001/content/public/android... File content/public/android/java/src/org/chromium/content_public/browser/WebContents.java (right): https://codereview.chromium.org/2407303005/diff/580001/content/public/android... content/public/android/java/src/org/chromium/content_public/browser/WebContents.java:373: public void dismissTextHandles(); On 2016/11/03 04:47:53, boliu wrote: > On 2016/11/03 01:21:58, Jinsuk Kim wrote: > > On 2016/11/02 22:13:40, boliu wrote: > > > this doesn't need to be on the public interface > > > > Removed public and moved below the declaration of copy/cut/selectAll() I think > > that's a better location. > > Ohh... existing code is so so wrong. > > First, not putting "public" on a method in an interface doesn't make it package > visible. Methods in an interface are by default public. I don't even now how you > make it package visible... > > Also, in native code, the pattern is: content implementation code is allowed > (and encouraged) to down cast to Impl type. In that case the methods don't need > to be on the interface at all. And I think java should follow that pattern in > general as well. > > But I guess existing code is already wrong, and this is just making it a little > bit more wrong, so whatever in this CL.. should fix all of this in a separate CL > later. Acknowledged.
https://codereview.chromium.org/2407303005/diff/610001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/AwActionModeCallback.java (right): https://codereview.chromium.org/2407303005/diff/610001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwActionModeCallback.java:43: menus.put(menuItem, mAwContents.isSelectActionModeAllowed(menuItem)); hmm, this list is set by the app, which means it can potentially change any time which I think is fine. have AwSettings pass the new value to here and then to the helper. and if it changed, probably should also invalidate any actionmodes as well, if anything changed. So AwSettings will need to be given a reference to this object *after* it's passed to AwContents. Then the logic in AwContents can be removed as well. Let me know if this doesn't make sense. Also just use the int bit field rather than use sparse arrays. https://codereview.chromium.org/2407303005/diff/610001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right): https://codereview.chromium.org/2407303005/diff/610001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:773: public interface ObserverNotifier { instead of this, you can just add a notifyContextualActionBarVisibilityChanged(bool) method here https://codereview.chromium.org/2407303005/diff/610001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java (right): https://codereview.chromium.org/2407303005/diff/610001/content/public/android... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:239: if (!isInsertion() || (!supportsFloatingActionMode() && !canPaste())) return; remove the isInsertion condition, it was removed on trunk by https://codereview.chromium.org/2468043002
PTAL https://codereview.chromium.org/2407303005/diff/610001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/AwActionModeCallback.java (right): https://codereview.chromium.org/2407303005/diff/610001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwActionModeCallback.java:43: menus.put(menuItem, mAwContents.isSelectActionModeAllowed(menuItem)); On 2016/11/03 22:46:52, boliu wrote: > hmm, this list is set by the app, which means it can potentially change any time > > which I think is fine. have AwSettings pass the new value to here and then to > the helper. and if it changed, probably should also invalidate any actionmodes > as well, if anything changed. So AwSettings will need to be given a reference to > this object *after* it's passed to AwContents. Then the logic in AwContents can > be removed as well. Let me know if this doesn't make sense. > > Also just use the int bit field rather than use sparse arrays. Understood. An alternative would be to set the configuration in onCreateActionMode() so that the updated setting can be reflected from the next action mode. It can avoid AwSettings having to be aware of this class. Please take a look. https://codereview.chromium.org/2407303005/diff/610001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right): https://codereview.chromium.org/2407303005/diff/610001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:773: public interface ObserverNotifier { On 2016/11/03 22:46:52, boliu wrote: > instead of this, you can just add a > notifyContextualActionBarVisibilityChanged(bool) method here Done. https://codereview.chromium.org/2407303005/diff/610001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java (right): https://codereview.chromium.org/2407303005/diff/610001/content/public/android... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:239: if (!isInsertion() || (!supportsFloatingActionMode() && !canPaste())) return; On 2016/11/03 22:46:52, boliu wrote: > remove the isInsertion condition, it was removed on trunk by > https://codereview.chromium.org/2468043002 Done.
lgtm
jinsukkim@chromium.org changed reviewers: + nyquist@chromium.org
nyquist@chromium.org: Please review changes in chrome/
On 2016/11/04 06:57:58, Jinsuk Kim wrote: > mailto:nyquist@chromium.org: Please review changes in chrome/ ping?
nyquist@chromium.org changed reviewers: + dtrainor@chromium.org
Could you look at //chrome ?
On 2016/11/08 20:26:54, nyquist wrote: > Could you look at //chrome ? Uhm... dtrainor: PTAL //chrome :-)
lgtm thanks!
https://codereview.chromium.org/2407303005/diff/620001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActionModeCallback.java (right): https://codereview.chromium.org/2407303005/diff/620001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeActionModeCallback.java:79: url, mHelper.isIncognito()); A few nit questions: - Should we assert somewhere that mHelper.isIncognito() == mTab.isIncognito()? - Should these be guaranteed to be the same? - Should we just use mTab.isIncognito()?
https://codereview.chromium.org/2407303005/diff/620001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActionModeCallback.java (right): https://codereview.chromium.org/2407303005/diff/620001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeActionModeCallback.java:79: url, mHelper.isIncognito()); On 2016/11/08 20:37:59, David Trainor wrote: > A few nit questions: > - Should we assert somewhere that mHelper.isIncognito() == mTab.isIncognito()? > - Should these be guaranteed to be the same? > - Should we just use mTab.isIncognito()? WebContents get incognito flag from Tab when initialized. So they are the same. I can switch to mTab if that seems more intuitive.
https://codereview.chromium.org/2407303005/diff/620001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActionModeCallback.java (right): https://codereview.chromium.org/2407303005/diff/620001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeActionModeCallback.java:79: url, mHelper.isIncognito()); On 2016/11/08 21:49:24, Jinsuk Kim wrote: > On 2016/11/08 20:37:59, David Trainor wrote: > > A few nit questions: > > - Should we assert somewhere that mHelper.isIncognito() == mTab.isIncognito()? > > - Should these be guaranteed to be the same? > > - Should we just use mTab.isIncognito()? > > WebContents get incognito flag from Tab when initialized. So they are the same. > I can switch to mTab if that seems more intuitive. +1 to switching to mTab. And if that allows removing isIncongito from the public content interface, even better
https://codereview.chromium.org/2407303005/diff/620001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActionModeCallback.java (right): https://codereview.chromium.org/2407303005/diff/620001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeActionModeCallback.java:79: url, mHelper.isIncognito()); On 2016/11/08 21:53:51, boliu wrote: > On 2016/11/08 21:49:24, Jinsuk Kim wrote: > > On 2016/11/08 20:37:59, David Trainor wrote: > > > A few nit questions: > > > - Should we assert somewhere that mHelper.isIncognito() == > mTab.isIncognito()? > > > - Should these be guaranteed to be the same? > > > - Should we just use mTab.isIncognito()? > > > > WebContents get incognito flag from Tab when initialized. So they are the > same. > > I can switch to mTab if that seems more intuitive. > > +1 to switching to mTab. And if that allows removing isIncongito from the public > content interface, even better Done. Removed isIncognito from public interface.
I think you can cq this now. dave already approved earlier
On 2016/11/08 23:18:02, boliu wrote: > I think you can cq this now. dave already approved earlier I'm testing build/test now. Will land the CL soon after verifying all the tests pass.
The CQ bit was checked by jinsukkim@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: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
Patchset #19 (id:660001) has been deleted
The CQ bit was checked by jinsukkim@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...)
Patchset #19 (id:680001) has been deleted
The CQ bit was checked by jinsukkim@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 jinsukkim@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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
Patchset #19 (id:700001) has been deleted
https://codereview.chromium.org/2407303005/diff/720001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2407303005/diff/720001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:644: mSelectionPopupController = new SelectionPopupController(mContext, windowAndroid, you can move this above setContainerView call on line 626, then you don't need these additional changes
https://codereview.chromium.org/2407303005/diff/720001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2407303005/diff/720001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:644: mSelectionPopupController = new SelectionPopupController(mContext, windowAndroid, On 2016/11/10 18:08:35, boliu wrote: > you can move this above setContainerView call on line 626, then you don't need > these additional changes There's a cyclic dependency. SelectionPopupController needs mImeAdapter to be ready and ImeAdapter needs mContainerView to be ready inside createImeAdapter().
ok, this is fine for now then lgtm
The CQ bit was checked by jinsukkim@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dtrainor@chromium.org Link to the patchset: https://codereview.chromium.org/2407303005/#ps720001 (title: "fixing tests")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Let embedder provide select action mode This CL tries to simplify selection action mode configuration being done through ContentViewClient that will be gone after refactoring. Now Embedders provide with a customized ActionMode.Callback. This effectively moves the relevant implementation details from ContentViewClient to the callback. SelectionPopupController pulls out all the select action mode-related methods and variables from ContentViewCore and encapsulates them. This also helps break up ContentViewCore. BUG=623783 ========== to ========== Let embedder provide select action mode This CL tries to simplify selection action mode configuration being done through ContentViewClient that will be gone after refactoring. Now Embedders provide with a customized ActionMode.Callback. This effectively moves the relevant implementation details from ContentViewClient to the callback. SelectionPopupController pulls out all the select action mode-related methods and variables from ContentViewCore and encapsulates them. This also helps break up ContentViewCore. BUG=623783 ==========
Message was sent while issue was closed.
Committed patchset #19 (id:720001)
Message was sent while issue was closed.
Description was changed from ========== Let embedder provide select action mode This CL tries to simplify selection action mode configuration being done through ContentViewClient that will be gone after refactoring. Now Embedders provide with a customized ActionMode.Callback. This effectively moves the relevant implementation details from ContentViewClient to the callback. SelectionPopupController pulls out all the select action mode-related methods and variables from ContentViewCore and encapsulates them. This also helps break up ContentViewCore. BUG=623783 ========== to ========== Let embedder provide select action mode This CL tries to simplify selection action mode configuration being done through ContentViewClient that will be gone after refactoring. Now Embedders provide with a customized ActionMode.Callback. This effectively moves the relevant implementation details from ContentViewClient to the callback. SelectionPopupController pulls out all the select action mode-related methods and variables from ContentViewCore and encapsulates them. This also helps break up ContentViewCore. BUG=623783 Committed: https://crrev.com/1eef196331dcff4093afabf4d4b5507fd84dfa94 Cr-Commit-Position: refs/heads/master@{#431299} ==========
Message was sent while issue was closed.
Patchset 19 (id:??) landed as https://crrev.com/1eef196331dcff4093afabf4d4b5507fd84dfa94 Cr-Commit-Position: refs/heads/master@{#431299} |
