Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2721813002:
Make SelectionPopupController.ShowPastePopup only be triggered by Blink (Closed)
|
Created:
3 years, 9 months ago by amaralp Modified:
3 years, 9 months ago CC:
agrieve+watch_chromium.org, chromium-reviews, darin-cc_chromium.org, jam, nona+watch_chromium.org, shuchen+watch_chromium.org, James Su, yusukes+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake SelectionPopupController.ShowPastePopup only be triggered by Blink
This patch is progress towards the goal of having Blink triggering all
of clank's SelectActionMode context menus. In particular this patch
has Blink trigger the menu when an insertion handle is tapped or
dragged.
BUG=627234
Review-Url: https://codereview.chromium.org/2721813002
Cr-Original-Commit-Position: refs/heads/master@{#454203}
Committed: https://chromium.googlesource.com/chromium/src/+/f88569b6a113509e7e7245ca297221701d07d4a3
Review-Url: https://codereview.chromium.org/2721813002
Cr-Commit-Position: refs/heads/master@{#458525}
Committed: https://chromium.googlesource.com/chromium/src/+/8c8279c96ec0483bfc86fddd66a860db2dccae18
Patch Set 1 #Patch Set 2 : git cl try #
Total comments: 5
Patch Set 3 : TODOs to move methods out of public interface #Patch Set 4 : INSERTION_HANDLE_MOVED should change location of paste menu without creating a new menu #Patch Set 5 : Rebased for Bo #
Total comments: 2
Patch Set 6 : Rebase #Messages
Total messages: 55 (29 generated)
amaralp@chromium.org changed reviewers: + aelias@chromium.org
PTAL
The CQ bit was checked by amaralp@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: This issue passed the CQ dry run.
The CQ bit was checked by amaralp@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: This issue passed the CQ dry run.
lgtm
amaralp@chromium.org changed reviewers: + boliu@chromium.org
Added boliu@ for content/browser/web_contents OWNER
amaralp@chromium.org changed reviewers: + jinsukkim@chromium.org
On 2017/02/28 at 21:44:52, amaralp wrote: > Added boliu@ for content/browser/web_contents OWNER boliu@, have you had a chance to take a look at this CL? I'd like to get it in before the branch cut tomorrow. Thanks in advance.
sorry, this one fell through my queue.. https://codereview.chromium.org/2721813002/diff/20001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java (right): https://codereview.chromium.org/2721813002/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:796: mWebContents.showContextMenuAtPoint(xAnchor, yAnchor); it's not clear to me why paste popup is being replaced with context menu here? https://codereview.chromium.org/2721813002/diff/20001/content/public/android/... File content/public/android/java/src/org/chromium/content_public/browser/WebContents.java (right): https://codereview.chromium.org/2721813002/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content_public/browser/WebContents.java:130: void showContextMenuAtPoint(int x, int y); this doesn't need to be on the public interface, and would go against the idea that blink triggers all context menus, since this exposes triggering context menu to content embedders Looks like there's other methods here that's called by SelectionPopupController only. They should be removed as well, and probably should not go through WebContentsImpl either. But that's for a refactor for another time.
https://codereview.chromium.org/2721813002/diff/20001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java (right): https://codereview.chromium.org/2721813002/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:796: mWebContents.showContextMenuAtPoint(xAnchor, yAnchor); On 2017/03/02 at 00:48:14, boliu wrote: > it's not clear to me why paste popup is being replaced with context menu here? After this CL showPastePopup should only be called because of a context menu event. In later patches (crrev.com/2722273002) additional information will be passed to showPastePopup() (like if "Select All" should be displayed) and SelectionPopupController wouldn't be able to pass that information (unless it stores extra state which I am trying to avoid). https://codereview.chromium.org/2721813002/diff/20001/content/public/android/... File content/public/android/java/src/org/chromium/content_public/browser/WebContents.java (right): https://codereview.chromium.org/2721813002/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content_public/browser/WebContents.java:130: void showContextMenuAtPoint(int x, int y); On 2017/03/02 at 00:48:14, boliu wrote: > this doesn't need to be on the public interface, and would go against the idea that blink triggers all context menus, since this exposes triggering context menu to content embedders > > Looks like there's other methods here that's called by SelectionPopupController only. They should be removed as well, and probably should not go through WebContentsImpl either. But that's for a refactor for another time. I had a feeling I was abusing the public interface. What would be the right way to do this? Should I call native methods directly from SelectionPopupController?
https://codereview.chromium.org/2721813002/diff/20001/content/public/android/... File content/public/android/java/src/org/chromium/content_public/browser/WebContents.java (right): https://codereview.chromium.org/2721813002/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content_public/browser/WebContents.java:130: void showContextMenuAtPoint(int x, int y); On 2017/03/02 01:11:33, amaralp wrote: > On 2017/03/02 at 00:48:14, boliu wrote: > > this doesn't need to be on the public interface, and would go against the idea > that blink triggers all context menus, since this exposes triggering context > menu to content embedders > > > > Looks like there's other methods here that's called by > SelectionPopupController only. They should be removed as well, and probably > should not go through WebContentsImpl either. But that's for a refactor for > another time. > > I had a feeling I was abusing the public interface. What would be the right way > to do this? Should I call native methods directly from SelectionPopupController? For this CL, just remove this, and remove @Override on Impl class. Long term, probably should add a native side to SelectionPopupController so it doesn't have to route things through WebContents.
On 2017/03/02 at 02:28:59, boliu wrote: > https://codereview.chromium.org/2721813002/diff/20001/content/public/android/... > File content/public/android/java/src/org/chromium/content_public/browser/WebContents.java (right): > > https://codereview.chromium.org/2721813002/diff/20001/content/public/android/... > content/public/android/java/src/org/chromium/content_public/browser/WebContents.java:130: void showContextMenuAtPoint(int x, int y); > On 2017/03/02 01:11:33, amaralp wrote: > > On 2017/03/02 at 00:48:14, boliu wrote: > > > this doesn't need to be on the public interface, and would go against the idea > > that blink triggers all context menus, since this exposes triggering context > > menu to content embedders > > > > > > Looks like there's other methods here that's called by > > SelectionPopupController only. They should be removed as well, and probably > > should not go through WebContentsImpl either. But that's for a refactor for > > another time. > > > > I had a feeling I was abusing the public interface. What would be the right way > > to do this? Should I call native methods directly from SelectionPopupController? > > For this CL, just remove this, and remove @Override on Impl class. > > Long term, probably should add a native side to SelectionPopupController so it doesn't have to route things through WebContents. WebContentsImpl isn't a public class so I can't access it outside of its package: https://cs.chromium.org/chromium/src/content/public/android/java/src/org/chro...
On 2017/03/02 02:49:43, amaralp wrote: > On 2017/03/02 at 02:28:59, boliu wrote: > > > https://codereview.chromium.org/2721813002/diff/20001/content/public/android/... > > File > content/public/android/java/src/org/chromium/content_public/browser/WebContents.java > (right): > > > > > https://codereview.chromium.org/2721813002/diff/20001/content/public/android/... > > > content/public/android/java/src/org/chromium/content_public/browser/WebContents.java:130: > void showContextMenuAtPoint(int x, int y); > > On 2017/03/02 01:11:33, amaralp wrote: > > > On 2017/03/02 at 00:48:14, boliu wrote: > > > > this doesn't need to be on the public interface, and would go against the > idea > > > that blink triggers all context menus, since this exposes triggering context > > > menu to content embedders > > > > > > > > Looks like there's other methods here that's called by > > > SelectionPopupController only. They should be removed as well, and probably > > > should not go through WebContentsImpl either. But that's for a refactor for > > > another time. > > > > > > I had a feeling I was abusing the public interface. What would be the right > way > > > to do this? Should I call native methods directly from > SelectionPopupController? > > > > For this CL, just remove this, and remove @Override on Impl class. > > > > Long term, probably should add a native side to SelectionPopupController so it > doesn't have to route things through WebContents. > > WebContentsImpl isn't a public class so I can't access it outside of its > package: > https://cs.chromium.org/chromium/src/content/public/android/java/src/org/chro... Err.. and that DEPS rule is definitely not in place. How about this. Add TODO comment on all the methods that removed that they should be removed?
On 2017/03/02 at 02:55:44, boliu wrote: > On 2017/03/02 02:49:43, amaralp wrote: > > On 2017/03/02 at 02:28:59, boliu wrote: > > > > > https://codereview.chromium.org/2721813002/diff/20001/content/public/android/... > > > File > > content/public/android/java/src/org/chromium/content_public/browser/WebContents.java > > (right): > > > > > > > > https://codereview.chromium.org/2721813002/diff/20001/content/public/android/... > > > > > content/public/android/java/src/org/chromium/content_public/browser/WebContents.java:130: > > void showContextMenuAtPoint(int x, int y); > > > On 2017/03/02 01:11:33, amaralp wrote: > > > > On 2017/03/02 at 00:48:14, boliu wrote: > > > > > this doesn't need to be on the public interface, and would go against the > > idea > > > > that blink triggers all context menus, since this exposes triggering context > > > > menu to content embedders > > > > > > > > > > Looks like there's other methods here that's called by > > > > SelectionPopupController only. They should be removed as well, and probably > > > > should not go through WebContentsImpl either. But that's for a refactor for > > > > another time. > > > > > > > > I had a feeling I was abusing the public interface. What would be the right > > way > > > > to do this? Should I call native methods directly from > > SelectionPopupController? > > > > > > For this CL, just remove this, and remove @Override on Impl class. > > > > > > Long term, probably should add a native side to SelectionPopupController so it > > doesn't have to route things through WebContents. > > > > WebContentsImpl isn't a public class so I can't access it outside of its > > package: > > https://cs.chromium.org/chromium/src/content/public/android/java/src/org/chro... > > Err.. and that DEPS rule is definitely not in place. How about this. Add TODO comment on all the methods that removed that they should be removed? So add a TODO to every non-public method in WebContents.java?
On 2017/03/02 03:48:43, amaralp wrote: > On 2017/03/02 at 02:55:44, boliu wrote: > > On 2017/03/02 02:49:43, amaralp wrote: > > > On 2017/03/02 at 02:28:59, boliu wrote: > > > > > > > > https://codereview.chromium.org/2721813002/diff/20001/content/public/android/... > > > > File > > > > content/public/android/java/src/org/chromium/content_public/browser/WebContents.java > > > (right): > > > > > > > > > > > > https://codereview.chromium.org/2721813002/diff/20001/content/public/android/... > > > > > > > > content/public/android/java/src/org/chromium/content_public/browser/WebContents.java:130: > > > void showContextMenuAtPoint(int x, int y); > > > > On 2017/03/02 01:11:33, amaralp wrote: > > > > > On 2017/03/02 at 00:48:14, boliu wrote: > > > > > > this doesn't need to be on the public interface, and would go against > the > > > idea > > > > > that blink triggers all context menus, since this exposes triggering > context > > > > > menu to content embedders > > > > > > > > > > > > Looks like there's other methods here that's called by > > > > > SelectionPopupController only. They should be removed as well, and > probably > > > > > should not go through WebContentsImpl either. But that's for a refactor > for > > > > > another time. > > > > > > > > > > I had a feeling I was abusing the public interface. What would be the > right > > > way > > > > > to do this? Should I call native methods directly from > > > SelectionPopupController? > > > > > > > > For this CL, just remove this, and remove @Override on Impl class. > > > > > > > > Long term, probably should add a native side to SelectionPopupController > so it > > > doesn't have to route things through WebContents. > > > > > > WebContentsImpl isn't a public class so I can't access it outside of its > > > package: > > > > https://cs.chromium.org/chromium/src/content/public/android/java/src/org/chro... > > > > Err.. and that DEPS rule is definitely not in place. How about this. Add TODO > comment on all the methods that removed that they should be removed? > > So add a TODO to every non-public method in WebContents.java? All methods that's accessed by SelectionPopupController *only*. I'm assuming stuff like copy/paste/selectAll falls into that group. Should say something like, this method should not be public. WebContents is an interface, so everything is public.
On 2017/03/02 at 03:55:28, boliu wrote: > On 2017/03/02 03:48:43, amaralp wrote: > > On 2017/03/02 at 02:55:44, boliu wrote: > > > On 2017/03/02 02:49:43, amaralp wrote: > > > > On 2017/03/02 at 02:28:59, boliu wrote: > > > > > > > > > > > https://codereview.chromium.org/2721813002/diff/20001/content/public/android/... > > > > > File > > > > > > content/public/android/java/src/org/chromium/content_public/browser/WebContents.java > > > > (right): > > > > > > > > > > > > > > > > https://codereview.chromium.org/2721813002/diff/20001/content/public/android/... > > > > > > > > > > > content/public/android/java/src/org/chromium/content_public/browser/WebContents.java:130: > > > > void showContextMenuAtPoint(int x, int y); > > > > > On 2017/03/02 01:11:33, amaralp wrote: > > > > > > On 2017/03/02 at 00:48:14, boliu wrote: > > > > > > > this doesn't need to be on the public interface, and would go against > > the > > > > idea > > > > > > that blink triggers all context menus, since this exposes triggering > > context > > > > > > menu to content embedders > > > > > > > > > > > > > > Looks like there's other methods here that's called by > > > > > > SelectionPopupController only. They should be removed as well, and > > probably > > > > > > should not go through WebContentsImpl either. But that's for a refactor > > for > > > > > > another time. > > > > > > > > > > > > I had a feeling I was abusing the public interface. What would be the > > right > > > > way > > > > > > to do this? Should I call native methods directly from > > > > SelectionPopupController? > > > > > > > > > > For this CL, just remove this, and remove @Override on Impl class. > > > > > > > > > > Long term, probably should add a native side to SelectionPopupController > > so it > > > > doesn't have to route things through WebContents. > > > > > > > > WebContentsImpl isn't a public class so I can't access it outside of its > > > > package: > > > > > > https://cs.chromium.org/chromium/src/content/public/android/java/src/org/chro... > > > > > > Err.. and that DEPS rule is definitely not in place. How about this. Add TODO > > comment on all the methods that removed that they should be removed? > > > > So add a TODO to every non-public method in WebContents.java? > > All methods that's accessed by SelectionPopupController *only*. I'm assuming stuff like copy/paste/selectAll falls into that group. Should say something like, this method should not be public. > > WebContents is an interface, so everything is public. I've added a TODO to all the methods in WebContents.java that are accessed by SelectionPopupController and aren't used outside of content.
lgtm
The CQ bit was checked by amaralp@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from aelias@chromium.org Link to the patchset: https://codereview.chromium.org/2721813002/#ps40001 (title: "TODOs to move methods out of public interface")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1488432661084850,
"parent_rev": "465d378adf5c4da7daf55b6a2d7ebdcbbea71327", "commit_rev":
"f88569b6a113509e7e7245ca297221701d07d4a3"}
Message was sent while issue was closed.
Description was changed from ========== Make SelectionPopupController.ShowPastePopup only be triggered by Blink This patch is progress towards the goal of having Blink triggering all of clank's SelectActionMode context menus. In particular this patch has Blink trigger the menu when an insertion handle is tapped or dragged. BUG=627234 ========== to ========== Make SelectionPopupController.ShowPastePopup only be triggered by Blink This patch is progress towards the goal of having Blink triggering all of clank's SelectActionMode context menus. In particular this patch has Blink trigger the menu when an insertion handle is tapped or dragged. BUG=627234 Review-Url: https://codereview.chromium.org/2721813002 Cr-Commit-Position: refs/heads/master@{#454203} Committed: https://chromium.googlesource.com/chromium/src/+/f88569b6a113509e7e7245ca2972... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/f88569b6a113509e7e7245ca2972...
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in https://codereview.chromium.org/2727203003/ by amaralp@chromium.org. The reason for reverting is: Caused flakiness (crbug.com/697934).
The CQ bit was checked by amaralp@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: This issue passed the CQ dry run.
PTAL, I've fixed the flakiness in that this CL caused by: 1) Landing CL (crrev.com/2733603003) which made insertion handles that were created by a tap/long-press trigger INSERTION_HANDLE_SHOWN instead of INSERTION_HANDLE_MOVED. 2) Adding Patch 4 of this CL which makes it so INSERTION_HANDLE_MOVED re-positions an existing menu instead of creating a new one. This should lead to less race conditions in general.
On 2017/03/17 22:28:31, amaralp wrote: > PTAL, I've fixed the flakiness in that this CL caused by: > > 1) Landing CL (crrev.com/2733603003) which made insertion handles that were > created by a tap/long-press trigger INSERTION_HANDLE_SHOWN instead of > INSERTION_HANDLE_MOVED. > > 2) Adding Patch 4 of this CL which makes it so INSERTION_HANDLE_MOVED > re-positions an existing menu instead of creating a new one. This should lead to > less race conditions in general. Can you upload a patch set that's the rebased version of what landed previously, so I can review only the additional changes
On 2017/03/17 at 22:30:50, boliu wrote: > On 2017/03/17 22:28:31, amaralp wrote: > > PTAL, I've fixed the flakiness in that this CL caused by: > > > > 1) Landing CL (crrev.com/2733603003) which made insertion handles that were > > created by a tap/long-press trigger INSERTION_HANDLE_SHOWN instead of > > INSERTION_HANDLE_MOVED. > > > > 2) Adding Patch 4 of this CL which makes it so INSERTION_HANDLE_MOVED > > re-positions an existing menu instead of creating a new one. This should lead to > > less race conditions in general. > > Can you upload a patch set that's the rebased version of what landed previously, so I can review only the additional changes Done.
On 2017/03/17 22:40:07, amaralp wrote: > On 2017/03/17 at 22:30:50, boliu wrote: > > On 2017/03/17 22:28:31, amaralp wrote: > > > PTAL, I've fixed the flakiness in that this CL caused by: > > > > > > 1) Landing CL (crrev.com/2733603003) which made insertion handles that were > > > created by a tap/long-press trigger INSERTION_HANDLE_SHOWN instead of > > > INSERTION_HANDLE_MOVED. > > > > > > 2) Adding Patch 4 of this CL which makes it so INSERTION_HANDLE_MOVED > > > re-positions an existing menu instead of creating a new one. This should > lead to > > > less race conditions in general. > > > > Can you upload a patch set that's the rebased version of what landed > previously, so I can review only the additional changes > > Done. Err, I meant upload the original (rebased) CL in completion, and then the new version in completion. code review tool can show diffs between patch sets. But I guess this works too..
https://codereview.chromium.org/2721813002/diff/80001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java (right): https://codereview.chromium.org/2721813002/diff/80001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:232: PastePopupMenuDelegate delegate = new PastePopupMenuDelegate() { does it matter if mPastePopupMenu already exists here? or should that never happen, so maybe there should be an assert here?
Patchset #8 (id:140001) has been deleted
Patchset #6 (id:100001) has been deleted
The CQ bit was checked by amaralp@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: This issue passed the CQ dry run.
https://codereview.chromium.org/2721813002/diff/80001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java (right): https://codereview.chromium.org/2721813002/diff/80001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:232: PastePopupMenuDelegate delegate = new PastePopupMenuDelegate() { On 2017/03/17 at 22:54:20, boliu wrote: > does it matter if mPastePopupMenu already exists here? or should that never happen, so maybe there should be an assert here? Addressed in crrev.com/2757313003.
If no one has any more comments then I'll go ahead and land this.
lgtm
The CQ bit was checked by amaralp@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from aelias@chromium.org Link to the patchset: https://codereview.chromium.org/2721813002/#ps120001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1490126227186330,
"parent_rev": "45bf07c61f3958ca4a8f7ce5754fa102234b9e0e", "commit_rev":
"8c8279c96ec0483bfc86fddd66a860db2dccae18"}
Message was sent while issue was closed.
Description was changed from ========== Make SelectionPopupController.ShowPastePopup only be triggered by Blink This patch is progress towards the goal of having Blink triggering all of clank's SelectActionMode context menus. In particular this patch has Blink trigger the menu when an insertion handle is tapped or dragged. BUG=627234 Review-Url: https://codereview.chromium.org/2721813002 Cr-Commit-Position: refs/heads/master@{#454203} Committed: https://chromium.googlesource.com/chromium/src/+/f88569b6a113509e7e7245ca2972... ========== to ========== Make SelectionPopupController.ShowPastePopup only be triggered by Blink This patch is progress towards the goal of having Blink triggering all of clank's SelectActionMode context menus. In particular this patch has Blink trigger the menu when an insertion handle is tapped or dragged. BUG=627234 Review-Url: https://codereview.chromium.org/2721813002 Cr-Original-Commit-Position: refs/heads/master@{#454203} Committed: https://chromium.googlesource.com/chromium/src/+/f88569b6a113509e7e7245ca2972... Review-Url: https://codereview.chromium.org/2721813002 Cr-Commit-Position: refs/heads/master@{#458525} Committed: https://chromium.googlesource.com/chromium/src/+/8c8279c96ec0483bfc86fddd66a8... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:120001) as https://chromium.googlesource.com/chromium/src/+/8c8279c96ec0483bfc86fddd66a8... |
