Remove wrench button from panel.

Remove wrench button from panel.

Panel.xib has been changed for the purpose of removing wrench button.

BUG=123182
TEST=existing tests and manual test to verify that the wrench button is gone

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

[jianli, 2012-04-16 18:21:41]

dimich, could you please review all panel related changes?
estade, could you please review changes to browser_titlebar.* for GTK?

[Evan Stade, 2012-04-16 19:23:48]

lgtm

http://codereview.chromium.org/9956145/diff/7001/chrome/browser/ui/gtk/browse...
File chrome/browser/ui/gtk/browser_titlebar.h (right):

http://codereview.chromium.org/9956145/diff/7001/chrome/browser/ui/gtk/browse...
chrome/browser/ui/gtk/browser_titlebar.h:78: // button, the close button does
not become clickable. Neither of
grammar

[Dmitry Titov, 2012-04-16 19:45:05]

lgtm with 2 notes:

You need to also remove the wrench button elements from Panel.xib

http://codereview.chromium.org/9956145/diff/7001/chrome/browser/ui/panels/pan...
File chrome/browser/ui/panels/panel_titlebar_view_cocoa.mm (left):

http://codereview.chromium.org/9956145/diff/7001/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_titlebar_view_cocoa.mm:381: int availableWidth =
NSWidth(bounds) - kButtonPadding * 4 -
this should go from 4 to 2 since there are no paddings around.

[jennb, 2012-04-16 21:17:05]

Did you actually try this build and test on each platform?

http://codereview.chromium.org/9956145/diff/7001/chrome/browser/ui/gtk/browse...
File chrome/browser/ui/gtk/browser_titlebar.h (right):

http://codereview.chromium.org/9956145/diff/7001/chrome/browser/ui/gtk/browse...
chrome/browser/ui/gtk/browser_titlebar.h:86: void
SendEnterNotifyToCloseButtonIfUnderMouse();
Is this still needed now that there's no wrench button?

http://codereview.chromium.org/9956145/diff/7001/chrome/browser/ui/panels/pan...
File chrome/browser/ui/panels/panel_browser_frame_view.cc (left):

http://codereview.chromium.org/9956145/diff/7001/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_browser_frame_view.cc:993: const Extension*
extension = panel_browser_view_->panel()->GetExtension();
Looks like all uses of Panel::GetExtension() have been deleted. Should also
delete that method from Panel class.

[Dmitry Titov, 2012-04-16 22:11:35]

On 2012/04/16 21:17:05, jennb wrote:
> Did you actually try this build and test on each platform?

It'd help if we all were more specific in the codereview notes. While tryjobs do
build and test on all platforms, I think you could have something different in
mind here. Could you provide more info?

[jennb, 2012-04-16 22:13:31]

I was thinking about interactive use, such as whether the GTK workaround
was verified as still needed. Jian and I talked offline. He said he did
confirm this and that the original diagnosis of the bug being related to
the wrench button was incorrect.

On Mon, Apr 16, 2012 at 3:11 PM, <dimich@chromium.org> wrote:

> On 2012/04/16 21:17:05, jennb wrote:
>
>> Did you actually try this build and test on each platform?
>>
>
> It'd help if we all were more specific in the codereview notes. While
> tryjobs do
> build and test on all platforms, I think you could have something
> different in
> mind here. Could you provide more info?
>
>
http://codereview.chromium.**org/9956145/<http://codereview.chromium.org/9956...
>

[jianli, 2012-04-16 22:21:42]

I've built and run chrome in all 3 platforms and make sure panels could be
launched and operated correctly, in addition to our existing automatic
tests.

For the GTK workaround, we still need it since the problem has nothing to
do with wrench button. The description in the bug report is correct and
does not say anything to do with other button. So I updated comment in the
workaround comment to get rid of the part related to wrench button. I will
make it more clear.

On Mon, Apr 16, 2012 at 3:13 PM, Jenn Braithwaite (胡慧鋒)
<jennb@chromium.org>wrote:

> I was thinking about interactive use, such as whether the GTK workaround
> was verified as still needed. Jian and I talked offline. He said he did
> confirm this and that the original diagnosis of the bug being related to
> the wrench button was incorrect.
>
>
> On Mon, Apr 16, 2012 at 3:11 PM, <dimich@chromium.org> wrote:
>
>> On 2012/04/16 21:17:05, jennb wrote:
>>
>>> Did you actually try this build and test on each platform?
>>>
>>
>> It'd help if we all were more specific in the codereview notes. While
>> tryjobs do
>> build and test on all platforms, I think you could have something
>> different in
>> mind here. Could you provide more info?
>>
>>
http://codereview.chromium.**org/9956145/<http://codereview.chromium.org/9956...
>>
>
>

[Dmitry Titov, 2012-04-16 22:23:40]

Thanks!

I'd add a note of my own about building:
Jian, when you update Panel.xib, we need to make sure that it compiles w/o
warnings (there was a bug on that recently, we should keep our xibs to compile
w/o warnings about incorrect outlets and so on). This won't break a tryjob, but
it'll get us a bug later.


On 2012/04/16 22:13:31, jennb wrote:
> I was thinking about interactive use, such as whether the GTK workaround
> was verified as still needed. Jian and I talked offline. He said he did
> confirm this and that the original diagnosis of the bug being related to
> the wrench button was incorrect.

[jianli, 2012-04-16 23:24:45]

All feedbacks are addressed. Will land the patch after the new round of try jobs
pass.

http://codereview.chromium.org/9956145/diff/7001/chrome/browser/ui/gtk/browse...
File chrome/browser/ui/gtk/browser_titlebar.h (right):

http://codereview.chromium.org/9956145/diff/7001/chrome/browser/ui/gtk/browse...
chrome/browser/ui/gtk/browser_titlebar.h:78: // button, the close button does
not become clickable. Neither of
On 2012/04/16 19:23:48, Evan Stade wrote:
> grammar

Done.

http://codereview.chromium.org/9956145/diff/7001/chrome/browser/ui/gtk/browse...
chrome/browser/ui/gtk/browser_titlebar.h:86: void
SendEnterNotifyToCloseButtonIfUnderMouse();
On 2012/04/16 21:17:05, jennb wrote:
> Is this still needed now that there's no wrench button? 

Yes, it is still needed. Update comment to make it clear.

http://codereview.chromium.org/9956145/diff/7001/chrome/browser/ui/panels/pan...
File chrome/browser/ui/panels/panel_browser_frame_view.cc (left):

http://codereview.chromium.org/9956145/diff/7001/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_browser_frame_view.cc:993: const Extension*
extension = panel_browser_view_->panel()->GetExtension();
On 2012/04/16 21:17:05, jennb wrote:
> Looks like all uses of Panel::GetExtension() have been deleted. Should also
> delete that method from Panel class.

Done.

http://codereview.chromium.org/9956145/diff/7001/chrome/browser/ui/panels/pan...
File chrome/browser/ui/panels/panel_titlebar_view_cocoa.mm (left):

http://codereview.chromium.org/9956145/diff/7001/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_titlebar_view_cocoa.mm:381: int availableWidth =
NSWidth(bounds) - kButtonPadding * 4 -
On 2012/04/16 19:45:05, Dmitry Titov wrote:
> this should go from 4 to 2 since there are no paddings around.

Done.

[commit-bot: I haz the power, 2012-04-17 03:57:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jianli@chromium.org/9956145/12002

[commit-bot: I haz the power, 2012-04-17 04:51:28]

Try job failure for 9956145-12002 (retry) on mac_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-04-17 06:42:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jianli@chromium.org/9956145/12002

[commit-bot: I haz the power, 2012-04-17 08:08:21]

Change committed as 132551