Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
425693003:
Single character is coming in search box after searching empty selection (Closed)
|
Created:
6 years, 4 months ago by Cyan Modified:
6 years, 4 months ago CC:
AKVT, ankit, AviD, chromium-reviews, darin-cc_chromium.org, jam, kingshuk.j, miu+watch_chromium.org, nona+watch_chromium.org, penghuang+watch_chromium.org, no sievers, James Su, yukishiino+watch_chromium.org, yusukes+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionProperly notify ContentViewCore of an empty selection.
ContentViewCore is not notified with the empty
selection. So if We drag any normal text selection
to make empty selection,last updated text is still
one single character which is being passed as an
argument for search.So,I have added the condition
to update ContentViewCore about the empty
selection.
BUG=397954
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=290589
Patch Set 1 #
Total comments: 3
Patch Set 2 : Checked content_view_core_ before using it. #
Total comments: 2
Patch Set 3 : Modified patch after text.empty() check. #Patch Set 4 : Corrected the code as per review comments #Patch Set 5 : Rebased the patch to avoid errors #Patch Set 6 : Rebased and modified patch #Messages
Total messages: 24 (0 generated)
@jdduke PTAL
On 2014/07/28 09:26:59, Cyan wrote: > @jdduke > PTAL Will this be solved by https://codereview.chromium.org/417373003/ or affected by in it in a meaningful way? In the future, please don't submit multiple small patches related to the same issue. Ideally a single patch would be a cohesive solution to a problem (unless the patch is very large and should be broken up, or needs to be two-sided). This makes it easier to cherry pick and *much* easier for reviewers to reason about.
On 2014/07/28 14:07:19, jdduke wrote: > On 2014/07/28 09:26:59, Cyan wrote: > > @jdduke > > PTAL > > Will this be solved by https://codereview.chromium.org/417373003/ or affected by > in it in a meaningful way? In the future, please don't submit multiple small > patches related to the same issue. Ideally a single patch would be a cohesive > solution to a problem (unless the patch is very large and should be broken up, > or needs to be two-sided). This makes it easier to cherry pick and *much* > easier for reviewers to reason about. @jdduke: although both the issues look similar while reproducing,the root causes are bit different.Please suggest me whether I can merge this one with https://codereview.chromium.org/417373003/. And as you mentioned, I will take care about submitting patches in the future.Thanks.
https://codereview.chromium.org/425693003/diff/1/content/browser/renderer_hos... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/425693003/diff/1/content/browser/renderer_hos... content/browser/renderer_host/render_widget_host_view_android.cc:661: if(range.is_empty()) { Will |range| always be empty if |text| is empty?
On 2014/07/28 18:42:24, jdduke wrote: > https://codereview.chromium.org/425693003/diff/1/content/browser/renderer_hos... > File content/browser/renderer_host/render_widget_host_view_android.cc (right): > > https://codereview.chromium.org/425693003/diff/1/content/browser/renderer_hos... > content/browser/renderer_host/render_widget_host_view_android.cc:661: > if(range.is_empty()) { > Will |range| always be empty if |text| is empty? @jdduke: I checked in non-edit fields cases while dragging the selection handler and in this case both the |text| and the |range|are empty. However,|range| is always empty when |text| is empty in edit fields. But |range| empty doesn't necessitate the |text| to be empty. As, search option is not enabled in edit field cases,second scenario will never come in the bug mentioned here.
https://codereview.chromium.org/425693003/diff/1/content/browser/renderer_hos... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/425693003/diff/1/content/browser/renderer_hos... content/browser/renderer_host/render_widget_host_view_android.cc:661: if(range.is_empty()) { On 2014/07/28 18:42:24, jdduke wrote: > Will |range| always be empty if |text| is empty? The |content_View_core_| null check needs to go before this.
https://codereview.chromium.org/425693003/diff/1/content/browser/renderer_hos... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/425693003/diff/1/content/browser/renderer_hos... content/browser/renderer_host/render_widget_host_view_android.cc:661: if(range.is_empty()) { On 2014/07/30 15:40:15, jdduke wrote: > On 2014/07/28 18:42:24, jdduke wrote: > > Will |range| always be empty if |text| is empty? > > The |content_View_core_| null check needs to go before this. Acknowledged.
On 2014/07/31 05:34:16, Cyan wrote: > https://codereview.chromium.org/425693003/diff/1/content/browser/renderer_hos... > File content/browser/renderer_host/render_widget_host_view_android.cc (right): > > https://codereview.chromium.org/425693003/diff/1/content/browser/renderer_hos... > content/browser/renderer_host/render_widget_host_view_android.cc:661: > if(range.is_empty()) { > On 2014/07/30 15:40:15, jdduke wrote: > > On 2014/07/28 18:42:24, jdduke wrote: > > > Will |range| always be empty if |text| is empty? > > > > The |content_View_core_| null check needs to go before this. > > Acknowledged. @jdduke: corrected the code. PTAL
https://codereview.chromium.org/425693003/diff/20001/content/browser/renderer... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/425693003/diff/20001/content/browser/renderer... content/browser/renderer_host/render_widget_host_view_android.cc:664: if(range.is_empty()) { There should always be a space between if and the opening paranthesis. The command "git cl format" should take care of style issues like this. https://codereview.chromium.org/425693003/diff/20001/content/browser/renderer... content/browser/renderer_host/render_widget_host_view_android.cc:669: if (text.empty()) This should be DCHECK(!text.empty()), as the text should never be empty if the range is empty.
@jdduke corrected the DCHECK and space problem. PTAL.
This looks reasonable, but I'm not an owner. sievers@? Also, please update the description. The first line should not state the problem, rather, it should state what the patch does, so instead of "Single character is coming in search box after searching empty selection" it should be something like "Properly notify ContentViewCore of an empty selection"
jdduke@ Thanks for the suggestion.I have modified the description. sievers@ PTAL
On 2014/08/01 15:44:33, Cyan wrote: > jdduke@ > Thanks for the suggestion.I have > modified the description. > sievers@ > PTAL @sievers, Please have a look.
sievers@ may still be on vacation, adding aelias@ as an alternate owner.
lgtm
The CQ bit was checked by sayan.nayak@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sayan.nayak@samsung.com/425693003/60001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/43567) win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/48741) android_aosp on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...) android_chromium_gn_compile_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...) android_clang_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...) android_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg/bui...) chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios_dbg_simulator on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...) ios_rel_device_ninja on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win8_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...) win_chromium_compile_dbg on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/48745) android_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg/bui...) chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios_dbg_simulator on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...) ios_rel_device_ninja on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win8_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
The CQ bit was checked by sayan.nayak@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sayan.nayak@samsung.com/425693003/100001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_dbg_triggered_tests on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tri...)
Message was sent while issue was closed.
Committed patchset #6 (100001) as 290589 |
