Move page actions over to ExtensionAction2

Move page actions over to ExtensionAction2 and get rid of
extension_action.*.

Final bit of refactor will be to rename ExtensionAction2 to
ExtensionAction will be the next CL.

BUG=24472, 25844
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=30133

[Aaron Boodman, 2009-10-24 07:21:40]

estade: can you please review the _gtk.* bits again?
mpcompete: everything else

Recommended order:
- extension_action2*
- extension*
- extension_page_actions_module*
- location_bar*
- everything else

http://codereview.chromium.org/332021/diff/1/14
File chrome/browser/views/location_bar_view.cc (left):

http://codereview.chromium.org/332021/diff/1/14#oldcode1238
Line 1238: ExtensionActionState::PaintBadge(canvas, gfx::Rect(width(),
height()),
This was weird. We were painting the badge both inside the image view and inside
its parent. I think this was a bug.

http://codereview.chromium.org/332021/diff/1/17
File chrome/common/extensions/extension.cc (left):

http://codereview.chromium.org/332021/diff/1/17#oldcode406
Line 406: result->SetDefaultIcon(path);
The logic for falling back to the default icon is in the UI now.

http://codereview.chromium.org/332021/diff/1/21
File chrome/common/extensions/extension_action2_unittest.cc (left):

http://codereview.chromium.org/332021/diff/1/21#oldcode108
Line 108: TEST(ExtensionAction2Test, IconOddCases) {
Moved the logic for handling missing bitmap icons and falling back to iconIndex,
and then to the default icon into the UI, so this test is no longer necessary.

http://codereview.chromium.org/332021/diff/1/22
File chrome/common/extensions/extension_unittest.cc (left):

http://codereview.chromium.org/332021/diff/1/22#oldcode341
Line 341: // Now try the same, but as a browser action.
page and browser actions parse into exactly the same structure now, so no need
to test them separately.

http://codereview.chromium.org/332021/diff/1/23
File chrome/test/data/extensions/samples/test_page_action/background.html
(left):

http://codereview.chromium.org/332021/diff/1/23#oldcode21
Line 21: chrome.pageAction.setBadgeTextColor({
badges no longer supported for page actions.

[Evan Stade, 2009-10-26 17:39:50]

mostly nits

http://codereview.chromium.org/332021/diff/2001/2009
File chrome/browser/gtk/location_bar_view_gtk.cc (right):

http://codereview.chromium.org/332021/diff/2001/2009#newcode410
Line 410: 
nit: extra line?

http://codereview.chromium.org/332021/diff/2001/2009#newcode765
Line 765: if (icon.getAddr32(0, 0) != last_icon_skbitmap_.getAddr32(0, 0)) {
use pixelRef() instead of getAddr32(0,0) maybe

http://codereview.chromium.org/332021/diff/2001/2009#newcode808
Line 808: int total_icons = page_action_->icon_paths()->size();
can we avoid the casts if you make this variable a size_t?

http://codereview.chromium.org/332021/diff/2001/2010
File chrome/browser/gtk/location_bar_view_gtk.h (right):

http://codereview.chromium.org/332021/diff/2001/2010#newcode138
Line 138: // A cache of all the different icon paths associated with this page
nit: can this comment fit on one line?

http://codereview.chromium.org/332021/diff/2001/2010#newcode140
Line 140: typedef std::map<std::string, GdkPixbuf*> PixBufMap;
nit: PixbufMap

[Matt Perry, 2009-10-26 18:16:48]

mostly LG

http://codereview.chromium.org/332021/diff/2001/2014
File chrome/browser/views/location_bar_view.cc (right):

http://codereview.chromium.org/332021/diff/2001/2014#newcode1321
Line 1321: // - The developer can set it dyanmically by index. It will be in
nit: dynamically

http://codereview.chromium.org/332021/diff/2001/2020
File chrome/common/extensions/extension_action2.h (right):

http://codereview.chromium.org/332021/diff/2001/2020#newcode131
Line 131: struct ValueTraits {
You shouldn't need this struct. In C++, t = T() 0-initializes intrinsic types.
So bools will be false, ints 0, etc.

http://codereview.chromium.org/332021/diff/2001/2026
File chrome/test/data/extensions/samples/test_page_action/manifest.json (right):

http://codereview.chromium.org/332021/diff/2001/2026#newcode9
Line 9: "icons": ["icon1.png", "icon2.png"]
this style is deprecated, right? we probably shouldn't include samples using the
deprecated formats.

[Aaron Boodman, 2009-10-26 23:05:00]

Ready for another look.

http://codereview.chromium.org/332021/diff/2001/2009
File chrome/browser/gtk/location_bar_view_gtk.cc (right):

http://codereview.chromium.org/332021/diff/2001/2009#newcode410
Line 410: 
On 2009/10/26 17:39:50, Evan Stade wrote:
> nit: extra line?

Done.

http://codereview.chromium.org/332021/diff/2001/2009#newcode765
Line 765: if (icon.getAddr32(0, 0) != last_icon_skbitmap_.getAddr32(0, 0)) {
On 2009/10/26 17:39:50, Evan Stade wrote:
> use pixelRef() instead of getAddr32(0,0) maybe

I changed it to this. I am not sure if it is correct. I guess we'll see how it
works.

http://codereview.chromium.org/332021/diff/2001/2009#newcode808
Line 808: int total_icons = page_action_->icon_paths()->size();
On 2009/10/26 17:39:50, Evan Stade wrote:
> can we avoid the casts if you make this variable a size_t?

Whoops, yeah. Done.

http://codereview.chromium.org/332021/diff/2001/2010
File chrome/browser/gtk/location_bar_view_gtk.h (right):

http://codereview.chromium.org/332021/diff/2001/2010#newcode138
Line 138: // A cache of all the different icon paths associated with this page
On 2009/10/26 17:39:50, Evan Stade wrote:
> nit: can this comment fit on one line?

Done.

http://codereview.chromium.org/332021/diff/2001/2010#newcode140
Line 140: typedef std::map<std::string, GdkPixbuf*> PixBufMap;
On 2009/10/26 17:39:50, Evan Stade wrote:
> nit: PixbufMap

Done.

http://codereview.chromium.org/332021/diff/2001/2014
File chrome/browser/views/location_bar_view.cc (right):

http://codereview.chromium.org/332021/diff/2001/2014#newcode1321
Line 1321: // - The developer can set it dyanmically by index. It will be in
On 2009/10/26 18:16:48, Matt Perry wrote:
> nit: dynamically

Done.

http://codereview.chromium.org/332021/diff/2001/2020
File chrome/common/extensions/extension_action2.h (right):

http://codereview.chromium.org/332021/diff/2001/2020#newcode131
Line 131: struct ValueTraits {
On 2009/10/26 18:16:48, Matt Perry wrote:
> You shouldn't need this struct. In C++, t = T() 0-initializes intrinsic types.
> So bools will be false, ints 0, etc.

Thanks, I did not know that. I want int to return -1 though, so I still need
this. I removed the boolean and SkColor cases though.

http://codereview.chromium.org/332021/diff/2001/2026
File chrome/test/data/extensions/samples/test_page_action/manifest.json (right):

http://codereview.chromium.org/332021/diff/2001/2026#newcode9
Line 9: "icons": ["icon1.png", "icon2.png"]
On 2009/10/26 18:16:48, Matt Perry wrote:
> this style is deprecated, right? we probably shouldn't include samples using
the
> deprecated formats.

Done.

[Evan Stade, 2009-10-26 23:07:22]

lgtm

http://codereview.chromium.org/332021/diff/8002/64
File chrome/browser/gtk/location_bar_view_gtk.h (right):

http://codereview.chromium.org/332021/diff/8002/64#newcode11
Line 11: #include <map>
nit: map before string

http://codereview.chromium.org/332021/diff/8002/64#newcode32
Line 32: class ExtensionAction2;
nit: alphabets

[Matt Perry, 2009-10-26 23:17:17]

lgtm

http://codereview.chromium.org/332021/diff/8002/74
File chrome/common/extensions/extension_action2.h (right):

http://codereview.chromium.org/332021/diff/8002/74#newcode182
Line 182: struct ExtensionAction2::ValueTraits<int> {
nit: can you move this inside the ExtensionAction2 class, below the ValueTraits
definition?

[Aaron Boodman, 2009-10-26 23:21:16]

On 2009/10/26 23:17:17, Matt Perry wrote:
> lgtm
> 
> http://codereview.chromium.org/332021/diff/8002/74
> File chrome/common/extensions/extension_action2.h (right):
> 
> http://codereview.chromium.org/332021/diff/8002/74#newcode182
> Line 182: struct ExtensionAction2::ValueTraits<int> {
> nit: can you move this inside the ExtensionAction2 class, below the
ValueTraits
> definition?

No, apparently that is not legal for gcc. (complains of template specialization
nested in non-namespace scope).