MacViews: Refactor aura dependency from ExtensionPopup

MacViews: Refactor aura dependency from ExtensionPopup

Aura specific code in ExtensionPopup won't compile with MacViews.
Refactor them into an Aura specific subclass.

BUG=425229
Committed: https://crrev.com/53f125e71fe78a1fa0e457622a83487fce117062
Cr-Commit-Position: refs/heads/master@{#319579}

[Andre, 2015-03-05 01:40:34]

Patchset #1 (id:1) has been deleted

[Andre, 2015-03-05 04:04:48]

Patchset #2 (id:40001) has been deleted

[Andre, 2015-03-05 06:53:05]

Patchset #3 (id:80001) has been deleted

[Andre, 2015-03-05 07:05:06]

Patchset #1 (id:20001) has been deleted

[Andre, 2015-03-06 00:59:38]

andresantoso@chromium.org changed reviewers:
+ finnur@chromium.org, msw@chromium.org

[Andre, 2015-03-06 00:59:39]

Michael, Finnur, PTAL.

https://codereview.chromium.org/980663003/diff/90001/chrome/browser/ui/views/...
File chrome/browser/ui/views/extensions/extension_popup_aura.cc (right):

https://codereview.chromium.org/980663003/diff/90001/chrome/browser/ui/views/...
chrome/browser/ui/views/extensions/extension_popup_aura.cc:42: if (widget ==
GetWidget()) {
This check was not there, but seems like it should be there?

https://codereview.chromium.org/980663003/diff/90001/chrome/browser/ui/views/...
chrome/browser/ui/views/extensions/extension_popup_aura.cc:59:
aura::client::GetActivationClient(root_window)->AddObserver(this);
This moved here from ExtensionPopup::ShowPopup().

[Finnur, 2015-03-06 15:23:47]

I suspect msw is a better candidate to review this, but LGTM if he's happy with
it.

[msw, 2015-03-06 19:26:49]

Just minor vague concerns; I recall this code being touchy and crashy :)

https://codereview.chromium.org/980663003/diff/90001/chrome/browser/ui/views/...
File chrome/browser/ui/views/extensions/extension_popup.cc (right):

https://codereview.chromium.org/980663003/diff/90001/chrome/browser/ui/views/...
chrome/browser/ui/views/extensions/extension_popup.cc:151: if (active && widget
== anchor_widget())
I feel like I had to explicitly check the NativeWindow of these widgets for some
reason, but I can't recall under what conditions that would be necessary, and my
latest change here doesn't explain why that would be the case... This should
probably be fine, but watch out for any regressions (might be win aero/classic
theme, cros, or win8 specific).

https://codereview.chromium.org/980663003/diff/90001/chrome/browser/ui/views/...
File chrome/browser/ui/views/extensions/extension_popup_aura.cc (right):

https://codereview.chromium.org/980663003/diff/90001/chrome/browser/ui/views/...
chrome/browser/ui/views/extensions/extension_popup_aura.cc:42: if (widget ==
GetWidget()) {
On 2015/03/06 00:59:39, Andre wrote:
> This check was not there, but seems like it should be there?

Probably, unless this was also observing the anchor widget.

https://codereview.chromium.org/980663003/diff/90001/chrome/browser/ui/views/...
chrome/browser/ui/views/extensions/extension_popup_aura.cc:59:
aura::client::GetActivationClient(root_window)->AddObserver(this);
On 2015/03/06 00:59:39, Andre wrote:
> This moved here from ExtensionPopup::ShowPopup().

Is it okay that this will call AddObserver multiple times?
(ie. user has devtools open, clicks bubble, away, and back)

[Andre, 2015-03-06 22:53:11]

Patchset #3 (id:110001) has been deleted

[Andre, 2015-03-06 22:53:21]

Patchset #3 (id:110001) has been deleted

[Andre, 2015-03-07 00:56:27]

https://codereview.chromium.org/980663003/diff/90001/chrome/browser/ui/views/...
File chrome/browser/ui/views/extensions/extension_popup.cc (right):

https://codereview.chromium.org/980663003/diff/90001/chrome/browser/ui/views/...
chrome/browser/ui/views/extensions/extension_popup.cc:151: if (active && widget
== anchor_widget())
On 2015/03/06 19:26:49, msw wrote:
> I feel like I had to explicitly check the NativeWindow of these widgets for
some
> reason, but I can't recall under what conditions that would be necessary, and
my
> latest change here doesn't explain why that would be the case... This should
> probably be fine, but watch out for any regressions (might be win aero/classic
> theme, cros, or win8 specific).

Ack, thanks!

https://codereview.chromium.org/980663003/diff/90001/chrome/browser/ui/views/...
File chrome/browser/ui/views/extensions/extension_popup_aura.cc (right):

https://codereview.chromium.org/980663003/diff/90001/chrome/browser/ui/views/...
chrome/browser/ui/views/extensions/extension_popup_aura.cc:59:
aura::client::GetActivationClient(root_window)->AddObserver(this);
On 2015/03/06 19:26:49, msw wrote:
> On 2015/03/06 00:59:39, Andre wrote:
> > This moved here from ExtensionPopup::ShowPopup().
> 
> Is it okay that this will call AddObserver multiple times?
> (ie. user has devtools open, clicks bubble, away, and back)

Good point. I moved it to Create() above.

[msw, 2015-03-07 02:14:34]

lgtm

[Andre, 2015-03-07 03:22:21]

The CQ bit was checked by andresantoso@chromium.org

[Andre, 2015-03-07 03:22:21]

The patchset sent to the CQ was uploaded after l-g-t-m from finnur@chromium.org
Link to the patchset: https://codereview.chromium.org/980663003/#ps130001
(title: "Fix for msw and rebase")

[commit-bot: I haz the power, 2015-03-07 03:23:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/980663003/130001

[commit-bot: I haz the power, 2015-03-07 05:23:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-03-07 05:23:46]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL)
Timed out jobs are not retried to avoid causing additional load on the builders
with large pending queues.

[Andre, 2015-03-07 05:54:51]

The CQ bit was checked by andresantoso@chromium.org

[commit-bot: I haz the power, 2015-03-07 05:56:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/980663003/130001

[commit-bot: I haz the power, 2015-03-07 07:56:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-03-07 07:56:36]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL)
Timed out jobs are not retried to avoid causing additional load on the builders
with large pending queues.

[Andre, 2015-03-07 15:27:11]

The CQ bit was checked by andresantoso@chromium.org

[commit-bot: I haz the power, 2015-03-07 15:27:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/980663003/130001

[commit-bot: I haz the power, 2015-03-07 17:28:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-03-07 17:28:11]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL)
Timed out jobs are not retried to avoid causing additional load on the builders
with large pending queues.

[Andre, 2015-03-07 19:32:19]

The CQ bit was checked by andresantoso@chromium.org

[commit-bot: I haz the power, 2015-03-07 19:32:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/980663003/130001

[commit-bot: I haz the power, 2015-03-07 21:33:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-03-07 21:33:21]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL)
Timed out jobs are not retried to avoid causing additional load on the builders
with large pending queues.

[Andre, 2015-03-08 02:27:23]

The CQ bit was checked by andresantoso@chromium.org

[commit-bot: I haz the power, 2015-03-08 02:33:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/980663003/130001

[commit-bot: I haz the power, 2015-03-08 03:58:03]

Committed patchset #3 (id:130001)

[commit-bot: I haz the power, 2015-03-08 03:58:44]

Patchset 3 (id:??) landed as
https://crrev.com/53f125e71fe78a1fa0e457622a83487fce117062
Cr-Commit-Position: refs/heads/master@{#319579}