Consolidate javascript_dialog_manager.h and javascript_dialog_manager_impl.h

Consolidate javascript_dialog_manager.h and javascript_dialog_manager_impl.h

* Renamed JavaScriptDialgoManagerImpl to JavaScriptDialogManager.
* Add namespace app_modal_dialogs
* Add namespace javascript_dialog_extensions_client
* Shortened InstallJavaScriptDialogExtensionsClient to InstallClient to fit the 80 chars limit.

BUG=410499
TBR=sky@chromium.org

Committed: https://crrev.com/0929be2a6d3e1880cfb047a7adc1c3e8e6dd5285
Cr-Commit-Position: refs/heads/master@{#304903}

[oshima, 2014-11-06 07:11:39]

Patchset #1 (id:1) has been deleted

[oshima, 2014-11-06 07:11:45]

Patchset #1 (id:20001) has been deleted

[oshima, 2014-11-06 07:11:50]

Patchset #2 (id:60001) has been deleted

[oshima, 2014-11-06 07:11:54]

Patchset #1 (id:40001) has been deleted

[oshima, 2014-11-06 07:14:43]

Patchset #1 (id:80001) has been deleted

[oshima, 2014-11-06 07:17:20]

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

[Avi (use Gerrit), 2014-11-06 15:48:36]

OK, I see what you mean by the duplicate name thing.

In the past, we've used prefixes:

class ChromeContentClient : public content::ContentClient

but I'm not sure about a prefix here... StandardJavaScriptDialogManager? mmmm

Maybe msw has some thoughts?

[oshima, 2014-11-06 16:04:32]

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

[oshima, 2014-11-06 16:04:33]

+msw@, do you have any suggestion?

[msw, 2014-11-06 17:12:39]

According to //src/components/README:
Code in a component should be placed in a namespace corresponding to the name of
the component; e.g. for a component living in //components/foo, code in that
component should be in the foo:: namespace.

So putting this class in an app_modal_dialogs namespace and being explicit about
the namespace used (content::JavaScriptDialogManager vs
app_modal_dialogs::JavaScriptDialogManager) should be sufficient, at least for
now.

https://codereview.chromium.org/688853005/diff/100001/components/app_modal_di...
File components/app_modal_dialogs/javascript_dialog_manager.h (right):

https://codereview.chromium.org/688853005/diff/100001/components/app_modal_di...
components/app_modal_dialogs/javascript_dialog_manager.h:25: // JavaScript
dialog may be opened by an extensions/app, thus needs
nit: consider "Set an |extension_client| to provide access to //extensions for
JavaScript dialogs that are opened by an extension or an app."

https://codereview.chromium.org/688853005/diff/100001/components/app_modal_di...
components/app_modal_dialogs/javascript_dialog_manager.h:31:
JavaScriptNativeDialogFactory* native_dialog_factory() {
nit: reorder adjacent to SetNativeDialogFactory, which I think may be inlined:
  void set_native_dialog_factory(
      scoped_ptr<JavaScriptNativeDialogFactory> factory) {
    native_dialog_factory_ = factory.Pass();
  }

[oshima, 2014-11-06 18:05:54]

ok I'll add namespace. avi@, is it sufficient? I just want to double check
because owners has different preferences.

https://codereview.chromium.org/688853005/diff/100001/components/app_modal_di...
File components/app_modal_dialogs/javascript_dialog_manager.h (right):

https://codereview.chromium.org/688853005/diff/100001/components/app_modal_di...
components/app_modal_dialogs/javascript_dialog_manager.h:31:
JavaScriptNativeDialogFactory* native_dialog_factory() {
On 2014/11/06 17:12:39, msw wrote:
> nit: reorder adjacent to SetNativeDialogFactory,

reordered.

> which I think may be inlined:
>   void set_native_dialog_factory(
>       scoped_ptr<JavaScriptNativeDialogFactory> factory) {
>     native_dialog_factory_ = factory.Pass();
>   }


That requires inclusion of javascript_native_dialog_factory.h.

[Avi (use Gerrit), 2014-11-06 18:57:15]

SGTM.

LGTM

[oshima, 2014-11-07 19:39:25]

uploaded new patch PTAL.

Additional changes:
* Add namespace app_modal_dialogs
* Add namespace javascript_dialog_extensions_client
* Shortened InstallJavaScriptDialogExtensionsClient to InstallClient to fit the
80 chars limit.

[msw, 2014-11-11 08:34:53]

Yikes, the namespace and the types combine to offer a lot of naming verbosity
and redundancy, like:
  app_modal_dialogs::NativeAppModalDialog
  app_modal_dialogs::AppModalDialog
  app_modal_dialogs::JavaScriptAppModalDialog

Can we rename the component and namespace to just "dialogs"?
Otherwise, maybe drop "AppModal" from the type names therein?

https://codereview.chromium.org/688853005/diff/140001/athena/extensions/athen...
File athena/extensions/athena_javascript_native_dialog_factory.cc (right):

https://codereview.chromium.org/688853005/diff/140001/athena/extensions/athen...
athena/extensions/athena_javascript_native_dialog_factory.cc:26: //
JavScriptNativeDialogFactory:
nit: app_modal_dialogs:: namespace in comment.

[oshima, 2014-11-11 18:19:48]

> Yikes, the namespace and the types combine to offer a lot
> of naming verbosity and redundancy, like:
>  app_modal_dialogs::NativeAppModalDialog
>  app_modal_dialogs::AppModalDialog
>  app_modal_dialogs::JavaScriptAppModalDialog

Welcome to chromium!

joking aside, many components has similar level of verbosity, and it's pretty
common
even in other part of chromium. (extensions, views has a lot of classes with the
namespace name in it)

> Can we rename the component and namespace to just "dialogs"?
> Otherwise, maybe drop "AppModal" from the type names therein?

"app_modal_dialogs" didn't bother me at all personally, but am happy to change
if that's what owners want. I think "dialogs" is too generic as this is not for
all dialogs though.
avi@, what do you think?

https://codereview.chromium.org/688853005/diff/140001/athena/extensions/athen...
File athena/extensions/athena_javascript_native_dialog_factory.cc (right):

https://codereview.chromium.org/688853005/diff/140001/athena/extensions/athen...
athena/extensions/athena_javascript_native_dialog_factory.cc:26: //
JavScriptNativeDialogFactory:
On 2014/11/11 08:34:53, msw wrote:
> nit: app_modal_dialogs:: namespace in comment.

Done.

[oshima, 2014-11-14 19:04:13]

ping?

[msw, 2014-11-15 00:51:57]

Naming the component and namespace "app_modal" would match the "web_modal"
component for web-modal dialogs. I think ultimately we'll want them to both be
in a common "dialog[s]" component, but I guess that's not necessary for now.
Otherwise, lgtm with nits.

https://codereview.chromium.org/688853005/diff/180001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_view.cc (right):

https://codereview.chromium.org/688853005/diff/180001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_view.cc:1513:
app_modal_dialogs::AppModalDialogQueue::GetInstance()->ActivateModalDialog();
nit: use |queue| here.

https://codereview.chromium.org/688853005/diff/180001/components/app_modal_di...
File components/app_modal_dialogs/app_modal_dialog.h (right):

https://codereview.chromium.org/688853005/diff/180001/components/app_modal_di...
components/app_modal_dialogs/app_modal_dialog.h:19: class NativeAppModalDialog;
nit: blank line before

https://codereview.chromium.org/688853005/diff/180001/components/app_modal_di...
File components/app_modal_dialogs/javascript_dialog_manager.h (right):

https://codereview.chromium.org/688853005/diff/180001/components/app_modal_di...
components/app_modal_dialogs/javascript_dialog_manager.h:27: // dialog window
implementation.
nit: s/implementation/instances/

https://codereview.chromium.org/688853005/diff/180001/components/app_modal_di...
components/app_modal_dialogs/javascript_dialog_manager.h:31: // JavaScript
dialog may be opened by an extensions/app, thus needs
nit: "dialogs", "thus they need" or consider something like: "Set a client that
gives extension and app dialogs access to //extensions."

[oshima, 2014-11-17 19:03:29]

On 2014/11/15 00:51:57, msw wrote:
> Naming the component and namespace "app_modal" would match the "web_modal"
> component for web-modal dialogs. I think ultimately we'll want them to both be
> in a common "dialog[s]" component, but I guess that's not necessary for now.
> Otherwise, lgtm with nits.

I'd rather rename the directory first if app_modal is preferable.
https://codereview.chromium.org/735473002/


> 
>
https://codereview.chromium.org/688853005/diff/180001/chrome/browser/ui/views...
> File chrome/browser/ui/views/frame/browser_view.cc (right):
> 
>
https://codereview.chromium.org/688853005/diff/180001/chrome/browser/ui/views...
> chrome/browser/ui/views/frame/browser_view.cc:1513:
> app_modal_dialogs::AppModalDialogQueue::GetInstance()->ActivateModalDialog();
> nit: use |queue| here.
> 
>
https://codereview.chromium.org/688853005/diff/180001/components/app_modal_di...
> File components/app_modal_dialogs/app_modal_dialog.h (right):
> 
>
https://codereview.chromium.org/688853005/diff/180001/components/app_modal_di...
> components/app_modal_dialogs/app_modal_dialog.h:19: class
NativeAppModalDialog;
> nit: blank line before
> 
>
https://codereview.chromium.org/688853005/diff/180001/components/app_modal_di...
> File components/app_modal_dialogs/javascript_dialog_manager.h (right):
> 
>
https://codereview.chromium.org/688853005/diff/180001/components/app_modal_di...
> components/app_modal_dialogs/javascript_dialog_manager.h:27: // dialog window
> implementation.
> nit: s/implementation/instances/
> 
>
https://codereview.chromium.org/688853005/diff/180001/components/app_modal_di...
> components/app_modal_dialogs/javascript_dialog_manager.h:31: // JavaScript
> dialog may be opened by an extensions/app, thus needs
> nit: "dialogs", "thus they need" or consider something like: "Set a client
that
> gives extension and app dialogs access to //extensions."

[msw, 2014-11-17 19:15:58]

On 2014/11/17 19:03:29, oshima wrote:
> On 2014/11/15 00:51:57, msw wrote:
> > Naming the component and namespace "app_modal" would match the "web_modal"
> > component for web-modal dialogs. I think ultimately we'll want them to both
be
> > in a common "dialog[s]" component, but I guess that's not necessary for now.
> > Otherwise, lgtm with nits.
> 
> I'd rather rename the directory first if app_modal is preferable.
> https://codereview.chromium.org/735473002/

sgtm

[oshima, 2014-11-18 17:42:41]

https://codereview.chromium.org/688853005/diff/180001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_view.cc (right):

https://codereview.chromium.org/688853005/diff/180001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_view.cc:1513:
app_modal_dialogs::AppModalDialogQueue::GetInstance()->ActivateModalDialog();
On 2014/11/15 00:51:56, msw wrote:
> nit: use |queue| here.

Done.

https://codereview.chromium.org/688853005/diff/180001/components/app_modal_di...
File components/app_modal_dialogs/javascript_dialog_manager.h (right):

https://codereview.chromium.org/688853005/diff/180001/components/app_modal_di...
components/app_modal_dialogs/javascript_dialog_manager.h:27: // dialog window
implementation.
On 2014/11/15 00:51:57, msw wrote:
> nit: s/implementation/instances/

Done.

https://codereview.chromium.org/688853005/diff/180001/components/app_modal_di...
components/app_modal_dialogs/javascript_dialog_manager.h:31: // JavaScript
dialog may be opened by an extensions/app, thus needs
On 2014/11/15 00:51:57, msw wrote:
> nit: "dialogs", "thus they need" or

Done

> consider something like: "Set a client that
> gives extension and app dialogs access to //extensions."

I used original sentense this because "extension and app dialogs" may be
confused with "dialogs that was implemented by extensions".

[Avi (use Gerrit), 2014-11-18 18:17:02]

On 2014/11/18 17:42:41, oshima wrote:
>
https://codereview.chromium.org/688853005/diff/180001/chrome/browser/ui/views...
> File chrome/browser/ui/views/frame/browser_view.cc (right):
> 
>
https://codereview.chromium.org/688853005/diff/180001/chrome/browser/ui/views...
> chrome/browser/ui/views/frame/browser_view.cc:1513:
> app_modal_dialogs::AppModalDialogQueue::GetInstance()->ActivateModalDialog();
> On 2014/11/15 00:51:56, msw wrote:
> > nit: use |queue| here.
> 
> Done.
> 
>
https://codereview.chromium.org/688853005/diff/180001/components/app_modal_di...
> File components/app_modal_dialogs/javascript_dialog_manager.h (right):
> 
>
https://codereview.chromium.org/688853005/diff/180001/components/app_modal_di...
> components/app_modal_dialogs/javascript_dialog_manager.h:27: // dialog window
> implementation.
> On 2014/11/15 00:51:57, msw wrote:
> > nit: s/implementation/instances/
> 
> Done.
> 
>
https://codereview.chromium.org/688853005/diff/180001/components/app_modal_di...
> components/app_modal_dialogs/javascript_dialog_manager.h:31: // JavaScript
> dialog may be opened by an extensions/app, thus needs
> On 2014/11/15 00:51:57, msw wrote:
> > nit: "dialogs", "thus they need" or
> 
> Done
> 
> > consider something like: "Set a client that
> > gives extension and app dialogs access to //extensions."
> 
> I used original sentense this because "extension and app dialogs" may be
> confused with "dialogs that was implemented by extensions".

SLGTM

[oshima, 2014-11-19 20:59:26]

used app_modal instead of app_modal_dialog. I'll go ahead and land this once all
bots get green results.

[oshima, 2014-11-19 21:29:28]

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

[oshima, 2014-11-19 21:29:52]

tbr'ing sky for trival namespace change in chrome/browser and chrome/test

[oshima, 2014-11-19 21:30:00]

The CQ bit was checked by oshima@chromium.org

[commit-bot: I haz the power, 2014-11-19 21:31:46]

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

[commit-bot: I haz the power, 2014-11-19 22:21:16]

Committed patchset #8 (id:240001)

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

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