Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1891573002:
Added simple url fixer for blimp toolbar (Closed)
|
Created:
4 years, 8 months ago by shaktisahu 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. |
DescriptionAdded simple url fixer for blimp toolbar
Currently, blimp requires user to enter the full URL.
This patch will add support for fixing URL as :
a) If there is no '.', toolbar would build a search query
b) If there is a '.', toolbar will use url_fixer to fix it.
BUG=603283
Committed: https://crrev.com/eb55219b5b01e1bf3dee1ba9a3d23a8ef8ebae79
Cr-Commit-Position: refs/heads/master@{#387677}
Patch Set 1 #Patch Set 2 : Used url_fixer for fixing URL #
Total comments: 11
Patch Set 3 : Addressed Nits #
Messages
Total messages: 26 (10 generated)
Description was changed from ========== Added simple url fix for blimp toolbar BUG=603283 ========== to ========== Added simple url fixer for blimp toolbar Currently, blimp requires user to enter the full URL. This patch will add support for fixing URL as : a) If there is no '.', toolbar would build a search query b) If there is a '.', toolbar will try to add "http://" to the URL, if it already doesn't have. BUG=603283 ==========
shaktisahu@chromium.org changed reviewers: + dtrainor@chromium.org, khushalsagar@chromium.org, nyquist@chromium.org
Description was changed from ========== Added simple url fixer for blimp toolbar Currently, blimp requires user to enter the full URL. This patch will add support for fixing URL as : a) If there is no '.', toolbar would build a search query b) If there is a '.', toolbar will try to add "http://" to the URL, if it already doesn't have. BUG=603283 ========== to ========== Added simple url fixer for blimp toolbar Currently, blimp requires user to enter the full URL. This patch will add support for fixing URL as : a) If there is no '.', toolbar would build a search query b) If there is a '.', toolbar will use url_fixer to fix it. BUG=603283 ==========
shaktisahu@chromium.org changed reviewers: + pkasting@chromium.org
lgtm
The CQ bit was checked by shaktisahu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1891573002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1891573002/20001
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...)
https://codereview.chromium.org/1891573002/diff/20001/blimp/client/app/androi... File blimp/client/app/android/toolbar.cc (right): https://codereview.chromium.org/1891573002/diff/20001/blimp/client/app/androi... blimp/client/app/android/toolbar.cc:62: // Build a search query, if |url| doesn't have a '.' anywhere. This will give you both false positives and false negatives. Why not use the AutocompleteClassifier instead? That will do intelligent fixup and transformation. You'll basically get something that will act like the Chrome omnibox would if you entered the text in question and then hit enter to select the default match. You can trim down the set of providers you construct the classifier with if certain bits aren't applicable to you, e.g. the builtins or shortcuts providers.
https://codereview.chromium.org/1891573002/diff/20001/blimp/client/app/androi... File blimp/client/app/android/toolbar.cc (right): https://codereview.chromium.org/1891573002/diff/20001/blimp/client/app/androi... blimp/client/app/android/toolbar.cc:62: // Build a search query, if |url| doesn't have a '.' anywhere. On 2016/04/14 04:34:01, Peter Kasting wrote: > This will give you both false positives and false negatives. > > Why not use the AutocompleteClassifier instead? That will do intelligent fixup > and transformation. You'll basically get something that will act like the > Chrome omnibox would if you entered the text in question and then hit enter to > select the default match. > > You can trim down the set of providers you construct the classifier with if > certain bits aren't applicable to you, e.g. the builtins or shortcuts providers. We looked at AutocompleteClassfier before, but it requires to add a lot of dependency to chrome. Currently in blimp, we don't want to add anything from chrome and make a very basic version ready. However, later during clank integration we want to revisit this and make it right.
https://codereview.chromium.org/1891573002/diff/20001/blimp/client/app/androi... File blimp/client/app/android/toolbar.cc (right): https://codereview.chromium.org/1891573002/diff/20001/blimp/client/app/androi... blimp/client/app/android/toolbar.cc:62: // Build a search query, if |url| doesn't have a '.' anywhere. On 2016/04/14 17:55:23, shaktisahu wrote: > On 2016/04/14 04:34:01, Peter Kasting wrote: > > This will give you both false positives and false negatives. > > > > Why not use the AutocompleteClassifier instead? That will do intelligent > fixup > > and transformation. You'll basically get something that will act like the > > Chrome omnibox would if you entered the text in question and then hit enter to > > select the default match. > > > > You can trim down the set of providers you construct the classifier with if > > certain bits aren't applicable to you, e.g. the builtins or shortcuts > providers. > > We looked at AutocompleteClassfier before, but it requires to add a lot of > dependency to chrome. Currently in blimp, we don't want to add anything from > chrome and make a very basic version ready. However, later during clank > integration we want to revisit this and make it right. How long ago did you look? This has all been componentized in the last months, and I don't believe you need any dependencies on chrome/ to use this now. Even if that isn't true, if you plan to do this better at some point, I'd rather do nothing at all now, and leave this so painfully wrong that it can't possibly ship this way, than make it marginally better but still not actually good enough for real use.
lgtm
https://codereview.chromium.org/1891573002/diff/20001/blimp/client/app/androi... File blimp/client/app/android/toolbar.cc (right): https://codereview.chromium.org/1891573002/diff/20001/blimp/client/app/androi... blimp/client/app/android/toolbar.cc:62: // Build a search query, if |url| doesn't have a '.' anywhere. On 2016/04/14 20:38:44, Peter Kasting wrote: > On 2016/04/14 17:55:23, shaktisahu wrote: > > On 2016/04/14 04:34:01, Peter Kasting wrote: > > > This will give you both false positives and false negatives. > > > > > > Why not use the AutocompleteClassifier instead? That will do intelligent > > fixup > > > and transformation. You'll basically get something that will act like the > > > Chrome omnibox would if you entered the text in question and then hit enter > to > > > select the default match. > > > > > > You can trim down the set of providers you construct the classifier with if > > > certain bits aren't applicable to you, e.g. the builtins or shortcuts > > providers. > > > > We looked at AutocompleteClassfier before, but it requires to add a lot of > > dependency to chrome. Currently in blimp, we don't want to add anything from > > chrome and make a very basic version ready. However, later during clank > > integration we want to revisit this and make it right. > > How long ago did you look? This has all been componentized in the last months, > and I don't believe you need any dependencies on chrome/ to use this now. > > Even if that isn't true, if you plan to do this better at some point, I'd rather > do nothing at all now, and leave this so painfully wrong that it can't possibly > ship this way, than make it marginally better but still not actually good enough > for real use. I believe the plan would be to merge this with the normal Chrome for Android toolbar.
https://codereview.chromium.org/1891573002/diff/20001/blimp/client/app/androi... File blimp/client/app/android/toolbar.cc (right): https://codereview.chromium.org/1891573002/diff/20001/blimp/client/app/androi... blimp/client/app/android/toolbar.cc:62: // Build a search query, if |url| doesn't have a '.' anywhere. On 2016/04/14 20:38:44, Peter Kasting wrote: > On 2016/04/14 17:55:23, shaktisahu wrote: > > On 2016/04/14 04:34:01, Peter Kasting wrote: > > > This will give you both false positives and false negatives. > > > > > > Why not use the AutocompleteClassifier instead? That will do intelligent > > fixup > > > and transformation. You'll basically get something that will act like the > > > Chrome omnibox would if you entered the text in question and then hit enter > to > > > select the default match. > > > > > > You can trim down the set of providers you construct the classifier with if > > > certain bits aren't applicable to you, e.g. the builtins or shortcuts > > providers. > > > > We looked at AutocompleteClassfier before, but it requires to add a lot of > > dependency to chrome. Currently in blimp, we don't want to add anything from > > chrome and make a very basic version ready. However, later during clank > > integration we want to revisit this and make it right. > > How long ago did you look? This has all been componentized in the last months, > and I don't believe you need any dependencies on chrome/ to use this now. > > Even if that isn't true, if you plan to do this better at some point, I'd rather > do nothing at all now, and leave this so painfully wrong that it can't possibly > ship this way, than make it marginally better but still not actually good enough > for real use. Actually, this is a temporary work. All this code will go away next month after it is integrated to chrome and we will use chrome omnibox. The purpose of this is primarily for the blimp demo since it is very hard to type the full URL every time.
LGTM https://codereview.chromium.org/1891573002/diff/20001/blimp/client/app/androi... File blimp/client/app/android/toolbar.cc (right): https://codereview.chromium.org/1891573002/diff/20001/blimp/client/app/androi... blimp/client/app/android/toolbar.cc:63: if (url.find(".") == std::string::npos) { Nit: {} not necessary https://codereview.chromium.org/1891573002/diff/20001/blimp/client/app/androi... blimp/client/app/android/toolbar.cc:64: url = std::string("http://www.google.com/search?q=") + url; Nit: std::string() not necessary https://codereview.chromium.org/1891573002/diff/20001/blimp/client/app/androi... blimp/client/app/android/toolbar.cc:66: GURL fixedUrl = url_formatter::FixupURL(url, std::string()); Nit: fixed_url
https://codereview.chromium.org/1891573002/diff/20001/blimp/client/app/androi... File blimp/client/app/android/toolbar.cc (right): https://codereview.chromium.org/1891573002/diff/20001/blimp/client/app/androi... blimp/client/app/android/toolbar.cc:63: if (url.find(".") == std::string::npos) { On 2016/04/15 18:35:52, Peter Kasting wrote: > Nit: {} not necessary Done. https://codereview.chromium.org/1891573002/diff/20001/blimp/client/app/androi... blimp/client/app/android/toolbar.cc:64: url = std::string("http://www.google.com/search?q=") + url; On 2016/04/15 18:35:53, Peter Kasting wrote: > Nit: std::string() not necessary Done. https://codereview.chromium.org/1891573002/diff/20001/blimp/client/app/androi... blimp/client/app/android/toolbar.cc:66: GURL fixedUrl = url_formatter::FixupURL(url, std::string()); On 2016/04/15 18:35:52, Peter Kasting wrote: > Nit: fixed_url Done.
The CQ bit was checked by shaktisahu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nyquist@chromium.org, dtrainor@chromium.org, pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/1891573002/#ps40001 (title: "Addressed Nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1891573002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1891573002/40001
Message was sent while issue was closed.
Description was changed from ========== Added simple url fixer for blimp toolbar Currently, blimp requires user to enter the full URL. This patch will add support for fixing URL as : a) If there is no '.', toolbar would build a search query b) If there is a '.', toolbar will use url_fixer to fix it. BUG=603283 ========== to ========== Added simple url fixer for blimp toolbar Currently, blimp requires user to enter the full URL. This patch will add support for fixing URL as : a) If there is no '.', toolbar would build a search query b) If there is a '.', toolbar will use url_fixer to fix it. BUG=603283 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Added simple url fixer for blimp toolbar Currently, blimp requires user to enter the full URL. This patch will add support for fixing URL as : a) If there is no '.', toolbar would build a search query b) If there is a '.', toolbar will use url_fixer to fix it. BUG=603283 ========== to ========== Added simple url fixer for blimp toolbar Currently, blimp requires user to enter the full URL. This patch will add support for fixing URL as : a) If there is no '.', toolbar would build a search query b) If there is a '.', toolbar will use url_fixer to fix it. BUG=603283 Committed: https://crrev.com/eb55219b5b01e1bf3dee1ba9a3d23a8ef8ebae79 Cr-Commit-Position: refs/heads/master@{#387677} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/eb55219b5b01e1bf3dee1ba9a3d23a8ef8ebae79 Cr-Commit-Position: refs/heads/master@{#387677} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
