remove toolstrips

remove toolstrips

BUG=51703
BUG=30178
BUG=25106
TEST=none

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

[Erik does not do reviews, 2010-08-10 23:41:08]

Apologies in advance for asking both of you to review this large change, but I
need you to look for whether this change breaks your respective projects.  It's
mostly not as bad as the size indicates, primarily a lot of code being deleted.

http://codereview.chromium.org/3129003/diff/2001/3040
File chrome/browser/views/frame/browser_view.cc (right):

http://codereview.chromium.org/3129003/diff/2001/3040#newcode426
chrome/browser/views/frame/browser_view.cc:426: {
anybody have a better style suggestion for this bit?

http://codereview.chromium.org/3129003/diff/2001/3048
File chrome/common/extensions/api/extension_api.json (left):

http://codereview.chromium.org/3129003/diff/2001/3048#oldcode2061
chrome/common/extensions/api/extension_api.json:2061: "namespace": "toolstrip",
Note that I didn't remove getToolstrips since that wasn't guarded by the
toolstrip permission.  Aaron, can you check to see if this is actually in use?

[Jeff Timanus, 2010-08-11 00:33:35]

Just a quick first-pass review of the CL.  I didn't see any major problems.  A
few changes are required in extension_process_bindings.cc.

I also noticed that you did not make any changes to the api_tests.  I know that
chrome\test\data\extensions\api_test\popup requires use of toolstrips.  Were you
planning on adding changes to that test (and potentially others) to this CL?

Jeff

http://codereview.chromium.org/3129003/diff/2001/3040
File chrome/browser/views/frame/browser_view.cc (right):

http://codereview.chromium.org/3129003/diff/2001/3040#newcode426
chrome/browser/views/frame/browser_view.cc:426: {
On 2010/08/10 23:41:08, Erik Kay wrote:
> anybody have a better style suggestion for this bit?
Seems the best alternative to me.

http://codereview.chromium.org/3129003/diff/4001/5007
File chrome/browser/cocoa/view_id_util_browsertest.mm (left):

http://codereview.chromium.org/3129003/diff/4001/5007#oldcode56
chrome/browser/cocoa/view_id_util_browsertest.mm:56: // Extension shelf is being
removed, http://crbug.com/30178.
Maybe add this bug to the change description?

http://codereview.chromium.org/3129003/diff/4001/5031
File chrome/browser/gtk/view_id_util_browsertest.cc (left):

http://codereview.chromium.org/3129003/diff/4001/5031#oldcode36
chrome/browser/gtk/view_id_util_browsertest.cc:36: // Extension shelf is being
removed, http://crbug.com/25106.
Also tag this bug in the CL?

http://codereview.chromium.org/3129003/diff/4001/5061
File chrome/renderer/extensions/extension_process_bindings.cc (right):

http://codereview.chromium.org/3129003/diff/4001/5061#newcode289
chrome/renderer/extensions/extension_process_bindings.cc:289: if
(viewtype_to_find != ViewType::EXTENSION_POPUP) {
You need to add the following condition here: 
&& viewtype_to_find != ViewType::TAB_CONTENTS

http://codereview.chromium.org/3129003/diff/4001/5061#newcode293
chrome/renderer/extensions/extension_process_bindings.cc:293: // Disallow
searching for a host view if we are a popup view.
This comment sounds awkward (even though it was something I wrote!)

I think it should be:
// Disallow searching for the same view type as the current view:  Popups can
only look for hosts, and hosts can only look for popups.

[Erik does not do reviews, 2010-08-11 15:52:09]

On 2010/08/11 00:33:35, Jeff Timanus wrote:
> Just a quick first-pass review of the CL.  I didn't see any major problems.  A
> few changes are required in extension_process_bindings.cc.

Cool.  I fixed those.


> I also noticed that you did not make any changes to the api_tests.  I know
that
> chrome\test\data\extensions\api_test\popup requires use of toolstrips.  Were
you
> planning on adding changes to that test (and potentially others) to this CL?

Actually I think all of the affected tests should be in this CL except for two:
autoupdate (I had already filed a separate bug for that one, and Antony will fix
that) and popup.

The reason I missed popup was that it appears that it doesn't fail.  After
looking a bit more, this makes sense to me.  The test is specified to only run
on Windows in the gyp file and in Windows it's marked as FLAKY.  Given that
fixing this test is likely to be somewhat complex and more in your arena, do you
mind if I file a bug and ask you to fix it?  (I can disable the test altogether
in the meantime)

[Jeff Timanus, 2010-08-11 18:42:56]

On 2010/08/11 15:52:09, Erik Kay wrote:
> On 2010/08/11 00:33:35, Jeff Timanus wrote:
> > Just a quick first-pass review of the CL.  I didn't see any major problems. 
A
> > few changes are required in extension_process_bindings.cc.
> 
> Cool.  I fixed those.
> 
> 
> > I also noticed that you did not make any changes to the api_tests.  I know
> that
> > chrome\test\data\extensions\api_test\popup requires use of toolstrips.  Were
> you
> > planning on adding changes to that test (and potentially others) to this CL?
> 
> Actually I think all of the affected tests should be in this CL except for
two:
> autoupdate (I had already filed a separate bug for that one, and Antony will
fix
> that) and popup.
> 
> The reason I missed popup was that it appears that it doesn't fail.  After
> looking a bit more, this makes sense to me.  The test is specified to only run
> on Windows in the gyp file and in Windows it's marked as FLAKY.  Given that
> fixing this test is likely to be somewhat complex and more in your arena, do
you
> mind if I file a bug and ask you to fix it?  (I can disable the test
altogether
> in the meantime)

Yes, that's fine.  Please file a bug for me.

[Erik does not do reviews, 2010-08-11 20:20:07]

On Wed, Aug 11, 2010 at 11:42 AM, <twiz@chromium.org> wrote:

> On 2010/08/11 15:52:09, Erik Kay wrote:
>
>> On 2010/08/11 00:33:35, Jeff Timanus wrote:
>> > Just a quick first-pass review of the CL.  I didn't see any major
>> problems.
>>
> A
>
>> > few changes are required in extension_process_bindings.cc.
>>
>
>  Cool.  I fixed those.
>>
>
>
>  > I also noticed that you did not make any changes to the api_tests.  I
>> know
>> that
>> > chrome\test\data\extensions\api_test\popup requires use of toolstrips.
>>  Were
>> you
>> > planning on adding changes to that test (and potentially others) to this
>> CL?
>>
>
>  Actually I think all of the affected tests should be in this CL except for
>>
> two:
>
>> autoupdate (I had already filed a separate bug for that one, and Antony
>> will
>>
> fix
>
>> that) and popup.
>>
>
>  The reason I missed popup was that it appears that it doesn't fail.  After
>> looking a bit more, this makes sense to me.  The test is specified to only
>> run
>> on Windows in the gyp file and in Windows it's marked as FLAKY.  Given
>> that
>> fixing this test is likely to be somewhat complex and more in your arena,
>> do
>>
> you
>
>> mind if I file a bug and ask you to fix it?  (I can disable the test
>>
> altogether
>
>> in the meantime)
>>
>
> Yes, that's fine.  Please file a bug for me.


Done and test disabled.

Erik


>
>
>
>
>
> http://codereview.chromium.org/3129003/show
>

[Aaron Boodman, 2010-08-11 20:41:29]

Sweet.

http://codereview.chromium.org/3129003/diff/16001/17013
File chrome/browser/extensions/extension_browsertests_misc.cc (left):

http://codereview.chromium.org/3129003/diff/16001/17013#oldcode101
chrome/browser/extensions/extension_browsertests_misc.cc:101: // Test for
compact language detection API. First navigate to a (static) html
do we have tests elsewhere for cld?

http://codereview.chromium.org/3129003/diff/16001/17013#oldcode140
chrome/browser/extensions/extension_browsertests_misc.cc:140:
host->render_view_host(), L"", L"testgetBackgroundPageAPI()", &result);
Do we have tests elsewhere for getBackgroundPage()?

http://codereview.chromium.org/3129003/diff/16001/17013#oldcode149
chrome/browser/extensions/extension_browsertests_misc.cc:149:
host->render_view_host(), L"", L"testgetExtensionTabsAPI()", &result);
Do we have tests elsewhere for getExtensionTabs().

[Erik does not do reviews, 2010-08-11 20:59:04]

http://codereview.chromium.org/3129003/diff/16001/17013
File chrome/browser/extensions/extension_browsertests_misc.cc (left):

http://codereview.chromium.org/3129003/diff/16001/17013#oldcode101
chrome/browser/extensions/extension_browsertests_misc.cc:101: // Test for
compact language detection API. First navigate to a (static) html
On 2010/08/11 20:41:29, Aaron Boodman wrote:
> do we have tests elsewhere for cld?

yes.  api_test/tabs/basics

http://codereview.chromium.org/3129003/diff/16001/17013#oldcode140
chrome/browser/extensions/extension_browsertests_misc.cc:140:
host->render_view_host(), L"", L"testgetBackgroundPageAPI()", &result);
On 2010/08/11 20:41:29, Aaron Boodman wrote:
> Do we have tests elsewhere for getBackgroundPage()?

yes.  api_test/get_views

http://codereview.chromium.org/3129003/diff/16001/17013#oldcode149
chrome/browser/extensions/extension_browsertests_misc.cc:149:
host->render_view_host(), L"", L"testgetExtensionTabsAPI()", &result);
On 2010/08/11 20:41:29, Aaron Boodman wrote:
> Do we have tests elsewhere for getExtensionTabs().

We don't appear to have an exact test for getExtensionTabs, but it is deprecated
and undocumented.  We do have a test for the functionality in get_views using
getViews({"type":"tab"}).  I can add a call to getExtensionTabs in there as well
if you'd like.

[Aaron Boodman, 2010-08-11 21:04:21]

LGTM

[Jeff Timanus, 2010-08-11 22:41:45]

On 2010/08/11 21:04:21, Aaron Boodman wrote:
> LGTM

LGTM

I verified that the removal does not break the experimental popup functionality.