Move the web dialogs code to src/ui/web_dialogs from src/chrome

Move the web dialogs code to src/ui/web_dialogs from src/chrome

Also moved the web dialogs classes into ui namespace.

The original patch was sent by Jake Helfert <jake@helfert.us>;.
This patch was created based on Jake's patch.

BUG=125841
TEST=try

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=139757

[Ben Goodger (Google), 2012-05-30 17:13:30]

Great stuff, thanks for working on this. LGTM.

[mazda, 2012-05-30 17:24:54]

+kmadhusu
Could you do an OWNERS review for chrome/browser/printing?
This is a mechanical change as to the printing code.

Thanks,

[tfarina, 2012-05-30 19:14:19]

http://codereview.chromium.org/10448066/diff/5002/ui/web_dialogs/web_dialog_d...
File ui/web_dialogs/web_dialog_delegate.h (right):

http://codereview.chromium.org/10448066/diff/5002/ui/web_dialogs/web_dialog_d...
ui/web_dialogs/web_dialog_delegate.h:36: class WEB_DIALOGS_EXPORT
WebDialogDelegate {
nit: I bet you can remove WEB_DIALOGS_EXPORT. But I'm fine in keep it as it's
consistent across ui/

http://codereview.chromium.org/10448066/diff/5002/ui/web_dialogs/web_dialog_d...
ui/web_dialogs/web_dialog_delegate.h:98: virtual bool HandleContextMenu(const
content::ContextMenuParams& params);
This doesn't have any implementations. Can you add {} here and in
HandleAddNewContents too?

[tfarina, 2012-05-30 19:16:19]

http://codereview.chromium.org/10448066/diff/5002/ui/web_dialogs/web_dialogs.gyp
File ui/web_dialogs/web_dialogs.gyp (right):

http://codereview.chromium.org/10448066/diff/5002/ui/web_dialogs/web_dialogs....
ui/web_dialogs/web_dialogs.gyp:1: # Copyright (c) 2012 The Chromium Authors. All
rights reserved.
btw, very nice and clean gyp file :) Yay.

http://codereview.chromium.org/10448066/diff/5002/ui/web_dialogs/web_dialogs....
ui/web_dialogs/web_dialogs.gyp:21: # All .cc, .h under web_dialogs, except
unittests
nit: end with period. Yeah, I know we have all the other comments without
periods unfortunately.

[tfarina, 2012-05-30 19:18:34]

http://codereview.chromium.org/10448066/diff/5002/chrome/browser/ui/gtk/web_d...
File chrome/browser/ui/gtk/web_dialog_gtk.cc (right):

http://codereview.chromium.org/10448066/diff/5002/chrome/browser/ui/gtk/web_d...
chrome/browser/ui/gtk/web_dialog_gtk.cc:21: #include
"content/public/browser/web_contents.h"
duplicated.

[kmadhusu, 2012-05-30 19:47:06]

lgtm

[mazda, 2012-05-30 20:14:11]

Thank you for the review.

tfarina,
I addressed the comments.
Please take another look.

http://codereview.chromium.org/10448066/diff/5002/chrome/browser/ui/gtk/web_d...
File chrome/browser/ui/gtk/web_dialog_gtk.cc (right):

http://codereview.chromium.org/10448066/diff/5002/chrome/browser/ui/gtk/web_d...
chrome/browser/ui/gtk/web_dialog_gtk.cc:21: #include
"content/public/browser/web_contents.h"
On 2012/05/30 19:18:35, tfarina wrote:
> duplicated.

Done.

http://codereview.chromium.org/10448066/diff/5002/ui/web_dialogs/web_dialog_d...
File ui/web_dialogs/web_dialog_delegate.h (right):

http://codereview.chromium.org/10448066/diff/5002/ui/web_dialogs/web_dialog_d...
ui/web_dialogs/web_dialog_delegate.h:36: class WEB_DIALOGS_EXPORT
WebDialogDelegate {
On 2012/05/30 19:14:19, tfarina wrote:
> nit: I bet you can remove WEB_DIALOGS_EXPORT. But I'm fine in keep it as it's
> consistent across ui/

It's necessary for HandleContextMenu, HandleOpenURLFromTab etc.

http://codereview.chromium.org/10448066/diff/5002/ui/web_dialogs/web_dialog_d...
ui/web_dialogs/web_dialog_delegate.h:98: virtual bool HandleContextMenu(const
content::ContextMenuParams& params);
On 2012/05/30 19:14:19, tfarina wrote:
> This doesn't have any implementations. Can you add {} here and in
> HandleAddNewContents too? 

The implementations were left in web_dialog_ui.cc in the process of extracting
WebDialogDelegate in your change (http://crrev.com/135085). :)
Since it is not allowed for virtual methods to have non-empty inline body
(http://www.chromium.org/developers/coding-style/chromium-style-checker-errors),
I created web_dialog_delegate.cc and moved the implementations from
web_dialog_ui.cc.

http://codereview.chromium.org/10448066/diff/5002/ui/web_dialogs/web_dialogs.gyp
File ui/web_dialogs/web_dialogs.gyp (right):

http://codereview.chromium.org/10448066/diff/5002/ui/web_dialogs/web_dialogs....
ui/web_dialogs/web_dialogs.gyp:1: # Copyright (c) 2012 The Chromium Authors. All
rights reserved.
On 2012/05/30 19:16:19, tfarina wrote:
> btw, very nice and clean gyp file :) Yay.
Thanks. :)

http://codereview.chromium.org/10448066/diff/5002/ui/web_dialogs/web_dialogs....
ui/web_dialogs/web_dialogs.gyp:21: # All .cc, .h under web_dialogs, except
unittests
On 2012/05/30 19:16:19, tfarina wrote:
> nit: end with period. Yeah, I know we have all the other comments without
> periods unfortunately.

Done.

[tfarina, 2012-05-30 20:18:35]

On 2012/05/30 20:14:11, mazda wrote:
> Thank you for the review.
> 
> tfarina,
> I addressed the comments.
> Please take another look.
> 
> The implementations were left in web_dialog_ui.cc in the process of extracting
> WebDialogDelegate in your change (http://crrev.com/135085). :)
Err, my bad. Sorry for that. :/

> Since it is not allowed for virtual methods to have non-empty inline body
>
(http://www.chromium.org/developers/coding-style/chromium-style-checker-errors),
Right.

> I created web_dialog_delegate.cc and moved the implementations from
> web_dialog_ui.cc.
> 
Thanks for fixing it here!

unofficial lgtm.

[commit-bot: I haz the power, 2012-05-30 20:22:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mazda@chromium.org/10448066/10072

[commit-bot: I haz the power, 2012-05-30 20:53:42]

Try job failure for 10448066-10072 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2012-05-31 02:43:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mazda@chromium.org/10448066/11012

[commit-bot: I haz the power, 2012-05-31 04:49:55]

Failed to apply patch for AUTHORS:
While running patch -p1 --forward --force;
patching file AUTHORS
Hunk #1 FAILED at 180.
1 out of 1 hunk FAILED -- saving rejects to file AUTHORS.rej

[commit-bot: I haz the power, 2012-05-31 05:22:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mazda@chromium.org/10448066/13008

[commit-bot: I haz the power, 2012-05-31 08:43:38]

Change committed as 139757