Move JavaScriptDialogManager, JavascriptAppModalDialogViews to components/app_modal_dialogs

Move JavaScriptDialogManager, JavascriptAppModalDialogViews to components/app_modal_dialogs.
(I first tried to move cocoa/android impl, but it turns out to be more difficult than I originally thought so I left them as they are)

Introduced JavaScriptDialogManagerClient for embedder to implement extensions related behaviors.

BUG=410499
TBR=blundell@chromium.org

Committed: https://crrev.com/758abebc6360e6c4bad50acd851a0c41c85695ac
Cr-Commit-Position: refs/heads/master@{#303004}

[oshima, 2014-10-28 22:22:45]

Patchset #7 (id:120001) has been deleted

[oshima, 2014-10-28 22:22:48]

Patchset #1 (id:1) has been deleted

[oshima, 2014-10-28 22:22:57]

Patchset #1 (id:20001) has been deleted

[oshima, 2014-10-28 22:23:04]

Patchset #1 (id:40001) has been deleted

[oshima, 2014-10-28 22:23:12]

Patchset #1 (id:60001) has been deleted

[oshima, 2014-10-28 22:23:19]

Patchset #1 (id:80001) has been deleted

[oshima, 2014-10-28 22:23:26]

Patchset #1 (id:100001) has been deleted

[oshima, 2014-10-29 00:00:24]

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

[oshima, 2014-10-29 00:12:06]

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

[oshima, 2014-10-29 00:20:57]

Patchset #3 (id:180001) has been deleted

[oshima, 2014-10-29 00:21:10]

Patchset #2 (id:160001) has been deleted

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

Patchset #1 (id:140001) has been deleted

[oshima, 2014-10-29 00:22:29]

On 2014/10/29 00:21:29, oshima wrote:

msw -> chrome/
avi -> owner of app_modal_dialogs

[Avi (use Gerrit), 2014-10-29 17:40:57]

https://codereview.chromium.org/666533007/diff/200001/chrome/browser/ui/app_m...
File
chrome/browser/ui/app_modal_dialogs/chrome_javascript_dialog_manager_client.h
(right):

https://codereview.chromium.org/666533007/diff/200001/chrome/browser/ui/app_m...
chrome/browser/ui/app_modal_dialogs/chrome_javascript_dialog_manager_client.h:35:
// specific files.
Are we eventually going to move everything over to the component?

https://codereview.chromium.org/666533007/diff/200001/components/app_modal_di...
File components/app_modal_dialogs.gypi (right):

https://codereview.chromium.org/666533007/diff/200001/components/app_modal_di...
components/app_modal_dialogs.gypi:12: '../content/content.gyp:content_common',
alphabetize?

https://codereview.chromium.org/666533007/diff/200001/components/app_modal_di...
File components/app_modal_dialogs/DEPS (right):

https://codereview.chromium.org/666533007/diff/200001/components/app_modal_di...
components/app_modal_dialogs/DEPS:3: "+content/public/common",
Or just +content/public ?

https://codereview.chromium.org/666533007/diff/200001/components/app_modal_di...
File components/app_modal_dialogs/javascript_dialog_manager_client.cc (right):

https://codereview.chromium.org/666533007/diff/200001/components/app_modal_di...
components/app_modal_dialogs/javascript_dialog_manager_client.cc:16: client =
new_client.release();
Wait, what?

This is clearly ownership semantics, so why would you use a scoped_ptr in the
interface (correct!) and then rip out its guts and manage the object directly?

Can't you do a LazyInstance instead, at least?

https://codereview.chromium.org/666533007/diff/200001/components/app_modal_di...
File components/app_modal_dialogs/javascript_dialog_manager_client.h (right):

https://codereview.chromium.org/666533007/diff/200001/components/app_modal_di...
components/app_modal_dialogs/javascript_dialog_manager_client.h:15: class
JavaScriptAppModalDialog;
Alphabetize

https://codereview.chromium.org/666533007/diff/200001/components/app_modal_di...
components/app_modal_dialogs/javascript_dialog_manager_client.h:28:
gfx::NativeWindow parent_window) = 0;
I'm having trouble getting my head around this. Embedders implement this client
interface for the component to ask it to do things. Why does the client
interface have "CreateNativeJavaScriptPrompt"? Isn't that what the client is
asking the component to do?

https://codereview.chromium.org/666533007/diff/200001/components/app_modal_di...
components/app_modal_dialogs/javascript_dialog_manager_client.h:48: std::string*
name_out) = 0;
I'm kinda weirded out by these three calls. Does this component have a
dependency on extensions? If not, then can we name these something more generic?

https://codereview.chromium.org/666533007/diff/200001/components/app_modal_di...
File components/app_modal_dialogs/native_app_modal_dialog.h (right):

https://codereview.chromium.org/666533007/diff/200001/components/app_modal_di...
components/app_modal_dialogs/native_app_modal_dialog.h:36: */
Don't commit commented-out code.

https://codereview.chromium.org/666533007/diff/200001/components/app_modal_di...
File components/app_modal_dialogs/views/javascript_app_modal_dialog_views.cc
(right):

https://codereview.chromium.org/666533007/diff/200001/components/app_modal_di...
components/app_modal_dialogs/views/javascript_app_modal_dialog_views.cc:167: */
Don't commit commented-out code.

[msw, 2014-10-30 00:20:32]

I've only just begun reviewing, but have a few comments. Overall, I can't
dedicate time to owning this code going forward. It would be best if Avi did the
main review for the component, and great if you found a more relevant c/b OWNER
for that side of the review.

https://codereview.chromium.org/666533007/diff/220001/chrome/browser/chrome_b...
File chrome/browser/chrome_browser_main_android.cc (right):

https://codereview.chromium.org/666533007/diff/220001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_main_android.cc:44:
SetJavaScriptDialogManagerClient(
Does this need to happen so early? Can someone more relevant review this bit?

https://codereview.chromium.org/666533007/diff/220001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_main_android.cc:45: make_scoped_ptr(new
ChromeJavaScriptDialogManagerClient));
Why do android, mac, and views all do this separately, is there no way for all
three to share the same client creation and destruction code?

https://codereview.chromium.org/666533007/diff/220001/chrome/browser/ui/app_m...
File
chrome/browser/ui/app_modal_dialogs/chrome_javascript_dialog_manager_client.h
(right):

https://codereview.chromium.org/666533007/diff/220001/chrome/browser/ui/app_m...
chrome/browser/ui/app_modal_dialogs/chrome_javascript_dialog_manager_client.h:8:
#include "base/memory/scoped_ptr.h"
Why is this needed here?

https://codereview.chromium.org/666533007/diff/220001/chrome/browser/ui/views...
File chrome/browser/ui/views/javascript_app_modal_dialog_views_x11.cc (right):

https://codereview.chromium.org/666533007/diff/220001/chrome/browser/ui/views...
chrome/browser/ui/views/javascript_app_modal_dialog_views_x11.cc:7: #include
"chrome/browser/ui/views/javascript_app_modal_event_blocker_x11.h"
It's a shame this is blocking the removal of chrome/browser as a dependency for
the dialog implementations... Perhaps there's another way to enforce this or
loop in this helper? +pkotwicz@chromium.org for thoughts.

[pkotwicz, 2014-10-30 02:46:13]

pkotwicz@chromium.org changed reviewers:
+ pkotwicz@chromium.org

[pkotwicz, 2014-10-30 02:46:15]

https://codereview.chromium.org/666533007/diff/220001/chrome/browser/ui/views...
File chrome/browser/ui/views/javascript_app_modal_dialog_views_x11.cc (right):

https://codereview.chromium.org/666533007/diff/220001/chrome/browser/ui/views...
chrome/browser/ui/views/javascript_app_modal_dialog_views_x11.cc:7: #include
"chrome/browser/ui/views/javascript_app_modal_event_blocker_x11.h"
The right place for this logic is in WindowModalityController. Checking this in
WindowModalityController would require some work (and perhaps a new
ui::ModalType: ui::MODAL_TYPE_APP, which better reflects the modal behavior of
JS dialogs) I can discuss in more details if you guys are interested.

There are other places where this logic could be moved but I cannot think of a
less hacky place.

https://codereview.chromium.org/666533007/diff/220001/chrome/browser/ui/views...
chrome/browser/ui/views/javascript_app_modal_dialog_views_x11.cc:11: //
JavaScriptAppModalDialogViews, public:
Get rid of this comment

https://codereview.chromium.org/666533007/diff/220001/chrome/browser/ui/views...
File chrome/browser/ui/views/javascript_app_modal_dialog_views_x11.h (right):

https://codereview.chromium.org/666533007/diff/220001/chrome/browser/ui/views...
chrome/browser/ui/views/javascript_app_modal_dialog_views_x11.h:5: #ifndef
COMPONENTS_APP_MODAL_DIALOGS_VIEWS_JAVASCRIPT_APP_MODAL_DIALOG_VIEWS_X11_H_
ifdef should be based on file path

https://codereview.chromium.org/666533007/diff/220001/chrome/browser/ui/views...
chrome/browser/ui/views/javascript_app_modal_dialog_views_x11.h:19: //
Overridden from JavaScriptAppModalDialogViews:
JavaScriptAppModalDialogViews:

https://codereview.chromium.org/666533007/diff/220001/chrome/browser/ui/views...
chrome/browser/ui/views/javascript_app_modal_dialog_views_x11.h:22: //
Overridden from views::DialogDelegate:
views::WidgetDelegate:

[oshima, 2014-10-31 15:54:45]

I separate JavaScriptDialogManagerClient into two
- JavaScriptDialogExtensionsClient to access //extensions functions.
- JavaScriptNativeDialogFactory to create platform specific dialog imp

Let JavaScriptDialogManagerImpl own them.

PTAL.

https://codereview.chromium.org/666533007/diff/200001/chrome/browser/ui/app_m...
File
chrome/browser/ui/app_modal_dialogs/chrome_javascript_dialog_manager_client.h
(right):

https://codereview.chromium.org/666533007/diff/200001/chrome/browser/ui/app_m...
chrome/browser/ui/app_modal_dialogs/chrome_javascript_dialog_manager_client.h:35:
// specific files.
On 2014/10/29 17:40:57, Avi wrote:
> Are we eventually going to move everything over to the component?

I looked into it, but this turns out to be more complex than I originally
thought, android code depends on JNI and java code, and linux desktop one
requires refactoring x11 specific code (see peter's comment).

Since there is no real benefit to move other impl to components, I want to leave
them as is unless there is a demand.

https://codereview.chromium.org/666533007/diff/200001/components/app_modal_di...
File components/app_modal_dialogs.gypi (right):

https://codereview.chromium.org/666533007/diff/200001/components/app_modal_di...
components/app_modal_dialogs.gypi:12: '../content/content.gyp:content_common',
On 2014/10/29 17:40:57, Avi wrote:
> alphabetize?

Done.

https://codereview.chromium.org/666533007/diff/200001/components/app_modal_di...
File components/app_modal_dialogs/DEPS (right):

https://codereview.chromium.org/666533007/diff/200001/components/app_modal_di...
components/app_modal_dialogs/DEPS:3: "+content/public/common",
On 2014/10/29 17:40:57, Avi wrote:
> Or just +content/public ?

It'll add other stuff like renderer/android etc, which is probably not good
idea.

https://codereview.chromium.org/666533007/diff/200001/components/app_modal_di...
File components/app_modal_dialogs/javascript_dialog_manager_client.cc (right):

https://codereview.chromium.org/666533007/diff/200001/components/app_modal_di...
components/app_modal_dialogs/javascript_dialog_manager_client.cc:16: client =
new_client.release();
On 2014/10/29 17:40:57, Avi wrote:
> Wait, what?
> 
> This is clearly ownership semantics, so why would you use a scoped_ptr in the
> interface (correct!) and then rip out its guts and manage the object directly?
> 
> Can't you do a LazyInstance instead, at least?

This needs to be set by embedder so we can't use Snigleton nor LazyInstance, but
I changed to set the instance to JavaScriptDialogManager impl. I hope you like
it.

https://codereview.chromium.org/666533007/diff/200001/components/app_modal_di...
File components/app_modal_dialogs/javascript_dialog_manager_client.h (right):

https://codereview.chromium.org/666533007/diff/200001/components/app_modal_di...
components/app_modal_dialogs/javascript_dialog_manager_client.h:15: class
JavaScriptAppModalDialog;
On 2014/10/29 17:40:57, Avi wrote:
> Alphabetize

Done.

https://codereview.chromium.org/666533007/diff/200001/components/app_modal_di...
components/app_modal_dialogs/javascript_dialog_manager_client.h:28:
gfx::NativeWindow parent_window) = 0;
On 2014/10/29 17:40:57, Avi wrote:
> I'm having trouble getting my head around this. Embedders implement this
client
> interface for the component to ask it to do things. Why does the client
> interface have "CreateNativeJavaScriptPrompt"? Isn't that what the client is
> asking the component to do?

app_modal_dialog provides the basic framework for web contents to create dialog,
and
embedder provides a native impl of dialog window using this interface.

https://codereview.chromium.org/666533007/diff/200001/components/app_modal_di...
components/app_modal_dialogs/javascript_dialog_manager_client.h:48: std::string*
name_out) = 0;
On 2014/10/29 17:40:57, Avi wrote:
> I'm kinda weirded out by these three calls. Does this component have a
> dependency on extensions? If not, then can we name these something more
generic?

app_modal_dialog assume existence of extensions (except for android).

https://codereview.chromium.org/666533007/diff/200001/components/app_modal_di...
File components/app_modal_dialogs/native_app_modal_dialog.h (right):

https://codereview.chromium.org/666533007/diff/200001/components/app_modal_di...
components/app_modal_dialogs/native_app_modal_dialog.h:36: */
On 2014/10/29 17:40:57, Avi wrote:
> Don't commit commented-out code.

oops sorry. done.

https://codereview.chromium.org/666533007/diff/200001/components/app_modal_di...
File components/app_modal_dialogs/views/javascript_app_modal_dialog_views.cc
(right):

https://codereview.chromium.org/666533007/diff/200001/components/app_modal_di...
components/app_modal_dialogs/views/javascript_app_modal_dialog_views.cc:167: */
On 2014/10/29 17:40:57, Avi wrote:
> Don't commit commented-out code.

Done.

https://codereview.chromium.org/666533007/diff/220001/chrome/browser/chrome_b...
File chrome/browser/chrome_browser_main_android.cc (right):

https://codereview.chromium.org/666533007/diff/220001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_main_android.cc:45: make_scoped_ptr(new
ChromeJavaScriptDialogManagerClient));
On 2014/10/30 00:20:31, msw wrote:
> Why do android, mac, and views all do this separately, is there no way for all
> three to share the same client creation and destruction code?

Moved to chrome_browser_main.cc. Let me know if you have any recomendation and
I'm happy to move.

https://codereview.chromium.org/666533007/diff/220001/chrome/browser/ui/views...
File chrome/browser/ui/views/javascript_app_modal_dialog_views_x11.cc (right):

https://codereview.chromium.org/666533007/diff/220001/chrome/browser/ui/views...
chrome/browser/ui/views/javascript_app_modal_dialog_views_x11.cc:11: //
JavaScriptAppModalDialogViews, public:
On 2014/10/30 02:46:14, pkotwicz wrote:
> Get rid of this comment

Done.

https://codereview.chromium.org/666533007/diff/220001/chrome/browser/ui/views...
File chrome/browser/ui/views/javascript_app_modal_dialog_views_x11.h (right):

https://codereview.chromium.org/666533007/diff/220001/chrome/browser/ui/views...
chrome/browser/ui/views/javascript_app_modal_dialog_views_x11.h:5: #ifndef
COMPONENTS_APP_MODAL_DIALOGS_VIEWS_JAVASCRIPT_APP_MODAL_DIALOG_VIEWS_X11_H_
On 2014/10/30 02:46:15, pkotwicz wrote:
> ifdef should be based on file path

DONE. Hmm, IIRC there used to be presubmit check for this. I wonder if it has
been removed or regressed.

https://codereview.chromium.org/666533007/diff/220001/chrome/browser/ui/views...
chrome/browser/ui/views/javascript_app_modal_dialog_views_x11.h:19: //
Overridden from JavaScriptAppModalDialogViews:
On 2014/10/30 02:46:15, pkotwicz wrote:
> JavaScriptAppModalDialogViews:

Done.

https://codereview.chromium.org/666533007/diff/220001/chrome/browser/ui/views...
chrome/browser/ui/views/javascript_app_modal_dialog_views_x11.h:22: //
Overridden from views::DialogDelegate:
On 2014/10/30 02:46:15, pkotwicz wrote:
> views::WidgetDelegate:

Done.

[oshima, 2014-10-31 16:33:10]

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

https://codereview.chromium.org/666533007/diff/280001/components/app_modal_di...
components/app_modal_dialogs/javascript_dialog_manager.h:1: // Copyright (c)
2012 The Chromium Authors. All rights reserved.
I can nuke this file and expose JavaScriptDialogManagerImpl instead
if you prefer that way. I don't have strong feeling about this, so please let me
know.

[pkotwicz, 2014-10-31 17:02:44]

https://codereview.chromium.org/666533007/diff/280001/chrome/browser/chrome_b...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/666533007/diff/280001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_main.cc:1070:
InstallJavaScriptDialogExtensionsClient();
We are leaking the pointer here. Does it matter?

[pkotwicz, 2014-10-31 17:07:41]

https://codereview.chromium.org/666533007/diff/280001/chrome/browser/chrome_b...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/666533007/diff/280001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_main.cc:1070:
InstallJavaScriptDialogExtensionsClient();
Never mind, I stand corrected

[oshima, 2014-11-03 21:51:08]

ping?

[Avi (use Gerrit), 2014-11-04 20:46:41]

I'm reasonably OK with this. It's rather unfortunate that we can't move all the
implementations over. :(

https://codereview.chromium.org/666533007/diff/280001/chrome/browser/ui/andro...
File
chrome/browser/ui/android/chrome_javascript_native_dialog_factory_android.cc
(right):

https://codereview.chromium.org/666533007/diff/280001/chrome/browser/ui/andro...
chrome/browser/ui/android/chrome_javascript_native_dialog_factory_android.cc:14:
class ChromeJavaScritpNativeDialogAndroidFactory
Typo: "JavaScritp"

https://codereview.chromium.org/666533007/diff/280001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/chrome_javascript_native_dialog_factory_cocoa.mm
(right):

https://codereview.chromium.org/666533007/diff/280001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/chrome_javascript_native_dialog_factory_cocoa.mm:14:
class ChromeJavaScritpNativeDialogCocoaFactory
typo:JavaScritp

https://codereview.chromium.org/666533007/diff/280001/chrome/browser/ui/views...
File chrome/browser/ui/views/chrome_javascript_native_dialog_factory_views.cc
(right):

https://codereview.chromium.org/666533007/diff/280001/chrome/browser/ui/views...
chrome/browser/ui/views/chrome_javascript_native_dialog_factory_views.cc:19:
class ChromeJavaScritpNativeDialogViewsFactory
typo:JavaScritp

https://codereview.chromium.org/666533007/diff/280001/components/app_modal_di...
File components/app_modal_dialogs/javascript_dialog_extensions_client.h (right):

https://codereview.chromium.org/666533007/diff/280001/components/app_modal_di...
components/app_modal_dialogs/javascript_dialog_extensions_client.h:17: // that
opened a javascript dialog.
JavaScript, not javascript.

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

https://codereview.chromium.org/666533007/diff/280001/components/app_modal_di...
components/app_modal_dialogs/javascript_dialog_manager.h:28: // access to
extensions functionality. This sets a client interfae to
typo:interfae

https://codereview.chromium.org/666533007/diff/280001/components/constrained_...
File components/constrained_window/constrained_window_views.h (right):

https://codereview.chromium.org/666533007/diff/280001/components/constrained_...
components/constrained_window/constrained_window_views.h:54: views::Widget*
CreateWindowModalDialogViews(views::DialogDelegate* dialog,
You talk about this in the CL description:

> Renamed CreateBrowserModalDialogViews to CreateWindowModalDialogViews because
it has nothing to do with Browser any more.

CreateBrowserModalDialogViews never had anything to do about the browser. It was
about being "browser-modal", or in other words, "app-modal". Your new name
implies that it's window-modal. Did you change the modality of the dialog? If
not, and if you don't like the old name because it has "browser" in it, pick a
new one that doesn't imply incorrect modality semantics.

(Note the use of "app modal" already in places like
"components/app_modal_dialogs".)

https://codereview.chromium.org/666533007/diff/280001/extensions/components/j...
File extensions/components/javascript_dialog_extensions_client/BUILD.gn (right):

https://codereview.chromium.org/666533007/diff/280001/extensions/components/j...
extensions/components/javascript_dialog_extensions_client/BUILD.gn:15:
"//extensions/common",
Is this supposed to be alphabetized? (Honest question; I don't know gn files.)

[oshima, 2014-11-04 23:09:14]

Patchset #6 (id:300001) has been deleted

[oshima, 2014-11-04 23:12:18]

PTAL

https://codereview.chromium.org/666533007/diff/280001/chrome/browser/ui/andro...
File
chrome/browser/ui/android/chrome_javascript_native_dialog_factory_android.cc
(right):

https://codereview.chromium.org/666533007/diff/280001/chrome/browser/ui/andro...
chrome/browser/ui/android/chrome_javascript_native_dialog_factory_android.cc:14:
class ChromeJavaScritpNativeDialogAndroidFactory
On 2014/11/04 20:46:41, Avi wrote:
> Typo: "JavaScritp"

Done. and other typos.

https://codereview.chromium.org/666533007/diff/280001/components/constrained_...
File components/constrained_window/constrained_window_views.h (right):

https://codereview.chromium.org/666533007/diff/280001/components/constrained_...
components/constrained_window/constrained_window_views.h:54: views::Widget*
CreateWindowModalDialogViews(views::DialogDelegate* dialog,
On 2014/11/04 20:46:41, Avi wrote:
> You talk about this in the CL description:
> 
> > Renamed CreateBrowserModalDialogViews to CreateWindowModalDialogViews
because
> it has nothing to do with Browser any more.
> 
> CreateBrowserModalDialogViews never had anything to do about the browser. It
was
> about being "browser-modal", or in other words, "app-modal". Your new name
> implies that it's window-modal. Did you change the modality of the dialog? If
> not, and if you don't like the old name because it has "browser" in it, pick a
> new one that doesn't imply incorrect modality semantics.
> 
> (Note the use of "app modal" already in places like
> "components/app_modal_dialogs".)

I see. I was confused by the comment. I changed to CreateAppModalDialogViews and
update both comment and the description.

App can be confusing too (as we have chrome apps), but I couldn't come up with
better name.

https://codereview.chromium.org/666533007/diff/280001/extensions/components/j...
File extensions/components/javascript_dialog_extensions_client/BUILD.gn (right):

https://codereview.chromium.org/666533007/diff/280001/extensions/components/j...
extensions/components/javascript_dialog_extensions_client/BUILD.gn:15:
"//extensions/common",
On 2014/11/04 20:46:41, Avi wrote:
> Is this supposed to be alphabetized? (Honest question; I don't know gn files.)

Yes, I think so. Done.

[Avi (use Gerrit), 2014-11-04 23:55:23]

LGTM

[msw, 2014-11-05 03:53:56]

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

https://codereview.chromium.org/666533007/diff/280001/components/app_modal_di...
components/app_modal_dialogs/javascript_dialog_manager.h:1: // Copyright (c)
2012 The Chromium Authors. All rights reserved.
On 2014/10/31 16:33:10, oshima wrote:
> I can nuke this file and expose JavaScriptDialogManagerImpl instead
> if you prefer that way. I don't have strong feeling about this, so please let
me
> know.

I think that would be good, but I don't have a strong opinion.

https://codereview.chromium.org/666533007/diff/280001/components/constrained_...
File components/constrained_window/constrained_window_views.h (right):

https://codereview.chromium.org/666533007/diff/280001/components/constrained_...
components/constrained_window/constrained_window_views.h:54: views::Widget*
CreateWindowModalDialogViews(views::DialogDelegate* dialog,
On 2014/11/04 23:12:18, oshima wrote:
> On 2014/11/04 20:46:41, Avi wrote:
> > You talk about this in the CL description:
> > 
> > > Renamed CreateBrowserModalDialogViews to CreateWindowModalDialogViews
> because
> > it has nothing to do with Browser any more.
> > 
> > CreateBrowserModalDialogViews never had anything to do about the browser. It
> was
> > about being "browser-modal", or in other words, "app-modal". Your new name
> > implies that it's window-modal. Did you change the modality of the dialog?
If
> > not, and if you don't like the old name because it has "browser" in it, pick
a
> > new one that doesn't imply incorrect modality semantics.
> > 
> > (Note the use of "app modal" already in places like
> > "components/app_modal_dialogs".)
> 
> I see. I was confused by the comment. I changed to CreateAppModalDialogViews
and
> update both comment and the description.
> 
> App can be confusing too (as we have chrome apps), but I couldn't come up with
> better name.

I wonder if "SystemModal" might be appropriate here (and to rename
[JavaScript|Native]SystemModalDialog), to better match ui::ModalType, although
I'm not entirely opposed to this name (as in "the modality extends to all
application instances in the system" i guess?) Maybe ui::ModalType's
MODAL_TYPE_SYSTEM should change to MODAL_TYPE_APP?
https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/ui_base_ty...

https://codereview.chromium.org/666533007/diff/340001/chrome/browser/chrome_b...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/666533007/diff/340001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_main.cc:200: #if defined(OS_MACOSX) ||
defined(OS_ANDROID) || defined(TOOLKIT_VIEWS)
nit: Should this just be !iOS? (keep the block below in sync).

https://codereview.chromium.org/666533007/diff/340001/chrome/browser/ui/BUILD.gn
File chrome/browser/ui/BUILD.gn (left):

https://codereview.chromium.org/666533007/diff/340001/chrome/browser/ui/BUILD...
chrome/browser/ui/BUILD.gn:49: "//components/app_modal_dialogs",
Doesn't chrome/browser/ui/browser.cc still use this?

https://codereview.chromium.org/666533007/diff/340001/chrome/browser/ui/andro...
File
chrome/browser/ui/android/chrome_javascript_native_dialog_factory_android.cc
(right):

https://codereview.chromium.org/666533007/diff/340001/chrome/browser/ui/andro...
chrome/browser/ui/android/chrome_javascript_native_dialog_factory_android.cc:1:
// Copyright 2014 The Chromium Authors. All rights reserved.
Perhaps everything in this file could still live in
javascript_app_modal_dialog_android.cc?

https://codereview.chromium.org/666533007/diff/340001/chrome/browser/ui/andro...
File chrome/browser/ui/android/javascript_app_modal_dialog_android.cc (right):

https://codereview.chromium.org/666533007/diff/340001/chrome/browser/ui/andro...
chrome/browser/ui/android/javascript_app_modal_dialog_android.cc:9: #include
"base/strings/utf_string_conversions.h"
Remove this, it's not used.

https://codereview.chromium.org/666533007/diff/340001/chrome/browser/ui/andro...
chrome/browser/ui/android/javascript_app_modal_dialog_android.cc:13: #include
"content/public/browser/web_contents.h"
Remove this, it's not used.

https://codereview.chromium.org/666533007/diff/340001/chrome/browser/ui/andro...
chrome/browser/ui/android/javascript_app_modal_dialog_android.cc:22: using
content::BrowserThread;
nit: inline for the one usage.

https://codereview.chromium.org/666533007/diff/340001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/chrome_javascript_native_dialog_factory_cocoa.mm
(right):

https://codereview.chromium.org/666533007/diff/340001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/chrome_javascript_native_dialog_factory_cocoa.mm:1: //
Copyright 2014 The Chromium Authors. All rights reserved.
Ditto: Perhaps everything in this file could still live in
javascript_app_modal_dialog_cocoa.mm?

https://codereview.chromium.org/666533007/diff/340001/chrome/browser/ui/views...
File chrome/browser/ui/views/chrome_javascript_native_dialog_factory.h (right):

https://codereview.chromium.org/666533007/diff/340001/chrome/browser/ui/views...
chrome/browser/ui/views/chrome_javascript_native_dialog_factory.h:1: //
Copyright 2014 The Chromium Authors. All rights reserved.
Is this file actually needed? I don't think you even added it to a gyp target.
Isn't
chrome/browser/ui/app_modal_dialogs/chrome_javascript_native_dialog_factory.h
sufficient?

https://codereview.chromium.org/666533007/diff/340001/chrome/browser/ui/views...
chrome/browser/ui/views/chrome_javascript_native_dialog_factory.h:5: #ifndef
CHROME_BROWSER_UI_APP_MODAL_DIALOGS_CHROME_JAVASCRIPT_NATIVE_DIALOG_FACTORY_H_
Fix this guard...

https://codereview.chromium.org/666533007/diff/340001/chrome/browser/ui/views...
File chrome/browser/ui/views/chrome_javascript_native_dialog_factory_views.cc
(right):

https://codereview.chromium.org/666533007/diff/340001/chrome/browser/ui/views...
chrome/browser/ui/views/chrome_javascript_native_dialog_factory_views.cc:1: //
Copyright 2014 The Chromium Authors. All rights reserved.
Ditto: Perhaps everything in this file could still live in
javascript_app_modal_dialog_views.cc?

https://codereview.chromium.org/666533007/diff/340001/chrome/browser/ui/views...
chrome/browser/ui/views/chrome_javascript_native_dialog_factory_views.cc:9:
#include
"components/app_modal_dialogs/views/javascript_app_modal_dialog_views.h"
Should this be in an #else with the x11 header below?

https://codereview.chromium.org/666533007/diff/340001/chrome/browser/ui/views...
chrome/browser/ui/views/chrome_javascript_native_dialog_factory_views.cc:11:
#include "ui/views/widget/widget.h"
Remove this, it's not used.

https://codereview.chromium.org/666533007/diff/340001/chrome/browser/ui/views...
chrome/browser/ui/views/chrome_javascript_native_dialog_factory_views.cc:30:
JavaScriptAppModalDialogViews* d =
nit: move the decl of |d| out of the pre-processor blocks.

https://codereview.chromium.org/666533007/diff/340001/chrome/browser/ui/views...
File chrome/browser/ui/views/update_recommended_message_box.cc (right):

https://codereview.chromium.org/666533007/diff/340001/chrome/browser/ui/views...
chrome/browser/ui/views/update_recommended_message_box.cc:27:
parent_window)->Show();
nit: fix indent

https://codereview.chromium.org/666533007/diff/340001/chrome/chrome_browser_u...
File chrome/chrome_browser_ui.gypi (right):

https://codereview.chromium.org/666533007/diff/340001/chrome/chrome_browser_u...
chrome/chrome_browser_ui.gypi:21:
'browser/ui/android/chrome_javascript_native_dialog_factory_android.cc',
Should these gypi changes also be made to a GN file somewhere?
(not sure it exists yet, but checking just in case)

https://codereview.chromium.org/666533007/diff/340001/components/app_modal_di...
File components/app_modal_dialogs.gypi (right):

https://codereview.chromium.org/666533007/diff/340001/components/app_modal_di...
components/app_modal_dialogs.gypi:40: ['toolkit_views==1 and OS != "mac"', {
nit: be consistent about equality operator spacing.

https://codereview.chromium.org/666533007/diff/340001/components/app_modal_di...
File components/app_modal_dialogs/android/DEPS (right):

https://codereview.chromium.org/666533007/diff/340001/components/app_modal_di...
components/app_modal_dialogs/android/DEPS:1: include_rules = [
Do DEPS apply to parent directories? (this dir contains no other files)

https://codereview.chromium.org/666533007/diff/340001/components/app_modal_di...
File components/app_modal_dialogs/cocoa/DEPS (right):

https://codereview.chromium.org/666533007/diff/340001/components/app_modal_di...
components/app_modal_dialogs/cocoa/DEPS:1: include_rules = [
Ditto: Do DEPS apply to parent directories? (this dir contains no other files)

https://codereview.chromium.org/666533007/diff/340001/components/app_modal_di...
File components/app_modal_dialogs/javascript_app_modal_dialog.cc (right):

https://codereview.chromium.org/666533007/diff/340001/components/app_modal_di...
components/app_modal_dialogs/javascript_app_modal_dialog.cc:9: #include
"components/app_modal_dialogs/native_app_modal_dialog.h"
I don't think this include is needed any more.

https://codereview.chromium.org/666533007/diff/340001/components/app_modal_di...
File components/app_modal_dialogs/javascript_dialog_manager.cc (right):

https://codereview.chromium.org/666533007/diff/340001/components/app_modal_di...
components/app_modal_dialogs/javascript_dialog_manager.cc:5: #include
"components/app_modal_dialogs/javascript_dialog_manager.h"
You can remove this file and let clients use the Impl class via
javascript_dialog_manager_impl.h directly (and rename that to drop "Impl").

https://codereview.chromium.org/666533007/diff/340001/components/app_modal_di...
components/app_modal_dialogs/javascript_dialog_manager.cc:7: #include
"base/memory/scoped_ptr.h"
nit: the header includes this, so it's not needed here.

https://codereview.chromium.org/666533007/diff/340001/components/app_modal_di...
components/app_modal_dialogs/javascript_dialog_manager.cc:18:
JavaScriptDialogManagerImpl::GetInstance()->SetExtensionsClient(
nit: use GetJavaScriptDialogManagerInstance.

https://codereview.chromium.org/666533007/diff/340001/components/app_modal_di...
components/app_modal_dialogs/javascript_dialog_manager.cc:22: void
SetJavaScriptNativeDialogFactory(
nit: reorder to match declarations (if you keep this file).

https://codereview.chromium.org/666533007/diff/340001/components/app_modal_di...
components/app_modal_dialogs/javascript_dialog_manager.cc:24:
JavaScriptDialogManagerImpl::GetInstance()->SetNativeDialogFactory(
ditto nit: use GetJavaScriptDialogManagerInstance.

https://codereview.chromium.org/666533007/diff/340001/components/app_modal_di...
File components/app_modal_dialogs/javascript_dialog_manager_impl.cc (right):

https://codereview.chromium.org/666533007/diff/340001/components/app_modal_di...
components/app_modal_dialogs/javascript_dialog_manager_impl.cc:13: #include
"components/app_modal_dialogs/javascript_app_modal_dialog.h"
This is included in the header and not needed here.

https://codereview.chromium.org/666533007/diff/340001/components/app_modal_di...
components/app_modal_dialogs/javascript_dialog_manager_impl.cc:16: #include
"components/app_modal_dialogs/native_app_modal_dialog.h"
I don't think this include is needed anymore.

https://codereview.chromium.org/666533007/diff/340001/components/app_modal_di...
components/app_modal_dialogs/javascript_dialog_manager_impl.cc:24: using
content::BrowserContext;
Remove this; the type is no longer used here.

https://codereview.chromium.org/666533007/diff/340001/components/app_modal_di...
components/app_modal_dialogs/javascript_dialog_manager_impl.cc:25: using
content::JavaScriptDialogManager;
Remove this; the type is no longer used here.

https://codereview.chromium.org/666533007/diff/340001/components/app_modal_di...
components/app_modal_dialogs/javascript_dialog_manager_impl.cc:26: using
content::WebContents;
nit: explicitly use "content::" and remove this, or remove "content::" from the
three new WebContents uses.

https://codereview.chromium.org/666533007/diff/340001/components/app_modal_di...
components/app_modal_dialogs/javascript_dialog_manager_impl.cc:55:
JavaScriptDialogManagerImpl::JavaScriptDialogManagerImpl()
nit: reorder to match decl.

https://codereview.chromium.org/666533007/diff/340001/components/app_modal_di...
components/app_modal_dialogs/javascript_dialog_manager_impl.cc:59:
JavaScriptDialogManagerImpl::~JavaScriptDialogManagerImpl() {
nit: reorder to match decl.

https://codereview.chromium.org/666533007/diff/340001/components/app_modal_di...
components/app_modal_dialogs/javascript_dialog_manager_impl.cc:204: if
(extensions_client_->GetExtensionName(web_contents, origin_url, &name)) {
nit: remove curly braces.

https://codereview.chromium.org/666533007/diff/340001/components/app_modal_di...
File components/app_modal_dialogs/javascript_native_dialog_factory.h (right):

https://codereview.chromium.org/666533007/diff/340001/components/app_modal_di...
components/app_modal_dialogs/javascript_native_dialog_factory.h:8: #include
<string>
Remove this, it's not used.

https://codereview.chromium.org/666533007/diff/340001/components/app_modal_di...
components/app_modal_dialogs/javascript_native_dialog_factory.h:10: #include
"base/memory/scoped_ptr.h"
Remove this, it's not used.

https://codereview.chromium.org/666533007/diff/340001/components/app_modal_di...
components/app_modal_dialogs/javascript_native_dialog_factory.h:13: class GURL;
Remove this, it's not used.

https://codereview.chromium.org/666533007/diff/340001/components/constrained_...
File components/constrained_window/constrained_window_views.cc (right):

https://codereview.chromium.org/666533007/diff/340001/components/constrained_...
components/constrained_window/constrained_window_views.cc:155: views::Widget*
CreateAppModalDialogViews(views::DialogDelegate* dialog,
This tangential function renaming affects too many files to be part of such a
large, complex CL; this really should be landed first to simplify the review of
the actual componentization changes.

https://codereview.chromium.org/666533007/diff/340001/components/constrained_...
components/constrained_window/constrained_window_views.cc:156: gfx::NativeWindow
parent) {
nit: fix indent.

https://codereview.chromium.org/666533007/diff/340001/extensions/components/e...
File extensions/components/extensions_components.gyp (right):

https://codereview.chromium.org/666533007/diff/340001/extensions/components/e...
extensions/components/extensions_components.gyp:12: 'includes': [
You need an extensions/OWNERS reviewer.

https://codereview.chromium.org/666533007/diff/340001/extensions/components/j...
File extensions/components/javascript_dialog_extensions_client/BUILD.gn (right):

https://codereview.chromium.org/666533007/diff/340001/extensions/components/j...
extensions/components/javascript_dialog_extensions_client/BUILD.gn:11: deps = [
Does this need a dependency for javascript_dialog_extension_client_impl.cc's
"content/public/browser/web_contents.h" include?

https://codereview.chromium.org/666533007/diff/340001/extensions/components/j...
File
extensions/components/javascript_dialog_extensions_client/javascript_dialog_extension_client_impl.cc
(right):

https://codereview.chromium.org/666533007/diff/340001/extensions/components/j...
extensions/components/javascript_dialog_extensions_client/javascript_dialog_extension_client_impl.cc:40:
content::WebContents* web_contents) override;
nit: you can inline these definitions if the class is declared here.

[oshima, 2014-11-05 21:42:19]

avi@, can you answer two questions below?

https://codereview.chromium.org/666533007/diff/280001/components/constrained_...
File components/constrained_window/constrained_window_views.h (right):

https://codereview.chromium.org/666533007/diff/280001/components/constrained_...
components/constrained_window/constrained_window_views.h:54: views::Widget*
CreateWindowModalDialogViews(views::DialogDelegate* dialog,
On 2014/11/05 03:53:54, msw wrote:
> On 2014/11/04 23:12:18, oshima wrote:
> > On 2014/11/04 20:46:41, Avi wrote:
> > > You talk about this in the CL description:
> > > 
> > > > Renamed CreateBrowserModalDialogViews to CreateWindowModalDialogViews
> > because
> > > it has nothing to do with Browser any more.
> > > 
> > > CreateBrowserModalDialogViews never had anything to do about the browser.
It
> > was
> > > about being "browser-modal", or in other words, "app-modal". Your new name
> > > implies that it's window-modal. Did you change the modality of the dialog?
> If
> > > not, and if you don't like the old name because it has "browser" in it,
pick
> a
> > > new one that doesn't imply incorrect modality semantics.
> > > 
> > > (Note the use of "app modal" already in places like
> > > "components/app_modal_dialogs".)
> > 
> > I see. I was confused by the comment. I changed to CreateAppModalDialogViews
> and
> > update both comment and the description.
> > 
> > App can be confusing too (as we have chrome apps), but I couldn't come up
with
> > better name.
> 
> I wonder if "SystemModal" might be appropriate here (and to rename
> [JavaScript|Native]SystemModalDialog), to better match ui::ModalType, although
> I'm not entirely opposed to this name (as in "the modality extends to all
> application instances in the system" i guess?) Maybe ui::ModalType's
> MODAL_TYPE_SYSTEM should change to MODAL_TYPE_APP?
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/ui_base_ty...
> 

Looks like we have inconsistent use of app/system. I actually thought about
"system modal", but I didn't pick it because while this is true on chromeos,
it's not really "system wide" modal on other platforms. ProcessModal is another
candidate, although I sounds ugly to me.

In any case, if this is controversal, I'll do that in a separate CL.

I'm happy to rename whatever you and avi@ agrees on though. avi@, what is your
suggestion?

https://codereview.chromium.org/666533007/diff/340001/chrome/browser/chrome_b...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/666533007/diff/340001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_main.cc:200: #if defined(OS_MACOSX) ||
defined(OS_ANDROID) || defined(TOOLKIT_VIEWS)
On 2014/11/05 03:53:54, msw wrote:
> nit: Should this just be !iOS? (keep the block below in sync).

Done.

https://codereview.chromium.org/666533007/diff/340001/chrome/browser/ui/BUILD.gn
File chrome/browser/ui/BUILD.gn (left):

https://codereview.chromium.org/666533007/diff/340001/chrome/browser/ui/BUILD...
chrome/browser/ui/BUILD.gn:49: "//components/app_modal_dialogs",
On 2014/11/05 03:53:54, msw wrote:
> Doesn't chrome/browser/ui/browser.cc still use this?

I thought this depends on browser but apparently not. reverted.

https://codereview.chromium.org/666533007/diff/340001/chrome/browser/ui/andro...
File
chrome/browser/ui/android/chrome_javascript_native_dialog_factory_android.cc
(right):

https://codereview.chromium.org/666533007/diff/340001/chrome/browser/ui/andro...
chrome/browser/ui/android/chrome_javascript_native_dialog_factory_android.cc:1:
// Copyright 2014 The Chromium Authors. All rights reserved.
On 2014/11/05 03:53:54, msw wrote:
> Perhaps everything in this file could still live in
> javascript_app_modal_dialog_android.cc?

I put this in separate file because I thought it's a bit too big to mix it into
that file, but
I don't have strong opinion.

avi@, are you ok with this?

https://codereview.chromium.org/666533007/diff/340001/chrome/browser/ui/andro...
File chrome/browser/ui/android/javascript_app_modal_dialog_android.cc (right):

https://codereview.chromium.org/666533007/diff/340001/chrome/browser/ui/andro...
chrome/browser/ui/android/javascript_app_modal_dialog_android.cc:9: #include
"base/strings/utf_string_conversions.h"
On 2014/11/05 03:53:55, msw wrote:
> Remove this, it's not used.

Done.

https://codereview.chromium.org/666533007/diff/340001/chrome/browser/ui/andro...
chrome/browser/ui/android/javascript_app_modal_dialog_android.cc:13: #include
"content/public/browser/web_contents.h"
On 2014/11/05 03:53:54, msw wrote:
> Remove this, it's not used.

Done.

https://codereview.chromium.org/666533007/diff/340001/chrome/browser/ui/andro...
chrome/browser/ui/android/javascript_app_modal_dialog_android.cc:22: using
content::BrowserThread;
On 2014/11/05 03:53:54, msw wrote:
> nit: inline for the one usage.

Done.

https://codereview.chromium.org/666533007/diff/340001/chrome/browser/ui/views...
File chrome/browser/ui/views/chrome_javascript_native_dialog_factory.h (right):

https://codereview.chromium.org/666533007/diff/340001/chrome/browser/ui/views...
chrome/browser/ui/views/chrome_javascript_native_dialog_factory.h:1: //
Copyright 2014 The Chromium Authors. All rights reserved.
On 2014/11/05 03:53:55, msw wrote:
> Is this file actually needed? I don't think you even added it to a gyp target.
> Isn't
> chrome/browser/ui/app_modal_dialogs/chrome_javascript_native_dialog_factory.h
> sufficient?

This was supposed to be removed. thanks you for the catch.

https://codereview.chromium.org/666533007/diff/340001/chrome/browser/ui/views...
File chrome/browser/ui/views/chrome_javascript_native_dialog_factory_views.cc
(right):

https://codereview.chromium.org/666533007/diff/340001/chrome/browser/ui/views...
chrome/browser/ui/views/chrome_javascript_native_dialog_factory_views.cc:9:
#include
"components/app_modal_dialogs/views/javascript_app_modal_dialog_views.h"
On 2014/11/05 03:53:55, msw wrote:
> Should this be in an #else with the x11 header below?

Done.

https://codereview.chromium.org/666533007/diff/340001/chrome/browser/ui/views...
chrome/browser/ui/views/chrome_javascript_native_dialog_factory_views.cc:11:
#include "ui/views/widget/widget.h"
On 2014/11/05 03:53:55, msw wrote:
> Remove this, it's not used.

Done.

https://codereview.chromium.org/666533007/diff/340001/chrome/browser/ui/views...
chrome/browser/ui/views/chrome_javascript_native_dialog_factory_views.cc:30:
JavaScriptAppModalDialogViews* d =
On 2014/11/05 03:53:55, msw wrote:
> nit: move the decl of |d| out of the pre-processor blocks.

Done.

https://codereview.chromium.org/666533007/diff/340001/chrome/browser/ui/views...
File chrome/browser/ui/views/update_recommended_message_box.cc (right):

https://codereview.chromium.org/666533007/diff/340001/chrome/browser/ui/views...
chrome/browser/ui/views/update_recommended_message_box.cc:27:
parent_window)->Show();
On 2014/11/05 03:53:55, msw wrote:
> nit: fix indent

Done.

https://codereview.chromium.org/666533007/diff/340001/chrome/chrome_browser_u...
File chrome/chrome_browser_ui.gypi (right):

https://codereview.chromium.org/666533007/diff/340001/chrome/chrome_browser_u...
chrome/chrome_browser_ui.gypi:21:
'browser/ui/android/chrome_javascript_native_dialog_factory_android.cc',
On 2014/11/05 03:53:55, msw wrote:
> Should these gypi changes also be made to a GN file somewhere?
> (not sure it exists yet, but checking just in case)

This change gets automatically propagated to GN via
chrome_browser_ui_non_ios_soruces defined above.

https://codereview.chromium.org/666533007/diff/340001/components/app_modal_di...
File components/app_modal_dialogs.gypi (right):

https://codereview.chromium.org/666533007/diff/340001/components/app_modal_di...
components/app_modal_dialogs.gypi:40: ['toolkit_views==1 and OS != "mac"', {
On 2014/11/05 03:53:55, msw wrote:
> nit: be consistent about equality operator spacing.

Done.

https://codereview.chromium.org/666533007/diff/340001/components/app_modal_di...
File components/app_modal_dialogs/cocoa/DEPS (right):

https://codereview.chromium.org/666533007/diff/340001/components/app_modal_di...
components/app_modal_dialogs/cocoa/DEPS:1: include_rules = [
On 2014/11/05 03:53:55, msw wrote:
> Ditto: Do DEPS apply to parent directories? (this dir contains no other files)

This should be gone too (this is leftover from attempt to move android code.)

https://codereview.chromium.org/666533007/diff/340001/components/app_modal_di...
File components/app_modal_dialogs/javascript_app_modal_dialog.cc (right):

https://codereview.chromium.org/666533007/diff/340001/components/app_modal_di...
components/app_modal_dialogs/javascript_app_modal_dialog.cc:9: #include
"components/app_modal_dialogs/native_app_modal_dialog.h"
On 2014/11/05 03:53:55, msw wrote:
> I don't think this include is needed any more.

Done.

https://codereview.chromium.org/666533007/diff/340001/components/app_modal_di...
File components/app_modal_dialogs/javascript_dialog_manager.cc (right):

https://codereview.chromium.org/666533007/diff/340001/components/app_modal_di...
components/app_modal_dialogs/javascript_dialog_manager.cc:5: #include
"components/app_modal_dialogs/javascript_dialog_manager.h"
On 2014/11/05 03:53:55, msw wrote:
> You can remove this file and let clients use the Impl class via
> javascript_dialog_manager_impl.h directly (and rename that to drop "Impl").

I did ask this question in #20, and avi@ seems to be ok with this.

There is already content::JavaScriptDialogManager, so I want to avoid having the
class with same name
even if it's in a separate namespace.

I'm happy to change to whatever you and avi@ agrees though. avi@, what is your
opinion?

https://codereview.chromium.org/666533007/diff/340001/components/app_modal_di...
components/app_modal_dialogs/javascript_dialog_manager.cc:7: #include
"base/memory/scoped_ptr.h"
On 2014/11/05 03:53:55, msw wrote:
> nit: the header includes this, so it's not needed here.

Done.

https://codereview.chromium.org/666533007/diff/340001/components/app_modal_di...
components/app_modal_dialogs/javascript_dialog_manager.cc:18:
JavaScriptDialogManagerImpl::GetInstance()->SetExtensionsClient(
On 2014/11/05 03:53:55, msw wrote:
> nit: use GetJavaScriptDialogManagerInstance.

The type needs to be Impl. (The method above returns
content::JavaScriptDialogManager*)

https://codereview.chromium.org/666533007/diff/340001/components/app_modal_di...
components/app_modal_dialogs/javascript_dialog_manager.cc:22: void
SetJavaScriptNativeDialogFactory(
On 2014/11/05 03:53:55, msw wrote:
> nit: reorder to match declarations (if you keep this file).

Done.

https://codereview.chromium.org/666533007/diff/340001/components/app_modal_di...
components/app_modal_dialogs/javascript_dialog_manager.cc:24:
JavaScriptDialogManagerImpl::GetInstance()->SetNativeDialogFactory(
On 2014/11/05 03:53:55, msw wrote:
> ditto nit: use GetJavaScriptDialogManagerInstance.

same here

https://codereview.chromium.org/666533007/diff/340001/components/app_modal_di...
File components/app_modal_dialogs/javascript_dialog_manager_impl.cc (right):

https://codereview.chromium.org/666533007/diff/340001/components/app_modal_di...
components/app_modal_dialogs/javascript_dialog_manager_impl.cc:16: #include
"components/app_modal_dialogs/native_app_modal_dialog.h"
On 2014/11/05 03:53:56, msw wrote:
> I don't think this include is needed anymore.

this is used to access native_dialog in HandleJavaScriptDialog

https://codereview.chromium.org/666533007/diff/340001/components/app_modal_di...
components/app_modal_dialogs/javascript_dialog_manager_impl.cc:24: using
content::BrowserContext;
On 2014/11/05 03:53:55, msw wrote:
> Remove this; the type is no longer used here.

Done.

https://codereview.chromium.org/666533007/diff/340001/components/app_modal_di...
components/app_modal_dialogs/javascript_dialog_manager_impl.cc:25: using
content::JavaScriptDialogManager;
On 2014/11/05 03:53:56, msw wrote:
> Remove this; the type is no longer used here.

Done.

https://codereview.chromium.org/666533007/diff/340001/components/app_modal_di...
components/app_modal_dialogs/javascript_dialog_manager_impl.cc:26: using
content::WebContents;
On 2014/11/05 03:53:56, msw wrote:
> nit: explicitly use "content::" and remove this, or remove "content::" from
the
> three new WebContents uses.

Done.

https://codereview.chromium.org/666533007/diff/340001/components/app_modal_di...
components/app_modal_dialogs/javascript_dialog_manager_impl.cc:55:
JavaScriptDialogManagerImpl::JavaScriptDialogManagerImpl()
On 2014/11/05 03:53:56, msw wrote:
> nit: reorder to match decl.

Done.

https://codereview.chromium.org/666533007/diff/340001/components/app_modal_di...
components/app_modal_dialogs/javascript_dialog_manager_impl.cc:59:
JavaScriptDialogManagerImpl::~JavaScriptDialogManagerImpl() {
On 2014/11/05 03:53:56, msw wrote:
> nit: reorder to match decl.

Done.

https://codereview.chromium.org/666533007/diff/340001/components/app_modal_di...
components/app_modal_dialogs/javascript_dialog_manager_impl.cc:204: if
(extensions_client_->GetExtensionName(web_contents, origin_url, &name)) {
On 2014/11/05 03:53:55, msw wrote:
> nit: remove curly braces.

Done.

https://codereview.chromium.org/666533007/diff/340001/components/app_modal_di...
File components/app_modal_dialogs/javascript_native_dialog_factory.h (right):

https://codereview.chromium.org/666533007/diff/340001/components/app_modal_di...
components/app_modal_dialogs/javascript_native_dialog_factory.h:8: #include
<string>
On 2014/11/05 03:53:56, msw wrote:
> Remove this, it's not used.

Done.

https://codereview.chromium.org/666533007/diff/340001/components/app_modal_di...
components/app_modal_dialogs/javascript_native_dialog_factory.h:10: #include
"base/memory/scoped_ptr.h"
On 2014/11/05 03:53:56, msw wrote:
> Remove this, it's not used.

Done.

https://codereview.chromium.org/666533007/diff/340001/components/app_modal_di...
components/app_modal_dialogs/javascript_native_dialog_factory.h:13: class GURL;
On 2014/11/05 03:53:56, msw wrote:
> Remove this, it's not used.

Done.

https://codereview.chromium.org/666533007/diff/340001/components/constrained_...
File components/constrained_window/constrained_window_views.cc (right):

https://codereview.chromium.org/666533007/diff/340001/components/constrained_...
components/constrained_window/constrained_window_views.cc:155: views::Widget*
CreateAppModalDialogViews(views::DialogDelegate* dialog,
On 2014/11/05 03:53:56, msw wrote:
> This tangential function renaming affects too many files to be part of such a
> large, complex CL; this really should be landed first to simplify the review
of
> the actual componentization changes.

This was to keep the name consistent with c14n change + comment, and I didn't
expect this is controversial. I agree that this should be done separately now.
Removed these changes.

https://codereview.chromium.org/666533007/diff/340001/components/constrained_...
components/constrained_window/constrained_window_views.cc:156: gfx::NativeWindow
parent) {
On 2014/11/05 03:53:56, msw wrote:
> nit: fix indent.

Done.

https://codereview.chromium.org/666533007/diff/340001/extensions/components/e...
File extensions/components/extensions_components.gyp (right):

https://codereview.chromium.org/666533007/diff/340001/extensions/components/e...
extensions/components/extensions_components.gyp:12: 'includes': [
On 2014/11/05 03:53:56, msw wrote:
> You need an extensions/OWNERS reviewer.

Will do. I first wanted to get reviews from avi and you.

https://codereview.chromium.org/666533007/diff/340001/extensions/components/j...
File extensions/components/javascript_dialog_extensions_client/BUILD.gn (right):

https://codereview.chromium.org/666533007/diff/340001/extensions/components/j...
extensions/components/javascript_dialog_extensions_client/BUILD.gn:11: deps = [
On 2014/11/05 03:53:56, msw wrote:
> Does this need a dependency for javascript_dialog_extension_client_impl.cc's
> "content/public/browser/web_contents.h" include?

Binary dependency is pulled from extensions. I can add if you want as it's
harmless. let me know.

https://codereview.chromium.org/666533007/diff/340001/extensions/components/j...
File
extensions/components/javascript_dialog_extensions_client/javascript_dialog_extension_client_impl.cc
(right):

https://codereview.chromium.org/666533007/diff/340001/extensions/components/j...
extensions/components/javascript_dialog_extensions_client/javascript_dialog_extension_client_impl.cc:40:
content::WebContents* web_contents) override;
On 2014/11/05 03:53:56, msw wrote:
> nit: you can inline these definitions if the class is declared here.

Done.

https://codereview.chromium.org/666533007/diff/340001/extensions/components/j...
extensions/components/javascript_dialog_extensions_client/javascript_dialog_extension_client_impl.cc:40:
content::WebContents* web_contents) override;
On 2014/11/05 03:53:56, msw wrote:
> nit: you can inline these definitions if the class is declared here.

Done.

[Avi (use Gerrit), 2014-11-05 21:50:40]

https://codereview.chromium.org/666533007/diff/280001/components/constrained_...
File components/constrained_window/constrained_window_views.h (right):

https://codereview.chromium.org/666533007/diff/280001/components/constrained_...
components/constrained_window/constrained_window_views.h:54: views::Widget*
CreateWindowModalDialogViews(views::DialogDelegate* dialog,
On 2014/11/05 21:42:17, oshima wrote:
> On 2014/11/05 03:53:54, msw wrote:
> > On 2014/11/04 23:12:18, oshima wrote:
> > > On 2014/11/04 20:46:41, Avi wrote:
> > > > You talk about this in the CL description:
> > > > 
> > > > > Renamed CreateBrowserModalDialogViews to CreateWindowModalDialogViews
> > > because
> > > > it has nothing to do with Browser any more.
> > > > 
> > > > CreateBrowserModalDialogViews never had anything to do about the
browser.
> It
> > > was
> > > > about being "browser-modal", or in other words, "app-modal". Your new
name
> > > > implies that it's window-modal. Did you change the modality of the
dialog?
> > If
> > > > not, and if you don't like the old name because it has "browser" in it,
> pick
> > a
> > > > new one that doesn't imply incorrect modality semantics.
> > > > 
> > > > (Note the use of "app modal" already in places like
> > > > "components/app_modal_dialogs".)
> > > 
> > > I see. I was confused by the comment. I changed to
CreateAppModalDialogViews
> > and
> > > update both comment and the description.
> > > 
> > > App can be confusing too (as we have chrome apps), but I couldn't come up
> with
> > > better name.
> > 
> > I wonder if "SystemModal" might be appropriate here (and to rename
> > [JavaScript|Native]SystemModalDialog), to better match ui::ModalType,
although
> > I'm not entirely opposed to this name (as in "the modality extends to all
> > application instances in the system" i guess?) Maybe ui::ModalType's
> > MODAL_TYPE_SYSTEM should change to MODAL_TYPE_APP?
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/ui_base_ty...
> > 
> 
> Looks like we have inconsistent use of app/system. I actually thought about
> "system modal", but I didn't pick it because while this is true on chromeos,
> it's not really "system wide" modal on other platforms. ProcessModal is
another
> candidate, although I sounds ugly to me.
> 
> In any case, if this is controversal, I'll do that in a separate CL.
> 
> I'm happy to rename whatever you and avi@ agrees on though. avi@, what is your
> suggestion?

I made my suggestion for "AppModal". I don't like SystemModal due to the
implication that it's across the entire system and all running apps.

https://codereview.chromium.org/666533007/diff/340001/chrome/browser/ui/andro...
File
chrome/browser/ui/android/chrome_javascript_native_dialog_factory_android.cc
(right):

https://codereview.chromium.org/666533007/diff/340001/chrome/browser/ui/andro...
chrome/browser/ui/android/chrome_javascript_native_dialog_factory_android.cc:1:
// Copyright 2014 The Chromium Authors. All rights reserved.
On 2014/11/05 21:42:18, oshima wrote:
> On 2014/11/05 03:53:54, msw wrote:
> > Perhaps everything in this file could still live in
> > javascript_app_modal_dialog_android.cc?
> 
> I put this in separate file because I thought it's a bit too big to mix it
into
> that file, but
> I don't have strong opinion.
> 
> avi@, are you ok with this?

I don't have strong opinions and would be OK either way.

[msw, 2014-11-05 22:59:06]

lgtm iff you remove the bad file:
  components/app_modal_dialogs/cocoa/DEPS.

Please also remove the CL description comment about renaming
CreateBrowserModalDialogViews and see my other nits.

https://codereview.chromium.org/666533007/diff/340001/chrome/browser/ui/andro...
File
chrome/browser/ui/android/chrome_javascript_native_dialog_factory_android.cc
(right):

https://codereview.chromium.org/666533007/diff/340001/chrome/browser/ui/andro...
chrome/browser/ui/android/chrome_javascript_native_dialog_factory_android.cc:1:
// Copyright 2014 The Chromium Authors. All rights reserved.
On 2014/11/05 21:50:40, Avi wrote:
> On 2014/11/05 21:42:18, oshima wrote:
> > On 2014/11/05 03:53:54, msw wrote:
> > > Perhaps everything in this file could still live in
> > > javascript_app_modal_dialog_android.cc?
> > 
> > I put this in separate file because I thought it's a bit too big to mix it
> into
> > that file, but
> > I don't have strong opinion.
> > 
> > avi@, are you ok with this?
> 
> I don't have strong opinions and would be OK either way.

I slightly favor combining the files, but don't have a strong opinion.
You can leave this as-is for now and consider a followup cleanup CL.

https://codereview.chromium.org/666533007/diff/340001/components/app_modal_di...
File components/app_modal_dialogs/javascript_dialog_manager.cc (right):

https://codereview.chromium.org/666533007/diff/340001/components/app_modal_di...
components/app_modal_dialogs/javascript_dialog_manager.cc:5: #include
"components/app_modal_dialogs/javascript_dialog_manager.h"
On 2014/11/05 21:42:18, oshima wrote:
> On 2014/11/05 03:53:55, msw wrote:
> > You can remove this file and let clients use the Impl class via
> > javascript_dialog_manager_impl.h directly (and rename that to drop "Impl").
> 
> I did ask this question in #20, and avi@ seems to be ok with this.
> 
> There is already content::JavaScriptDialogManager, so I want to avoid having
the
> class with same name
> even if it's in a separate namespace.
> 
> I'm happy to change to whatever you and avi@ agrees though. avi@, what is your
> opinion?
> 

Avi missed this question, and I don't have a strong opinion. You can leave it
as-is for now and consider removing this file in a followup cleanup CL.

https://codereview.chromium.org/666533007/diff/340001/extensions/components/j...
File extensions/components/javascript_dialog_extensions_client/BUILD.gn (right):

https://codereview.chromium.org/666533007/diff/340001/extensions/components/j...
extensions/components/javascript_dialog_extensions_client/BUILD.gn:11: deps = [
On 2014/11/05 21:42:19, oshima wrote:
> On 2014/11/05 03:53:56, msw wrote:
> > Does this need a dependency for javascript_dialog_extension_client_impl.cc's
> > "content/public/browser/web_contents.h" include?
> 
> Binary dependency is pulled from extensions. I can add if you want as it's
> harmless. let me know.

Sure, it doesn't hurt to explicitly list dependencies used directly.

https://codereview.chromium.org/666533007/diff/360001/chrome/browser/ui/views...
File chrome/browser/ui/views/chrome_javascript_native_dialog_factory_views.cc
(right):

https://codereview.chromium.org/666533007/diff/360001/chrome/browser/ui/views...
chrome/browser/ui/views/chrome_javascript_native_dialog_factory_views.cc:29:
JavaScriptAppModalDialogViews* d =
nit: it'd be slightly cleaner to do "d = nullptr;" in this line and "d = new
JavaSc...;" in the lines below.

https://codereview.chromium.org/666533007/diff/360001/components/app_modal_di...
File components/app_modal_dialogs/cocoa/DEPS (right):

https://codereview.chromium.org/666533007/diff/360001/components/app_modal_di...
components/app_modal_dialogs/cocoa/DEPS:1: include_rules = [
Please remove this file! As per the earlier comment:

https://codereview.chromium.org/666533007/diff/340001/components/app_modal_di...

https://codereview.chromium.org/666533007/diff/360001/components/constrained_...
File components/constrained_window/constrained_window_views.cc (right):

https://codereview.chromium.org/666533007/diff/360001/components/constrained_...
components/constrained_window/constrained_window_views.cc:156: gfx::NativeWindow
parent) {
nit: fix this indent.

[oshima, 2014-11-05 23:17:50]

Patchset #9 (id:380001) has been deleted

[oshima, 2014-11-05 23:21:30]

Patchset #9 (id:400001) has been deleted

[oshima, 2014-11-05 23:22:49]

msw@ PTAL

avi@, what is your opinion about msw@'s comment for
components/app_modal_dialogs/javascript_dialog_manager.h ?

https://codereview.chromium.org/666533007/diff/340001/chrome/browser/ui/andro...
File
chrome/browser/ui/android/chrome_javascript_native_dialog_factory_android.cc
(right):

https://codereview.chromium.org/666533007/diff/340001/chrome/browser/ui/andro...
chrome/browser/ui/android/chrome_javascript_native_dialog_factory_android.cc:1:
// Copyright 2014 The Chromium Authors. All rights reserved.
On 2014/11/05 22:59:06, msw wrote:
> On 2014/11/05 21:50:40, Avi wrote:
> > On 2014/11/05 21:42:18, oshima wrote:
> > > On 2014/11/05 03:53:54, msw wrote:
> > > > Perhaps everything in this file could still live in
> > > > javascript_app_modal_dialog_android.cc?
> > > 
> > > I put this in separate file because I thought it's a bit too big to mix it
> > into
> > > that file, but
> > > I don't have strong opinion.
> > > 
> > > avi@, are you ok with this?
> > 
> > I don't have strong opinions and would be OK either way.
> 
> I slightly favor combining the files, but don't have a strong opinion.
> You can leave this as-is for now and consider a followup cleanup CL.

I've moved to javascript_app_modal_dialog_android.cc

https://codereview.chromium.org/666533007/diff/340001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/chrome_javascript_native_dialog_factory_cocoa.mm
(right):

https://codereview.chromium.org/666533007/diff/340001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/chrome_javascript_native_dialog_factory_cocoa.mm:1: //
Copyright 2014 The Chromium Authors. All rights reserved.
On 2014/11/05 03:53:55, msw wrote:
> Ditto: Perhaps everything in this file could still live in
> javascript_app_modal_dialog_cocoa.mm?

Done.

https://codereview.chromium.org/666533007/diff/340001/chrome/browser/ui/views...
File chrome/browser/ui/views/chrome_javascript_native_dialog_factory_views.cc
(right):

https://codereview.chromium.org/666533007/diff/340001/chrome/browser/ui/views...
chrome/browser/ui/views/chrome_javascript_native_dialog_factory_views.cc:1: //
Copyright 2014 The Chromium Authors. All rights reserved.
On 2014/11/05 03:53:55, msw wrote:
> Ditto: Perhaps everything in this file could still live in
> javascript_app_modal_dialog_views.cc?

This needs to stay because there is no javascript_app_modal_dialog_views under
chrome/browser.

https://codereview.chromium.org/666533007/diff/340001/extensions/components/j...
File extensions/components/javascript_dialog_extensions_client/BUILD.gn (right):

https://codereview.chromium.org/666533007/diff/340001/extensions/components/j...
extensions/components/javascript_dialog_extensions_client/BUILD.gn:11: deps = [
On 2014/11/05 22:59:06, msw wrote:
> On 2014/11/05 21:42:19, oshima wrote:
> > On 2014/11/05 03:53:56, msw wrote:
> > > Does this need a dependency for
javascript_dialog_extension_client_impl.cc's
> > > "content/public/browser/web_contents.h" include?
> > 
> > Binary dependency is pulled from extensions. I can add if you want as it's
> > harmless. let me know.
> 
> Sure, it doesn't hurt to explicitly list dependencies used directly.

Done.

https://codereview.chromium.org/666533007/diff/360001/chrome/browser/ui/views...
File chrome/browser/ui/views/chrome_javascript_native_dialog_factory_views.cc
(right):

https://codereview.chromium.org/666533007/diff/360001/chrome/browser/ui/views...
chrome/browser/ui/views/chrome_javascript_native_dialog_factory_views.cc:29:
JavaScriptAppModalDialogViews* d =
On 2014/11/05 22:59:06, msw wrote:
> nit: it'd be slightly cleaner to do "d = nullptr;" in this line and "d = new
> JavaSc...;" in the lines below.

Done.

https://codereview.chromium.org/666533007/diff/360001/components/app_modal_di...
File components/app_modal_dialogs/cocoa/DEPS (right):

https://codereview.chromium.org/666533007/diff/360001/components/app_modal_di...
components/app_modal_dialogs/cocoa/DEPS:1: include_rules = [
On 2014/11/05 22:59:06, msw wrote:
> Please remove this file! As per the earlier comment:
> 
>
https://codereview.chromium.org/666533007/diff/340001/components/app_modal_di...

Done.

https://codereview.chromium.org/666533007/diff/360001/components/constrained_...
File components/constrained_window/constrained_window_views.cc (right):

https://codereview.chromium.org/666533007/diff/360001/components/constrained_...
components/constrained_window/constrained_window_views.cc:156: gfx::NativeWindow
parent) {
On 2014/11/05 22:59:06, msw wrote:
> nit: fix this indent.

reverted.

[msw, 2014-11-05 23:34:37]

lgtm with nits.

https://codereview.chromium.org/666533007/diff/340001/chrome/browser/ui/views...
File chrome/browser/ui/views/chrome_javascript_native_dialog_factory_views.cc
(right):

https://codereview.chromium.org/666533007/diff/340001/chrome/browser/ui/views...
chrome/browser/ui/views/chrome_javascript_native_dialog_factory_views.cc:1: //
Copyright 2014 The Chromium Authors. All rights reserved.
On 2014/11/05 23:22:48, oshima wrote:
> On 2014/11/05 03:53:55, msw wrote:
> > Ditto: Perhaps everything in this file could still live in
> > javascript_app_modal_dialog_views.cc?
> 
> This needs to stay because there is no javascript_app_modal_dialog_views under
> chrome/browser.

Acknowledged.

https://codereview.chromium.org/666533007/diff/420001/chrome/browser/ui/andro...
File chrome/browser/ui/android/javascript_app_modal_dialog_android.cc (right):

https://codereview.chromium.org/666533007/diff/420001/chrome/browser/ui/andro...
chrome/browser/ui/android/javascript_app_modal_dialog_android.cc:9: #include
"chrome/browser/ui/android/javascript_app_modal_dialog_android.h"
nit: Remove redundant include (same as first).

https://codereview.chromium.org/666533007/diff/420001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/javascript_app_modal_dialog_cocoa.mm (right):

https://codereview.chromium.org/666533007/diff/420001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/javascript_app_modal_dialog_cocoa.mm:15: #include
"chrome/browser/ui/cocoa/javascript_app_modal_dialog_cocoa.h"
nit: Remove redundant include (same as first).

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

i'll address javascript_dialog_manager.{h|cc} in a separate CL if necessary.

https://codereview.chromium.org/666533007/diff/420001/chrome/browser/ui/andro...
File chrome/browser/ui/android/javascript_app_modal_dialog_android.cc (right):

https://codereview.chromium.org/666533007/diff/420001/chrome/browser/ui/andro...
chrome/browser/ui/android/javascript_app_modal_dialog_android.cc:9: #include
"chrome/browser/ui/android/javascript_app_modal_dialog_android.h"
On 2014/11/05 23:34:37, msw wrote:
> nit: Remove redundant include (same as first).

Done.

https://codereview.chromium.org/666533007/diff/420001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/javascript_app_modal_dialog_cocoa.mm (right):

https://codereview.chromium.org/666533007/diff/420001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/javascript_app_modal_dialog_cocoa.mm:15: #include
"chrome/browser/ui/cocoa/javascript_app_modal_dialog_cocoa.h"
On 2014/11/05 23:34:37, msw wrote:
> nit: Remove redundant include (same as first).

Done.

[oshima, 2014-11-06 00:04:39]

The CQ bit was checked by oshima@chromium.org

[commit-bot: I haz the power, 2014-11-06 00:05:17]

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

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

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

[commit-bot: I haz the power, 2014-11-06 00:15:23]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[oshima, 2014-11-06 00:36:30]

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

[oshima, 2014-11-06 00:43:34]

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

[oshima, 2014-11-06 00:48:46]

yoz, scheib, can one of you review extensions/components ?

[Yoyo Zhou, 2014-11-06 01:05:03]

LGTM with nits

https://chromiumcodereview.appspot.com/666533007/diff/440001/components/app_m...
File components/app_modal_dialogs/javascript_dialog_extensions_client.h (right):

https://chromiumcodereview.appspot.com/666533007/diff/440001/components/app_m...
components/app_modal_dialogs/javascript_dialog_extensions_client.h:25: virtual
void IncrementLazyKeepaliveCount(
I think I'd prefer if these were called, e.g. OnDialogOpened and OnDialogClosed.
The keepalive count is really an implementation detail.

https://chromiumcodereview.appspot.com/666533007/diff/440001/components/app_m...
components/app_modal_dialogs/javascript_dialog_extensions_client.h:35: // Sets
the name of the extensions associated with the |web_contents|
The grammar here is a little unusual. How about:
Sets the name of the extension associated with the |web_contents| in the
|name_out| if there is one, returning true; returns false otherwise.

https://chromiumcodereview.appspot.com/666533007/diff/440001/extensions/compo...
File
extensions/components/javascript_dialog_extensions_client/javascript_dialog_extension_client_impl.cc
(right):

https://chromiumcodereview.appspot.com/666533007/diff/440001/extensions/compo...
extensions/components/javascript_dialog_extensions_client/javascript_dialog_extension_client_impl.cc:19:
extensions::ProcessManager* GetExtensionsProcessManager(
nit: I'd drop the Extensions from this function name.

[oshima, 2014-11-06 01:38:50]

oshima@chromium.org changed reviewers:
+ thestig@chromium.org
- scheib@chromium.org

[oshima, 2014-11-06 01:39:19]

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

[oshima, 2014-11-06 01:47:44]

thestig@, can you approve changes in 

chrome/browser/chrome_browser_main.cc
chrome/browser/ui/android/javascript_app_modal_dialog_android.cc

?

tbrin'g blundel for components/BUILD.gn

https://codereview.chromium.org/666533007/diff/440001/components/app_modal_di...
File components/app_modal_dialogs/javascript_dialog_extensions_client.h (right):

https://codereview.chromium.org/666533007/diff/440001/components/app_modal_di...
components/app_modal_dialogs/javascript_dialog_extensions_client.h:25: virtual
void IncrementLazyKeepaliveCount(
On 2014/11/06 01:05:03, Yoyo Zhou wrote:
> I think I'd prefer if these were called, e.g. OnDialogOpened and
OnDialogClosed.
> The keepalive count is really an implementation detail.

Done.

https://codereview.chromium.org/666533007/diff/440001/components/app_modal_di...
components/app_modal_dialogs/javascript_dialog_extensions_client.h:35: // Sets
the name of the extensions associated with the |web_contents|
On 2014/11/06 01:05:02, Yoyo Zhou wrote:
> The grammar here is a little unusual. How about:
> Sets the name of the extension associated with the |web_contents| in the
> |name_out| if there is one, returning true; returns false otherwise.

Done.

[Lei Zhang, 2014-11-06 03:47:38]

On 2014/11/06 01:47:44, oshima wrote:
> thestig@, can you approve changes in 
> 
> chrome/browser/chrome_browser_main.cc
> chrome/browser/ui/android/javascript_app_modal_dialog_android.cc

lgtm with nit

https://codereview.chromium.org/666533007/diff/460001/chrome/browser/chrome_b...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/666533007/diff/460001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_main.cc:200: #if !defined(OS_IOS)
nit: Group the OS_FOO #ifdefs above the DISABLE/ENABLE ones.

[Avi (use Gerrit), 2014-11-06 05:26:57]

https://codereview.chromium.org/666533007/diff/340001/components/app_modal_di...
File components/app_modal_dialogs/javascript_dialog_manager.cc (right):

https://codereview.chromium.org/666533007/diff/340001/components/app_modal_di...
components/app_modal_dialogs/javascript_dialog_manager.cc:5: #include
"components/app_modal_dialogs/javascript_dialog_manager.h"
Hmmm. Yes, I did miss this.

Impl makes sense in something like content, where there is a public/ directory
with an interface, and a private implementation. What does an impl class mean
for a component? Is it meaningful at all?

I'm completely OK with two classes named the same thing in different namespaces,
if it makes sense, and I think that it easily makes sense here.

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

https://codereview.chromium.org/666533007/diff/340001/components/app_modal_di...
File components/app_modal_dialogs/javascript_dialog_manager.cc (right):

https://codereview.chromium.org/666533007/diff/340001/components/app_modal_di...
components/app_modal_dialogs/javascript_dialog_manager.cc:5: #include
"components/app_modal_dialogs/javascript_dialog_manager.h"
On 2014/11/06 05:26:57, Avi wrote:
> Hmmm. Yes, I did miss this.
> 
> Impl makes sense in something like content, where there is a public/ directory
> with an interface, and a private implementation. What does an impl class mean
> for a component? Is it meaningful at all? 

JavaScriptDialogManager is an interface in public directory
(content/public/browser),
and this provides the implementation of it. This used to be
ChromeJavaScriptDialogManager when it was in
chrome/browser, and I just needed some name to replace it.

> I'm completely OK with two classes named the same thing in different
namespaces,
> if it makes sense, and I think that it easily makes sense here.

I'm happy to do that if that's what you want, but let me to do it in a separate
CL.

https://codereview.chromium.org/666533007/diff/460001/chrome/browser/chrome_b...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/666533007/diff/460001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_main.cc:200: #if !defined(OS_IOS)
On 2014/11/06 03:47:38, Lei Zhang wrote:
> nit: Group the OS_FOO #ifdefs above the DISABLE/ENABLE ones.

Done.

[oshima, 2014-11-06 06:32:25]

The CQ bit was checked by oshima@chromium.org

[commit-bot: I haz the power, 2014-11-06 06:32:51]

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

[oshima, 2014-11-06 06:53:14]

Patchset #13 (id:500001) has been deleted

[blundell, 2014-11-06 06:55:36]

I'm a little confused by this CL. It doesn't seem to have anything I need to
review, and additionally it doesn't seem to be doing exactly what the CL
description says (as far as I can see, JavaScriptDialogManager is not being
moved from //chrome in this CL).

[blundell, 2014-11-06 06:56:55]

On 2014/11/06 06:55:36, blundell wrote:
> I'm a little confused by this CL. It doesn't seem to have anything I need to
> review, and additionally it doesn't seem to be doing exactly what the CL
> description says (as far as I can see, JavaScriptDialogManager is not being
> moved from //chrome in this CL).

nm, there was some weirdness with the display where I wasn't seeing the whole CL
on rietveld.

[blundell, 2014-11-06 06:58:21]

//components/BUILD.gn lgtm

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

The CQ bit was checked by oshima@chromium.org

[commit-bot: I haz the power, 2014-11-06 10:54:43]

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

[commit-bot: I haz the power, 2014-11-06 10:56:00]

Committed patchset #12 (id:480001)

[commit-bot: I haz the power, 2014-11-06 10:56:46]

Patchset 12 (id:??) landed as
https://crrev.com/758abebc6360e6c4bad50acd851a0c41c85695ac
Cr-Commit-Position: refs/heads/master@{#303004}

[Jiang Jiang, 2014-11-18 13:03:00]

jiangj@opera.com changed reviewers:
+ jiangj@opera.com

[Jiang Jiang, 2014-11-18 13:03:05]

https://codereview.chromium.org/666533007/diff/480001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/javascript_app_modal_dialog_cocoa.mm (right):

https://codereview.chromium.org/666533007/diff/480001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/javascript_app_modal_dialog_cocoa.mm:445:
ChromeJavaScritpNativeDialogCocoaFactory() {}
Typo.

[oshima, 2014-11-18 14:58:20]

On 2014/11/18 13:03:05, Jiang Jiang wrote:
>
https://codereview.chromium.org/666533007/diff/480001/chrome/browser/ui/cocoa...
> File chrome/browser/ui/cocoa/javascript_app_modal_dialog_cocoa.mm (right):
> 
>
https://codereview.chromium.org/666533007/diff/480001/chrome/browser/ui/cocoa...
> chrome/browser/ui/cocoa/javascript_app_modal_dialog_cocoa.mm:445:
> ChromeJavaScritpNativeDialogCocoaFactory() {}
> Typo.

Thank you for the catch. Look like I skipped mm when converting them.