JS dialogs for athena

JS dialogs for athena

Adds athena impl for following client/host

web_modal::ModalDialogHost
ConstrainedWindowViewsClient
JavaScriptDialogManagerClient

This depends on https://codereview.chromium.org/666533007/

BUG=410499
TBR=yoz@chromium.org, wittman@chromium.rg, msw@chromium.org,  avi@chromium.org

Committed: https://crrev.com/8ce602046fc244109a4b0cbc1dcd04d34596ca47
Cr-Commit-Position: refs/heads/master@{#303062}

[oshima, 2014-10-29 16:10:29]

oshima@chromium.org changed reviewers:
+ skuhne@chromium.org

[oshima, 2014-10-29 16:10:38]

Patchset #2 (id:20001) has been deleted

[oshima, 2014-10-29 16:10:41]

Patchset #1 (id:1) has been deleted

[Mr4D (OOO till 08-26), 2014-10-29 22:07:44]

When adding so much code - please add unit and / or browser tests!

I'll have a look shortly.

[Mr4D (OOO till 08-26), 2014-10-29 23:10:07]

https://codereview.chromium.org/692563002/diff/40001/athena/extensions/athena...
File athena/extensions/athena_constrained_window_views_client.cc (right):

https://codereview.chromium.org/692563002/diff/40001/athena/extensions/athena...
athena/extensions/athena_constrained_window_views_client.cc:21: 
Could you add a comment what this class actually does?

https://codereview.chromium.org/692563002/diff/40001/athena/extensions/athena...
athena/extensions/athena_constrained_window_views_client.cc:26: // stores as
owned property if it doesn't exist.
Maybe "if it doesn't exist it creates one and stores it as owned property"?

Also: Could we change the name to something like
"EnsureModalDialogHostExistsAndGet()"?

But somehow I do not like this behavior - it's similar to GetInstance() which
you didn't like.

https://codereview.chromium.org/692563002/diff/40001/athena/extensions/athena...
File athena/extensions/athena_javascript_dialog_manager_client.cc (right):

https://codereview.chromium.org/692563002/diff/40001/athena/extensions/athena...
athena/extensions/athena_javascript_dialog_manager_client.cc:10: #include
"components/constrained_window/constrained_window_views.h"
Are you sure you need this?

https://codereview.chromium.org/692563002/diff/40001/athena/extensions/athena...
athena/extensions/athena_javascript_dialog_manager_client.cc:20: 
No newline

[oshima, 2014-10-31 00:28:52]

Patchset #2 (id:60001) has been deleted

[oshima, 2014-10-31 00:37:50]

I reduce the code by moving the sharable code to dependent CL.

This depends on contents + athena, so we need athena_browsertests. I'll add it
in separate CL.

https://codereview.chromium.org/692563002/diff/40001/athena/extensions/athena...
File athena/extensions/athena_constrained_window_views_client.cc (right):

https://codereview.chromium.org/692563002/diff/40001/athena/extensions/athena...
athena/extensions/athena_constrained_window_views_client.cc:21: 
On 2014/10/29 23:10:07, Mr4D wrote:
> Could you add a comment what this class actually does?

This implements WebContentsModalDialogHost and the contract is described there.

https://codereview.chromium.org/692563002/diff/40001/athena/extensions/athena...
athena/extensions/athena_constrained_window_views_client.cc:26: // stores as
owned property if it doesn't exist.
On 2014/10/29 23:10:07, Mr4D wrote:
> Maybe "if it doesn't exist it creates one and stores it as owned property"?

Done.

> 
> Also: Could we change the name to something like
> "EnsureModalDialogHostExistsAndGet()"?

This is pretty common pattern so I think it's overkill.
ex:
https://code.google.com/p/chromium/codesearch#chromium/src/ui/wm/core/transie...

 
> But somehow I do not like this behavior - it's similar to GetInstance() which
> you didn't like.

I do not like Singleton in general, but GetInstance() is fine depending on impl.

https://codereview.chromium.org/692563002/diff/40001/athena/extensions/athena...
File athena/extensions/athena_javascript_dialog_manager_client.cc (right):

https://codereview.chromium.org/692563002/diff/40001/athena/extensions/athena...
athena/extensions/athena_javascript_dialog_manager_client.cc:10: #include
"components/constrained_window/constrained_window_views.h"
On 2014/10/29 23:10:07, Mr4D wrote:
> Are you sure you need this?

Yes (:49)

https://codereview.chromium.org/692563002/diff/40001/athena/extensions/athena...
athena/extensions/athena_javascript_dialog_manager_client.cc:20: 
On 2014/10/29 23:10:07, Mr4D wrote:
> No newline

Done.

[Mr4D (OOO till 08-26), 2014-10-31 04:26:01]

I am not sure why you need an "athena_browsertest" and the normal one does not
do it. It initializes Athena with all modules as we need it. But please add
them! Thanks!

lgtm

https://codereview.chromium.org/692563002/diff/40001/athena/extensions/athena...
File athena/extensions/athena_javascript_dialog_manager_client.cc (right):

https://codereview.chromium.org/692563002/diff/40001/athena/extensions/athena...
athena/extensions/athena_javascript_dialog_manager_client.cc:10: #include
"components/constrained_window/constrained_window_views.h"
On 2014/10/31 00:37:50, oshima wrote:
> On 2014/10/29 23:10:07, Mr4D wrote:
> > Are you sure you need this?
> 
> Yes (:49)

Acknowledged.

[oshima, 2014-11-06 17:42:21]

Patchset #5 (id:140001) has been deleted

[oshima, 2014-11-06 17:53:21]

oshima@chromium.org changed reviewers:
+ avi@chromium.org, msw@chromium.org, wittman@chromium.org, yoz@chromium.org

[oshima, 2014-11-06 17:54:35]

tbring for following DEPS additions

yoz@ -> extensions
wittman@ -> components/web_modal
avi@ -> component/sapp_modal_dialogs
msw@ -> components/constrained_window

[oshima, 2014-11-06 17:54:42]

The CQ bit was checked by oshima@chromium.org

[commit-bot: I haz the power, 2014-11-06 17:55:43]

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

[commit-bot: I haz the power, 2014-11-06 18:15:40]

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

[commit-bot: I haz the power, 2014-11-06 18:15:41]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_32 on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[oshima, 2014-11-06 18:26:02]

The CQ bit was checked by oshima@chromium.org

[commit-bot: I haz the power, 2014-11-06 18:26:17]

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

[commit-bot: I haz the power, 2014-11-06 19:11:07]

Committed patchset #7 (id:200001)

[commit-bot: I haz the power, 2014-11-06 19:11:49]

Patchset 7 (id:??) landed as
https://crrev.com/8ce602046fc244109a4b0cbc1dcd04d34596ca47
Cr-Commit-Position: refs/heads/master@{#303062}