Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2505933005:
Auto-Generating Search Engines - Remove HTTP Restriction (Closed)
DescriptionAuto-Generating Search Engines - Remove HTTP Restriction
Remove the restriction that, under certain conditions, we require
the protocol of an auto-created custom search engine be HTTP. The
condition has no connection with a need for privacy. There is no
problem with creating custom search engines for secure sites. In
fact, we should be encouraging more sites to be secure, not penalizing
those that do so by making it more difficult for them to create a
custom search engine.
BUG=642848
TEST=amazon.com now gets a custom search engine created
Committed: https://crrev.com/0bc8993c2a2766309e91904b742936202bed2b45
Cr-Commit-Position: refs/heads/master@{#434706}
Patch Set 1 #
Total comments: 2
Patch Set 2 : added test #
Messages
Total messages: 34 (23 generated)
Description was changed from ========== Auto-Generating Search Engines - Remove HTTP Restriction Remove the restriction that, under certain conditions, we require the protocol of an auto-created custom search engine be HTTP. The condition has no connection with a need for privacy. There is no problem with creating custom search engines for secure sites. In fact, we should be encouraging more sites to be secure, not penalizing those that do so by making it more difficult for them to create a custom search engine. BUG=642848 TEST=amazon.com now gets a custom search engine created ========== to ========== Auto-Generating Search Engines - Remove HTTP Restriction Remove the restriction that, under certain conditions, we require the protocol of an auto-created custom search engine be HTTP. The condition has no connection with a need for privacy. There is no problem with creating custom search engines for secure sites. In fact, we should be encouraging more sites to be secure, not penalizing those that do so by making it more difficult for them to create a custom search engine. BUG=642848 TEST=amazon.com now gets a custom search engine created ==========
mpearson@chromium.org changed reviewers: + pkasting@chromium.org
Peter, Is this the proper way to submit Blink code? I haven't done this before. It'd be nice if it this easy post-WebKit-Blink split. The Blink page makes it sound that way, i.e., same as chromium. https://www.chromium.org/blink#TOC-Committing-and-reviewing-code Anyway, I'd also appreciate a review in addition to an answer to the above question. thanks, mark
The CQ bit was checked by mpearson@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...
Yes, submitting code to Blink is Just That Easy, but you still need to follow standard procedure on finding the right OWNER/reviewer. In this case, look in https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/OWNERS for the right person. https://codereview.chromium.org/2505933005/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/web/WebSearchableFormData.cpp (left): https://codereview.chromium.org/2505933005/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/WebSearchableFormData.cpp:240: (!isHTTPFormSubmit(*formElement) && !inputElement)) This change allows forms from other schemes as well (file:? data:?). Can such cases get here? Do we want to allow them?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
mpearson@chromium.org changed reviewers: + tkent@chromium.org
tkent@, can you please a look at the OWNER and most-frequent-toucher of this file? thanks, mark https://codereview.chromium.org/2505933005/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/web/WebSearchableFormData.cpp (left): https://codereview.chromium.org/2505933005/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/WebSearchableFormData.cpp:240: (!isHTTPFormSubmit(*formElement) && !inputElement)) On 2016/11/17 01:08:57, Peter Kasting wrote: > This change allows forms from other schemes as well (file:? data:?). Can such > cases get here? Do we want to allow them? I'm not sure if other schemes get here. But I am okay with allowing them. ftp: for example seems fine. file: schemes don't usually have a hostname and thus wouldn't create a custom search engine, and those that have a hostname I think would be fine. data: don't have hostnames and hence cannot create an engine.
Please add a test.
On 2016/11/17 05:20:24, tkent wrote: > Please add a test. Good idea. It'll take me some time to become more familiar how to properly configure this as a Blink test. I'll probably get back to you in two weeks-ish. In the meantime, I'll ask you a question. I see that you've been contributing to Blink/WebKit for a long time--hopefully you'll know the answer. It seems to me that Safari is doing auto-generating custom search engines better than Chrome. For example, Safari already creates custom search engines for https:// pages. I think its logic for this is better with its apparently more lenient rules. Can you find the Safari/WebKit code that corresponds to the Chrome/Blink code above--surprisingly I can't (at least through searching on github in the WebKit tree)--so I can see how Safari's heuristics differ from ours? thanks, mark
On 2016/11/18 at 22:57:07, mpearson wrote: > On 2016/11/17 05:20:24, tkent wrote: > > Please add a test. > > Good idea. It'll take me some time to become more familiar how to properly configure this as a Blink test. I'll probably get back to you in two weeks-ish. I think adding a static function like "static bool isSupportableFormEelement(const HTMLFormElement&)" would make adding C++ unit test easier. > In the meantime, I'll ask you a question. I see that you've been contributing to Blink/WebKit for a long time--hopefully you'll know the answer. It seems to me that Safari is doing auto-generating custom search engines better than Chrome. For example, Safari already creates custom search engines for https:// pages. I think its logic for this is better with its apparently more lenient rules. Can you find the Safari/WebKit code that corresponds to the Chrome/Blink code above--surprisingly I can't (at least through searching on github in the WebKit tree)--so I can see how Safari's heuristics differ from ours? I haven't seen such code in WebKit. Probably the code is in Safari, which is a closed-source product, rather than in WebKit.
Added a simple test by creating a new input test file. This seemed the most straight-forward solution. This allows a simply test that a search engine is created upon seeing HTML that looks a particular way. Please take a look at your leisure (no hurry). > I haven't seen such code in WebKit. Probably the code is > in Safari, which is a closed-source product, rather than > in WebKit. Thanks for checking. By the way, as I dive into annoying issues where we're not creating search engines when we should, as I find more bugs, I might add another test or two representing problematic HTML files. (That's why I think this testing strategy is best.) --mark
lgtm
The CQ bit was checked by mpearson@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: blimp_linux_dbg on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by mpearson@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: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by mpearson@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 mpearson@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": 20001, "attempt_start_ts": 1480354064562680,
"parent_rev": "0e86cee990a9d8de4b0ee00a37e57113a28589a2", "commit_rev":
"2beb95c1ad5d4e089ada725e6f61dba39da63237"}
Message was sent while issue was closed.
Description was changed from ========== Auto-Generating Search Engines - Remove HTTP Restriction Remove the restriction that, under certain conditions, we require the protocol of an auto-created custom search engine be HTTP. The condition has no connection with a need for privacy. There is no problem with creating custom search engines for secure sites. In fact, we should be encouraging more sites to be secure, not penalizing those that do so by making it more difficult for them to create a custom search engine. BUG=642848 TEST=amazon.com now gets a custom search engine created ========== to ========== Auto-Generating Search Engines - Remove HTTP Restriction Remove the restriction that, under certain conditions, we require the protocol of an auto-created custom search engine be HTTP. The condition has no connection with a need for privacy. There is no problem with creating custom search engines for secure sites. In fact, we should be encouraging more sites to be secure, not penalizing those that do so by making it more difficult for them to create a custom search engine. BUG=642848 TEST=amazon.com now gets a custom search engine created ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Auto-Generating Search Engines - Remove HTTP Restriction Remove the restriction that, under certain conditions, we require the protocol of an auto-created custom search engine be HTTP. The condition has no connection with a need for privacy. There is no problem with creating custom search engines for secure sites. In fact, we should be encouraging more sites to be secure, not penalizing those that do so by making it more difficult for them to create a custom search engine. BUG=642848 TEST=amazon.com now gets a custom search engine created ========== to ========== Auto-Generating Search Engines - Remove HTTP Restriction Remove the restriction that, under certain conditions, we require the protocol of an auto-created custom search engine be HTTP. The condition has no connection with a need for privacy. There is no problem with creating custom search engines for secure sites. In fact, we should be encouraging more sites to be secure, not penalizing those that do so by making it more difficult for them to create a custom search engine. BUG=642848 TEST=amazon.com now gets a custom search engine created Committed: https://crrev.com/0bc8993c2a2766309e91904b742936202bed2b45 Cr-Commit-Position: refs/heads/master@{#434706} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/0bc8993c2a2766309e91904b742936202bed2b45 Cr-Commit-Position: refs/heads/master@{#434706} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
