Implement IsSearchProviderInstalled and a test for it.

Implement IsSearchProviderInstalled and a test for it.

It is currently off by default and one must pass in a flag
(--enable-search-provider-api-v2) to use it. Api details are
in the bug.

BUG=38475
TEST=ui_tests --gtest_filter=SearchProviderTest.TestIsSearchProviderInstalled

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=52778

[Paweł Hajdan Jr., 2010-07-15 15:26:17]

Drive-by with a minor test comment.

http://codereview.chromium.org/2823042/diff/6001/7010
File chrome/renderer/external_extension_uitest.cc (right):

http://codereview.chromium.org/2823042/diff/6001/7010#newcode55
chrome/renderer/external_extension_uitest.cc:55: GURL local_url =
Better ASSERT_TRUE(server_) before that to avoid crashing entire ui_tests.

[sky, 2010-07-15 15:50:12]

http://codereview.chromium.org/2823042/diff/6001/7002
File chrome/browser/renderer_host/render_view_host.cc (right):

http://codereview.chromium.org/2823042/diff/6001/7002#newcode779
chrome/browser/renderer_host/render_view_host.cc:779:
IPC_MESSAGE_HANDLER_DELAY_REPLY(ViewHostMsg_GetSearchProviderType,
OnMsgGetSearchProviderType)
line > 80

http://codereview.chromium.org/2823042/diff/6001/7004
File chrome/browser/renderer_host/render_view_host_delegate.h (right):

http://codereview.chromium.org/2823042/diff/6001/7004#newcode655
chrome/browser/renderer_host/render_view_host_delegate.h:655: virtual int
GetSearchProviderType(const GURL& url) { return 0; }
Description?

http://codereview.chromium.org/2823042/diff/6001/7005
File chrome/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/2823042/diff/6001/7005#newcode2714
chrome/browser/tab_contents/tab_contents.cc:2714: static bool IsSameOrigin(const
GURL& requested_origin, const TemplateURL* template_url) {
> 80

http://codereview.chromium.org/2823042/diff/6001/7005#newcode2714
chrome/browser/tab_contents/tab_contents.cc:2714: static bool IsSameOrigin(const
GURL& requested_origin, const TemplateURL* template_url) {
Description?

http://codereview.chromium.org/2823042/diff/6001/7005#newcode2720
chrome/browser/tab_contents/tab_contents.cc:2720: int
TabContents::GetSearchProviderType(const GURL& requested_host) {
Since you have a bunch of different return types use an enum.

http://codereview.chromium.org/2823042/diff/6001/7005#newcode2722
chrome/browser/tab_contents/tab_contents.cc:2722: GURL page_origin =
GURL(GetURL()).GetOrigin();
It would be safer if you pass up the actual URL rather than using GetURL. If the
user starts a navigation while ViewHostMsg_GetSearchProviderType is in flight
GetURL is going to return the url of the new navigation.

http://codereview.chromium.org/2823042/diff/6001/7005#newcode2728
chrome/browser/tab_contents/tab_contents.cc:2728: if
(profile()->IsOffTheRecord())
Shouldn't this check be first?

http://codereview.chromium.org/2823042/diff/6001/7005#newcode2747
chrome/browser/tab_contents/tab_contents.cc:2747: return 1;
2 space indent.

http://codereview.chromium.org/2823042/diff/6001/7008
File chrome/common/render_messages_internal.h (right):

http://codereview.chromium.org/2823042/diff/6001/7008#newcode1551
chrome/common/render_messages_internal.h:1551:
IPC_SYNC_MESSAGE_ROUTED1_1(ViewHostMsg_GetSearchProviderType, GURL, int /* 0,1,2
*/ )
> 80

http://codereview.chromium.org/2823042/diff/6001/7009
File chrome/renderer/external_extension.cc (right):

http://codereview.chromium.org/2823042/diff/6001/7009#newcode33
chrome/renderer/external_extension.cc:33: static const char* const
kSearchProviderApiV1 = SEARCH_PROVIDER_API_V1;
Why bother with the defines when you just assign it to fields?

http://codereview.chromium.org/2823042/diff/6001/7009#newcode91
chrome/renderer/external_extension.cc:91: static v8::Handle<v8::Value>
IsSearchProviderInstalled(const v8::Arguments& args) {
> 80

[levin, 2010-07-15 22:34:10]

I believe that I have addressed all feedback.

There seems to be an issue on Windows so I have to find a Windows machine and
debug that there.

http://codereview.chromium.org/2823042/diff/6001/7001
File chrome/browser/renderer_host/browser_render_process_host.cc (right):

http://codereview.chromium.org/2823042/diff/6001/7001#newcode538
chrome/browser/renderer_host/browser_render_process_host.cc:538:
switches::kEnableSearchProviderApiV2,
Removed this as it was part of the previous check-in (see 3 lines up).

http://codereview.chromium.org/2823042/diff/6001/7002
File chrome/browser/renderer_host/render_view_host.cc (right):

http://codereview.chromium.org/2823042/diff/6001/7002#newcode779
chrome/browser/renderer_host/render_view_host.cc:779:
IPC_MESSAGE_HANDLER_DELAY_REPLY(ViewHostMsg_GetSearchProviderType,
OnMsgGetSearchProviderType)
On 2010/07/15 15:50:12, sky wrote:
> line > 80

Done.

http://codereview.chromium.org/2823042/diff/6001/7004
File chrome/browser/renderer_host/render_view_host_delegate.h (right):

http://codereview.chromium.org/2823042/diff/6001/7004#newcode655
chrome/browser/renderer_host/render_view_host_delegate.h:655: virtual int
GetSearchProviderType(const GURL& url) { return 0; }
On 2010/07/15 15:50:12, sky wrote:
> Description?

Done.

http://codereview.chromium.org/2823042/diff/6001/7005
File chrome/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/2823042/diff/6001/7005#newcode2714
chrome/browser/tab_contents/tab_contents.cc:2714: static bool IsSameOrigin(const
GURL& requested_origin, const TemplateURL* template_url) {
On 2010/07/15 15:50:12, sky wrote:
> > 80

Done.

http://codereview.chromium.org/2823042/diff/6001/7005#newcode2714
chrome/browser/tab_contents/tab_contents.cc:2714: static bool IsSameOrigin(const
GURL& requested_origin, const TemplateURL* template_url) {
On 2010/07/15 15:50:12, sky wrote:
> Description?

Done.

http://codereview.chromium.org/2823042/diff/6001/7005#newcode2720
chrome/browser/tab_contents/tab_contents.cc:2720: int
TabContents::GetSearchProviderType(const GURL& requested_host) {
On 2010/07/15 15:50:12, sky wrote:
> Since you have a bunch of different return types use an enum.

Done.

http://codereview.chromium.org/2823042/diff/6001/7005#newcode2722
chrome/browser/tab_contents/tab_contents.cc:2722: GURL page_origin =
GURL(GetURL()).GetOrigin();
On 2010/07/15 15:50:12, sky wrote:
> It would be safer if you pass up the actual URL rather than using GetURL. If
the
> user starts a navigation while ViewHostMsg_GetSearchProviderType is in flight
> GetURL is going to return the url of the new navigation.
That was a bad mistake.  Thanks!

I'd rather not trust any url provided by the page just in case the renderer was
broken into (defense in depth), but GetURL was incorrect, so I switched to a
technique I saw used elsewhere in TabContents.

http://codereview.chromium.org/2823042/diff/6001/7005#newcode2728
chrome/browser/tab_contents/tab_contents.cc:2728: if
(profile()->IsOffTheRecord())
On 2010/07/15 15:50:12, sky wrote:
> Shouldn't this check be first?

Since it wasn't clear, I added a comment to explain why this is where it is.

http://codereview.chromium.org/2823042/diff/6001/7005#newcode2747
chrome/browser/tab_contents/tab_contents.cc:2747: return 1;
On 2010/07/15 15:50:12, sky wrote:
> 2 space indent.

Done.

http://codereview.chromium.org/2823042/diff/6001/7008
File chrome/common/render_messages_internal.h (right):

http://codereview.chromium.org/2823042/diff/6001/7008#newcode1551
chrome/common/render_messages_internal.h:1551:
IPC_SYNC_MESSAGE_ROUTED1_1(ViewHostMsg_GetSearchProviderType, GURL, int /* 0,1,2
*/ )
On 2010/07/15 15:50:12, sky wrote:
> > 80

Done.

http://codereview.chromium.org/2823042/diff/6001/7009
File chrome/renderer/external_extension.cc (right):

http://codereview.chromium.org/2823042/diff/6001/7009#newcode33
chrome/renderer/external_extension.cc:33: static const char* const
kSearchProviderApiV1 = SEARCH_PROVIDER_API_V1;
On 2010/07/15 15:50:12, sky wrote:
> Why bother with the defines when you just assign it to fields?
I can do that for the second one.

For the first, I need to have a string that has both put together, so a define
seemed like the right way to do that. I tried constructing a string on the fly
in the constructor using std::string, but then I found that v8::Extension just
stores a point to this string, so that resulted in a lot of badness.

http://codereview.chromium.org/2823042/diff/6001/7009#newcode91
chrome/renderer/external_extension.cc:91: static v8::Handle<v8::Value>
IsSearchProviderInstalled(const v8::Arguments& args) {
On 2010/07/15 15:50:12, sky wrote:
> > 80

Also, while I was following a pattern here, I realized that I was violatiing:
"Defining a function in the class declaration is an implicit instruction to the
compiler to inline the function.  Only do this when you truly want the function
to be inlined.  Even with a class that's local to a single .cc file, declaring
and defining things separately is a good idea, and keeps the declaration more
readable."

So I fixed that for my additions and the methods previously present (as well as
adding descriptions and adding the standard DISALLOW_COPY_AND_ASSIGN macro).

http://codereview.chromium.org/2823042/diff/6001/7010
File chrome/renderer/external_extension_uitest.cc (right):

http://codereview.chromium.org/2823042/diff/6001/7010#newcode55
chrome/renderer/external_extension_uitest.cc:55: GURL local_url =
On 2010/07/15 15:26:17, Paweł Hajdan Jr. wrote:
> Better ASSERT_TRUE(server_) before that to avoid crashing entire ui_tests.

Done (and an equivalent check in the constructor as well).

[sky, 2010-07-15 23:24:49]

LGTM

http://codereview.chromium.org/2823042/diff/17001/18005
File chrome/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/2823042/diff/17001/18005#newcode2731
chrome/browser/tab_contents/tab_contents.cc:2731: GURL page_origin = entry ?
entry->virtual_url().GetOrigin() : GURL::EmptyGURL();
> 80, also, use GURL() here instead of EmptyGURL() as you're not returning a
reference.

http://codereview.chromium.org/2823042/diff/17001/18005#newcode2743
chrome/browser/tab_contents/tab_contents.cc:2743: 
nit: nuke this line.

http://codereview.chromium.org/2823042/diff/17001/18005#newcode2752
chrome/browser/tab_contents/tab_contents.cc:2752: return
ViewHostMsg_GetSearchProviderInstallState_Params::InstalledAsDefault();
> 80

http://codereview.chromium.org/2823042/diff/17001/18008
File chrome/common/render_messages.h (right):

http://codereview.chromium.org/2823042/diff/17001/18008#newcode172
chrome/common/render_messages.h:172: : state(DENIED) {
indent 4

http://codereview.chromium.org/2823042/diff/17001/18008#newcode176
chrome/common/render_messages.h:176: : state(s) {
indent 4

http://codereview.chromium.org/2823042/diff/17001/18009
File chrome/common/render_messages_internal.h (right):

http://codereview.chromium.org/2823042/diff/17001/18009#newcode1551
chrome/common/render_messages_internal.h:1551: // Find out if the given url's
security origin is installed as a search provider.
> 80

[Paweł Hajdan Jr., 2010-07-16 00:14:32]

http://codereview.chromium.org/2823042/diff/17001/18011
File chrome/renderer/external_extension_uitest.cc (right):

http://codereview.chromium.org/2823042/diff/17001/18011#newcode32
chrome/renderer/external_extension_uitest.cc:32: if (!server_)
How about just ASSERT/EXPECT_TRUE(server_)? Failing silently might be dangerous.

Also, maybe even remove this check from ctor. I think it's fine to check it
before first usage in a test. Up to you.

[levin, 2010-07-16 00:29:10]

On 2010/07/16 00:14:32, Paweł Hajdan Jr. wrote:
> http://codereview.chromium.org/2823042/diff/17001/18011
> File chrome/renderer/external_extension_uitest.cc (right):
> 
> http://codereview.chromium.org/2823042/diff/17001/18011#newcode32
> chrome/renderer/external_extension_uitest.cc:32: if (!server_)
> How about just ASSERT/EXPECT_TRUE(server_)? Failing silently might be
dangerous.
> 
> Also, maybe even remove this check from ctor. I think it's fine to check it
> before first usage in a test. Up to you.

Thanks for your earlier comment which caught this issue, and  thanks for looking
again, Pawel!

Short answer: The changes suggested are problematic, so I'm not doing them.
Also, it won't fail silently as written.

Detailed answer:

> Failing silently might be dangerous.
The test case will get run regardless of this check in the ctor, and it will do
the ASSERT_TRUE there, so the test will fail if !server_.

> Regarding not checking server_ in the constructor.
If that is not done, then the test will crash (which is bad as you pointed out
due to skipping all other tests) because server_ is used in the ctor.

> Regarding using EXPECT_TRUE in the ctor.
I actually did started to address your earlier comment by putting ASSERT_TRUE in
the ctor, and that's how I found that it was problemantic.

This won't work because it tries to return a value (bool?) which doesn't work in
the constructor.

> Regarding using ASSERT_TRUE in the ctor.
Not needed because we'd still need a NULL check to avoid the crash and the test
will fail without this.

[Paweł Hajdan Jr., 2010-07-16 00:32:25]

Code I commented in the drive-by LGTM.

[levin, 2010-07-16 14:50:56]

Sky, Pawel,
ISSUE 1: I'd like to check in the code with your previous comments fixed and
make the test DISABLED_. ok?

Why?
I have several fixes to do and I'd like to keep my patches small. I'll enable
the test after these two fixes.

Details:
I discovered two bugs about the code last night that I'd like to fix in
subsequent check ins.
1 Bug makes the test crash on windows due to sync ipc msg w/o doing message
pumping. (Pretty simple fix I think.)

The second bug results in potential flakiness. (This became a lot more evident
when I tried to make the test faster.) This is because the template url model is
loaded async and my code doesn't wait for that to happen. This fix is slightly
more involved but I think I know how to fix it.

ISSUE 2: Pawel, I must have lied to you about ASSERT_TRUE. It was unintentional,
but I'm sorry.

Details:
The other function I have ASSERT_TRUE in doesn't return a value, so ASSERT_TRUE
must not return a value. I know that I hit a compile error (maybe it was due to
some other issues   in the code at same that time). I think the code is fine as
is though because the test will still fail, etc.

dave

[sky, 2010-07-16 14:57:13]

On Fri, Jul 16, 2010 at 7:50 AM,  <levin@chromium.org> wrote:
> Sky, Pawel,
> ISSUE 1: I'd like to check in the code with your previous comments fixed and
> make the test DISABLED_. ok?

SGTM

> Why?
> I have several fixes to do and I'd like to keep my patches small. I'll
> enable
> the test after these two fixes.
>
> Details:
> I discovered two bugs about the code last night that I'd like to fix in
> subsequent check ins.
> 1 Bug makes the test crash on windows due to sync ipc msg w/o doing message
> pumping. (Pretty simple fix I think.)
>
> The second bug results in potential flakiness. (This became a lot more
> evident
> when I tried to make the test faster.) This is because the template url
> model is
> loaded async and my code doesn't wait for that to happen. This fix is
> slightly
> more involved but I think I know how to fix it.
>
> ISSUE 2: Pawel, I must have lied to you about ASSERT_TRUE. It was
> unintentional,
> but I'm sorry.
>
> Details:
> The other function I have ASSERT_TRUE in doesn't return a value, so
> ASSERT_TRUE
> must not return a value. I know that I hit a compile error (maybe it was due
> to
> some other issues   in the code at same that time). I think the code is fine
> as
> is though because the test will still fail, etc.
>
> dave
>
> http://codereview.chromium.org/2823042/show
>

[Paweł Hajdan Jr., 2010-07-16 18:23:35]

SGTM as well.

On Fri, Jul 16, 2010 at 07:56, Scott Violet <sky@chromium.org> wrote:

> On Fri, Jul 16, 2010 at 7:50 AM,  <levin@chromium.org> wrote:
> > Sky, Pawel,
> > ISSUE 1: I'd like to check in the code with your previous comments fixed
> and
> > make the test DISABLED_. ok?
>
> SGTM
>
> > Why?
> > I have several fixes to do and I'd like to keep my patches small. I'll
> > enable
> > the test after these two fixes.
> >
> > Details:
> > I discovered two bugs about the code last night that I'd like to fix in
> > subsequent check ins.
> > 1 Bug makes the test crash on windows due to sync ipc msg w/o doing
> message
> > pumping. (Pretty simple fix I think.)
> >
> > The second bug results in potential flakiness. (This became a lot more
> > evident
> > when I tried to make the test faster.) This is because the template url
> > model is
> > loaded async and my code doesn't wait for that to happen. This fix is
> > slightly
> > more involved but I think I know how to fix it.
> >
> > ISSUE 2: Pawel, I must have lied to you about ASSERT_TRUE. It was
> > unintentional,
> > but I'm sorry.
> >
> > Details:
> > The other function I have ASSERT_TRUE in doesn't return a value, so
> > ASSERT_TRUE
> > must not return a value. I know that I hit a compile error (maybe it was
> due
> > to
> > some other issues   in the code at same that time). I think the code is
> fine
> > as
> > is though because the test will still fail, etc.
> >
> > dave
> >
> > http://codereview.chromium.org/2823042/show
> >
>