Show "About panel" bubble on Windows.

Show "About panel" bubble on Windows.

BUG=none
TEST=panel_browser_view_browsertest
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=86712

[jianli, 2011-05-12 01:56:38]

I introduced PanelOriginatorDelegate interface that is used to provide the info
needed to populate "About Panel" bubble. For now, we only support extension as
originator for panel.

Ben, I added GetAsPanel() to BrowserWindow interface for getting Panel instance
when BrowserWindow is indeed a panel. This is used in browser.cc. Please review
this change.

Jenn, could you please review all other panel related stuffs?

[jennb, 2011-05-12 18:15:51]

It's disconcerting that browser.cc needs to GetAsPanel() and do special panel
logic to set the originator.  Also, there are other places that create Panels
for extensions that aren't covered in this patch.  Is there another way to
determine that the panel belongs to an extension and extract information from
that extension for the About Panel dialog?  Also, how do you plan to handle
About Panel dialog [in future] if we allow Panels to be created outside of
extensions/apps?

http://codereview.chromium.org/7011015/diff/1/chrome/app/generated_resources.grd
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/7011015/diff/1/chrome/app/generated_resources....
chrome/app/generated_resources.grd:12360: Uninstall <ph
name="EXTENSION_NAME">$1<ex>Sample Extension</ex></ph>
The extension name should already be visible in the "About Panel" bubble.  Do we
really need to repeat it here?

http://codereview.chromium.org/7011015/diff/1/chrome/app/resources/locale_set...
File chrome/app/resources/locale_settings.grd (right):

http://codereview.chromium.org/7011015/diff/1/chrome/app/resources/locale_set...
chrome/app/resources/locale_settings.grd:549: <!-- characters and lines (See
above). -->
See above where?  I'm not sure where I should be looking and what I should be
looking for...

http://codereview.chromium.org/7011015/diff/1/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/7011015/diff/1/chrome/browser/ui/browser.cc#ne...
chrome/browser/ui/browser.cc:602: Panel* panel =
browser->window()->GetAsPanel();
There are other places where a Browser of type PANEL can be created that will
end up without an originator set.

http://codereview.chromium.org/7011015/diff/1/chrome/browser/ui/panels/panel.cc
File chrome/browser/ui/panels/panel.cc (right):

http://codereview.chromium.org/7011015/diff/1/chrome/browser/ui/panels/panel....
chrome/browser/ui/panels/panel.cc:14: originator_(NULL),
ordering in .h has originator_ before bounds_

[jianli, 2011-05-24 00:56:24]

I removed GetAsPanel since I am able to get the extension from the browser.

http://codereview.chromium.org/7011015/diff/1/chrome/app/resources/locale_set...
File chrome/app/resources/locale_settings.grd (right):

http://codereview.chromium.org/7011015/diff/1/chrome/app/resources/locale_set...
chrome/app/resources/locale_settings.grd:549: <!-- characters and lines (See
above). -->
On 2011/05/12 18:15:51, jennb wrote:
> See above where?  I'm not sure where I should be looking and what I should be
> looking for...

The comment "See above" is copied from other comment.

http://codereview.chromium.org/7011015/diff/1/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/7011015/diff/1/chrome/browser/ui/browser.cc#ne...
chrome/browser/ui/browser.cc:602: Panel* panel =
browser->window()->GetAsPanel();
On 2011/05/12 18:15:51, jennb wrote:
> There are other places where a Browser of type PANEL can be created that will
> end up without an originator set.

Removed since it is not needed now.

http://codereview.chromium.org/7011015/diff/1/chrome/browser/ui/panels/panel.cc
File chrome/browser/ui/panels/panel.cc (right):

http://codereview.chromium.org/7011015/diff/1/chrome/browser/ui/panels/panel....
chrome/browser/ui/panels/panel.cc:14: originator_(NULL),
On 2011/05/12 18:15:51, jennb wrote:
> ordering in .h has originator_ before bounds_

originator_ has been removed.

[jennb, 2011-05-24 05:29:14]

http://codereview.chromium.org/7011015/diff/8001/chrome/app/resources/locale_...
File chrome/app/resources/locale_settings.grd (right):

http://codereview.chromium.org/7011015/diff/8001/chrome/app/resources/locale_...
chrome/app/resources/locale_settings.grd:556: <message
name="IDS_ABOUTPANELBUBBLE_DIALOG_WIDTH_CHARS" use_name_for_id="true">
Having both Bubble and Dialog seems redundant.

http://codereview.chromium.org/7011015/diff/8001/chrome/browser/ui/panels/abo...
File chrome/browser/ui/panels/about_panel_bubble.cc (right):

http://codereview.chromium.org/7011015/diff/8001/chrome/browser/ui/panels/abo...
chrome/browser/ui/panels/about_panel_bubble.cc:61: // Find the extension. When
we create a panel from an extension, the extension
Maybe we should move all this logic out of the ctor and into an Init method. 
Ctors should never fail and looks like there is opportunity to fail in all this
logic.

http://codereview.chromium.org/7011015/diff/8001/chrome/browser/ui/panels/abo...
chrome/browser/ui/panels/about_panel_bubble.cc:121: int width = canvas.width() -
kBubblePadding * 3 - icon_size.width();
why *3 ?  I was expecting *2.

http://codereview.chromium.org/7011015/diff/8001/chrome/browser/ui/panels/abo...
File chrome/browser/ui/panels/about_panel_bubble.h (right):

http://codereview.chromium.org/7011015/diff/8001/chrome/browser/ui/panels/abo...
chrome/browser/ui/panels/about_panel_bubble.h:21: class AboutPanelBubble :
public Bubble ,
extra space before comma

http://codereview.chromium.org/7011015/diff/8001/chrome/browser/ui/panels/abo...
chrome/browser/ui/panels/about_panel_bubble.h:24: static AboutPanelBubble*
Show(views::Widget* parent,
could this ever fail to create an AboutPanelBubble?

http://codereview.chromium.org/7011015/diff/8001/chrome/browser/ui/panels/abo...
chrome/browser/ui/panels/about_panel_bubble.h:34: class AboutPanelBubbleView :
public views::View,
Does this really need to be a nested class?

http://codereview.chromium.org/7011015/diff/8001/chrome/browser/ui/panels/pan...
File chrome/browser/ui/panels/panel_browser_view_browsertest.cc (right):

http://codereview.chromium.org/7011015/diff/8001/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_browser_view_browsertest.cc:381: // Create a
testing extension.
Could you do like the extension tests and create an extension under
chrome/test/data/extensions/...?

http://codereview.chromium.org/7011015/diff/8001/chrome/browser/web_applicati...
File chrome/browser/web_applications/web_app.h (right):

http://codereview.chromium.org/7011015/diff/8001/chrome/browser/web_applicati...
chrome/browser/web_applications/web_app.h:33: // Extracts the extension id from
the app name.
Somtimes the app_name was not generated from an extension id. It could have been
generated by a URL.

[jianli, 2011-05-24 18:20:24]

http://codereview.chromium.org/7011015/diff/8001/chrome/app/resources/locale_...
File chrome/app/resources/locale_settings.grd (right):

http://codereview.chromium.org/7011015/diff/8001/chrome/app/resources/locale_...
chrome/app/resources/locale_settings.grd:556: <message
name="IDS_ABOUTPANELBUBBLE_DIALOG_WIDTH_CHARS" use_name_for_id="true">
On 2011/05/24 05:29:14, jennb wrote:
> Having both Bubble and Dialog seems redundant.

Removed DIALOG.

http://codereview.chromium.org/7011015/diff/8001/chrome/browser/ui/panels/abo...
File chrome/browser/ui/panels/about_panel_bubble.cc (right):

http://codereview.chromium.org/7011015/diff/8001/chrome/browser/ui/panels/abo...
chrome/browser/ui/panels/about_panel_bubble.cc:61: // Find the extension. When
we create a panel from an extension, the extension
On 2011/05/24 05:29:14, jennb wrote:
> Maybe we should move all this logic out of the ctor and into an Init method. 
> Ctors should never fail and looks like there is opportunity to fail in all
this
> logic.

Move this logic to AboutPanelBubble::Show and make it return NULL if the
extension cannot be found.

http://codereview.chromium.org/7011015/diff/8001/chrome/browser/ui/panels/abo...
chrome/browser/ui/panels/about_panel_bubble.cc:121: int width = canvas.width() -
kBubblePadding * 3 - icon_size.width();
On 2011/05/24 05:29:14, jennb wrote:
> why *3 ?  I was expecting *2.

Indeed one kBubblePadding is for the spacing between the icon and the remaing
contents. Used a new constant for this.

http://codereview.chromium.org/7011015/diff/8001/chrome/browser/ui/panels/abo...
File chrome/browser/ui/panels/about_panel_bubble.h (right):

http://codereview.chromium.org/7011015/diff/8001/chrome/browser/ui/panels/abo...
chrome/browser/ui/panels/about_panel_bubble.h:21: class AboutPanelBubble :
public Bubble ,
On 2011/05/24 05:29:14, jennb wrote:
> extra space before comma

Done.

http://codereview.chromium.org/7011015/diff/8001/chrome/browser/ui/panels/abo...
chrome/browser/ui/panels/about_panel_bubble.h:24: static AboutPanelBubble*
Show(views::Widget* parent,
On 2011/05/24 05:29:14, jennb wrote:
> could this ever fail to create an AboutPanelBubble?

It can fail now since I moved the extension finding logic to this creator. Added
a comment here.

http://codereview.chromium.org/7011015/diff/8001/chrome/browser/ui/panels/abo...
chrome/browser/ui/panels/about_panel_bubble.h:34: class AboutPanelBubbleView :
public views::View,
On 2011/05/24 05:29:14, jennb wrote:
> Does this really need to be a nested class?

Yes, we do not want it be accessible from other places. I could move it to .cc
file. However, I have to put it here for testing purpose. Any other better
suggestion?

http://codereview.chromium.org/7011015/diff/8001/chrome/browser/ui/panels/pan...
File chrome/browser/ui/panels/panel_browser_view_browsertest.cc (right):

http://codereview.chromium.org/7011015/diff/8001/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_browser_view_browsertest.cc:381: // Create a
testing extension.
On 2011/05/24 05:29:14, jennb wrote:
> Could you do like the extension tests and create an extension under
> chrome/test/data/extensions/...?

I do not think we need to host a real extension since we're only interested in
obtaining name and description of the extension. So a simple fake extension like
this should do the work, just like what background page test did.

http://codereview.chromium.org/7011015/diff/8001/chrome/browser/web_applicati...
File chrome/browser/web_applications/web_app.h (right):

http://codereview.chromium.org/7011015/diff/8001/chrome/browser/web_applicati...
chrome/browser/web_applications/web_app.h:33: // Extracts the extension id from
the app name.
On 2011/05/24 05:29:14, jennb wrote:
> Somtimes the app_name was not generated from an extension id. It could have
been
> generated by a URL.

It will not collide with the name generated from an URL since we add the prefix
"_crx_" and "_" cannot be part of the host (see comment in kCrxAppPrefix
definition in web_app.cc)

[jennb, 2011-05-24 22:50:29]

LGTM after fixing test failure. If the fix changes the code significantly,
please ping for another look first.

http://codereview.chromium.org/7011015/diff/8004/chrome/browser/ui/panels/abo...
File chrome/browser/ui/panels/about_panel_bubble.cc (right):

http://codereview.chromium.org/7011015/diff/8004/chrome/browser/ui/panels/abo...
chrome/browser/ui/panels/about_panel_bubble.cc:121: int width = canvas.width() -
kBubblePadding * 2 - icon_size.width() -
Is this (canvas.width() - start_x - kBubblePadding)?  Wondering if it would be
easier to understand if start_x is reused in the formula.  

Maybe another name for start_x that better explains what it is?  Is it the
left_margin? 

Would width be clearer as content_width?

[jianli, 2011-05-24 23:56:57]

The test failure is caused by an assert that complains about the extension
install preferences not set. Added a call to
"MessageLoop::current()->RunAllPending()" in the test to fix this problem.

http://codereview.chromium.org/7011015/diff/8004/chrome/browser/ui/panels/abo...
File chrome/browser/ui/panels/about_panel_bubble.cc (right):

http://codereview.chromium.org/7011015/diff/8004/chrome/browser/ui/panels/abo...
chrome/browser/ui/panels/about_panel_bubble.cc:121: int width = canvas.width() -
kBubblePadding * 2 - icon_size.width() -
On 2011/05/24 22:50:29, jennb wrote:
> Is this (canvas.width() - start_x - kBubblePadding)?  Wondering if it would be
> easier to understand if start_x is reused in the formula.  
> 
> Maybe another name for start_x that better explains what it is?  Is it the
> left_margin? 
> 
> Would width be clearer as content_width? 

Done.

[oshima, 2011-06-01 02:07:29]

Hi jianli, jennb

We're about to remove STYLE_MULTILINE in textfield in
(http://codereview.chromium.org/7027014/) but noticed that
about_panel_bubble now uses it.

I'm guessing that you took the implementation from old about
dialog, which is now gone, and we're wondering if you
can eliminate the use of STYLE_MULTILINE.

Thanks,

[jianli, 2011-06-01 02:23:07]

I am going to remove the about panel bubble per the latest panel UI decision
and the patch will be sent out tomorrow morning. If you can wait till that
time, the use of STYLE_MULTILINE will be removed with the incoming patch.
Otherwise, you could also change it to using any other style.

On Tue, May 31, 2011 at 7:07 PM, <oshima@chromium.org> wrote:

> Hi jianli, jennb
>
> We're about to remove STYLE_MULTILINE in textfield in
> (http://codereview.chromium.org/7027014/) but noticed that
> about_panel_bubble now uses it.
>
> I'm guessing that you took the implementation from old about
> dialog, which is now gone, and we're wondering if you
> can eliminate the use of STYLE_MULTILINE.
>
> Thanks,
>
>
>
> http://codereview.chromium.org/7011015/
>

[oshima, 2011-06-01 02:26:22]

On Tue, May 31, 2011 at 7:22 PM, Jian Li <jianli@chromium.org> wrote:

> I am going to remove the about panel bubble per the latest panel UI
> decision and the patch will be sent out tomorrow morning. If you can wait
> till that time, the use of STYLE_MULTILINE will be removed with the incoming
> patch. Otherwise, you could also change it to using any other style.
>
>
That's great. I'll let saintlou decide which way to go.

Thanks!
- oshima


> On Tue, May 31, 2011 at 7:07 PM, <oshima@chromium.org> wrote:
>
>> Hi jianli, jennb
>>
>> We're about to remove STYLE_MULTILINE in textfield in
>> (http://codereview.chromium.org/7027014/) but noticed that
>> about_panel_bubble now uses it.
>>
>> I'm guessing that you took the implementation from old about
>> dialog, which is now gone, and we're wondering if you
>> can eliminate the use of STYLE_MULTILINE.
>>
>> Thanks,
>>
>>
>>
>> http://codereview.chromium.org/7011015/
>>
>
>

[Emmanuel Saint-loubert-Bié, 2011-06-01 02:50:17]

Yes I can wait, thanks for the update :-)

-- Emmanuel

On Tue, May 31, 2011 at 7:25 PM, oshima <oshima@chromium.org> wrote:

>
>
> On Tue, May 31, 2011 at 7:22 PM, Jian Li <jianli@chromium.org> wrote:
>
>> I am going to remove the about panel bubble per the latest panel UI
>> decision and the patch will be sent out tomorrow morning. If you can wait
>> till that time, the use of STYLE_MULTILINE will be removed with the incoming
>> patch. Otherwise, you could also change it to using any other style.
>>
>>
> That's great. I'll let saintlou decide which way to go.
>
> Thanks!
> - oshima
>
>
>> On Tue, May 31, 2011 at 7:07 PM, <oshima@chromium.org> wrote:
>>
>>> Hi jianli, jennb
>>>
>>> We're about to remove STYLE_MULTILINE in textfield in
>>> (http://codereview.chromium.org/7027014/) but noticed that
>>> about_panel_bubble now uses it.
>>>
>>> I'm guessing that you took the implementation from old about
>>> dialog, which is now gone, and we're wondering if you
>>> can eliminate the use of STYLE_MULTILINE.
>>>
>>> Thanks,
>>>
>>>
>>>
>>> http://codereview.chromium.org/7011015/
>>>
>>
>>
>