Issue metadata
Sign in to add a comment
|
Allow Omnibox to Match in Port Number
Reported by
zhifangy...@gmail.com,
Mar 17 2016
|
||||||||||||||||||||
Issue descriptionUserAgent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_3) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/49.0.2623.87 Safari/537.36 Steps to reproduce the problem: 1. visit this url http://101.200.175.188:9880/account?id=2fb4ba81e9d211e5870300163e00371a 2. open new tab 3. type 9880 What is the expected behavior? the url should show up in auto completion list What went wrong? the url now showing up in auto completion list Did this work before? No Chrome version: 49.0.2623.87 Channel: stable OS Version: OS X 10.11.3 Flash Version: Shockwave Flash 21.0 r0 currently, change step3 to: type 101, then the url will show up in completion list, but it is not good enough, 9880 is more identifiable. Why can't I search any part of the url??
,
Mar 19 2016
The only thing that works is typing 101 - i.e. typing 200 or any other part of the URL does not cause the URL to show up in the completion list. So I think the intent is to match the start of the IP address rather than any portion of it.
,
Mar 19 2016
We're supposed to do mid-string matches in the hostname, so 200 should work. As for searching the port, I suspect we've just never indexed that, because it's so rarely important. But by that same token indexing it and searching in it probably wouldn't be hugely detrimental, it would just rarely have an effect. Mark, thoughts?
,
Mar 28 2016
It's very important sometime! I am a developer and I use different ports for different apps in dev server, it will be very convenient if I can just type the port and hit enter to visit different apps. Chrome has been a very good tool for developers, please make it better.
,
Sep 19 2016
I finally looked into this. Chrome indexes the full URL. However, the part of the system that score/returns URL gives no credit to matches in what Chrome thinks is the top-level domain part of the URL (e.g., .com, .gov). It looks like this code is inadvertently preventing matches in the port section as well. This is an easy fix. I don't think we need to test it given how few URLs people run into nowadays have port numbers explicitly in them. pkasting@: you have an instinct for these type of things. Do you think port numbers should score as highly as the rest of the hostname, or more like a path component, or more like a ?cgi fragment?
,
Sep 19 2016
Not hostname. One of the other two I think. I'd probably start with CGI fragment (which is the lowest-scoring, right?) and see what that gets us.
,
Sep 19 2016
,
Sep 20 2016
Marking as available. I will not have time to fix this correctly. Also see bug 448659, which covers a similar situation that could be fixed by the more general solution of using the proper URL segmentation mechanism.
,
Sep 20 2016
[clarifying summary]
,
Dec 7 2016
For reference, the way to fix this properly is expressed in pkasting's comment on https://codereview.chromium.org/2355053003/ (which was my simple fix). The central point is >>> It seems like rewriting [scored_history_match GetTopicality()] to use the standard machinery for parsing URLs can locate things like the hostname boundaries, port, CGI params, etc. with complete accuracy, in noticeably less code, and I'm not convinced will lose performance. I think it is worth taking the time to do. >>>
,
Jul 7 2017
Gimme
,
Jul 10 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/efcd3a7e1ec0e95cec770ff5340cf1ba6b430385 commit efcd3a7e1ec0e95cec770ff5340cf1ba6b430385 Author: Tommy C. Li <tommycli@chromium.org> Date: Mon Jul 10 23:42:15 2017 Omnibox: Refactor ScoredHistoryMatch URL parsing Previously, we determined which part of the URL the match was in by by searching the formatted URL string (in string16 form) for delimiters. After this refactor, we do this by using the "official" GURL component offsets adjusted for URL formatting. This is good both as a refactor, and also to lay the groundwork for adding |match_in_subdomain| and |match_in_path| flags. This CL doesn't actually add the above flags, and is intended to provide identical functionality as before (which is why no tests changed). Bug: 732582 , 595524, 448659 Change-Id: I133c2ecb462597941b7284fd88f99e55f341f6b4 Reviewed-on: https://chromium-review.googlesource.com/564300 Commit-Queue: Tommy Li <tommycli@chromium.org> Reviewed-by: Justin Donnelly <jdonnelly@chromium.org> Cr-Commit-Position: refs/heads/master@{#485446} [modify] https://crrev.com/efcd3a7e1ec0e95cec770ff5340cf1ba6b430385/components/omnibox/browser/scored_history_match.cc [modify] https://crrev.com/efcd3a7e1ec0e95cec770ff5340cf1ba6b430385/components/omnibox/browser/scored_history_match.h [modify] https://crrev.com/efcd3a7e1ec0e95cec770ff5340cf1ba6b430385/components/omnibox/browser/scored_history_match_unittest.cc
,
Jul 11 2017
Nice! For the record, I would like to be at least CCed or the reviewer on code used in ranking, especially code that I wrote. :-) tommycli@: are you planning to finish the rest of this bug? It should be easy, and as I wrote in comment #5, I don't think we need to test it as a field trial given how few URLs people run into nowadays have port numbers explicitly in them.
,
Jul 11 2017
Hey mpearson: I am indeed planning on taking this to the end. I will be marking you as the reviewer on that CL. I wasn't trying to bypass you -- I didn't realize you were the author of the ranking code. ;) Thanks!
,
Jul 19 2017
,
Aug 8 2017
mpearson@: Is it worth adding a per-file OWNERS entry for things you particularly care about? tommycli@: Is this still on your plate?
,
Aug 8 2017
re c#16: Yes I'm still planning on tackling this eventually, and my work in the patch above is partially relevant. If someone else has the motivation to investigate this before me though, by all means take it.
,
Aug 8 2017
> mpearson@: Is it worth adding a per-file OWNERS entry for > things you particularly care about? I think that's too heavy-handed. I think stating a preference and letting people use their judgment about when it's appropriate is fine.
,
Aug 8 2017
Marking as Available so someone else can tackle as wanted. It's not an immediate TODO for me.
Next person... looking at the rfind('.', ...) in the above CL is the right place to start to make this work correctly for IP addresses...
,
Aug 8 2017
FWIW, I strongly encourage mpearson being listed as an OWNER in the directory list alongside other people, or at least per-file. OWNERS lists aren't heavy-handed so much as informational. When people come in to an area, they tell you who you can send reviews to and who you can contact to learn more. I don't view being listed in one as a way to control things but as a recognition of "I know things here and I'm happy to help". In that sense Mark would be eminently appropriate.
,
Aug 8 2017
Agree with Peter. I'm thinking of adding myself to the OWNER files on some of them, but I feel that Mark should go in way before me.
,
Aug 8 2017
I don't understand. I am already an OWNER of the directory. I suppose I could add a restriction to the directory so I'm the sole owner of things like scored_history_match*, in_memory_url_index_*, and maybe a few others but that seems heavy-handed. And I feel like putting a comment in the OWNERS files about preferred reviewers per-file seems not likely to do much. It's not like we're getting many reviews like the one that brought up this issue from outside the team, and people inside the team aren't likely to bother reading the owners files for comments. Besides, don't most people pick reviewers by looking at who seems to be familiar with the relevant code? I don't see how a comment adds value here.
,
Aug 8 2017
My comment was based on the assumption that you were not listed as an OWNER in the overall list. If you are, great! It's sufficient in my book if other OWNERS know "Mark is the best reviewer for this area".
,
Aug 9
This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue. Sorry for the inconvenience if the bug really should have been left as Available. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Aug 9
Still valid, still low priority.
,
Jan 8
The NextAction date has arrived: 2019-01-08 |
|||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||
Comment 1 by zhifangy...@gmail.com
, Mar 17 2016