Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(380)

Issue 2823042: Implement IsSearchProviderInstalled and a test for it. (Closed)

Created:
10 years, 5 months ago by levin
Modified:
9 years, 7 months ago
Reviewers:
sky, Paweł Hajdan Jr.
CC:
chromium-reviews, ben+cc_chromium.org, Erik does not do reviews, Aaron Boodman, pam+watch_chromium.org, brettw-cc_chromium.org, darin-cc_chromium.org, Paweł Hajdan Jr.
Visibility:
Public.

Description

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

Patch Set 1 #

Patch Set 2 : Now with the whole patch. #

Patch Set 3 : latest version #

Total comments: 25

Patch Set 4 : Addressed feedback. #

Total comments: 7
Unified diffs Side-by-side diffs Delta from patch set Stats (+496 lines, -37 lines) Patch
M chrome/browser/renderer_host/render_view_host.h View 2 3 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/browser/renderer_host/render_view_host.cc View 2 3 2 chunks +10 lines, -0 lines 0 comments Download
M chrome/browser/renderer_host/render_view_host_delegate.h View 2 3 2 chunks +6 lines, -0 lines 0 comments Download
M chrome/browser/renderer_host/render_view_host_delegate.cc View 2 chunks +6 lines, -0 lines 0 comments Download
M chrome/browser/tab_contents/tab_contents.h View 2 3 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/browser/tab_contents/tab_contents.cc View 1 2 3 1 chunk +53 lines, -0 lines 3 comments Download
M chrome/chrome_tests.gypi View 1 chunk +1 line, -0 lines 0 comments Download
M chrome/common/render_messages.h View 2 chunks +93 lines, -0 lines 2 comments Download
M chrome/common/render_messages_internal.h View 1 2 3 2 chunks +9 lines, -3 lines 1 comment Download
M chrome/renderer/external_extension.cc View 1 2 3 2 chunks +109 lines, -34 lines 0 comments Download
A chrome/renderer/external_extension_uitest.cc View 1 2 3 1 chunk +98 lines, -0 lines 1 comment Download
M chrome/renderer/render_view.h View 2 3 1 chunk +4 lines, -0 lines 0 comments Download
M chrome/renderer/render_view.cc View 2 3 1 chunk +12 lines, -0 lines 0 comments Download
A chrome/test/data/is_search_provider_installed.html View 1 2 3 1 chunk +91 lines, -0 lines 0 comments Download

Messages

Total messages: 11 (0 generated)
levin
10 years, 5 months ago (2010-07-15 08:07:15 UTC) #1
Paweł Hajdan Jr.
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 = ...
10 years, 5 months ago (2010-07-15 15:26:17 UTC) #2
sky
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): ...
10 years, 5 months ago (2010-07-15 15:50:12 UTC) #3
levin
I believe that I have addressed all feedback. There seems to be an issue on ...
10 years, 5 months ago (2010-07-15 22:34:10 UTC) #4
sky
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(); ...
10 years, 5 months ago (2010-07-15 23:24:49 UTC) #5
Paweł Hajdan Jr.
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 ...
10 years, 5 months ago (2010-07-16 00:14:32 UTC) #6
levin
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): > > ...
10 years, 5 months ago (2010-07-16 00:29:10 UTC) #7
Paweł Hajdan Jr.
Code I commented in the drive-by LGTM.
10 years, 5 months ago (2010-07-16 00:32:25 UTC) #8
levin
Sky, Pawel, ISSUE 1: I'd like to check in the code with your previous comments ...
10 years, 5 months ago (2010-07-16 14:50:56 UTC) #9
sky
On Fri, Jul 16, 2010 at 7:50 AM, <levin@chromium.org> wrote: > Sky, Pawel, > ISSUE ...
10 years, 5 months ago (2010-07-16 14:57:13 UTC) #10
Paweł Hajdan Jr.
10 years, 5 months ago (2010-07-16 18:23:35 UTC) #11
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
> >
>

Powered by Google App Engine
This is Rietveld 408576698