Componentize app_modal_dialog

Componentize app_modal_dialog

Notable changes (other than just moving code aruond)

* kJavaScriptMessageExpectedDelay constant has been moved from chrome_constants to javascript_dialog_manager.cc because that's the only place used.
* NOTIFICATION_APP_MODAL_DIALOG_SHOWN notification has been removed and replaced by test only waiter class. (as this notification was used only in tests)
* Moved and creates string resource for app_modal_dialog.

Next step: move dialog implementations to compoents/app_modal_dialogs (javascript_dialog_manager/  javascript_app_modal_dialogs_views/, javascript_app_modal_dialogs_cocoa ....)

BUG=422092
TBR=sky@chromium.org

Committed: https://crrev.com/82f7248c0e48ee71c4b78f9d305caefc7fa9bab2
Cr-Commit-Position: refs/heads/master@{#301095}

[oshima, 2014-10-10 22:11:37]

Patchset #1 (id:1) has been deleted

[oshima, 2014-10-10 23:39:00]

Patchset #2 (id:530001) has been deleted

[oshima, 2014-10-10 23:39:04]

Patchset #1 (id:460001) has been deleted

[oshima, 2014-10-10 23:41:24]

Patchset #2 (id:690001) has been deleted

[oshima, 2014-10-10 23:41:33]

Patchset #1 (id:650001) has been deleted

[oshima, 2014-10-10 23:45:04]

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

[oshima, 2014-10-10 23:45:16]

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

[oshima, 2014-10-11 00:06:55]

Patchset #1 (id:760001) has been deleted

[oshima, 2014-10-13 16:19:45]

Patchset #4 (id:1120001) has been deleted

[oshima, 2014-10-13 16:19:49]

Patchset #1 (id:230002) has been deleted

[oshima, 2014-10-13 16:19:58]

Patchset #1 (id:890015) has been deleted

[oshima, 2014-10-13 16:20:10]

Patchset #1 (id:100002) has been deleted

[oshima, 2014-10-13 20:02:32]

Patchset #1 (id:1220001) has been deleted

[oshima, 2014-10-13 20:02:40]

Patchset #1 (id:1340001) has been deleted

[oshima, 2014-10-13 20:02:48]

Patchset #2 (id:1650001) has been deleted

[oshima, 2014-10-13 20:02:51]

Patchset #1 (id:1540001) has been deleted

[oshima, 2014-10-14 17:03:21]

Patchset #1 (id:1680001) has been deleted

[oshima, 2014-10-14 17:03:30]

Patchset #2 (id:1850001) has been deleted

[oshima, 2014-10-17 17:24:11]

Patchset #3 (id:1990001) has been deleted

[oshima, 2014-10-19 21:29:46]

Patchset #6 (id:2060001) has been deleted

[oshima, 2014-10-19 21:29:50]

Patchset #4 (id:1500002) has been deleted

[oshima, 2014-10-19 21:29:54]

Patchset #2 (id:1970001) has been deleted

[oshima, 2014-10-19 21:30:03]

Patchset #2 (id:2010001) has been deleted

[oshima, 2014-10-19 21:30:16]

Patchset #2 (id:2040001) has been deleted

[oshima, 2014-10-20 20:00:54]

Patchset #9 (id:2220001) has been deleted

[oshima, 2014-10-21 00:52:40]

Patchset #11 (id:2280001) has been deleted

[oshima, 2014-10-21 00:53:07]

Patchset #1 (id:1780001) has been deleted

[oshima, 2014-10-21 00:53:19]

Patchset #1 (id:2080001) has been deleted

[oshima, 2014-10-21 00:53:33]

Patchset #1 (id:2100001) has been deleted

[oshima, 2014-10-21 00:53:45]

Patchset #1 (id:2120001) has been deleted

[oshima, 2014-10-21 00:53:56]

Patchset #1 (id:2140001) has been deleted

[oshima, 2014-10-21 00:54:05]

Patchset #1 (id:2160001) has been deleted

[oshima, 2014-10-21 00:54:14]

Patchset #1 (id:2180001) has been deleted

[oshima, 2014-10-21 00:54:28]

Patchset #1 (id:2200001) has been deleted

[oshima, 2014-10-21 00:54:35]

Patchset #1 (id:2240001) has been deleted

[oshima, 2014-10-21 00:54:47]

Patchset #1 (id:2260001) has been deleted

[oshima, 2014-10-21 03:05:47]

Patchset #3 (id:2340001) has been deleted

[oshima, 2014-10-21 03:05:51]

Patchset #2 (id:2320001) has been deleted

[oshima, 2014-10-21 16:18:02]

Patchset #1 (id:2300001) has been deleted

[oshima, 2014-10-21 16:18:56]

avi for app_modal_dialog change.
blundell for c14n change.

[Avi (use Gerrit), 2014-10-21 17:14:29]

https://codereview.chromium.org/648833004/diff/2370001/components/app_modal_d...
File components/app_modal_dialogs/app_modal_dialog.cc (left):

https://codereview.chromium.org/648833004/diff/2370001/components/app_modal_d...
components/app_modal_dialogs/app_modal_dialog.cc:35:
chrome::NOTIFICATION_APP_MODAL_DIALOG_SHOWN,
If you are removing the firing of this notification, remove its definition as
well.

https://codereview.chromium.org/648833004/diff/2370001/components/app_modal_d...
File components/app_modal_dialogs/app_modal_dialog.h (right):

https://codereview.chromium.org/648833004/diff/2370001/components/app_modal_d...
components/app_modal_dialogs/app_modal_dialog.h:92: // An interface used to wait
until the modal dialog gets shwon.
s/shwon/shown/

[oshima, 2014-10-21 18:27:15]

https://codereview.chromium.org/648833004/diff/2370001/components/app_modal_d...
File components/app_modal_dialogs/app_modal_dialog.cc (left):

https://codereview.chromium.org/648833004/diff/2370001/components/app_modal_d...
components/app_modal_dialogs/app_modal_dialog.cc:35:
chrome::NOTIFICATION_APP_MODAL_DIALOG_SHOWN,
On 2014/10/21 17:14:29, Avi wrote:
> If you are removing the firing of this notification, remove its definition as
> well.

Looks like I accidentally reverted the change while working no this CL. Done.

https://codereview.chromium.org/648833004/diff/2370001/components/app_modal_d...
File components/app_modal_dialogs/app_modal_dialog.h (right):

https://codereview.chromium.org/648833004/diff/2370001/components/app_modal_d...
components/app_modal_dialogs/app_modal_dialog.h:92: // An interface used to wait
until the modal dialog gets shwon.
On 2014/10/21 17:14:29, Avi wrote:
> s/shwon/shown/

Done.

[Avi (use Gerrit), 2014-10-21 20:10:49]

https://codereview.chromium.org/648833004/diff/2390001/components/app_modal_d...
File components/app_modal_dialogs/app_modal_dialog.h (right):

https://codereview.chromium.org/648833004/diff/2390001/components/app_modal_d...
components/app_modal_dialogs/app_modal_dialog.h:103: };
Usually for something like this we'd have some kind of Observer base class,
Add/Remove functions, and so on. Here, the mechanism of having to subclass it in
order to use it appears unprecedented to me. Why do it this way?

[oshima, 2014-10-21 21:39:45]

https://codereview.chromium.org/648833004/diff/2390001/components/app_modal_d...
File components/app_modal_dialogs/app_modal_dialog.h (right):

https://codereview.chromium.org/648833004/diff/2390001/components/app_modal_d...
components/app_modal_dialogs/app_modal_dialog.h:103: };
On 2014/10/21 20:10:48, Avi wrote:
> Usually for something like this we'd have some kind of Observer base class,
> Add/Remove functions, and so on. Here, the mechanism of having to subclass it
in
> order to use it appears unprecedented to me. Why do it this way?

It's a simplified observer. I can't add Add/Remove to this class because the
object doesn't exist
at the time it starts observing. (This is to wait the modal dialog gets created
and shown)

I decided not to use ObserverList class because I think it's over kill for two
reasons.
1) Test only needs one observer.
2) To use ObserverList class, I need to allocate a global object first.

If you feel strongly about it, I can change it so. Please let me know.

[Avi (use Gerrit), 2014-10-21 23:24:48]

On 2014/10/21 21:39:45, oshima wrote:
>
https://codereview.chromium.org/648833004/diff/2390001/components/app_modal_d...
> File components/app_modal_dialogs/app_modal_dialog.h (right):
> 
>
https://codereview.chromium.org/648833004/diff/2390001/components/app_modal_d...
> components/app_modal_dialogs/app_modal_dialog.h:103: };
> On 2014/10/21 20:10:48, Avi wrote:
> > Usually for something like this we'd have some kind of Observer base class,
> > Add/Remove functions, and so on. Here, the mechanism of having to subclass
it
> in
> > order to use it appears unprecedented to me. Why do it this way?
> 
> It's a simplified observer. I can't add Add/Remove to this class because the
> object doesn't exist
> at the time it starts observing. (This is to wait the modal dialog gets
created
> and shown)
> 
> I decided not to use ObserverList class because I think it's over kill for two
> reasons.
> 1) Test only needs one observer.
> 2) To use ObserverList class, I need to allocate a global object first.
> 
> If you feel strongly about it, I can change it so. Please let me know.

No, I don't feel that strongly about it. LGTM

[blundell, 2014-10-22 13:47:51]

The new component needs OWNERS. One of those OWNERS should review the code going
into the component, and then I can provide an overall //components review.

Thanks,

Colin

[oshima, 2014-10-22 15:51:09]

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

[oshima, 2014-10-22 15:51:10]

+mws for c/b/ui/OWNERS

I added avi as an owner based on the commit history.
Who else should be owner? should I add same owners to this component?

[oshima, 2014-10-22 15:51:36]

On 2014/10/22 15:51:10, oshima wrote:
> +mws for c/b/ui/OWNERS
oops, sorry for typo.

+msw@

> I added avi as an owner based on the commit history.
> Who else should be owner? should I add same owners to this component?

[msw, 2014-10-22 22:53:32]

These need more thorough review from actual [prospective] OWNERS!

https://codereview.chromium.org/648833004/diff/2410001/chrome/browser/chrome_...
File chrome/browser/chrome_notification_types.h (left):

https://codereview.chromium.org/648833004/diff/2410001/chrome/browser/chrome_...
chrome/browser/chrome_notification_types.h:127: // Application-modal dialogs
-----------------------------------------------
nit: if you remove the only actually dialog-related notification here, maybe the
comment should be updated; not sure to what...

https://codereview.chromium.org/648833004/diff/2410001/chrome/browser/lifetim...
File chrome/browser/lifetime/browser_close_manager_browsertest.cc (right):

https://codereview.chromium.org/648833004/diff/2410001/chrome/browser/lifetim...
chrome/browser/lifetime/browser_close_manager_browsertest.cc:79: // Reset before
creation because only one instance of the waiter is allowd.
nit: "allowed"; for one-liner: s/instance of the waiter/waiter instance/

https://codereview.chromium.org/648833004/diff/2410001/chrome/browser/lifetim...
chrome/browser/lifetime/browser_close_manager_browsertest.cc:81: waiter_ =
ui_test_utils::CreateAppModalDialogWaiter();
Why not just have a GetNextDialog-local |waiter|? Better yet, why not just call
ui_test_utils::WaitForAppModalDialog()? You could just scrap
AppModalDialogObserver and have a couple Accept/Cancel local helper functions
like this:

void WaitForAndAcceptDialog() {
  AppModalDialog* dialog = ui_test_utils::WaitForAppModalDialog();
  EXPECT_TRUE(dialog->IsJavaScriptModalDialog());
  dialog->native_dialog()->AcceptAppModalDialog();
}

https://codereview.chromium.org/648833004/diff/2410001/chrome/browser/ui/app_...
File chrome/browser/ui/app_modal_dialogs/javascript_dialog_manager.cc (right):

https://codereview.chromium.org/648833004/diff/2410001/chrome/browser/ui/app_...
chrome/browser/ui/app_modal_dialogs/javascript_dialog_manager.cc:41: const int
kJavaScriptMessageExpectedDelay = 1000;
optional nit: move this within the function's scope.

https://codereview.chromium.org/648833004/diff/2410001/chrome/browser/ui/view...
File chrome/browser/ui/views/javascript_app_modal_dialog_views.cc (right):

https://codereview.chromium.org/648833004/diff/2410001/chrome/browser/ui/view...
chrome/browser/ui/views/javascript_app_modal_dialog_views.cc:9: #include
"chrome/grit/generated_resources.h"
nit: I think you can remove this include.

https://codereview.chromium.org/648833004/diff/2410001/chrome/chrome_browser_...
File chrome/chrome_browser_ui.gypi (right):

https://codereview.chromium.org/648833004/diff/2410001/chrome/chrome_browser_...
chrome/chrome_browser_ui.gypi:2632:
'../components/components.gyp:app_modal_dialogs',
nit: sort this alphabetically

https://codereview.chromium.org/648833004/diff/2410001/chrome/test/base/ui_te...
File chrome/test/base/ui_test_utils.cc (right):

https://codereview.chromium.org/648833004/diff/2410001/chrome/test/base/ui_te...
chrome/test/base/ui_test_utils.cc:110: class AppModalDialogWaiterImpl : public
AppModalDialogWaiter {
Why did you scrap the notification, which seem more canonical and re-usable?
While pushing this to test code seems nice, having a one-off impl seems worse
than using a notification like before.

https://codereview.chromium.org/648833004/diff/2410001/chrome/test/base/ui_te...
chrome/test/base/ui_test_utils.cc:141: bool running_;
Why not just use content::MessageLoopRunner::loop_running() and remove this
local member?

https://codereview.chromium.org/648833004/diff/2410001/chrome/test/base/ui_te...
File chrome/test/base/ui_test_utils.h (right):

https://codereview.chromium.org/648833004/diff/2410001/chrome/test/base/ui_te...
chrome/test/base/ui_test_utils.h:131: // Creates an waiter object to observe
that an application modal dialog
nit: "a waiter"

https://codereview.chromium.org/648833004/diff/2410001/components/OWNERS
File components/OWNERS (right):

https://codereview.chromium.org/648833004/diff/2410001/components/OWNERS#newc...
components/OWNERS:6: per-file app_modal_dialog*=avi@chromium.org
I don't have any better owners suggestions, and don't think inheriting owners
from c/b/ui is a generally workable solution. The code authors should be owners,
and if that isn't sufficient or workable, then perhaps the area doesn't need a
specific owner (for the time being)?

https://codereview.chromium.org/648833004/diff/2410001/components/app_modal_d...
File components/app_modal_dialogs.gypi (right):

https://codereview.chromium.org/648833004/diff/2410001/components/app_modal_d...
components/app_modal_dialogs.gypi:11: '../content/content.gyp:content_browser',
nit: fix indent on this line and the line below.

https://codereview.chromium.org/648833004/diff/2410001/components/app_modal_d...
components/app_modal_dialogs.gypi:23:
'app_modal_dialogs/app_modal_dialogs/app_modal_dialog_queue.h',
Remove one of the app_modal_dialogs dir mentions...

https://codereview.chromium.org/648833004/diff/2410001/components/app_modal_d...
File components/app_modal_dialogs/BUILD.gn (right):

https://codereview.chromium.org/648833004/diff/2410001/components/app_modal_d...
components/app_modal_dialogs/BUILD.gn:22: "//content/public/common",
Is this needed for content/public/browser? Is an explicit dep needed?

https://codereview.chromium.org/648833004/diff/2410001/components/app_modal_d...
components/app_modal_dialogs/BUILD.gn:23: "//extensions/common",
Where is extensions used at all? Ditto for the dep below?

https://codereview.chromium.org/648833004/diff/2410001/components/app_modal_d...
components/app_modal_dialogs/BUILD.gn:25: "//skia",
Is this needed for ui/gfx or something? Why is an explicit dep needed?

https://codereview.chromium.org/648833004/diff/2410001/components/app_modal_d...
File components/app_modal_dialogs/DEPS (right):

https://codereview.chromium.org/648833004/diff/2410001/components/app_modal_d...
components/app_modal_dialogs/DEPS:3: "+content/public/common",
I don't think anything from ui/base is included [yet].

https://codereview.chromium.org/648833004/diff/2410001/components/app_modal_d...
components/app_modal_dialogs/DEPS:7: "+ui/base",
Is anything from ui/base actually included?

https://codereview.chromium.org/648833004/diff/2410001/components/app_modal_d...
File components/app_modal_dialogs/app_modal_dialog.h (right):

https://codereview.chromium.org/648833004/diff/2410001/components/app_modal_d...
components/app_modal_dialogs/app_modal_dialog.h:5: #ifndef
COMPONENTS_SYSTEM_MODAL_DIALOGS_APP_MODAL_DIALOG_H_
Shouldn't these be COMPONENTS_APP_MODAL_*?

https://codereview.chromium.org/648833004/diff/2410001/components/app_modal_d...
File components/app_modal_dialogs/app_modal_dialog_queue.h (right):

https://codereview.chromium.org/648833004/diff/2410001/components/app_modal_d...
components/app_modal_dialogs/app_modal_dialog_queue.h:5: #ifndef
COMPONENTS_SYSTEM_MODAL_DIALOGS_APP_MODAL_DIALOG_QUEUE_H_
Shouldn't these be COMPONENTS_APP_MODAL_*?

https://codereview.chromium.org/648833004/diff/2410001/components/app_modal_d...
File components/app_modal_dialogs/javascript_app_modal_dialog.h (right):

https://codereview.chromium.org/648833004/diff/2410001/components/app_modal_d...
components/app_modal_dialogs/javascript_app_modal_dialog.h:5: #ifndef
EXTENSIONS_COMPONENTS_SYSTEM_MODAL_DIALOGS_JAVASCRIPT_APP_MODAL_DIALOG_H_
Shouldn't these be COMPONENTS_APP_MODAL_*?

https://codereview.chromium.org/648833004/diff/2410001/components/app_modal_d...
File components/app_modal_dialogs/native_app_modal_dialog.h (right):

https://codereview.chromium.org/648833004/diff/2410001/components/app_modal_d...
components/app_modal_dialogs/native_app_modal_dialog.h:5: #ifndef
EXTENSIONS_COMPONENTS_SYSTEM_MODAL_DIALOGS_NATIVE_APP_MODAL_DIALOG_H_
Shouldn't these be COMPONENTS_APP_MODAL_*?

https://codereview.chromium.org/648833004/diff/2410001/components/app_modal_d...
File components/app_modal_dialogs_strings.grdp (right):

https://codereview.chromium.org/648833004/diff/2410001/components/app_modal_d...
components/app_modal_dialogs_strings.grdp:46: <message
name="IDS_JAVASCRIPT_MESSAGEBOX_SUPPRESS_OPTION" desc="Optional UI shown on the
message box, in the form of a checkbox, allowing the user to suppress additional
message boxes from the page.">
Why un-group this from the other IDS_JAVASCRIPT_[MESSAGEBOX|ALERT]_* strings?

[oshima, 2014-10-23 00:36:50]

Patchset #7 (id:2470001) has been deleted

[oshima, 2014-10-23 00:36:58]

Patchset #6 (id:2450001) has been deleted

[oshima, 2014-10-23 00:37:07]

Patchset #5 (id:2430001) has been deleted

[oshima, 2014-10-23 00:43:30]

Patchset #5 (id:2490001) has been deleted

[oshima, 2014-10-23 00:43:51]

Patchset #5 (id:2490001) has been deleted

[oshima, 2014-10-23 00:44:12]

https://codereview.chromium.org/648833004/diff/2410001/chrome/browser/chrome_...
File chrome/browser/chrome_notification_types.h (left):

https://codereview.chromium.org/648833004/diff/2410001/chrome/browser/chrome_...
chrome/browser/chrome_notification_types.h:127: // Application-modal dialogs
-----------------------------------------------
On 2014/10/22 22:53:31, msw wrote:
> nit: if you remove the only actually dialog-related notification here, maybe
the
> comment should be updated; not sure to what...

I didn't remove the section and that's why I didn't remove it. You seems to be
correct that the above title is only for this notification.

Maybe  // Inforbar --- ? I can change whatever you want so please let me know.

https://codereview.chromium.org/648833004/diff/2410001/chrome/browser/lifetim...
File chrome/browser/lifetime/browser_close_manager_browsertest.cc (right):

https://codereview.chromium.org/648833004/diff/2410001/chrome/browser/lifetim...
chrome/browser/lifetime/browser_close_manager_browsertest.cc:81: waiter_ =
ui_test_utils::CreateAppModalDialogWaiter();
On 2014/10/22 22:53:31, msw wrote:
> Why not just have a GetNextDialog-local |waiter|? Better yet, why not just
call
> ui_test_utils::WaitForAppModalDialog()? You could just scrap
> AppModalDialogObserver and have a couple Accept/Cancel local helper functions
> like this:
> 
> void WaitForAndAcceptDialog() {
>   AppModalDialog* dialog = ui_test_utils::WaitForAppModalDialog();
>   EXPECT_TRUE(dialog->IsJavaScriptModalDialog());
>   dialog->native_dialog()->AcceptAppModalDialog();
> }

Sure, done.

https://codereview.chromium.org/648833004/diff/2410001/chrome/browser/ui/app_...
File chrome/browser/ui/app_modal_dialogs/javascript_dialog_manager.cc (right):

https://codereview.chromium.org/648833004/diff/2410001/chrome/browser/ui/app_...
chrome/browser/ui/app_modal_dialogs/javascript_dialog_manager.cc:41: const int
kJavaScriptMessageExpectedDelay = 1000;
On 2014/10/22 22:53:31, msw wrote:
> optional nit: move this within the function's scope.

Done.

https://codereview.chromium.org/648833004/diff/2410001/chrome/browser/ui/view...
File chrome/browser/ui/views/javascript_app_modal_dialog_views.cc (right):

https://codereview.chromium.org/648833004/diff/2410001/chrome/browser/ui/view...
chrome/browser/ui/views/javascript_app_modal_dialog_views.cc:9: #include
"chrome/grit/generated_resources.h"
On 2014/10/22 22:53:31, msw wrote:
> nit: I think you can remove this include.

Done.

https://codereview.chromium.org/648833004/diff/2410001/chrome/chrome_browser_...
File chrome/chrome_browser_ui.gypi (right):

https://codereview.chromium.org/648833004/diff/2410001/chrome/chrome_browser_...
chrome/chrome_browser_ui.gypi:2632:
'../components/components.gyp:app_modal_dialogs',
On 2014/10/22 22:53:31, msw wrote:
> nit: sort this alphabetically

Done.

https://codereview.chromium.org/648833004/diff/2410001/chrome/test/base/ui_te...
File chrome/test/base/ui_test_utils.cc (right):

https://codereview.chromium.org/648833004/diff/2410001/chrome/test/base/ui_te...
chrome/test/base/ui_test_utils.cc:110: class AppModalDialogWaiterImpl : public
AppModalDialogWaiter {
On 2014/10/22 22:53:32, msw wrote:
> Why did you scrap the notification, which seem more canonical and re-usable?
> While pushing this to test code seems nice, having a one-off impl seems worse
> than using a notification like before.

Notification is considered deprecated and we're eliminating them as we
componentize the code
whereever possible.

https://codereview.chromium.org/648833004/diff/2410001/chrome/test/base/ui_te...
chrome/test/base/ui_test_utils.cc:141: bool running_;
On 2014/10/22 22:53:31, msw wrote:
> Why not just use content::MessageLoopRunner::loop_running() and remove this
> local member?

Done.

https://codereview.chromium.org/648833004/diff/2410001/components/OWNERS
File components/OWNERS (right):

https://codereview.chromium.org/648833004/diff/2410001/components/OWNERS#newc...
components/OWNERS:6: per-file app_modal_dialog*=avi@chromium.org
On 2014/10/22 22:53:32, msw wrote:
> I don't have any better owners suggestions, and don't think inheriting owners
> from c/b/ui is a generally workable solution. The code authors should be
owners,
> and if that isn't sufficient or workable, then perhaps the area doesn't need a
> specific owner (for the time being)?

I picked avi based on commit history in app_modal_dialog directory for last 4
years. Let me know if you have better owners.

https://codereview.chromium.org/648833004/diff/2410001/components/app_modal_d...
File components/app_modal_dialogs.gypi (right):

https://codereview.chromium.org/648833004/diff/2410001/components/app_modal_d...
components/app_modal_dialogs.gypi:11: '../content/content.gyp:content_browser',
On 2014/10/22 22:53:32, msw wrote:
> nit: fix indent on this line and the line below.

indent looks correct?

https://codereview.chromium.org/648833004/diff/2410001/components/app_modal_d...
components/app_modal_dialogs.gypi:23:
'app_modal_dialogs/app_modal_dialogs/app_modal_dialog_queue.h',
On 2014/10/22 22:53:32, msw wrote:
> Remove one of the app_modal_dialogs dir mentions...

Done.

https://codereview.chromium.org/648833004/diff/2410001/components/app_modal_d...
File components/app_modal_dialogs/BUILD.gn (right):

https://codereview.chromium.org/648833004/diff/2410001/components/app_modal_d...
components/app_modal_dialogs/BUILD.gn:23: "//extensions/common",
On 2014/10/22 22:53:32, msw wrote:
> Where is extensions used at all? Ditto for the dep below?

They're in javascript_dialog_manager. I moved that to the next CL, so that's why
you didn't see it. I removed them and will be added in the next CL.

https://codereview.chromium.org/648833004/diff/2410001/components/app_modal_d...
components/app_modal_dialogs/BUILD.gn:25: "//skia",
On 2014/10/22 22:53:32, msw wrote:
> Is this needed for ui/gfx or something? Why is an explicit dep needed?

This is for SkMatrix44.h which is included via aura -> compositor -> gfx ->
skia.
My undestanding is that you need this skia dependency in this case. I'm happy to
change if this is wrong, so please let me know.

https://codereview.chromium.org/648833004/diff/2410001/components/app_modal_d...
File components/app_modal_dialogs/DEPS (right):

https://codereview.chromium.org/648833004/diff/2410001/components/app_modal_d...
components/app_modal_dialogs/DEPS:7: "+ui/base",
On 2014/10/22 22:53:32, msw wrote:
> Is anything from ui/base actually included?

They are in javascript_dialog_manager, which I moved to next CL.

https://codereview.chromium.org/648833004/diff/2410001/components/app_modal_d...
File components/app_modal_dialogs/app_modal_dialog.h (right):

https://codereview.chromium.org/648833004/diff/2410001/components/app_modal_d...
components/app_modal_dialogs/app_modal_dialog.h:5: #ifndef
COMPONENTS_SYSTEM_MODAL_DIALOGS_APP_MODAL_DIALOG_H_
On 2014/10/22 22:53:32, msw wrote:
> Shouldn't these be COMPONENTS_APP_MODAL_*?

Done.

https://codereview.chromium.org/648833004/diff/2410001/components/app_modal_d...
components/app_modal_dialogs/app_modal_dialog.h:5: #ifndef
COMPONENTS_SYSTEM_MODAL_DIALOGS_APP_MODAL_DIALOG_H_
On 2014/10/22 22:53:32, msw wrote:
> Shouldn't these be COMPONENTS_APP_MODAL_*?

Done. looks like mass-rename.py didn't do what I expected when I re-renamed
files.

https://codereview.chromium.org/648833004/diff/2410001/components/app_modal_d...
File components/app_modal_dialogs_strings.grdp (right):

https://codereview.chromium.org/648833004/diff/2410001/components/app_modal_d...
components/app_modal_dialogs_strings.grdp:46: <message
name="IDS_JAVASCRIPT_MESSAGEBOX_SUPPRESS_OPTION" desc="Optional UI shown on the
message box, in the form of a checkbox, allowing the user to suppress additional
message boxes from the page.">
On 2014/10/22 22:53:32, msw wrote:
> Why un-group this from the other IDS_JAVASCRIPT_[MESSAGEBOX|ALERT]_* strings?

Done.

[oshima, 2014-10-23 11:17:17]

Patchset #6 (id:2530001) has been deleted

[msw, 2014-10-23 18:41:22]

lgtm with nits

https://codereview.chromium.org/648833004/diff/2410001/chrome/browser/chrome_...
File chrome/browser/chrome_notification_types.h (left):

https://codereview.chromium.org/648833004/diff/2410001/chrome/browser/chrome_...
chrome/browser/chrome_notification_types.h:127: // Application-modal dialogs
-----------------------------------------------
On 2014/10/23 00:44:11, oshima wrote:
> On 2014/10/22 22:53:31, msw wrote:
> > nit: if you remove the only actually dialog-related notification here, maybe
> the
> > comment should be updated; not sure to what...
> 
> I didn't remove the section and that's why I didn't remove it. You seems to be
> correct that the above title is only for this notification.
> 
> Maybe  // Inforbar --- ? I can change whatever you want so please let me know.

NOTIFICATION_METRIC_EVENT_DURATION can probably be removed, it's not observed
anywhere, afaict. NOTIFICATION_TAB_CONTENTS_APPLICATION_EXTENSION_CHANGED seems
like it ought to be moved to extensions/browser/notification_types.h. If you
update those two, then you can retitle this section "Infobars" for the remaining
two infobar notifications.

Otherwise, I'd just remove this title and let them fall under
"Application-wide", I guess?

https://codereview.chromium.org/648833004/diff/2410001/chrome/browser/lifetim...
File chrome/browser/lifetime/browser_close_manager_browsertest.cc (right):

https://codereview.chromium.org/648833004/diff/2410001/chrome/browser/lifetim...
chrome/browser/lifetime/browser_close_manager_browsertest.cc:81: waiter_ =
ui_test_utils::CreateAppModalDialogWaiter();
On 2014/10/23 00:44:11, oshima wrote:
> On 2014/10/22 22:53:31, msw wrote:
> > Why not just have a GetNextDialog-local |waiter|? Better yet, why not just
> call
> > ui_test_utils::WaitForAppModalDialog()? You could just scrap
> > AppModalDialogObserver and have a couple Accept/Cancel local helper
functions
> > like this:
> > 
> > void WaitForAndAcceptDialog() {
> >   AppModalDialog* dialog = ui_test_utils::WaitForAppModalDialog();
> >   EXPECT_TRUE(dialog->IsJavaScriptModalDialog());
> >   dialog->native_dialog()->AcceptAppModalDialog();
> > }
> 
> Sure, done.

The class isn't needed, but it's still a great improvement, thanks!

https://codereview.chromium.org/648833004/diff/2410001/chrome/test/base/ui_te...
File chrome/test/base/ui_test_utils.cc (right):

https://codereview.chromium.org/648833004/diff/2410001/chrome/test/base/ui_te...
chrome/test/base/ui_test_utils.cc:110: class AppModalDialogWaiterImpl : public
AppModalDialogWaiter {
On 2014/10/23 00:44:12, oshima wrote:
> On 2014/10/22 22:53:32, msw wrote:
> > Why did you scrap the notification, which seem more canonical and re-usable?
> > While pushing this to test code seems nice, having a one-off impl seems
worse
> > than using a notification like before.
> 
> Notification is considered deprecated and we're eliminating them as we
> componentize the code
> whereever possible.

Acknowledged.

https://codereview.chromium.org/648833004/diff/2410001/components/app_modal_d...
File components/app_modal_dialogs.gypi (right):

https://codereview.chromium.org/648833004/diff/2410001/components/app_modal_d...
components/app_modal_dialogs.gypi:11: '../content/content.gyp:content_browser',
On 2014/10/23 00:44:12, oshima wrote:
> On 2014/10/22 22:53:32, msw wrote:
> > nit: fix indent on this line and the line below.
> 
> indent looks correct?

Sorry... I'm losing it!

https://codereview.chromium.org/648833004/diff/2410001/components/app_modal_d...
File components/app_modal_dialogs/BUILD.gn (right):

https://codereview.chromium.org/648833004/diff/2410001/components/app_modal_d...
components/app_modal_dialogs/BUILD.gn:23: "//extensions/common",
On 2014/10/23 00:44:12, oshima wrote:
> On 2014/10/22 22:53:32, msw wrote:
> > Where is extensions used at all? Ditto for the dep below?
> 
> They're in javascript_dialog_manager. I moved that to the next CL, so that's
why
> you didn't see it. I removed them and will be added in the next CL.

Acknowledged.

https://codereview.chromium.org/648833004/diff/2410001/components/app_modal_d...
components/app_modal_dialogs/BUILD.gn:25: "//skia",
On 2014/10/23 00:44:12, oshima wrote:
> On 2014/10/22 22:53:32, msw wrote:
> > Is this needed for ui/gfx or something? Why is an explicit dep needed?
> 
> This is for SkMatrix44.h which is included via aura -> compositor -> gfx ->
> skia.
> My undestanding is that you need this skia dependency in this case. I'm happy
to
> change if this is wrong, so please let me know.
> 

Acknowledged.

https://codereview.chromium.org/648833004/diff/2410001/components/app_modal_d...
File components/app_modal_dialogs/DEPS (right):

https://codereview.chromium.org/648833004/diff/2410001/components/app_modal_d...
components/app_modal_dialogs/DEPS:7: "+ui/base",
On 2014/10/23 00:44:12, oshima wrote:
> On 2014/10/22 22:53:32, msw wrote:
> > Is anything from ui/base actually included?
> 
> They are in javascript_dialog_manager, which I moved to next CL.

Acknowledged.

https://codereview.chromium.org/648833004/diff/2590001/chrome/browser/ui/app_...
File chrome/browser/ui/app_modal_dialogs/javascript_dialog_manager.cc (right):

https://codereview.chromium.org/648833004/diff/2590001/chrome/browser/ui/app_...
chrome/browser/ui/app_modal_dialogs/javascript_dialog_manager.cc:160: // If a
WebContents is impolite and displays a second JavaScript
nit: Move down, combine this comment with the one at line 179.

https://codereview.chromium.org/648833004/diff/2590001/chrome/test/base/ui_te...
File chrome/test/base/ui_test_utils.cc (right):

https://codereview.chromium.org/648833004/diff/2590001/chrome/test/base/ui_te...
chrome/test/base/ui_test_utils.cc:115: virtual ~AppModalDialogWaiter() {
nit: I think even this can be marked override without the virtual.

https://codereview.chromium.org/648833004/diff/2590001/chrome/test/base/ui_te...
chrome/test/base/ui_test_utils.cc:131: if (!message_loop_runner_.get() ||
!message_loop_runner_->loop_running())
nit: reverse to conditionally Quit().

https://codereview.chromium.org/648833004/diff/2590001/components/app_modal_d...
File components/app_modal_dialogs/app_modal_dialog.h (right):

https://codereview.chromium.org/648833004/diff/2590001/components/app_modal_d...
components/app_modal_dialogs/app_modal_dialog.h:93: class AppModalDialogNotifier
{
nit: technically this is a notify-ee (one that is notified)... Consider
AppModalDialogObserver?

https://codereview.chromium.org/648833004/diff/2590001/components/app_modal_d...
components/app_modal_dialogs/app_modal_dialog.h:100: };
nit: private DISALLOW_COPY_AND_ASSIGN?

https://codereview.chromium.org/648833004/diff/2590001/components/app_modal_d...
File components/app_modal_dialogs_strings.grdp (right):

https://codereview.chromium.org/648833004/diff/2590001/components/app_modal_d...
components/app_modal_dialogs_strings.grdp:17: 
nit: remove blank line

[oshima, 2014-10-23 21:44:18]

https://codereview.chromium.org/648833004/diff/2410001/chrome/browser/chrome_...
File chrome/browser/chrome_notification_types.h (left):

https://codereview.chromium.org/648833004/diff/2410001/chrome/browser/chrome_...
chrome/browser/chrome_notification_types.h:127: // Application-modal dialogs
-----------------------------------------------
On 2014/10/23 18:41:21, msw wrote:
> On 2014/10/23 00:44:11, oshima wrote:
> > On 2014/10/22 22:53:31, msw wrote:
> > > nit: if you remove the only actually dialog-related notification here,
maybe
> > the
> > > comment should be updated; not sure to what...
> > 
> > I didn't remove the section and that's why I didn't remove it. You seems to
be
> > correct that the above title is only for this notification.
> > 
> > Maybe  // Inforbar --- ? I can change whatever you want so please let me
know.
> 
> NOTIFICATION_METRIC_EVENT_DURATION can probably be removed, it's not observed
> anywhere, afaict. NOTIFICATION_TAB_CONTENTS_APPLICATION_EXTENSION_CHANGED
seems
> like it ought to be moved to extensions/browser/notification_types.h. If you
> update those two, then you can retitle this section "Infobars" for the
remaining
> two infobar notifications.
> 
> Otherwise, I'd just remove this title and let them fall under
> "Application-wide", I guess?

I sent you CL that removes NOTIFICATION_METRIC_EVENT_DURATION and the code that
notifies this.
I'll merge it once it's landed.

For NOTIFICATION_TAB_CONTENTS_APPLICATION_EXTENSION_CHANGED, I talked to one of
extensions
team and they want to keep it here because it's notified/observered inside
chrome.

I removed the title and moved them to "Application-wide" instead.

https://codereview.chromium.org/648833004/diff/2410001/chrome/browser/lifetim...
File chrome/browser/lifetime/browser_close_manager_browsertest.cc (right):

https://codereview.chromium.org/648833004/diff/2410001/chrome/browser/lifetim...
chrome/browser/lifetime/browser_close_manager_browsertest.cc:81: waiter_ =
ui_test_utils::CreateAppModalDialogWaiter();
On 2014/10/23 18:41:21, msw wrote:
> On 2014/10/23 00:44:11, oshima wrote:
> > On 2014/10/22 22:53:31, msw wrote:
> > > Why not just have a GetNextDialog-local |waiter|? Better yet, why not just
> > call
> > > ui_test_utils::WaitForAppModalDialog()? You could just scrap
> > > AppModalDialogObserver and have a couple Accept/Cancel local helper
> functions
> > > like this:
> > > 
> > > void WaitForAndAcceptDialog() {
> > >   AppModalDialog* dialog = ui_test_utils::WaitForAppModalDialog();
> > >   EXPECT_TRUE(dialog->IsJavaScriptModalDialog());
> > >   dialog->native_dialog()->AcceptAppModalDialog();
> > > }
> > 
> > Sure, done.
> 
> The class isn't needed, but it's still a great improvement, thanks!

I see. Removed the class and replaced with function.

https://codereview.chromium.org/648833004/diff/2590001/chrome/browser/ui/app_...
File chrome/browser/ui/app_modal_dialogs/javascript_dialog_manager.cc (right):

https://codereview.chromium.org/648833004/diff/2590001/chrome/browser/ui/app_...
chrome/browser/ui/app_modal_dialogs/javascript_dialog_manager.cc:160: // If a
WebContents is impolite and displays a second JavaScript
On 2014/10/23 18:41:21, msw wrote:
> nit: Move down, combine this comment with the one at line 179.

Done.

https://codereview.chromium.org/648833004/diff/2590001/chrome/test/base/ui_te...
File chrome/test/base/ui_test_utils.cc (right):

https://codereview.chromium.org/648833004/diff/2590001/chrome/test/base/ui_te...
chrome/test/base/ui_test_utils.cc:115: virtual ~AppModalDialogWaiter() {
On 2014/10/23 18:41:21, msw wrote:
> nit: I think even this can be marked override without the virtual.

Done.

https://codereview.chromium.org/648833004/diff/2590001/chrome/test/base/ui_te...
chrome/test/base/ui_test_utils.cc:131: if (!message_loop_runner_.get() ||
!message_loop_runner_->loop_running())
On 2014/10/23 18:41:21, msw wrote:
> nit: reverse to conditionally Quit().

Done.

https://codereview.chromium.org/648833004/diff/2590001/components/app_modal_d...
File components/app_modal_dialogs/app_modal_dialog.h (right):

https://codereview.chromium.org/648833004/diff/2590001/components/app_modal_d...
components/app_modal_dialogs/app_modal_dialog.h:93: class AppModalDialogNotifier
{
On 2014/10/23 18:41:22, msw wrote:
> nit: technically this is a notify-ee (one that is notified)... Consider
> AppModalDialogObserver?

Done.

https://codereview.chromium.org/648833004/diff/2590001/components/app_modal_d...
components/app_modal_dialogs/app_modal_dialog.h:100: };
On 2014/10/23 18:41:22, msw wrote:
> nit: private DISALLOW_COPY_AND_ASSIGN?

Done.

https://codereview.chromium.org/648833004/diff/2590001/components/app_modal_d...
File components/app_modal_dialogs_strings.grdp (right):

https://codereview.chromium.org/648833004/diff/2590001/components/app_modal_d...
components/app_modal_dialogs_strings.grdp:17: 
On 2014/10/23 18:41:22, msw wrote:
> nit: remove blank line

Done.

[oshima, 2014-10-23 21:45:14]

blundell, could you please review components/ ?

Thanks!

[msw, 2014-10-23 22:01:24]

lgtm with two more minor optional nits.

https://codereview.chromium.org/648833004/diff/2410001/chrome/browser/chrome_...
File chrome/browser/chrome_notification_types.h (left):

https://codereview.chromium.org/648833004/diff/2410001/chrome/browser/chrome_...
chrome/browser/chrome_notification_types.h:127: // Application-modal dialogs
-----------------------------------------------
On 2014/10/23 21:44:18, oshima wrote:
> On 2014/10/23 18:41:21, msw wrote:
> > On 2014/10/23 00:44:11, oshima wrote:
> > > On 2014/10/22 22:53:31, msw wrote:
> > > > nit: if you remove the only actually dialog-related notification here,
> maybe
> > > the
> > > > comment should be updated; not sure to what...
> > > 
> > > I didn't remove the section and that's why I didn't remove it. You seems
to
> be
> > > correct that the above title is only for this notification.
> > > 
> > > Maybe  // Inforbar --- ? I can change whatever you want so please let me
> know.
> > 
> > NOTIFICATION_METRIC_EVENT_DURATION can probably be removed, it's not
observed
> > anywhere, afaict. NOTIFICATION_TAB_CONTENTS_APPLICATION_EXTENSION_CHANGED
> seems
> > like it ought to be moved to extensions/browser/notification_types.h. If you
> > update those two, then you can retitle this section "Infobars" for the
> remaining
> > two infobar notifications.
> > 
> > Otherwise, I'd just remove this title and let them fall under
> > "Application-wide", I guess?
> 
> I sent you CL that removes NOTIFICATION_METRIC_EVENT_DURATION and the code
that
> notifies this.
> I'll merge it once it's landed.
> 
> For NOTIFICATION_TAB_CONTENTS_APPLICATION_EXTENSION_CHANGED, I talked to one
of
> extensions
> team and they want to keep it here because it's notified/observered inside
> chrome.
> 
> I removed the title and moved them to "Application-wide" instead.

Acknowledged.

https://codereview.chromium.org/648833004/diff/2410001/chrome/browser/lifetim...
File chrome/browser/lifetime/browser_close_manager_browsertest.cc (right):

https://codereview.chromium.org/648833004/diff/2410001/chrome/browser/lifetim...
chrome/browser/lifetime/browser_close_manager_browsertest.cc:81: waiter_ =
ui_test_utils::CreateAppModalDialogWaiter();
On 2014/10/23 21:44:18, oshima wrote:
> On 2014/10/23 18:41:21, msw wrote:
> > On 2014/10/23 00:44:11, oshima wrote:
> > > On 2014/10/22 22:53:31, msw wrote:
> > > > Why not just have a GetNextDialog-local |waiter|? Better yet, why not
just
> > > call
> > > > ui_test_utils::WaitForAppModalDialog()? You could just scrap
> > > > AppModalDialogObserver and have a couple Accept/Cancel local helper
> > functions
> > > > like this:
> > > > 
> > > > void WaitForAndAcceptDialog() {
> > > >   AppModalDialog* dialog = ui_test_utils::WaitForAppModalDialog();
> > > >   EXPECT_TRUE(dialog->IsJavaScriptModalDialog());
> > > >   dialog->native_dialog()->AcceptAppModalDialog();
> > > > }
> > > 
> > > Sure, done.
> > 
> > The class isn't needed, but it's still a great improvement, thanks!
> 
> I see. Removed the class and replaced with function.

Great, thank you!

https://codereview.chromium.org/648833004/diff/2610001/chrome/browser/ui/app_...
File chrome/browser/ui/app_modal_dialogs/javascript_dialog_manager.cc (right):

https://codereview.chromium.org/648833004/diff/2610001/chrome/browser/ui/app_...
chrome/browser/ui/app_modal_dialogs/javascript_dialog_manager.cc:160: const int
kJavaScriptMessageExpectedDelay = 1000;
nit: move this down to where it's used too... or remove the blank line above.

https://codereview.chromium.org/648833004/diff/2610001/components/app_modal_d...
File components/app_modal_dialogs/app_modal_dialog.cc (right):

https://codereview.chromium.org/648833004/diff/2610001/components/app_modal_d...
components/app_modal_dialogs/app_modal_dialog.cc:18: AppModalDialogObserver*
observer_ = NULL;
nit: s/observer_/app_modal_dialog_observer/

[blundell, 2014-10-24 06:55:05]

LGTM, but please follow up immediately with agable@ re: strings.

https://codereview.chromium.org/648833004/diff/2630001/components/OWNERS
File components/OWNERS (right):

https://codereview.chromium.org/648833004/diff/2630001/components/OWNERS#newc...
components/OWNERS:6: per-file app_modal_dialog*=avi@chromium.org
There should be an OWNERS file in //components/app_modal_dialog too.

https://codereview.chromium.org/648833004/diff/2630001/components/app_modal_d...
File components/app_modal_dialogs_strings.grdp (right):

https://codereview.chromium.org/648833004/diff/2630001/components/app_modal_d...
components/app_modal_dialogs_strings.grdp:5: <message
name="IDS_JAVASCRIPT_ALERT_DEFAULT_TITLE" desc="Title for JavaScript alert
originating from a webpage if there is no hostname to display">
You need to ensure that these strings get translated: when a new grd(p) file is
added to the project, the strings won't get translated by default. Please reach
out to agable@ for the steps to take to do that.

[oshima, 2014-10-24 11:17:04]

I'll contact agable@ once this is landed.

https://codereview.chromium.org/648833004/diff/2610001/chrome/browser/ui/app_...
File chrome/browser/ui/app_modal_dialogs/javascript_dialog_manager.cc (right):

https://codereview.chromium.org/648833004/diff/2610001/chrome/browser/ui/app_...
chrome/browser/ui/app_modal_dialogs/javascript_dialog_manager.cc:160: const int
kJavaScriptMessageExpectedDelay = 1000;
On 2014/10/23 22:01:24, msw wrote:
> nit: move this down to where it's used too... or remove the blank line above.

Done.

https://codereview.chromium.org/648833004/diff/2610001/components/app_modal_d...
File components/app_modal_dialogs/app_modal_dialog.cc (right):

https://codereview.chromium.org/648833004/diff/2610001/components/app_modal_d...
components/app_modal_dialogs/app_modal_dialog.cc:18: AppModalDialogObserver*
observer_ = NULL;
On 2014/10/23 22:01:24, msw wrote:
> nit: s/observer_/app_modal_dialog_observer/

Done.

https://codereview.chromium.org/648833004/diff/2630001/components/OWNERS
File components/OWNERS (right):

https://codereview.chromium.org/648833004/diff/2630001/components/OWNERS#newc...
components/OWNERS:6: per-file app_modal_dialog*=avi@chromium.org
On 2014/10/24 06:55:04, blundell wrote:
> There should be an OWNERS file in //components/app_modal_dialog too.

Done.

[oshima, 2014-10-24 11:17:09]

I'll contact agable@ once this is landed.

[oshima, 2014-10-24 13:26:49]

tbr'ing sky for include path changes under chrome/ (c/b/ui is reviewed by msw)

[oshima, 2014-10-24 13:27:01]

The CQ bit was checked by oshima@chromium.org

[oshima, 2014-10-24 13:27:04]

The CQ bit was unchecked by oshima@chromium.org

[oshima, 2014-10-24 13:27:20]

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

[oshima, 2014-10-24 13:27:33]

The CQ bit was checked by oshima@chromium.org

[commit-bot: I haz the power, 2014-10-24 13:28:07]

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

[commit-bot: I haz the power, 2014-10-24 14:14:44]

Committed patchset #11 (id:2650001)

[commit-bot: I haz the power, 2014-10-24 14:15:29]

Patchset 11 (id:??) landed as
https://crrev.com/82f7248c0e48ee71c4b78f9d305caefc7fa9bab2
Cr-Commit-Position: refs/heads/master@{#301095}