Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2577963002:
Always show 'WEB SEARCH' item in action mode on Chrome (Closed)
|
Created:
4 years ago by Jinsuk Kim Modified:
4 years ago CC:
chromium-reviews, jam, darin-cc_chromium.org, agrieve+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAlways show 'WEB SEARCH' item in action mode on Chrome
There was a regression not showing web search menu item in
select action mode on Chrome if an activity for ACTION_WEB_SEARCH
cannot be found in the system. Chrome should always show it
since it can show the search result on a new tab. This CL fixes it
by showing the item by default, and having other embedder (webview)
configure it.
BUG=674104
Committed: https://crrev.com/1f1083f8bf08eeda6a0e9a48dc9e16a8dca15b9f
Cr-Commit-Position: refs/heads/master@{#439000}
Patch Set 1 #
Total comments: 2
Patch Set 2 : android_webview #
Messages
Total messages: 24 (16 generated)
jinsukkim@chromium.org changed reviewers: + tedchoc@chromium.org
FYI this was handled by ContentViewClient.doesPerformWebsearch(). Had thought we could do without it since embedders can handle menu items in more direct manner (by overriding ActionMode.Callback#onActionItemClicked) but it was also used for visibility control, which went missing in the refactoring https://crrrev.com/2407303005 by mistake. I'm restoring it in this CL.
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: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
https://codereview.chromium.org/2577963002/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java (right): https://codereview.chromium.org/2577963002/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:961: private boolean isWebSearchAvailable() { I think this entire logic is better to be in AwActionModeCallback. In Chrome, we always want to show web search, so I would move this there and change how it calculates its allowedItems list.
Patchset #2 (id:20001) has been deleted
Description was changed from ========== Always show 'WEB SEARCH' item in action mode on Chrome There was a regression not showing web search menu item in select action mode on Chrome if an activity for ACTION_WEB_SEARCH cannot be found in the system. Chrome should always show it since it can show the search result on a new tab. This CL fixes it. BUG=674104 ========== to ========== Always show 'WEB SEARCH' item in action mode on Chrome There was a regression not showing web search menu item in select action mode on Chrome if an activity for ACTION_WEB_SEARCH cannot be found in the system. Chrome should always show it since it can show the search result on a new tab. This CL fixes it by showing the item by default, and having other embedder (webview) configure it. BUG=674104 ==========
jinsukkim@chromium.org changed reviewers: + boliu@chromium.org
https://codereview.chromium.org/2577963002/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java (right): https://codereview.chromium.org/2577963002/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:961: private boolean isWebSearchAvailable() { On 2016/12/15 18:42:19, Ted C wrote: > I think this entire logic is better to be in AwActionModeCallback. In Chrome, > we always want to show web search, so I would move this there and change how it > calculates its allowedItems list. Done. I take it that what to show/hide on the action bar should be pre-configured through allowedItem list as much as possible.
lgtm
On 2016/12/15 23:20:05, Jinsuk Kim wrote: > https://codereview.chromium.org/2577963002/diff/1/content/public/android/java... > File > content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java > (right): > > https://codereview.chromium.org/2577963002/diff/1/content/public/android/java... > content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:961: > private boolean isWebSearchAvailable() { > On 2016/12/15 18:42:19, Ted C wrote: > > I think this entire logic is better to be in AwActionModeCallback. In Chrome, > > we always want to show web search, so I would move this there and change how > it > > calculates its allowedItems list. > > Done. I take it that what to show/hide on the action bar should be > pre-configured through allowedItem list as much as possible. lgtm ... That is how I understand the new class layout.
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: This issue passed the CQ dry run.
The CQ bit was checked by jinsukkim@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1481852443669080,
"parent_rev": "f5ea39716da778a86cca95c692cccc0ff4ff19ef", "commit_rev":
"66e06097afc05ae3c5f7d5224b11461e24546df8"}
Message was sent while issue was closed.
Description was changed from ========== Always show 'WEB SEARCH' item in action mode on Chrome There was a regression not showing web search menu item in select action mode on Chrome if an activity for ACTION_WEB_SEARCH cannot be found in the system. Chrome should always show it since it can show the search result on a new tab. This CL fixes it by showing the item by default, and having other embedder (webview) configure it. BUG=674104 ========== to ========== Always show 'WEB SEARCH' item in action mode on Chrome There was a regression not showing web search menu item in select action mode on Chrome if an activity for ACTION_WEB_SEARCH cannot be found in the system. Chrome should always show it since it can show the search result on a new tab. This CL fixes it by showing the item by default, and having other embedder (webview) configure it. BUG=674104 Review-Url: https://codereview.chromium.org/2577963002 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Always show 'WEB SEARCH' item in action mode on Chrome There was a regression not showing web search menu item in select action mode on Chrome if an activity for ACTION_WEB_SEARCH cannot be found in the system. Chrome should always show it since it can show the search result on a new tab. This CL fixes it by showing the item by default, and having other embedder (webview) configure it. BUG=674104 Review-Url: https://codereview.chromium.org/2577963002 ========== to ========== Always show 'WEB SEARCH' item in action mode on Chrome There was a regression not showing web search menu item in select action mode on Chrome if an activity for ACTION_WEB_SEARCH cannot be found in the system. Chrome should always show it since it can show the search result on a new tab. This CL fixes it by showing the item by default, and having other embedder (webview) configure it. BUG=674104 Committed: https://crrev.com/1f1083f8bf08eeda6a0e9a48dc9e16a8dca15b9f Cr-Commit-Position: refs/heads/master@{#439000} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/1f1083f8bf08eeda6a0e9a48dc9e16a8dca15b9f Cr-Commit-Position: refs/heads/master@{#439000} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
