Add tests for panel app icon functionaility.

Add tests for panel app icon functionaility.

BUG=150025
TEST=These are tests.

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

[jianli, 2012-09-25 21:43:39]

Jenn, please review all panel related changes?

Yoshiki, please review my change to re-enable the failed test now passes?

[dcheng, 2012-09-25 21:50:27]

http://codereview.chromium.org/10987037/diff/2001/chrome/browser/ui/panels/ba...
File chrome/browser/ui/panels/base_panel_browser_test.cc (right):

http://codereview.chromium.org/10987037/diff/2001/chrome/browser/ui/panels/ba...
chrome/browser/ui/panels/base_panel_browser_test.cc:429:
BasePanelBrowserTest::LoadExtensionFromManifest(
Is it possible to share this code with ExtensionBrowserTest? I notice we're
manually calling back OnExtensionInstalled and that makes me a little sad.

[jennb, 2012-09-25 22:43:09]

Agree with dcheng's concerns.

http://codereview.chromium.org/10987037/diff/2001/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_view_browsertest.cc (right):

http://codereview.chromium.org/10987037/diff/2001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_view_browsertest.cc:142:
IN_PROC_BROWSER_TEST_F(PanelViewTest, PanelAppIcon) {
If I'm reading this right, this test verifies that there is an app icon, it can
be converted to a bitmap and it's size is 32x32. It doesn't check that the icon
is the icon from the extension. Does this sufficiently verify the taskbar icon
change?

[jianli, 2012-09-25 22:50:04]

http://codereview.chromium.org/10987037/diff/2001/chrome/browser/ui/panels/ba...
File chrome/browser/ui/panels/base_panel_browser_test.cc (right):

http://codereview.chromium.org/10987037/diff/2001/chrome/browser/ui/panels/ba...
chrome/browser/ui/panels/base_panel_browser_test.cc:429:
BasePanelBrowserTest::LoadExtensionFromManifest(
On 2012/09/25 21:50:27, dcheng wrote:
> Is it possible to share this code with ExtensionBrowserTest? I notice we're
> manually calling back OnExtensionInstalled and that makes me a little sad.

Will change BasePanelBrowserTest to inherit from ExtensionBrowserTest.

http://codereview.chromium.org/10987037/diff/2001/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_view_browsertest.cc (right):

http://codereview.chromium.org/10987037/diff/2001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_view_browsertest.cc:142:
IN_PROC_BROWSER_TEST_F(PanelViewTest, PanelAppIcon) {
On 2012/09/25 22:43:09, jennb wrote:
> If I'm reading this right, this test verifies that there is an app icon, it
can
> be converted to a bitmap and it's size is 32x32. It doesn't check that the
icon
> is the icon from the extension. Does this sufficiently verify the taskbar icon
> change?

Yes, it is sufficient to verify that we have the right resolution. Converting to
a SkBitmap is a simple way to get its width and height for verification.

We can't verify if the icon image data are exactly same since we get back from
the system in different format.

[jennb, 2012-09-25 22:50:35]

http://codereview.chromium.org/10987037/diff/6001/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_view_browsertest.cc (right):

http://codereview.chromium.org/10987037/diff/6001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_view_browsertest.cc:153: // Validate the app icon
(for Windows 7 and later).
You could make a NativePanelTesting API for getting/verifying the window icon
and put this Win code in there. Then do the same for GTK. The original change
affected GTK and Win so the test should cover both. There is a
gtk_window_get_icon.

[Dmitry Titov, 2012-09-25 23:34:21]

Drive-by:

http://codereview.chromium.org/10987037/diff/6001/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_view_browsertest.cc (right):

http://codereview.chromium.org/10987037/diff/6001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_view_browsertest.cc:141: #if defined(OS_WIN) &&
!defined(USE_AURA)
This seems to be a "Views" test file, but here it tests WIN functionality. Can
we create a separate test (panel_win_browsertest.cc) ? Or some better way
perhaps?
I'm moving this file into browser/ui/views location and technically speaking, I
should not move a file with "#if defined(OS_WIN)" there...

[jianli, 2012-09-25 23:41:26]

On Tue, Sep 25, 2012 at 4:34 PM, <dimich@chromium.org> wrote:

> Drive-by:
>
>
>
> http://codereview.chromium.**org/10987037/diff/6001/chrome/**
>
browser/ui/panels/panel_view_**browsertest.cc<http://codereview.chromium.org/10987037/diff/6001/chrome/browser/ui/panels/panel_view_browsertest.cc>
> File chrome/browser/ui/panels/**panel_view_browsertest.cc (right):
>
> http://codereview.chromium.**org/10987037/diff/6001/chrome/**
>
browser/ui/panels/panel_view_**browsertest.cc#newcode141<http://codereview.chromium.org/10987037/diff/6001/chrome/browser/ui/panels/panel_view_browsertest.cc#newcode141>
> chrome/browser/ui/panels/**panel_view_browsertest.cc:141: #if
> defined(OS_WIN) && !defined(USE_AURA)
> This seems to be a "Views" test file, but here it tests WIN
> functionality. Can we create a separate test (panel_win_browsertest.cc)
> ? Or some better way perhaps?
> I'm moving this file into browser/ui/views location and technically
> speaking, I should not move a file with "#if defined(OS_WIN)" there...
>

The other way is that we can convert those tests to platform-independent
tests.

>
>
http://codereview.chromium.**org/10987037/<http://codereview.chromium.org/109...
>

[jianli, 2012-10-01 23:58:23]

As to the helper function to load extension from manifest, I still keep it in
BasePanelBrowserTest since trying to reuse the one from ExtensionBrowserTest via
inheritance adds undesired complexity to our panel test base class. Since the
code to load extension is quite simple, I think it makes more manageable to keep
our implementation there.

http://codereview.chromium.org/10987037/diff/6001/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_view_browsertest.cc (right):

http://codereview.chromium.org/10987037/diff/6001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_view_browsertest.cc:141: #if defined(OS_WIN) &&
!defined(USE_AURA)
On 2012/09/25 23:34:21, Dmitry Titov wrote:
> This seems to be a "Views" test file, but here it tests WIN functionality. Can
> we create a separate test (panel_win_browsertest.cc) ? Or some better way
> perhaps?
> I'm moving this file into browser/ui/views location and technically speaking,
I
> should not move a file with "#if defined(OS_WIN)" there...

Changed to platform independent test.

http://codereview.chromium.org/10987037/diff/6001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_view_browsertest.cc:153: // Validate the app icon
(for Windows 7 and later).
On 2012/09/25 22:50:35, jennb wrote:
> You could make a NativePanelTesting API for getting/verifying the window icon
> and put this Win code in there. Then do the same for GTK. The original change
> affected GTK and Win so the test should cover both. There is a
> gtk_window_get_icon.

Done.

[jennb, 2012-10-02 17:51:07]

http://codereview.chromium.org/10987037/diff/20004/chrome/browser/ui/gtk/pane...
File chrome/browser/ui/gtk/panels/panel_gtk.cc (right):

http://codereview.chromium.org/10987037/diff/20004/chrome/browser/ui/gtk/pane...
chrome/browser/ui/gtk/panels/panel_gtk.cc:1118: gdk_pixbuf_get_width(icon) == 32
&&
Should this 32 be a defined constant somewhere, maybe panel_constants.h?

http://codereview.chromium.org/10987037/diff/20004/chrome/browser/ui/panels/b...
File chrome/browser/ui/panels/base_panel_browser_test.cc (right):

http://codereview.chromium.org/10987037/diff/20004/chrome/browser/ui/panels/b...
chrome/browser/ui/panels/base_panel_browser_test.cc:439:
BasePanelBrowserTest::LoadExtensionFromManifest(
This logic makes assumptions about extension implementation details, like the
manifest file being json, that may change and violates abstractions. Even the
ExtensionBrowserTest extension loading code does not get into this level of
detail.  Can you do something more like
ExtensionBrowserTest::LoadExtensionWithFlags? If you must copy code, at least
make it look like the other usages.

http://codereview.chromium.org/10987037/diff/20004/chrome/browser/ui/panels/p...
File chrome/browser/ui/panels/panel_browsertest.cc (right):

http://codereview.chromium.org/10987037/diff/20004/chrome/browser/ui/panels/p...
chrome/browser/ui/panels/panel_browsertest.cc:1693:
IN_PROC_BROWSER_TEST_F(PanelBrowserTest, PanelAppIcon) {
Neither of these two tests rely on BasePanelBrowserTest. You could create a new
test type:

PanelExtensionBrowserTest : public ExtensionBrowserTest

and put these 2 tests there. Use PanelManager::Create() to create the panel for
the tests instead of CreatePanelWithParams. That allows these 2 tests to run as
browser_tests instead of interactive_ui_tests, which should be reserved for
tests that cannot be run in parallel.

http://codereview.chromium.org/10987037/diff/20004/chrome/browser/ui/views/pa...
File chrome/browser/ui/views/panels/panel_view.cc (right):

http://codereview.chromium.org/10987037/diff/20004/chrome/browser/ui/views/pa...
chrome/browser/ui/views/panels/panel_view.cc:142: #if defined(OS_WIN) &&
!defined(USE_AURA)
Should this be USE_ASH?  Aura is the hw accelerated UI framework.

http://codereview.chromium.org/10987037/diff/20004/chrome/common/chrome_notif...
File chrome/common/chrome_notification_types.h (right):

http://codereview.chromium.org/10987037/diff/20004/chrome/common/chrome_notif...
chrome/common/chrome_notification_types.h:1230: NOTIFICATION_PANEL_ICON_KNOWN,
"known" is vague. How about NOTIFICATION_PANEL_APP_ICON_LOADED ?

[jianli, 2012-10-02 21:37:26]

http://codereview.chromium.org/10987037/diff/20004/chrome/browser/ui/gtk/pane...
File chrome/browser/ui/gtk/panels/panel_gtk.cc (right):

http://codereview.chromium.org/10987037/diff/20004/chrome/browser/ui/gtk/pane...
chrome/browser/ui/gtk/panels/panel_gtk.cc:1118: gdk_pixbuf_get_width(icon) == 32
&&
On 2012/10/02 17:51:08, jennb wrote:
> Should this 32 be a defined constant somewhere, maybe panel_constants.h?

Done.

http://codereview.chromium.org/10987037/diff/20004/chrome/browser/ui/panels/b...
File chrome/browser/ui/panels/base_panel_browser_test.cc (right):

http://codereview.chromium.org/10987037/diff/20004/chrome/browser/ui/panels/b...
chrome/browser/ui/panels/base_panel_browser_test.cc:439:
BasePanelBrowserTest::LoadExtensionFromManifest(
On 2012/10/02 17:51:08, jennb wrote:
> This logic makes assumptions about extension implementation details, like the
> manifest file being json, that may change and violates abstractions. Even the
> ExtensionBrowserTest extension loading code does not get into this level of
> detail.  Can you do something more like
> ExtensionBrowserTest::LoadExtensionWithFlags? If you must copy code, at least
> make it look like the other usages. 

Moved it out to the new file and changed it to use LoadExtension.

http://codereview.chromium.org/10987037/diff/20004/chrome/browser/ui/panels/p...
File chrome/browser/ui/panels/panel_browsertest.cc (right):

http://codereview.chromium.org/10987037/diff/20004/chrome/browser/ui/panels/p...
chrome/browser/ui/panels/panel_browsertest.cc:1693:
IN_PROC_BROWSER_TEST_F(PanelBrowserTest, PanelAppIcon) {
On 2012/10/02 17:51:08, jennb wrote:
> Neither of these two tests rely on BasePanelBrowserTest. You could create a
new
> test type:
> 
> PanelExtensionBrowserTest : public ExtensionBrowserTest
> 
> and put these 2 tests there. Use PanelManager::Create() to create the panel
for
> the tests instead of CreatePanelWithParams. That allows these 2 tests to run
as
> browser_tests instead of interactive_ui_tests, which should be reserved for
> tests that cannot be run in parallel.

Done.

http://codereview.chromium.org/10987037/diff/20004/chrome/browser/ui/views/pa...
File chrome/browser/ui/views/panels/panel_view.cc (right):

http://codereview.chromium.org/10987037/diff/20004/chrome/browser/ui/views/pa...
chrome/browser/ui/views/panels/panel_view.cc:142: #if defined(OS_WIN) &&
!defined(USE_AURA)
On 2012/10/02 17:51:08, jennb wrote:
> Should this be USE_ASH?  Aura is the hw accelerated UI framework.

This should be USE_AURA since there could be a build with USE_AURA defined but
not with USE_ASH (like win_aura bot). We only want the verification code work
for standalone Windows build.

http://codereview.chromium.org/10987037/diff/20004/chrome/common/chrome_notif...
File chrome/common/chrome_notification_types.h (right):

http://codereview.chromium.org/10987037/diff/20004/chrome/common/chrome_notif...
chrome/common/chrome_notification_types.h:1230: NOTIFICATION_PANEL_ICON_KNOWN,
On 2012/10/02 17:51:08, jennb wrote:
> "known" is vague. How about NOTIFICATION_PANEL_APP_ICON_LOADED ?

Done.

[jennb, 2012-10-03 00:51:58]

LGTM

Thanks for breaking out the new tests.

[commit-bot: I haz the power, 2012-10-08 20:33:50]

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

[commit-bot: I haz the power, 2012-10-08 20:33:56]

Presubmit check for 10987037-33002 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome

Presubmit checks took 2.4s to calculate.

[jianli, 2012-10-08 23:37:30]

Add thakis for owner approval to the change to
chrome/common/chrome_notification_types.h.

[jianli, 2012-10-09 18:26:51]

thestig, can you approve as owner for my change to
chrome/common/chrome_notification_types.h? Thanks.

[Lei Zhang, 2012-10-09 18:33:39]

chrome/ OWNERS lgtm with nit below:

http://codereview.chromium.org/10987037/diff/33002/chrome/browser/ui/panels/p...
File chrome/browser/ui/panels/panel_browsertest.cc (right):

http://codereview.chromium.org/10987037/diff/33002/chrome/browser/ui/panels/p...
chrome/browser/ui/panels/panel_browsertest.cc:1692: 
This file shouldn't be in the CL?

[jianli, 2012-10-09 18:35:33]

http://codereview.chromium.org/10987037/diff/33002/chrome/browser/ui/panels/p...
File chrome/browser/ui/panels/panel_browsertest.cc (right):

http://codereview.chromium.org/10987037/diff/33002/chrome/browser/ui/panels/p...
chrome/browser/ui/panels/panel_browsertest.cc:1692: 
On 2012/10/09 18:33:39, Lei Zhang wrote:
> This file shouldn't be in the CL?

Yes. But it would be nice to clean up the unneeded empty line.

[Lei Zhang, 2012-10-09 18:42:36]

http://codereview.chromium.org/10987037/diff/33002/chrome/browser/ui/panels/p...
File chrome/browser/ui/panels/panel_browsertest.cc (right):

http://codereview.chromium.org/10987037/diff/33002/chrome/browser/ui/panels/p...
chrome/browser/ui/panels/panel_browsertest.cc:1692: 
On 2012/10/09 18:35:33, jianli wrote:
> On 2012/10/09 18:33:39, Lei Zhang wrote:
> > This file shouldn't be in the CL?
> 
> Yes. But it would be nice to clean up the unneeded empty line.

Err? The diff shows +1. You are adding an extra empty line.

[jianli, 2012-10-09 18:44:02]

http://codereview.chromium.org/10987037/diff/33002/chrome/browser/ui/panels/p...
File chrome/browser/ui/panels/panel_browsertest.cc (right):

http://codereview.chromium.org/10987037/diff/33002/chrome/browser/ui/panels/p...
chrome/browser/ui/panels/panel_browsertest.cc:1692: 
On 2012/10/09 18:42:36, Lei Zhang wrote:
> On 2012/10/09 18:35:33, jianli wrote:
> > On 2012/10/09 18:33:39, Lei Zhang wrote:
> > > This file shouldn't be in the CL?
> > 
> > Yes. But it would be nice to clean up the unneeded empty line.
> 
> Err? The diff shows +1. You are adding an extra empty line.

Sorry, my bad. I had a wrong perception. Let me remove this file and submit a
new patch to land. Thanks for catching it.

[commit-bot: I haz the power, 2012-10-09 18:47:56]

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

[commit-bot: I haz the power, 2012-10-09 20:57:22]

Change committed as 160920