Extract a minimal subset of WebDialogUI/WebDialogDelegate from src/chrome -> src/ui/web_dialogs

Extract a minimal subset of WebDialogUI/WebDialogDelegate from src/chrome -> src/ui/web_dialogs

BUG=125841
TEST=Verify web dialogs function correctly after refactor.

[mazda, 2012-05-03 22:56:22]

https://chromiumcodereview.appspot.com/10353007/diff/1/chrome/browser/printin...
File chrome/browser/printing/print_preview_tab_controller.cc (right):

https://chromiumcodereview.appspot.com/10353007/diff/1/chrome/browser/printin...
chrome/browser/printing/print_preview_tab_controller.cc:26: #include
"chrome/browser/ui/webui/chrome_web_dialog_web_contents_delegate.h"
Please sort alphabetically by running tools/sort-headers.py.

https://chromiumcodereview.appspot.com/10353007/diff/1/ui/views/views.gyp
File ui/views/views.gyp (right):

https://chromiumcodereview.appspot.com/10353007/diff/1/ui/views/views.gyp#new...
ui/views/views.gyp:368: '../web_dialogs/web_dialog_ui.h',
You need to create a new gyp file for these.
ui/aura/aura.gyp or ui/oak/oak.gyp would be a good example.

https://chromiumcodereview.appspot.com/10353007/diff/1/ui/web_dialogs/web_dia...
File ui/web_dialogs/web_dialog_ui.h (right):

https://chromiumcodereview.appspot.com/10353007/diff/1/ui/web_dialogs/web_dia...
ui/web_dialogs/web_dialog_ui.h:12: #include "ui/views/views_export.h"
You need to create "ui/web_dialogs/web_dialogs_export.h"

https://chromiumcodereview.appspot.com/10353007/diff/1/ui/web_dialogs/web_dia...
ui/web_dialogs/web_dialog_ui.h:37: namespace views {
This should not be in views namespace.

https://chromiumcodereview.appspot.com/10353007/diff/1/ui/web_dialogs/web_dia...
ui/web_dialogs/web_dialog_ui.h:39: class VIEWS_EXPORT WebDialogDelegate {
WEB_DIALOGS_EXPORT

[tfarina, 2012-05-03 22:59:53]

https://chromiumcodereview.appspot.com/10353007/diff/1/ui/web_dialogs/web_dia...
File ui/web_dialogs/web_dialog_ui.h (right):

https://chromiumcodereview.appspot.com/10353007/diff/1/ui/web_dialogs/web_dia...
ui/web_dialogs/web_dialog_ui.h:39: class VIEWS_EXPORT WebDialogDelegate {
And this class should be in its own header file per my commit in this BUG.
Thanks!

[tfarina, 2012-05-03 23:17:29]

https://chromiumcodereview.appspot.com/10353007/diff/1/ui/web_dialogs/web_dia...
File ui/web_dialogs/web_dialog_ui.h (right):

https://chromiumcodereview.appspot.com/10353007/diff/1/ui/web_dialogs/web_dia...
ui/web_dialogs/web_dialog_ui.h:37: namespace views {
On 2012/05/03 22:56:22, mazda wrote:
> This should not be in views namespace.

either ui or web_dialogs. My preference is for ui namespace.

[jakesays, 2012-05-03 23:23:32]

Yeah my patch didn't have your new header in it, but I'm fixing that now.

-Jake

On Thu, May 3, 2012 at 5:17 PM, <tfarina@chromium.org> wrote:

>
> https://chromiumcodereview.**appspot.com/10353007/diff/1/**
>
ui/web_dialogs/web_dialog_ui.h<https://chromiumcodereview.appspot.com/10353007/diff/1/ui/web_dialogs/web_dialog_ui.h>
> File ui/web_dialogs/web_dialog_ui.h (right):
>
> https://chromiumcodereview.**appspot.com/10353007/diff/1/**
>
ui/web_dialogs/web_dialog_ui.**h#newcode37<https://chromiumcodereview.appspot.com/10353007/diff/1/ui/web_dialogs/web_dialog_ui.h#newcode37>
> ui/web_dialogs/web_dialog_ui.**h:37: namespace views {
> On 2012/05/03 22:56:22, mazda wrote:
>
>> This should not be in views namespace.
>>
>
> either ui or web_dialogs. My preference is for ui namespace.
>
>
https://chromiumcodereview.**appspot.com/10353007/<https://chromiumcodereview...
>

[mazda, 2012-05-04 05:01:38]

I ran try bot on your behalf.
http://www.chromium.org/developers/testing/try-server-usage

If there are any compile errors, please fix them as well.

http://codereview.chromium.org/10353007/diff/24056/chrome/browser/printing/cl...
File chrome/browser/printing/cloud_print/cloud_print_setup_flow.h (right):

http://codereview.chromium.org/10353007/diff/24056/chrome/browser/printing/cl...
chrome/browser/printing/cloud_print/cloud_print_setup_flow.h:13: #include
"ui/web_dialogs/web_dialog_delegate.h"
Please sort headers alphabetically.
You can do it by running command something like the following.

tools/sort-headers.py `git diff master --name-only`

http://codereview.chromium.org/10353007/diff/24056/chrome/browser/ui/browser.h
File chrome/browser/ui/browser.h (right):

http://codereview.chromium.org/10353007/diff/24056/chrome/browser/ui/browser....
chrome/browser/ui/browser.h:74: namespace web_dialogs {
Please move this below gfx.

http://codereview.chromium.org/10353007/diff/24056/chrome/browser/ui/browser_...
File chrome/browser/ui/browser_dialogs.h (right):

http://codereview.chromium.org/10353007/diff/24056/chrome/browser/ui/browser_...
chrome/browser/ui/browser_dialogs.h:20: namespace web_dialogs {
Please move this below content.

http://codereview.chromium.org/10353007/diff/24056/chrome/browser/ui/webui/ex...
File chrome/browser/ui/webui/external_web_dialog_ui.h (right):

http://codereview.chromium.org/10353007/diff/24056/chrome/browser/ui/webui/ex...
chrome/browser/ui/webui/external_web_dialog_ui.h:41: class ExternalWebDialogUI :
public web_dialogs::WebDialogUI {
Why not move this class to ui/web_dialogs?

http://codereview.chromium.org/10353007/diff/24056/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/10353007/diff/24056/chrome/chrome_browser.gypi...
chrome/chrome_browser.gypi:3886:
'browser/ui/webui/chrome_web_dialog_web_contents_delegate.cc',
Please sort the file names.

http://codereview.chromium.org/10353007/diff/24056/ui/web_dialogs/web_dialog_...
File ui/web_dialogs/web_dialog_ui.cc (right):

http://codereview.chromium.org/10353007/diff/24056/ui/web_dialogs/web_dialog_...
ui/web_dialogs/web_dialog_ui.cc:6: #include "ui/web_dialogs/web_dialog_ui.h"
Please move this to the top.

http://codereview.chromium.org/10353007/diff/24056/ui/web_dialogs/web_dialog_...
File ui/web_dialogs/web_dialog_ui.h (right):

http://codereview.chromium.org/10353007/diff/24056/ui/web_dialogs/web_dialog_...
ui/web_dialogs/web_dialog_ui.h:10: #include <vector>
Please remove unnecessary headers.

[jakesays, 2012-05-04 05:07:58]

https://chromiumcodereview.appspot.com/10353007/diff/24056/chrome/browser/ui/...
File chrome/browser/ui/webui/external_web_dialog_ui.h (right):

https://chromiumcodereview.appspot.com/10353007/diff/24056/chrome/browser/ui/...
chrome/browser/ui/webui/external_web_dialog_ui.h:41: class ExternalWebDialogUI :
public web_dialogs::WebDialogUI {
On 2012/05/04 05:01:38, mazda wrote:
> Why not move this class to ui/web_dialogs?

It felt like it was chrome specific (as in an implementation of the generic
functionality that is WebDialogUI). But I can move it as well.

[jakesays, 2012-05-04 05:10:53]

https://chromiumcodereview.appspot.com/10353007/diff/24056/chrome/browser/pri...
File chrome/browser/printing/cloud_print/cloud_print_setup_flow.h (right):

https://chromiumcodereview.appspot.com/10353007/diff/24056/chrome/browser/pri...
chrome/browser/printing/cloud_print/cloud_print_setup_flow.h:13: #include
"ui/web_dialogs/web_dialog_delegate.h"
On 2012/05/04 05:01:38, mazda wrote:
> Please sort headers alphabetically.
> You can do it by running command something like the following.
> 
> tools/sort-headers.py `git diff master --name-only`

just an FYI, but I can't get sort-headers.py to work on windows.

[tfarina, 2012-05-04 18:09:13]

-me. Moving myself to CC list.

You should probably get Ben to review this as well.

[tfarina, 2012-05-04 18:10:39]

http://codereview.chromium.org/10353007/diff/25015/ui/web_dialogs/web_dialog_...
File ui/web_dialogs/web_dialog_delegate.h (right):

http://codereview.chromium.org/10353007/diff/25015/ui/web_dialogs/web_dialog_...
ui/web_dialogs/web_dialog_delegate.h:5: #ifndef
UI_WEB_DIALOGS_WEB_DIALOG_DELEGATE_H_
Please, fix this! You should move in a way that it keeps the history. The way
you moved lost all the original authorship.

[tfarina, 2012-05-04 18:12:21]

http://codereview.chromium.org/10353007/diff/25015/ui/web_dialogs/web_dialog_...
File ui/web_dialogs/web_dialog_ui.h (right):

http://codereview.chromium.org/10353007/diff/25015/ui/web_dialogs/web_dialog_...
ui/web_dialogs/web_dialog_ui.h:35: namespace web_dialogs {
Just to note, web_dialogs seems not a good namespace to me. Most if not all
classes in ui/ directory are in ui namespace. And web_dialogs seems redundat
because all classes in this API/module are prefixed with WebDialog.

[jakesays, 2012-05-04 18:15:47]

I'm ok with whatever namespace you guys decide on. I used web_dialogs based on
feedback from Ben.

On 2012/05/04 18:12:21, tfarina wrote:
>
http://codereview.chromium.org/10353007/diff/25015/ui/web_dialogs/web_dialog_...
> File ui/web_dialogs/web_dialog_ui.h (right):
> 
>
http://codereview.chromium.org/10353007/diff/25015/ui/web_dialogs/web_dialog_...
> ui/web_dialogs/web_dialog_ui.h:35: namespace web_dialogs {
> Just to note, web_dialogs seems not a good namespace to me. Most if not all
> classes in ui/ directory are in ui namespace. And web_dialogs seems redundat
> because all classes in this API/module are prefixed with WebDialog.

[tfarina, 2012-05-04 18:16:37]

http://codereview.chromium.org/10353007/diff/25015/chrome/browser/ui/browser.h
File chrome/browser/ui/browser.h (right):

http://codereview.chromium.org/10353007/diff/25015/chrome/browser/ui/browser....
chrome/browser/ui/browser.h:498:
BrowserShowWebDialog(web_dialogs::WebDialogDelegate* delegate,
the old indentation is right, when the parameter exceeds 80 chars, you should
move the parameter to the next line and indent 4 spaces. The function name stays
on line 497

[jakesays, 2012-05-04 18:18:09]

https://chromiumcodereview.appspot.com/10353007/diff/25015/ui/web_dialogs/web...
File ui/web_dialogs/web_dialog_delegate.h (right):

https://chromiumcodereview.appspot.com/10353007/diff/25015/ui/web_dialogs/web...
ui/web_dialogs/web_dialog_delegate.h:5: #ifndef
UI_WEB_DIALOGS_WEB_DIALOG_DELEGATE_H_
How do I go about doing that? I used the same process for this file that I did
for the others.

On 2012/05/04 18:10:39, tfarina wrote:
> Please, fix this! You should move in a way that it keeps the history. The way
> you moved lost all the original authorship.

[tfarina, 2012-05-04 18:21:02]

https://chromiumcodereview.appspot.com/10353007/diff/25015/ui/web_dialogs/web...
File ui/web_dialogs/web_dialog_delegate.h (right):

https://chromiumcodereview.appspot.com/10353007/diff/25015/ui/web_dialogs/web...
ui/web_dialogs/web_dialog_delegate.h:5: #ifndef
UI_WEB_DIALOGS_WEB_DIALOG_DELEGATE_H_
On 2012/05/04 18:18:09, jakesays wrote:
> How do I go about doing that? I used the same process for this file that I did
> for the others.
>
I don't know what you did. Did you move using svn or git? When I move files with
git, rietveld shows correct the history. The way it's now seems that this file
was written from scratch, but it wasn't.

[jakesays, 2012-05-04 18:26:44]

https://chromiumcodereview.appspot.com/10353007/diff/25015/ui/web_dialogs/web...
File ui/web_dialogs/web_dialog_delegate.h (right):

https://chromiumcodereview.appspot.com/10353007/diff/25015/ui/web_dialogs/web...
ui/web_dialogs/web_dialog_delegate.h:5: #ifndef
UI_WEB_DIALOGS_WEB_DIALOG_DELEGATE_H_
Eh, it didn't even occur to me to move. I just added/deleted. Sorry about that -
I'll upload a new patch when I return from lunch.

On 2012/05/04 18:21:02, tfarina wrote:
> On 2012/05/04 18:18:09, jakesays wrote:
> > How do I go about doing that? I used the same process for this file that I
did
> > for the others.
> >
> I don't know what you did. Did you move using svn or git? When I move files
with
> git, rietveld shows correct the history. The way it's now seems that this file
> was written from scratch, but it wasn't.
>