Removal of chrome.experimental.popup set of APIs

Removing the experimental Chrome extension popup API. This API will not be added to the set of available extension APIs. 

I also removed some of the plumbing associated with the popup api: 
- Drop-shadow support removed from BrowserBubble. This had only been supported on Windows. 
- Removed the ExtensionPopup::PopupChrome type. Only popups from the popup API supported rectangle chrome. 
- Removed the activate-on-show parameter from ExtensionPopup. This was only used for the popup API. All popups activate on show, by default. 
- Removed the AddRef/Release magic from ExtensionPopup. The API required these semantics because of the complex, asynchronous lifetime management required by the popup API. See ExtensionPopup::Observer::ExtensionPopupClosed. 
- Removed unneeded methods from ExtensionPopup::Observer, and ExtensionFunctionDispatcher::Delegate.

BUG=None 
TEST=None 


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

[Jeff Timanus, 2011-02-04 03:38:45]

PTAL.

[Jeff Timanus, 2011-02-08 00:05:12]

On 2011/02/04 03:38:45, Jeff Timanus wrote:
> PTAL.

Aaron, will you have a moment to look at this change?  Not much to see here,
other than the removal of a lot of now-dead code.

Jeff

[Jeff Timanus, 2011-02-08 15:48:31]

On 2011/02/08 00:05:12, Jeff Timanus wrote:
> On 2011/02/04 03:38:45, Jeff Timanus wrote:
> > PTAL.
> 
> Aaron, will you have a moment to look at this change?  Not much to see here,
> other than the removal of a lot of now-dead code.
> 
> Jeff

Please have a look when one of you has a moment.

Jeff

[Aaron Boodman, 2011-02-08 20:37:20]

Reviewing, but trybots seem very unhappy.

[Aaron Boodman, 2011-02-08 20:40:53]

LGTM, but I'm not the best person to review the deleted bits in the bubble code.

[Aaron Boodman, 2011-02-08 20:41:55]

Raf, do you mind taking a look at the modifications to the bubble code?

Also, Jeff, Nit about the description for this CL. You aren't deprecating the
API, you're removing it.

[Aaron Boodman, 2011-02-08 20:43:45]

One more thing. Don't forget to remove the entries from extension_api.json.

[Jeff Timanus, 2011-02-08 20:44:26]

On 2011/02/08 20:37:20, Aaron Boodman wrote:
> Reviewing, but trybots seem very unhappy.

Thanks.  Double checking the try-bot issue.  Spoke earlier with maruel, and the
failures seem to be related to my removal of the files.  Somehow the patch is
failing.  Will resolve.

[rafaelw, 2011-02-09 22:08:28]

http://codereview.chromium.org/6334101/diff/5002/chrome/browser/ui/views/brow...
File chrome/browser/ui/views/browser_bubble.h (left):

http://codereview.chromium.org/6334101/diff/5002/chrome/browser/ui/views/brow...
chrome/browser/ui/views/browser_bubble.h:123: bool drop_shadow_enabled_;
You'll need to remove the DCHECK(!drop_shadow_enabled_) from
browser_bubble_gtk::InitPopup().

http://codereview.chromium.org/6334101/diff/5002/chrome/browser/ui/views/exte...
File chrome/browser/ui/views/extensions/extension_popup.cc (left):

http://codereview.chromium.org/6334101/diff/5002/chrome/browser/ui/views/exte...
chrome/browser/ui/views/extensions/extension_popup.cc:17: #include
"chrome/browser/ui/browser_list.h"
Is this still needed?

http://codereview.chromium.org/6334101/diff/5002/chrome/browser/ui/views/exte...
chrome/browser/ui/views/extensions/extension_popup.cc:25: #include
"third_party/skia/include/core/SkColor.h"
Is this still needed?

http://codereview.chromium.org/6334101/diff/5002/chrome/browser/ui/views/exte...
chrome/browser/ui/views/extensions/extension_popup.cc:84: max_size_(kMaxWidth,
kMaxHeight),
Can we revert the effect of http://codereview.chromium.org/5254007

http://codereview.chromium.org/6334101/diff/5002/chrome/browser/ui/views/exte...
chrome/browser/ui/views/extensions/extension_popup.cc:158: void
ExtensionPopup::SetArrowPosition(
I don't think this is needed anymore.

http://codereview.chromium.org/1774012/

http://codereview.chromium.org/6334101/diff/5002/chrome/browser/ui/views/exte...
chrome/browser/ui/views/extensions/extension_popup.cc:179: if
(frame_->GetWindow())
This line appears to have been added in http://codereview.chromium.org/434046. I
think we can assume that there's a window now, right?

http://codereview.chromium.org/6334101/diff/5002/chrome/browser/ui/views/exte...
chrome/browser/ui/views/extensions/extension_popup.cc:396: Widget* frame_widget
= Widget::GetWidgetFromNativeWindow(frame_window);
Looks like you can undo all of this: http://codereview.chromium.org/434046

Including removing the frame_window and profile arguments above.

http://codereview.chromium.org/6334101/diff/5002/chrome/browser/ui/views/exte...
File chrome/browser/ui/views/extensions/extension_popup.cc (right):

http://codereview.chromium.org/6334101/diff/5002/chrome/browser/ui/views/exte...
chrome/browser/ui/views/extensions/extension_popup.cc:132:
border_widget_->MoveAbove(popup_);
Are you sure this is still needed? 

http://codereview.chromium.org/454019/diff/8013/chrome/browser/views/extensio...

http://codereview.chromium.org/6334101/diff/5002/chrome/browser/ui/views/exte...
chrome/browser/ui/views/extensions/extension_popup.cc:172: gfx::Rect rect =
GetOuterBounds();
Please consider putting this back the way it was:

http://codereview.chromium.org/577015/diff/14036/chrome/browser/views/extensi...

Including inlining GetOuterBounds. It was simpler that way.

[Jeff Timanus, 2011-02-10 00:17:49]

Thanks for your review, Rafael.

I made the changes you suggested.

I will try to determine why the bots are failing to apply my patch - I want to
make sure I did not break the other platforms.

Will update once they are green.

http://codereview.chromium.org/6334101/diff/5002/chrome/browser/ui/views/brow...
File chrome/browser/ui/views/browser_bubble.h (left):

http://codereview.chromium.org/6334101/diff/5002/chrome/browser/ui/views/brow...
chrome/browser/ui/views/browser_bubble.h:123: bool drop_shadow_enabled_;
On 2011/02/09 22:08:28, rafaelw wrote:
> You'll need to remove the DCHECK(!drop_shadow_enabled_) from
> browser_bubble_gtk::InitPopup().

Done.

http://codereview.chromium.org/6334101/diff/5002/chrome/browser/ui/views/exte...
File chrome/browser/ui/views/extensions/extension_popup.cc (left):

http://codereview.chromium.org/6334101/diff/5002/chrome/browser/ui/views/exte...
chrome/browser/ui/views/extensions/extension_popup.cc:17: #include
"chrome/browser/ui/browser_list.h"
On 2011/02/09 22:08:28, rafaelw wrote:
> Is this still needed?

Done.

http://codereview.chromium.org/6334101/diff/5002/chrome/browser/ui/views/exte...
chrome/browser/ui/views/extensions/extension_popup.cc:25: #include
"third_party/skia/include/core/SkColor.h"
On 2011/02/09 22:08:28, rafaelw wrote:
> Is this still needed?

Done.

http://codereview.chromium.org/6334101/diff/5002/chrome/browser/ui/views/exte...
chrome/browser/ui/views/extensions/extension_popup.cc:84: max_size_(kMaxWidth,
kMaxHeight),
On 2011/02/09 22:08:28, rafaelw wrote:
> Can we revert the effect of http://codereview.chromium.org/5254007

Done.

http://codereview.chromium.org/6334101/diff/5002/chrome/browser/ui/views/exte...
chrome/browser/ui/views/extensions/extension_popup.cc:158: void
ExtensionPopup::SetArrowPosition(
On 2011/02/09 22:08:28, rafaelw wrote:
> I don't think this is needed anymore.
> 
> http://codereview.chromium.org/1774012/

Done.

http://codereview.chromium.org/6334101/diff/5002/chrome/browser/ui/views/exte...
chrome/browser/ui/views/extensions/extension_popup.cc:179: if
(frame_->GetWindow())
On 2011/02/09 22:08:28, rafaelw wrote:
> This line appears to have been added in http://codereview.chromium.org/434046.
I
> think we can assume that there's a window now, right?

My memory is also fuzzy on this issue.  I just reviewed the code, and it seems
that, yes, there should be a window.  

Done.

http://codereview.chromium.org/6334101/diff/5002/chrome/browser/ui/views/exte...
chrome/browser/ui/views/extensions/extension_popup.cc:396: Widget* frame_widget
= Widget::GetWidgetFromNativeWindow(frame_window);
On 2011/02/09 22:08:28, rafaelw wrote:
> Looks like you can undo all of this: http://codereview.chromium.org/434046
> 
> Including removing the frame_window and profile arguments above.

Done.

http://codereview.chromium.org/6334101/diff/5002/chrome/browser/ui/views/exte...
File chrome/browser/ui/views/extensions/extension_popup.cc (right):

http://codereview.chromium.org/6334101/diff/5002/chrome/browser/ui/views/exte...
chrome/browser/ui/views/extensions/extension_popup.cc:132:
border_widget_->MoveAbove(popup_);
On 2011/02/09 22:08:28, rafaelw wrote:
> Are you sure this is still needed? 
> 
>
http://codereview.chromium.org/454019/diff/8013/chrome/browser/views/extensio...

It's not absolutely needed, because I've changed the code back to always
activate the popup on show.  However, the code is a little fragile without this
z-reordering.  I think it is better to keep the explicit z-reorder here, so that
we are not indirectly depending on activation.

http://codereview.chromium.org/6334101/diff/5002/chrome/browser/ui/views/exte...
chrome/browser/ui/views/extensions/extension_popup.cc:172: gfx::Rect rect =
GetOuterBounds();
On 2011/02/09 22:08:28, rafaelw wrote:
> Please consider putting this back the way it was:
> 
>
http://codereview.chromium.org/577015/diff/14036/chrome/browser/views/extensi...
> 
> Including inlining GetOuterBounds. It was simpler that way.

Done.

[aa, 2011-02-10 00:20:37]

I don't know if it is related, but in case you didn't know, the try
scripts (git try and gcl try) take an -r flag that lets you specify
the revision to try against. Useful so that you don't end up trying
your patch against some revision that is randomly broken, or
incompatible.

- a

On Wed, Feb 9, 2011 at 4:17 PM,  <twiz@chromium.org> wrote:
> Thanks for your review, Rafael.
>
> I made the changes you suggested.
>
> I will try to determine why the bots are failing to apply my patch - I want
> to
> make sure I did not break the other platforms.
>
> Will update once they are green.
>
>
>
http://codereview.chromium.org/6334101/diff/5002/chrome/browser/ui/views/brow...
> File chrome/browser/ui/views/browser_bubble.h (left):
>
>
http://codereview.chromium.org/6334101/diff/5002/chrome/browser/ui/views/brow...
> chrome/browser/ui/views/browser_bubble.h:123: bool drop_shadow_enabled_;
> On 2011/02/09 22:08:28, rafaelw wrote:
>>
>> You'll need to remove the DCHECK(!drop_shadow_enabled_) from
>> browser_bubble_gtk::InitPopup().
>
> Done.
>
>
http://codereview.chromium.org/6334101/diff/5002/chrome/browser/ui/views/exte...
> File chrome/browser/ui/views/extensions/extension_popup.cc (left):
>
>
http://codereview.chromium.org/6334101/diff/5002/chrome/browser/ui/views/exte...
> chrome/browser/ui/views/extensions/extension_popup.cc:17: #include
> "chrome/browser/ui/browser_list.h"
> On 2011/02/09 22:08:28, rafaelw wrote:
>>
>> Is this still needed?
>
> Done.
>
>
http://codereview.chromium.org/6334101/diff/5002/chrome/browser/ui/views/exte...
> chrome/browser/ui/views/extensions/extension_popup.cc:25: #include
> "third_party/skia/include/core/SkColor.h"
> On 2011/02/09 22:08:28, rafaelw wrote:
>>
>> Is this still needed?
>
> Done.
>
>
http://codereview.chromium.org/6334101/diff/5002/chrome/browser/ui/views/exte...
> chrome/browser/ui/views/extensions/extension_popup.cc:84:
> max_size_(kMaxWidth, kMaxHeight),
> On 2011/02/09 22:08:28, rafaelw wrote:
>>
>> Can we revert the effect of http://codereview.chromium.org/5254007
>
> Done.
>
>
http://codereview.chromium.org/6334101/diff/5002/chrome/browser/ui/views/exte...
> chrome/browser/ui/views/extensions/extension_popup.cc:158: void
> ExtensionPopup::SetArrowPosition(
> On 2011/02/09 22:08:28, rafaelw wrote:
>>
>> I don't think this is needed anymore.
>
>> http://codereview.chromium.org/1774012/
>
> Done.
>
>
http://codereview.chromium.org/6334101/diff/5002/chrome/browser/ui/views/exte...
> chrome/browser/ui/views/extensions/extension_popup.cc:179: if
> (frame_->GetWindow())
> On 2011/02/09 22:08:28, rafaelw wrote:
>>
>> This line appears to have been added in
>
> http://codereview.chromium.org/434046. I
>>
>> think we can assume that there's a window now, right?
>
> My memory is also fuzzy on this issue.  I just reviewed the code, and it
> seems that, yes, there should be a window.
>
> Done.
>
>
http://codereview.chromium.org/6334101/diff/5002/chrome/browser/ui/views/exte...
> chrome/browser/ui/views/extensions/extension_popup.cc:396: Widget*
> frame_widget = Widget::GetWidgetFromNativeWindow(frame_window);
> On 2011/02/09 22:08:28, rafaelw wrote:
>>
>> Looks like you can undo all of this:
>
> http://codereview.chromium.org/434046
>
>> Including removing the frame_window and profile arguments above.
>
> Done.
>
>
http://codereview.chromium.org/6334101/diff/5002/chrome/browser/ui/views/exte...
> File chrome/browser/ui/views/extensions/extension_popup.cc (right):
>
>
http://codereview.chromium.org/6334101/diff/5002/chrome/browser/ui/views/exte...
> chrome/browser/ui/views/extensions/extension_popup.cc:132:
> border_widget_->MoveAbove(popup_);
> On 2011/02/09 22:08:28, rafaelw wrote:
>>
>> Are you sure this is still needed?
>
>
>
http://codereview.chromium.org/454019/diff/8013/chrome/browser/views/extensio...
>
> It's not absolutely needed, because I've changed the code back to always
> activate the popup on show.  However, the code is a little fragile
> without this z-reordering.  I think it is better to keep the explicit
> z-reorder here, so that we are not indirectly depending on activation.
>
>
http://codereview.chromium.org/6334101/diff/5002/chrome/browser/ui/views/exte...
> chrome/browser/ui/views/extensions/extension_popup.cc:172: gfx::Rect
> rect = GetOuterBounds();
> On 2011/02/09 22:08:28, rafaelw wrote:
>>
>> Please consider putting this back the way it was:
>
>
>
http://codereview.chromium.org/577015/diff/14036/chrome/browser/views/extensi...
>
>> Including inlining GetOuterBounds. It was simpler that way.
>
> Done.
>
> http://codereview.chromium.org/6334101/
>

[rafaelw, 2011-02-10 20:22:00]

lgtm w/ remaining nits.

http://codereview.chromium.org/6334101/diff/15005/chrome/browser/ui/views/ext...
File chrome/browser/ui/views/extensions/extension_popup.h (right):

http://codereview.chromium.org/6334101/diff/15005/chrome/browser/ui/views/ext...
chrome/browser/ui/views/extensions/extension_popup.h:47: // |profile| is the
user profile instance associated with the popup.  A
remove

http://codereview.chromium.org/6334101/diff/15005/chrome/browser/ui/views/ext...
chrome/browser/ui/views/extensions/extension_popup.h:144:
BubbleBorder::ArrowLocation anchor_position_;
remove

[Jeff Timanus, 2011-02-10 21:51:29]

http://codereview.chromium.org/6334101/diff/15005/chrome/browser/ui/views/ext...
File chrome/browser/ui/views/extensions/extension_popup.h (right):

http://codereview.chromium.org/6334101/diff/15005/chrome/browser/ui/views/ext...
chrome/browser/ui/views/extensions/extension_popup.h:47: // |profile| is the
user profile instance associated with the popup.  A
On 2011/02/10 20:22:00, rafaelw wrote:
> remove

Done.

http://codereview.chromium.org/6334101/diff/15005/chrome/browser/ui/views/ext...
chrome/browser/ui/views/extensions/extension_popup.h:144:
BubbleBorder::ArrowLocation anchor_position_;
On 2011/02/10 20:22:00, rafaelw wrote:
> remove

Done.

http://codereview.chromium.org/6334101/diff/22002/chrome/renderer/extensions/...
File chrome/renderer/extensions/extension_process_bindings.cc (right):

http://codereview.chromium.org/6334101/diff/22002/chrome/renderer/extensions/...
chrome/renderer/extensions/extension_process_bindings.cc:135: return true;
Added this check to prevent a regression in ExtensionApiTest.Infobars.  I had
accidentally removed the browser identity check.

[Jeff Timanus, 2011-02-14 18:38:17]

On 2011/02/08 20:44:26, Jeff Timanus wrote:
> On 2011/02/08 20:37:20, Aaron Boodman wrote:
> > Reviewing, but trybots seem very unhappy.
> 
> Thanks.  Double checking the try-bot issue.  Spoke earlier with maruel, and
the
> failures seem to be related to my removal of the files.  Somehow the patch is
> failing.  Will resolve.

Bots were failing because some of the test files did not end with newlines. 
Removed those files, and observed successful try passes.

See trys here:
http://build.chromium.org/p/tryserver.chromium/builders/linux/builds/12274
http://build.chromium.org/p/tryserver.chromium/builders/linux_view/builds/2830
http://build.chromium.org/p/tryserver.chromium/builders/mac/builds/12722
http://build.chromium.org/p/tryserver.chromium/builders/linux_chromeos/builds...

Jeff