Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1869783005:
[Blimp Client] Fixes up URL before sending it to Engine. (Closed)
|
Created:
4 years, 8 months ago by haibinlu Modified:
4 years, 8 months ago CC:
chromium-reviews, anandc+watch-blimp_chromium.org, maniscalco+watch-blimp_chromium.org, sriramsr+watch-blimp_chromium.org, nyquist+watch-blimp_chromium.org, marcinjb+watch-blimp_chromium.org, jessicag+watch-blimp_chromium.org, kmarshall+watch-blimp_chromium.org, dtrainor+watch-blimp_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Blimp Client] Fixes up URL before sending it to Engine.
Uses url_formatter::FixupURL to convert "google.com" to "http://google.com".
It also convert "movie" to "http://movie/" (which is a valid GURL). We check if the host has a '.' in it. If not, we use google search. This heuristic is wrong since valid GRULs such as "http://[2001:db8::2]/" (IPv6 literals) and "chrome://version/" do not have '.' in their host. However, it allows us to test v0.5 in a much easier way.
BUG=601117
Committed: https://crrev.com/87714dd50611f6ea33b93383d9af4572f7ccb661
Cr-Commit-Position: refs/heads/master@{#389543}
Patch Set 1 #
Total comments: 13
Patch Set 2 : #
Total comments: 2
Patch Set 3 : #
Total comments: 4
Patch Set 4 : #Patch Set 5 : Fixes NavigationFeatureTest #
Total comments: 6
Patch Set 6 : #
Messages
Total messages: 28 (9 generated)
Description was changed from ========== [Blimp Client] Fixes up URL before sending it to Engine. Uses url_formatter::FixupURL to get "google.com" to "http://google.com". It will convert "movie" to "http://movie/" (which is a valid GURL), we check if the host has a '.' in it. If not, we use google search. This latter heuristic is wrong since "http://[2001:db8::2]/" contains valid IPv6 literals, and ""chrome://version/" does not have '.'. However, it allows us to test v0.5 in a much easiler way. BUG= ========== to ========== [Blimp Client] Fixes up URL before sending it to Engine. Uses url_formatter::FixupURL to convert "google.com" to "http://google.com". It also convert "movie" to "http://movie/" (which is a valid GURL). We check if the host has a '.' in it. If not, we use google search. This heuristic is wrong since valid GRULs such as "http://[2001:db8::2]/" (IPv6 literals) and "chrome://version/" do not have '.' in their host. However, it allows us to test v0.5 in a much easier way. BUG=601117 ==========
haibinlu@chromium.org changed reviewers: + kmarshall@chromium.org
https://codereview.chromium.org/1869783005/diff/1/blimp/client/feature/naviga... File blimp/client/feature/navigation_feature.cc (right): https://codereview.chromium.org/1869783005/diff/1/blimp/client/feature/naviga... blimp/client/feature/navigation_feature.cc:43: // Simplified heuristic for v0.5 only. Add explanation as to *what* this is doing. https://codereview.chromium.org/1869783005/diff/1/blimp/client/feature/naviga... blimp/client/feature/navigation_feature.cc:46: if (!url.is_valid() || url.host().find('.') == std::string::npos) { Add comment on the reason and behavior of this heuristic. https://codereview.chromium.org/1869783005/diff/1/blimp/client/feature/naviga... blimp/client/feature/navigation_feature.cc:47: url = url_formatter::FixupURL("https://www.google.com/#q=" + url_text, ""); url_text needs to be querystring encoded in this context. https://codereview.chromium.org/1869783005/diff/1/blimp/client/feature/naviga... blimp/client/feature/navigation_feature.cc:50: DCHECK(url.is_valid()); DCHECK is a poor fit here ("url" is built from user input; DCHECKs don't exist in release builds). Can we just log a message and drop the navigation request? https://codereview.chromium.org/1869783005/diff/1/blimp/engine/session/blimp_... File blimp/engine/session/blimp_engine_session.cc (right): https://codereview.chromium.org/1869783005/diff/1/blimp/engine/session/blimp_... blimp/engine/session/blimp_engine_session.cc:328: // TODO(dtrainor, haibinlu): Fix up the URL with url_fixer.h. If that doesn't Remove this comment
https://codereview.chromium.org/1869783005/diff/1/blimp/client/feature/naviga... File blimp/client/feature/navigation_feature.cc (right): https://codereview.chromium.org/1869783005/diff/1/blimp/client/feature/naviga... blimp/client/feature/navigation_feature.cc:42: const std::string& url_text) { Add unit tests for fixing up the URL.
https://codereview.chromium.org/1869783005/diff/1/blimp/client/feature/naviga... File blimp/client/feature/navigation_feature.cc (right): https://codereview.chromium.org/1869783005/diff/1/blimp/client/feature/naviga... blimp/client/feature/navigation_feature.cc:42: const std::string& url_text) { On 2016/04/07 20:17:00, Kevin M wrote: > Add unit tests for fixing up the URL. This is a temp (and wrong) fix for v0.5. not sure if a unit test is necessary. https://codereview.chromium.org/1869783005/diff/1/blimp/client/feature/naviga... blimp/client/feature/navigation_feature.cc:43: // Simplified heuristic for v0.5 only. On 2016/04/07 20:12:30, Kevin M wrote: > Add explanation as to *what* this is doing. Done. https://codereview.chromium.org/1869783005/diff/1/blimp/client/feature/naviga... blimp/client/feature/navigation_feature.cc:46: if (!url.is_valid() || url.host().find('.') == std::string::npos) { On 2016/04/07 20:12:30, Kevin M wrote: > Add comment on the reason and behavior of this heuristic. Done. https://codereview.chromium.org/1869783005/diff/1/blimp/client/feature/naviga... blimp/client/feature/navigation_feature.cc:47: url = url_formatter::FixupURL("https://www.google.com/#q=" + url_text, ""); On 2016/04/07 20:12:30, Kevin M wrote: > url_text needs to be querystring encoded in this context. FixupURL will do that. https://codereview.chromium.org/1869783005/diff/1/blimp/client/feature/naviga... blimp/client/feature/navigation_feature.cc:50: DCHECK(url.is_valid()); On 2016/04/07 20:12:30, Kevin M wrote: > DCHECK is a poor fit here ("url" is built from user input; DCHECKs don't exist > in release builds). Can we just log a message and drop the navigation request? removed the DCHECK. with FixupURL on google search, it should be valid.
PTAL
https://codereview.chromium.org/1869783005/diff/1/blimp/client/feature/naviga... File blimp/client/feature/navigation_feature.cc (right): https://codereview.chromium.org/1869783005/diff/1/blimp/client/feature/naviga... blimp/client/feature/navigation_feature.cc:47: url = url_formatter::FixupURL("https://www.google.com/#q=" + url_text, ""); On 2016/04/07 22:28:59, haibinlu wrote: > On 2016/04/07 20:12:30, Kevin M wrote: > > url_text needs to be querystring encoded in this context. > > FixupURL will do that. What if the searched text was "1+2"? Would FixupURL turn that into 1+2 or 1%2B2? I imagine it would leave it as the former, whereas we want the latter. https://codereview.chromium.org/1869783005/diff/20001/blimp/client/feature/na... File blimp/client/feature/navigation_feature.cc (right): https://codereview.chromium.org/1869783005/diff/20001/blimp/client/feature/na... blimp/client/feature/navigation_feature.cc:44: // It also converts "starwar" to "http://starwar/" (a valid GURL but no starwar => "example" (don't want trademarks in code)
PTAL https://codereview.chromium.org/1869783005/diff/1/blimp/client/feature/naviga... File blimp/client/feature/navigation_feature.cc (right): https://codereview.chromium.org/1869783005/diff/1/blimp/client/feature/naviga... blimp/client/feature/navigation_feature.cc:47: url = url_formatter::FixupURL("https://www.google.com/#q=" + url_text, ""); On 2016/04/07 23:00:19, Kevin M wrote: > On 2016/04/07 22:28:59, haibinlu wrote: > > On 2016/04/07 20:12:30, Kevin M wrote: > > > url_text needs to be querystring encoded in this context. > > > > FixupURL will do that. > > What if the searched text was "1+2"? Would FixupURL turn that into 1+2 or 1%2B2? > I imagine it would leave it as the former, whereas we want the latter. you are right. it needs encoding. https://codereview.chromium.org/1869783005/diff/20001/blimp/client/feature/na... File blimp/client/feature/navigation_feature.cc (right): https://codereview.chromium.org/1869783005/diff/20001/blimp/client/feature/na... blimp/client/feature/navigation_feature.cc:44: // It also converts "starwar" to "http://starwar/" (a valid GURL but no On 2016/04/07 23:00:19, Kevin M wrote: > starwar => "example" (don't want trademarks in code) Done.
https://codereview.chromium.org/1869783005/diff/40001/blimp/client/feature/na... File blimp/client/feature/navigation_feature.cc (right): https://codereview.chromium.org/1869783005/diff/40001/blimp/client/feature/na... blimp/client/feature/navigation_feature.cc:56: url::EncodeURIComponent(url_text.data(), strlen(url_text.data()), &buffer); url_text.size() https://codereview.chromium.org/1869783005/diff/40001/blimp/client/feature/na... blimp/client/feature/navigation_feature.cc:58: url = GURL("https://www.google.com/#q=" + encoded_query); DCHECK for validity
PTAL https://codereview.chromium.org/1869783005/diff/40001/blimp/client/feature/na... File blimp/client/feature/navigation_feature.cc (right): https://codereview.chromium.org/1869783005/diff/40001/blimp/client/feature/na... blimp/client/feature/navigation_feature.cc:56: url::EncodeURIComponent(url_text.data(), strlen(url_text.data()), &buffer); On 2016/04/08 00:14:33, Kevin M wrote: > url_text.size() Done. https://codereview.chromium.org/1869783005/diff/40001/blimp/client/feature/na... blimp/client/feature/navigation_feature.cc:58: url = GURL("https://www.google.com/#q=" + encoded_query); On 2016/04/08 00:14:33, Kevin M wrote: > DCHECK for validity Done.
lgtm
The CQ bit was checked by haibinlu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1869783005/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1869783005/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
haibinlu@chromium.org changed reviewers: + brettw@chromium.org
+brettw@ for DEPS on url_formatter component. Thanks!
Ping brettw :)
lgtm https://codereview.chromium.org/1869783005/diff/80001/blimp/client/feature/na... File blimp/client/feature/navigation_feature.cc (right): https://codereview.chromium.org/1869783005/diff/80001/blimp/client/feature/na... blimp/client/feature/navigation_feature.cc:50: // valid GURL such as "chrome://version/" do not have '.'. The heuristic is This will REALLY need to be changed since there's a lot of extra features that the omnibox does (like typing "foo/" should navigate). I assume this is just for a separate demo app so I don't really care what you do for now. https://codereview.chromium.org/1869783005/diff/80001/blimp/client/feature/na... blimp/client/feature/navigation_feature.cc:53: GURL url = url_formatter::FixupURL(url_text, ""); "" -> std::string() https://codereview.chromium.org/1869783005/diff/80001/blimp/client/feature/na... blimp/client/feature/navigation_feature.cc:54: if (!url.is_valid() || url.host().find('.') == std::string::npos) { host() -> host_piece() (this saves a copy)
https://codereview.chromium.org/1869783005/diff/80001/blimp/client/feature/na... File blimp/client/feature/navigation_feature.cc (right): https://codereview.chromium.org/1869783005/diff/80001/blimp/client/feature/na... blimp/client/feature/navigation_feature.cc:50: // valid GURL such as "chrome://version/" do not have '.'. The heuristic is On 2016/04/14 20:34:02, brettw wrote: > This will REALLY need to be changed since there's a lot of extra features that > the omnibox does (like typing "foo/" should navigate). I assume this is just for > a separate demo app so I don't really care what you do for now. Yes, this is for current demo app. The next demo app will be part of Clank and use Clank's omnibox. This change will be reverted (tracked at crbug/606411) https://codereview.chromium.org/1869783005/diff/80001/blimp/client/feature/na... blimp/client/feature/navigation_feature.cc:53: GURL url = url_formatter::FixupURL(url_text, ""); On 2016/04/14 20:34:02, brettw wrote: > "" -> std::string() Done. https://codereview.chromium.org/1869783005/diff/80001/blimp/client/feature/na... blimp/client/feature/navigation_feature.cc:54: if (!url.is_valid() || url.host().find('.') == std::string::npos) { On 2016/04/14 20:34:02, brettw wrote: > host() -> host_piece() > (this saves a copy) Done.
The CQ bit was checked by haibinlu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kmarshall@chromium.org, brettw@chromium.org Link to the patchset: https://codereview.chromium.org/1869783005/#ps100001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1869783005/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1869783005/100001
Message was sent while issue was closed.
Description was changed from ========== [Blimp Client] Fixes up URL before sending it to Engine. Uses url_formatter::FixupURL to convert "google.com" to "http://google.com". It also convert "movie" to "http://movie/" (which is a valid GURL). We check if the host has a '.' in it. If not, we use google search. This heuristic is wrong since valid GRULs such as "http://[2001:db8::2]/" (IPv6 literals) and "chrome://version/" do not have '.' in their host. However, it allows us to test v0.5 in a much easier way. BUG=601117 ========== to ========== [Blimp Client] Fixes up URL before sending it to Engine. Uses url_formatter::FixupURL to convert "google.com" to "http://google.com". It also convert "movie" to "http://movie/" (which is a valid GURL). We check if the host has a '.' in it. If not, we use google search. This heuristic is wrong since valid GRULs such as "http://[2001:db8::2]/" (IPv6 literals) and "chrome://version/" do not have '.' in their host. However, it allows us to test v0.5 in a much easier way. BUG=601117 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== [Blimp Client] Fixes up URL before sending it to Engine. Uses url_formatter::FixupURL to convert "google.com" to "http://google.com". It also convert "movie" to "http://movie/" (which is a valid GURL). We check if the host has a '.' in it. If not, we use google search. This heuristic is wrong since valid GRULs such as "http://[2001:db8::2]/" (IPv6 literals) and "chrome://version/" do not have '.' in their host. However, it allows us to test v0.5 in a much easier way. BUG=601117 ========== to ========== [Blimp Client] Fixes up URL before sending it to Engine. Uses url_formatter::FixupURL to convert "google.com" to "http://google.com". It also convert "movie" to "http://movie/" (which is a valid GURL). We check if the host has a '.' in it. If not, we use google search. This heuristic is wrong since valid GRULs such as "http://[2001:db8::2]/" (IPv6 literals) and "chrome://version/" do not have '.' in their host. However, it allows us to test v0.5 in a much easier way. BUG=601117 Committed: https://crrev.com/87714dd50611f6ea33b93383d9af4572f7ccb661 Cr-Commit-Position: refs/heads/master@{#389543} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/87714dd50611f6ea33b93383d9af4572f7ccb661 Cr-Commit-Position: refs/heads/master@{#389543} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
