Panels back behind a flag.

Panels back behind a flag except for dev/canary.
Giving it more time to bake.

BUG=none
TEST=tests updated


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

[jennb, 2011-12-08 00:57:53]

asargent for extension changes.
dimich for panel changes.

[Dmitry Titov, 2011-12-08 01:34:52]

Lets not land this until we get a final nod from others. I can serve as a
conduit here.

That being said, I think we should add the following:
1. It is beneficial to keep it enabled on Dev and Canary. These builds are
widely used in development process and we will get more user feedback if we do
that. chrome::VersionInfo provides necessary flags.

2. There are 3 more (4 total) extensions that needs to be enabled for testing. I
have the list.

[jennb, 2011-12-08 06:50:44]

On 2011/12/08 01:34:52, Dmitry Titov wrote:
> Lets not land this until we get a final nod from others. I can serve as a
> conduit here.
> 
> That being said, I think we should add the following:
> 1. It is beneficial to keep it enabled on Dev and Canary. These builds are
> widely used in development process and we will get more user feedback if we do
> that. chrome::VersionInfo provides necessary flags.
> 
> 2. There are 3 more (4 total) extensions that needs to be enabled for testing.
I
> have the list.

What do you expect to see in about:flags? Doesn't look possible to
conditionalize what appears for panels based on versioninfo. It's a const table.

[asargent_no_longer_on_chrome, 2011-12-08 17:59:19]

As it stands, extension parts LGTM. From Dmitry's message it sounds like you
might need to add an extra check with a few more id's to the ShouldUsePanels
method.

[jennb, 2011-12-08 20:12:54]

Changed to have panels always enabled in dev/canary for more productive baking
time.

Removed changes to Extension class. Logic is now in PanelManager.

[Dmitry Titov, 2011-12-08 20:59:01]

lgtm with a naming nit:

http://codereview.chromium.org/8863011/diff/7001/chrome/browser/ui/panels/pan...
File chrome/browser/ui/panels/panel_manager.h (right):

http://codereview.chromium.org/8863011/diff/7001/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_manager.h:39: static bool ShouldUsePanels(const
std::string& id);
could we name the param 'extension_id' to make it more descriptive?
Another variant would be to name the method EnablePanelsForExtension(string id);

[asargent_no_longer_on_chrome, 2011-12-08 21:21:26]

lgtm

http://codereview.chromium.org/8863011/diff/7001/chrome/browser/ui/panels/pan...
File chrome/browser/ui/panels/panel_manager.h (right):

http://codereview.chromium.org/8863011/diff/7001/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_manager.h:39: static bool ShouldUsePanels(const
std::string& id);
nit: it might make sense to either rename the parameter extension_id, or mention
in the comment something like "...used for the extension with |id|."

[commit-bot: I haz the power, 2011-12-09 21:02:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jennb@chromium.org/8863011/4011

[commit-bot: I haz the power, 2011-12-09 21:02:38]

Can't apply patch for file chrome/browser/extensions/extension_tabs_module.cc.
While running patch -p1 --forward --force;
patching file chrome/browser/extensions/extension_tabs_module.cc
Hunk #1 FAILED at 31.
Hunk #2 succeeded at 462 (offset 1 line).
1 out of 2 hunks FAILED -- saving rejects to file
chrome/browser/extensions/extension_tabs_module.cc.rej

[commit-bot: I haz the power, 2011-12-09 21:16:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jennb@chromium.org/8863011/16001

[commit-bot: I haz the power, 2011-12-09 22:24:58]

Change committed as 113866

[mihaip_google.com, 2011-12-10 02:26:39]

Will this be merged into the M16 branch?

Mihai

On Fri, Dec 9, 2011 at 2:24 PM, <commit-bot@chromium.org> wrote:

> Change committed as 113866
>
>
http://codereview.chromium.**org/8863011/<http://codereview.chromium.org/8863...
>

[Dmitry Titov, 2011-12-10 18:21:40]

Not into M16... Panels were disabled by default there because they were not
ready yet. It is in preparation to merging it into M17 beta...

On Fri, Dec 9, 2011 at 6:26 PM, Mihai Parparita <mihaip@google.com> wrote:

> Will this be merged into the M16 branch?
>
> Mihai
>
>
> On Fri, Dec 9, 2011 at 2:24 PM, <commit-bot@chromium.org> wrote:
>
>> Change committed as 113866
>>
>>
http://codereview.chromium.**org/8863011/<http://codereview.chromium.org/8863...
>>
>
>

[mihaip_google.com, 2011-12-10 21:11:13]

Oops, I meant M17. Good to hear that you'll be merging it there.

Mihai

On Sat, Dec 10, 2011 at 10:21 AM, Dmitry Titov <dimich@chromium.org> wrote:

> Not into M16... Panels were disabled by default there because they were
> not ready yet. It is in preparation to merging it into M17 beta...
>
>
> On Fri, Dec 9, 2011 at 6:26 PM, Mihai Parparita <mihaip@google.com> wrote:
>
>> Will this be merged into the M16 branch?
>>
>> Mihai
>>
>>
>> On Fri, Dec 9, 2011 at 2:24 PM, <commit-bot@chromium.org> wrote:
>>
>>> Change committed as 113866
>>>
>>>
http://codereview.chromium.**org/8863011/<http://codereview.chromium.org/8863...
>>>
>>
>>
>