Rip Out the Sidebar API

Rip Out the Sidebar API

This fix removes the sidebar api and all its references from the code (primarily relating to chrome/common/extensions and chrome/browser/ui).

BUG=107646
TEST=Covered by existing tests.

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

[Ben Goodger (Google), 2011-12-20 20:20:16]

On 2011/12/20 20:15:54, D Cronin wrote:

Are you also going to remove traces of it from the browser frame code? (It's OK
if that's done in a separate CL)

[Devlin, 2011-12-20 20:39:57]

I can certainly do that as well.  Thanks for pointing out where I missed it! :)

On 2011/12/20 20:20:16, Ben Goodger (Google) wrote:
> On 2011/12/20 20:15:54, D Cronin wrote:
> 
> Are you also going to remove traces of it from the browser frame code? (It's
OK
> if that's done in a separate CL)

[Aaron Boodman, 2011-12-20 21:22:00]

After I patched this in, I still see a few results that look related for 'git
grep -i sidebar' (besides the stuff under chrome/browser that ben mentions):

chrome/app/generated_resource.grd
chrome/common/chrome_notification_types.h
chrome/common/extensions/extension_constants.*
chrome/common/extensions/manifest.cc
chrome/common/extensions/manifest_unittest.cc
chrome/common/pref_names.*
chrome/renderer/extensions/schema_generated_bindings.cc
chrome/renderer/resources/extensions/schema_generated_bindings.js
chrome/test/data/extensions/api_test/sidebar
chrome/test/data/extensions/manifest_tests/sidebar*

[Devlin, 2011-12-21 02:11:27]

The latest patch includes removal of the sidebar from more places in
chrome/common, as well as removal from chrome/browser/ui.  I ran unit_tests,
browser_tests, and ui_tests locally, and it passes all of them (or rather, it
passes all of them that a clean checkout passes), but would appreciate an extra
set of eyes, particularly in the commented areas.

By the way, there is no rush on this - please don't feel the obligated to get to
it before the new year. :)

http://codereview.chromium.org/9006027/diff/8001/chrome/browser/ui/views/fram...
File chrome/browser/ui/views/frame/browser_view.cc (left):

http://codereview.chromium.org/9006027/diff/8001/chrome/browser/ui/views/fram...
chrome/browser/ui/views/frame/browser_view.cc:1414:
///////////////////////////////////////////////////////////////////////////////
Am I right in thinking that, since the sidebar was the only notification, we no
longer need to implement the observe method or inherit from
NotificationObserver?

http://codereview.chromium.org/9006027/diff/8001/chrome/browser/ui/views/fram...
File chrome/browser/ui/views/frame/browser_view.h (right):

http://codereview.chromium.org/9006027/diff/8001/chrome/browser/ui/views/fram...
chrome/browser/ui/views/frame/browser_view.h:580: // |Page content (contents_)  
                                  ||
I think this is the proper new layout, but would appreciate confirmation.

http://codereview.chromium.org/9006027/diff/8001/chrome/browser/ui/views/fram...
File chrome/browser/ui/views/frame/browser_view_layout.cc (left):

http://codereview.chromium.org/9006027/diff/8001/chrome/browser/ui/views/fram...
chrome/browser/ui/views/frame/browser_view_layout.cc:436:
contents_split_->CalculateChildrenBounds(
Looking at the CalculateChildrenBounds method, it appeared that if the sidebar
was never used, any time calculate children was called it would populate the
second/third arguments with a new gfx::Rect, which means there should be no
point in calling it...however, I may have grossly misunderstood this.  Please
let me know.

[Aaron Boodman, 2011-12-21 08:38:20]

OK, net's a little too wide now.

http://codereview.chromium.org/9006027/diff/8001/chrome/browser/resources/com...
File chrome/browser/resources/component_extension_resources.grd (left):

http://codereview.chromium.org/9006027/diff/8001/chrome/browser/resources/com...
chrome/browser/resources/component_extension_resources.grd:49: <include
name="IDR_FILE_MANAGER_OPEN_SIDEBAR_ICON"
file="file_manager/images/open_sidebar.png" type="BINDATA" />
I think this is a different 'sidebar'. They should stay.

http://codereview.chromium.org/9006027/diff/8001/chrome/browser/resources/fil...
File chrome/browser/resources/file_manager/css/file_manager.css (left):

http://codereview.chromium.org/9006027/diff/8001/chrome/browser/resources/fil...
chrome/browser/resources/file_manager/css/file_manager.css:121: .dialog-sidebar
{
Also wrong sidebar. I believe that this is an element of the file manager UI
(inside the tab), not the extension sidebar.

http://codereview.chromium.org/9006027/diff/8001/chrome/browser/resources/fil...
chrome/browser/resources/file_manager/css/file_manager.css:212:
.dialog-container[sidebar] .dialog-header {
Same with everything in the file_manager directory.

http://codereview.chromium.org/9006027/diff/8001/chrome/browser/resources/pri...
File chrome/browser/resources/print_preview/margin_settings.js (left):

http://codereview.chromium.org/9006027/diff/8001/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/margin_settings.js:334:
onSidebarMouseOver_: function(e) {
The occurrences of 'sidebar' in print_preview should also stay.

http://codereview.chromium.org/9006027/diff/8001/chrome/browser/ui/browser.h
File chrome/browser/ui/browser.h (right):

http://codereview.chromium.org/9006027/diff/8001/chrome/browser/ui/browser.h#...
chrome/browser/ui/browser.h:113: FEATURE_DOWNLOADSHELF = 128
Can move this to 64 now.

http://codereview.chromium.org/9006027/diff/8001/chrome/browser/ui/views/fram...
File chrome/browser/ui/views/frame/browser_view.cc (left):

http://codereview.chromium.org/9006027/diff/8001/chrome/browser/ui/views/fram...
chrome/browser/ui/views/frame/browser_view.cc:1414:
///////////////////////////////////////////////////////////////////////////////
On 2011/12/21 02:11:27, D Cronin wrote:
> Am I right in thinking that, since the sidebar was the only notification, we
no
> longer need to implement the observe method or inherit from
> NotificationObserver?

Yes. Also good job removing registrar_.

http://codereview.chromium.org/9006027/diff/8001/chrome/browser/ui/views/fram...
File chrome/browser/ui/views/frame/browser_view.h (right):

http://codereview.chromium.org/9006027/diff/8001/chrome/browser/ui/views/fram...
chrome/browser/ui/views/frame/browser_view.h:25: #include
"content/public/browser/notification_registrar.h"
This can go.

http://codereview.chromium.org/9006027/diff/8001/chrome/browser/ui/views/fram...
File chrome/browser/ui/views/frame/browser_view_layout.cc (left):

http://codereview.chromium.org/9006027/diff/8001/chrome/browser/ui/views/fram...
chrome/browser/ui/views/frame/browser_view_layout.cc:436:
contents_split_->CalculateChildrenBounds(
On 2011/12/21 02:11:27, D Cronin wrote:
> Looking at the CalculateChildrenBounds method, it appeared that if the sidebar
> was never used, any time calculate children was called it would populate the
> second/third arguments with a new gfx::Rect, which means there should be no
> point in calling it...however, I may have grossly misunderstood this.  Please
> let me know.

Ben should confirm this.

http://codereview.chromium.org/9006027/diff/8001/chrome/common/extensions/api...
File chrome/common/extensions/api/devtools_api.json (left):

http://codereview.chromium.org/9006027/diff/8001/chrome/common/extensions/api...
chrome/common/extensions/api/devtools_api.json:291: "id":
"ExtensionSidebarPane",
Wrong 'sidebar'. This should stay.

http://codereview.chromium.org/9006027/diff/8001/chrome/common/extensions/doc...
File chrome/common/extensions/docs/a11y.html (left):

http://codereview.chromium.org/9006027/diff/8001/chrome/common/extensions/doc...
chrome/common/extensions/docs/a11y.html:18: <script type="text/javascript"
src="js/sidebar.js"></script>
Wrong sidebar. This should stay.

[Devlin, 2011-12-21 18:07:52]

All requested changes have been made.  Sorry about removing in excess - I
guessed that running all the ui/browser tests and the unittests I modified would
have caught that....Is there another test suite I should be running in the
future?

As always, thank you very much for your time.

On 2011/12/21 08:38:20, Aaron Boodman wrote:
> OK, net's a little too wide now.
> 
>
http://codereview.chromium.org/9006027/diff/8001/chrome/browser/resources/com...
> File chrome/browser/resources/component_extension_resources.grd (left):
> 
>
http://codereview.chromium.org/9006027/diff/8001/chrome/browser/resources/com...
> chrome/browser/resources/component_extension_resources.grd:49: <include
> name="IDR_FILE_MANAGER_OPEN_SIDEBAR_ICON"
> file="file_manager/images/open_sidebar.png" type="BINDATA" />
> I think this is a different 'sidebar'. They should stay.
> 
>
http://codereview.chromium.org/9006027/diff/8001/chrome/browser/resources/fil...
> File chrome/browser/resources/file_manager/css/file_manager.css (left):
> 
>
http://codereview.chromium.org/9006027/diff/8001/chrome/browser/resources/fil...
> chrome/browser/resources/file_manager/css/file_manager.css:121:
.dialog-sidebar
> {
> Also wrong sidebar. I believe that this is an element of the file manager UI
> (inside the tab), not the extension sidebar.
> 
>
http://codereview.chromium.org/9006027/diff/8001/chrome/browser/resources/fil...
> chrome/browser/resources/file_manager/css/file_manager.css:212:
> .dialog-container[sidebar] .dialog-header {
> Same with everything in the file_manager directory.
> 
>
http://codereview.chromium.org/9006027/diff/8001/chrome/browser/resources/pri...
> File chrome/browser/resources/print_preview/margin_settings.js (left):
> 
>
http://codereview.chromium.org/9006027/diff/8001/chrome/browser/resources/pri...
> chrome/browser/resources/print_preview/margin_settings.js:334:
> onSidebarMouseOver_: function(e) {
> The occurrences of 'sidebar' in print_preview should also stay.
> 
> http://codereview.chromium.org/9006027/diff/8001/chrome/browser/ui/browser.h
> File chrome/browser/ui/browser.h (right):
> 
>
http://codereview.chromium.org/9006027/diff/8001/chrome/browser/ui/browser.h#...
> chrome/browser/ui/browser.h:113: FEATURE_DOWNLOADSHELF = 128
> Can move this to 64 now.
> 
>
http://codereview.chromium.org/9006027/diff/8001/chrome/browser/ui/views/fram...
> File chrome/browser/ui/views/frame/browser_view.cc (left):
> 
>
http://codereview.chromium.org/9006027/diff/8001/chrome/browser/ui/views/fram...
> chrome/browser/ui/views/frame/browser_view.cc:1414:
>
///////////////////////////////////////////////////////////////////////////////
> On 2011/12/21 02:11:27, D Cronin wrote:
> > Am I right in thinking that, since the sidebar was the only notification, we
> no
> > longer need to implement the observe method or inherit from
> > NotificationObserver?
> 
> Yes. Also good job removing registrar_.
> 
>
http://codereview.chromium.org/9006027/diff/8001/chrome/browser/ui/views/fram...
> File chrome/browser/ui/views/frame/browser_view.h (right):
> 
>
http://codereview.chromium.org/9006027/diff/8001/chrome/browser/ui/views/fram...
> chrome/browser/ui/views/frame/browser_view.h:25: #include
> "content/public/browser/notification_registrar.h"
> This can go.
> 
>
http://codereview.chromium.org/9006027/diff/8001/chrome/browser/ui/views/fram...
> File chrome/browser/ui/views/frame/browser_view_layout.cc (left):
> 
>
http://codereview.chromium.org/9006027/diff/8001/chrome/browser/ui/views/fram...
> chrome/browser/ui/views/frame/browser_view_layout.cc:436:
> contents_split_->CalculateChildrenBounds(
> On 2011/12/21 02:11:27, D Cronin wrote:
> > Looking at the CalculateChildrenBounds method, it appeared that if the
sidebar
> > was never used, any time calculate children was called it would populate the
> > second/third arguments with a new gfx::Rect, which means there should be no
> > point in calling it...however, I may have grossly misunderstood this. 
Please
> > let me know.
> 
> Ben should confirm this.
> 
>
http://codereview.chromium.org/9006027/diff/8001/chrome/common/extensions/api...
> File chrome/common/extensions/api/devtools_api.json (left):
> 
>
http://codereview.chromium.org/9006027/diff/8001/chrome/common/extensions/api...
> chrome/common/extensions/api/devtools_api.json:291: "id":
> "ExtensionSidebarPane",
> Wrong 'sidebar'. This should stay.
> 
>
http://codereview.chromium.org/9006027/diff/8001/chrome/common/extensions/doc...
> File chrome/common/extensions/docs/a11y.html (left):
> 
>
http://codereview.chromium.org/9006027/diff/8001/chrome/common/extensions/doc...
> chrome/common/extensions/docs/a11y.html:18: <script type="text/javascript"
> src="js/sidebar.js"></script>
> Wrong sidebar. This should stay.

[Aaron Boodman, 2011-12-21 18:49:00]

http://codereview.chromium.org/9006027/diff/13009/chrome/common/extensions/do...
File chrome/common/extensions/docs/template/page_shell.html (left):

http://codereview.chromium.org/9006027/diff/13009/chrome/common/extensions/do...
chrome/common/extensions/docs/template/page_shell.html:22: <script
type="text/javascript" src="js/sidebar.js"></script>
You still need to undo this (and regen the docs)

[Devlin, 2011-12-21 19:47:12]

Sorry for missing that - I made the mistake of not realizing that your comment
in a11y.html applied to all the docs.  Apologies - it should be fixed now.

On 2011/12/21 18:49:00, Aaron Boodman wrote:
>
http://codereview.chromium.org/9006027/diff/13009/chrome/common/extensions/do...
> File chrome/common/extensions/docs/template/page_shell.html (left):
> 
>
http://codereview.chromium.org/9006027/diff/13009/chrome/common/extensions/do...
> chrome/common/extensions/docs/template/page_shell.html:22: <script
> type="text/javascript" src="js/sidebar.js"></script>
> You still need to undo this (and regen the docs)

http://codereview.chromium.org/9006027/diff/11016/chrome/common/extensions/do...
File chrome/common/extensions/docs/images/intermediate/i18n-after.graffle
(left):

http://codereview.chromium.org/9006027/diff/11016/chrome/common/extensions/do...
chrome/common/extensions/docs/images/intermediate/i18n-after.graffle:2134:
<key>RightSidebar</key>
Is this (and others in chrome/common/docs/images/intermediate/) the same
sidebar?

[Aaron Boodman, 2011-12-21 20:31:00]

http://codereview.chromium.org/9006027/diff/11016/chrome/common/extensions/do...
File chrome/common/extensions/docs/images/intermediate/i18n-after.graffle
(left):

http://codereview.chromium.org/9006027/diff/11016/chrome/common/extensions/do...
chrome/common/extensions/docs/images/intermediate/i18n-after.graffle:2134:
<key>RightSidebar</key>
On 2011/12/21 19:47:13, D Cronin wrote:
> Is this (and others in chrome/common/docs/images/intermediate/) the same
> sidebar?

This should also not be removed. Thanks for bringing it up.

[Devlin, 2011-12-22 00:58:21]

Fixed.  Anything else? :)

On 2011/12/21 20:31:00, Aaron Boodman wrote:
>
http://codereview.chromium.org/9006027/diff/11016/chrome/common/extensions/do...
> File chrome/common/extensions/docs/images/intermediate/i18n-after.graffle
> (left):
> 
>
http://codereview.chromium.org/9006027/diff/11016/chrome/common/extensions/do...
> chrome/common/extensions/docs/images/intermediate/i18n-after.graffle:2134:
> <key>RightSidebar</key>
> On 2011/12/21 19:47:13, D Cronin wrote:
> > Is this (and others in chrome/common/docs/images/intermediate/) the same
> > sidebar?
> 
> This should also not be removed. Thanks for bringing it up.

[Aaron Boodman, 2011-12-22 01:03:22]

LGTM, but a UI person should weigh in on the browser frame changes. Let me try
and scare someone up.

[Aaron Boodman, 2011-12-22 01:04:12]

+some UI people. Looking for someone to review the changes to
browser_view_layout.cc.

Thanks!

[Peter Kasting, 2011-12-22 02:10:51]

I don't really know browser_view_layout.cc very well but the changes seem
obvious enough, LGTM

http://codereview.chromium.org/9006027/diff/14014/chrome/browser/ui/views/fra...
File chrome/browser/ui/views/frame/browser_view_layout.cc (right):

http://codereview.chromium.org/9006027/diff/14014/chrome/browser/ui/views/fra...
chrome/browser/ui/views/frame/browser_view_layout.cc:416: // The ultimate idea
is to calcualte bounds and reserved areas for all
Nit: While here, calcualte -> calculate

http://codereview.chromium.org/9006027/diff/14014/chrome/browser/ui/views/fra...
chrome/browser/ui/views/frame/browser_view_layout.cc:433: // Layout resize
corner and calculate reserved contents
Nit: Rewrap comments

[Devlin, 2011-12-22 16:44:53]

Done.  Thanks for the input.

On 2011/12/22 02:10:51, Peter Kasting wrote:
> I don't really know browser_view_layout.cc very well but the changes seem
> obvious enough, LGTM
> 
>
http://codereview.chromium.org/9006027/diff/14014/chrome/browser/ui/views/fra...
> File chrome/browser/ui/views/frame/browser_view_layout.cc (right):
> 
>
http://codereview.chromium.org/9006027/diff/14014/chrome/browser/ui/views/fra...
> chrome/browser/ui/views/frame/browser_view_layout.cc:416: // The ultimate idea
> is to calcualte bounds and reserved areas for all
> Nit: While here, calcualte -> calculate
> 
>
http://codereview.chromium.org/9006027/diff/14014/chrome/browser/ui/views/fra...
> chrome/browser/ui/views/frame/browser_view_layout.cc:433: // Layout resize
> corner and calculate reserved contents
> Nit: Rewrap comments

[Aaron Boodman, 2011-12-26 19:39:02]

Ok, can you please sync, resolve any changes, run relevant* tests locally, and
then upload a fresh change. I will land after that.

relevant* ~= unit_tests --gtest_filter=Extension*; browser_tests
--gtest_filter=Extension*

[Devlin, 2012-01-02 18:05:39]

Done.  Changes resolved, passing all relevant tests locally.

On 2011/12/26 19:39:02, Aaron Boodman wrote:
> Ok, can you please sync, resolve any changes, run relevant* tests locally, and
> then upload a fresh change. I will land after that.
> 
> relevant* ~= unit_tests --gtest_filter=Extension*; browser_tests
> --gtest_filter=Extension*

[commit-bot: I haz the power, 2012-01-03 00:25:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/9006027/25001

[commit-bot: I haz the power, 2012-01-03 00:26:14]

Can't apply patch for file chrome/browser/sidebar/sidebar_browsertest.cc.
While running patch -p0 --forward --force;
patching file chrome/browser/sidebar/sidebar_browsertest.cc
Hunk #1 FAILED at 1.
File chrome/browser/sidebar/sidebar_browsertest.cc is not empty after patch, as
expected
1 out of 1 hunk FAILED -- saving rejects to file
chrome/browser/sidebar/sidebar_browsertest.cc.rej

[Aaron Boodman, 2012-01-03 06:47:45]

Tried patching locally, get same issue. Can you let me know what revision this
patch is against?

[Devlin, 2012-01-04 02:52:56]

On 2012/01/03 06:47:45, Aaron Boodman wrote:
> Tried patching locally, get same issue. Can you let me know what revision this
> patch is against?

It was against revision 116101, which was current at the time...Would
resyncing/resolving/re-uploading fix the issue?

[Aaron Boodman, 2012-01-05 23:23:56]

I ran this through the trybot, and it had some problems on os x:

http://build.chromium.org/p/tryserver.chromium/builders/mac_rel/builds/7482

I can't immediately see what the issue is, but I'm in the process of getting you
trybot access so that you can figure it out yourself :)

[Aaron Boodman, 2012-01-06 02:34:23]

Devlin, this work is starting to block some other people in the area. Can you
please take a look at the mac errors and see if you can understand what's going
on and/or use your new trybot powers to test possible fixes?

If not, let me know and I'll try to help.

Thanks!

[Aaron Boodman, 2012-01-06 02:39:40]

Crap, I gave you the wrong trybot results. Actual results here:

http://build.chromium.org/p/tryserver.chromium/builders/mac_rel/builds/7472

Looking at this again, I don't understand what it has to do with your change.
I'll start another try job on mac_rel in the meantime.

[Devlin, 2012-01-07 22:01:00]

All bugs tracked down, thanks to the try-bot access :)  Latest patch fails only
flaky/failing tests on linux-rel, mac-rel, and win-rel.  Hopefully everything is
good to go.

[commit-bot: I haz the power, 2012-01-07 22:19:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/9006027/46002

[commit-bot: I haz the power, 2012-01-07 22:19:34]

Presubmit check for 9006027-46002 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
chrome/browser/ui/cocoa/sidebar_controller.mm,chrome/browser/ui/cocoa/view_id_util_browsertest.mm,chrome/browser/ui/cocoa/browser_window_controller.h,chrome/browser/ui/cocoa/browser_window_controller.mm,chrome/browser/ui/cocoa/browser_window_controller_unittest.mm,chrome/browser/ui/cocoa/browser_window_cocoa.h,chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm,chrome/browser/ui/gtk/view_id_util_browsertest.cc,chrome/browser/ui/cocoa/browser_window_cocoa.mm,chrome/browser/ui/cocoa/sidebar_controller.h

Presubmit checks took 5.1s to calculate.

[Aaron Boodman, 2012-01-08 06:53:01]

+cocoa and gtk owners

[Nico, 2012-01-08 07:03:23]

Cool!

Can you also undo the root window resizer rect changes that were done when this
landed ( http://codereview.chromium.org/3547008 )? Then you'll fix
http://crbug.com/70482.

[Aaron Boodman, 2012-01-08 07:36:36]

On Sat, Jan 7, 2012 at 11:03 PM,  <thakis@chromium.org> wrote:
> Can you also undo the root window resizer rect changes that were done when
> this
> landed ( http://codereview.chromium.org/3547008 )? Then you'll fix
> http://crbug.com/70482.

I would rather that someone more familiar with the mac UI did that.

- a

[Nico, 2012-01-08 16:31:58]

Makes sense. Mac parts lgtm.
On Jan 7, 2012 11:36 PM, "Aaron Boodman" <aa@chromium.org> wrote:

> On Sat, Jan 7, 2012 at 11:03 PM,  <thakis@chromium.org> wrote:
> > Can you also undo the root window resizer rect changes that were done
> when
> > this
> > landed ( http://codereview.chromium.org/3547008 )? Then you'll fix
> > http://crbug.com/70482.
>
> I would rather that someone more familiar with the mac UI did that.
>
> - a
>

[Elliot Glaysher, 2012-01-09 17:34:26]

two line gtk patch lgtm

[commit-bot: I haz the power, 2012-01-09 18:40:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/9006027/46002

[commit-bot: I haz the power, 2012-01-09 19:53:33]

Change committed as 116901