Change 'Add Page' to show a simple input dialog with --use-more-webui.

Change 'Add Page' to show a simple input dialog with --use-more-webui.

- Refactored InputWindowDialog so that it takes more than two inputs.
- Renamed BookmarkFolderEdtorController to BookmarkInputWindowDialogController
  for reflecting the change.

BUG=101412
TEST=manually with use_aura=1


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

[mazda, 2011-11-02 13:57:30]

Added screensthot:
http://chromium.googlecode.com/issues/attachment?aid=1014120009000&name=addpa...

[flackr, 2011-11-02 15:38:18]

http://codereview.chromium.org/8438037/diff/1/chrome/browser/bookmarks/bookma...
File chrome/browser/bookmarks/bookmark_context_menu_controller.cc (right):

http://codereview.chromium.org/8438037/diff/1/chrome/browser/bookmarks/bookma...
chrome/browser/bookmarks/bookmark_context_menu_controller.cc:143: if
(ChromeWebUI::IsMoreWebUI()) {
There is already a check for IsMoreWebUI in BookmarkEditor::Show, you should
trigger BookmarkInputWindowDialogController::Show from BookmarkEditor::Show to
keep the handling of IsMoreWebUI and how to edit a URL in one place.

http://codereview.chromium.org/8438037/diff/1/chrome/browser/bookmarks/bookma...
chrome/browser/bookmarks/bookmark_context_menu_controller.cc:158:
BookmarkInputWindowDialogController::Show(
This could also be handled in BookmarkEditor::Show, as the means for editing
folder nodes.

http://codereview.chromium.org/8438037/diff/1/chrome/browser/bookmarks/bookma...
chrome/browser/bookmarks/bookmark_context_menu_controller.cc:184: if
(ChromeWebUI::IsMoreWebUI()) {
Same as above, I would prefer to see bookmark editing handled in one place.
BookmarkEditor::EditDetails::AddNodeInFolder will need to be changed to support
passing the index parameter.

http://codereview.chromium.org/8438037/diff/1/chrome/browser/bookmarks/bookma...
chrome/browser/bookmarks/bookmark_context_menu_controller.cc:216:
BookmarkInputWindowDialogController::Show(
Same as above, call BookmarkEditor::Show with new folder and handle this case
there.

http://codereview.chromium.org/8438037/diff/1/chrome/browser/bookmarks/bookma...
File chrome/browser/bookmarks/bookmark_input_window_dialog_controller.h (right):

http://codereview.chromium.org/8438037/diff/1/chrome/browser/bookmarks/bookma...
chrome/browser/bookmarks/bookmark_input_window_dialog_controller.h:25: enum Type
{
BookmarkEditor::EditDetails already encapsulates this information. You should
make use of this class to pass the type and node information.

http://codereview.chromium.org/8438037/diff/1/chrome/browser/bookmarks/bookma...
chrome/browser/bookmarks/bookmark_input_window_dialog_controller.h:34: static
void Show(Profile* profile,
Avoid exposing a public method which takes arguments which are only relevant for
certain operations (i.e. index only relevant for NEW_BOOKMARK_*). Ideally you
can take in BookmarkEditor::EditDetails and get all of the relevant information
from this structure.

http://codereview.chromium.org/8438037/diff/1/chrome/browser/resources/input_...
File chrome/browser/resources/input_window_dialog.css (right):

http://codereview.chromium.org/8438037/diff/1/chrome/browser/resources/input_...
chrome/browser/resources/input_window_dialog.css:17: -webkit-box-orient:
vertical;
nit: Sort properties.

http://codereview.chromium.org/8438037/diff/1/chrome/browser/resources/input_...
File chrome/browser/resources/input_window_dialog.js (right):

http://codereview.chromium.org/8438037/diff/1/chrome/browser/resources/input_...
chrome/browser/resources/input_window_dialog.js:25: if
($('url-area').style.display != 'none') {
Use class to determine whether a URL is being edited so that the logic is not
dependent on the view.

http://codereview.chromium.org/8438037/diff/1/chrome/browser/resources/input_...
chrome/browser/resources/input_window_dialog.js:46: $('url-area').style.display
= '-webkit-box';
Use class to toggle CSS properties.

http://codereview.chromium.org/8438037/diff/1/chrome/browser/resources/input_...
chrome/browser/resources/input_window_dialog.js:63: if
($('url-area').style.display != 'none') {
Same as above, check class instead of style.

[mazda, 2011-11-07 09:59:23]

Thank you for the review.
Could you take another look at the change?

http://codereview.chromium.org/8438037/diff/1/chrome/browser/bookmarks/bookma...
File chrome/browser/bookmarks/bookmark_context_menu_controller.cc (right):

http://codereview.chromium.org/8438037/diff/1/chrome/browser/bookmarks/bookma...
chrome/browser/bookmarks/bookmark_context_menu_controller.cc:143: if
(ChromeWebUI::IsMoreWebUI()) {
On 2011/11/02 15:38:18, flackr wrote:
> There is already a check for IsMoreWebUI in BookmarkEditor::Show, you should
> trigger BookmarkInputWindowDialogController::Show from BookmarkEditor::Show to
> keep the handling of IsMoreWebUI and how to edit a URL in one place.

Done.

http://codereview.chromium.org/8438037/diff/1/chrome/browser/bookmarks/bookma...
chrome/browser/bookmarks/bookmark_context_menu_controller.cc:158:
BookmarkInputWindowDialogController::Show(
On 2011/11/02 15:38:18, flackr wrote:
> This could also be handled in BookmarkEditor::Show, as the means for editing
> folder nodes.

Done.

http://codereview.chromium.org/8438037/diff/1/chrome/browser/bookmarks/bookma...
chrome/browser/bookmarks/bookmark_context_menu_controller.cc:184: if
(ChromeWebUI::IsMoreWebUI()) {
On 2011/11/02 15:38:18, flackr wrote:
> Same as above, I would prefer to see bookmark editing handled in one place.
> BookmarkEditor::EditDetails::AddNodeInFolder will need to be changed to
support
> passing the index parameter.

Done.

http://codereview.chromium.org/8438037/diff/1/chrome/browser/bookmarks/bookma...
chrome/browser/bookmarks/bookmark_context_menu_controller.cc:216:
BookmarkInputWindowDialogController::Show(
On 2011/11/02 15:38:18, flackr wrote:
> Same as above, call BookmarkEditor::Show with new folder and handle this case
> there.

Done.

http://codereview.chromium.org/8438037/diff/1/chrome/browser/bookmarks/bookma...
File chrome/browser/bookmarks/bookmark_input_window_dialog_controller.h (right):

http://codereview.chromium.org/8438037/diff/1/chrome/browser/bookmarks/bookma...
chrome/browser/bookmarks/bookmark_input_window_dialog_controller.h:25: enum Type
{
On 2011/11/02 15:38:18, flackr wrote:
> BookmarkEditor::EditDetails already encapsulates this information. You should
> make use of this class to pass the type and node information.

Deleted this enum.

http://codereview.chromium.org/8438037/diff/1/chrome/browser/bookmarks/bookma...
chrome/browser/bookmarks/bookmark_input_window_dialog_controller.h:34: static
void Show(Profile* profile,
On 2011/11/02 15:38:18, flackr wrote:
> Avoid exposing a public method which takes arguments which are only relevant
for
> certain operations (i.e. index only relevant for NEW_BOOKMARK_*). Ideally you
> can take in BookmarkEditor::EditDetails and get all of the relevant
information
> from this structure.

I added index to BookmarkEditor::EditaDetails and deleted index argument from
this function.

http://codereview.chromium.org/8438037/diff/1/chrome/browser/resources/input_...
File chrome/browser/resources/input_window_dialog.css (right):

http://codereview.chromium.org/8438037/diff/1/chrome/browser/resources/input_...
chrome/browser/resources/input_window_dialog.css:17: -webkit-box-orient:
vertical;
On 2011/11/02 15:38:18, flackr wrote:
> nit: Sort properties.

Done.

http://codereview.chromium.org/8438037/diff/1/chrome/browser/resources/input_...
File chrome/browser/resources/input_window_dialog.js (right):

http://codereview.chromium.org/8438037/diff/1/chrome/browser/resources/input_...
chrome/browser/resources/input_window_dialog.js:25: if
($('url-area').style.display != 'none') {
On 2011/11/02 15:38:18, flackr wrote:
> Use class to determine whether a URL is being edited so that the logic is not
> dependent on the view.

Done.
I added a function to check if a URL field is shown.

http://codereview.chromium.org/8438037/diff/1/chrome/browser/resources/input_...
chrome/browser/resources/input_window_dialog.js:46: $('url-area').style.display
= '-webkit-box';
On 2011/11/02 15:38:18, flackr wrote:
> Use class to toggle CSS properties.

Done.

http://codereview.chromium.org/8438037/diff/1/chrome/browser/resources/input_...
chrome/browser/resources/input_window_dialog.js:63: if
($('url-area').style.display != 'none') {
On 2011/11/02 15:38:18, flackr wrote:
> Same as above, check class instead of style.

Done.

[flackr, 2011-11-07 15:19:14]

http://codereview.chromium.org/8438037/diff/8001/chrome/browser/bookmarks/boo...
File chrome/browser/bookmarks/bookmark_input_window_dialog_controller.cc
(right):

http://codereview.chromium.org/8438037/diff/8001/chrome/browser/bookmarks/boo...
chrome/browser/bookmarks/bookmark_input_window_dialog_controller.cc:126:
model_->AddFolder(details_.parent_node, details_.index, texts[0]);
At this point we could add details_.urls to the folder to allow for bookmark all
tabs behaviour.

http://codereview.chromium.org/8438037/diff/8001/chrome/browser/ui/input_wind...
File chrome/browser/ui/input_window_dialog_win.cc (right):

http://codereview.chromium.org/8438037/diff/8001/chrome/browser/ui/input_wind...
chrome/browser/ui/input_window_dialog_win.cc:265: DCHECK_EQ(1U,
label_contents_pairs.size());
Can you use InputWindowDialogWebUI here too on --use-more-webui? There has been
some debate over using WebUI dialogs on all platforms so it would be nice to
have it work behind a flag.

[mazda, 2011-11-08 11:24:24]

Please take another look.

http://codereview.chromium.org/8438037/diff/8001/chrome/browser/bookmarks/boo...
File chrome/browser/bookmarks/bookmark_input_window_dialog_controller.cc
(right):

http://codereview.chromium.org/8438037/diff/8001/chrome/browser/bookmarks/boo...
chrome/browser/bookmarks/bookmark_input_window_dialog_controller.cc:126:
model_->AddFolder(details_.parent_node, details_.index, texts[0]);
On 2011/11/07 15:19:15, flackr wrote:
> At this point we could add details_.urls to the folder to allow for bookmark
all
> tabs behaviour.

May I do it in another CL? Adding an argument makes this CL a bit big.
I wrote a TODO comment.

http://codereview.chromium.org/8438037/diff/8001/chrome/browser/ui/input_wind...
File chrome/browser/ui/input_window_dialog_win.cc (right):

http://codereview.chromium.org/8438037/diff/8001/chrome/browser/ui/input_wind...
chrome/browser/ui/input_window_dialog_win.cc:265: DCHECK_EQ(1U,
label_contents_pairs.size());
On 2011/11/07 15:19:15, flackr wrote:
> Can you use InputWindowDialogWebUI here too on --use-more-webui? There has
been
> some debate over using WebUI dialogs on all platforms so it would be nice to
> have it work behind a flag.

Done.

[flackr, 2011-11-08 13:48:54]

LGTM, this is great.

http://codereview.chromium.org/8438037/diff/8001/chrome/browser/bookmarks/boo...
File chrome/browser/bookmarks/bookmark_input_window_dialog_controller.cc
(right):

http://codereview.chromium.org/8438037/diff/8001/chrome/browser/bookmarks/boo...
chrome/browser/bookmarks/bookmark_input_window_dialog_controller.cc:126:
model_->AddFolder(details_.parent_node, details_.index, texts[0]);
On 2011/11/08 11:24:24, mazda wrote:
> On 2011/11/07 15:19:15, flackr wrote:
> > At this point we could add details_.urls to the folder to allow for bookmark
> all
> > tabs behaviour.
> 
> May I do it in another CL? Adding an argument makes this CL a bit big.
> I wrote a TODO comment.

Of course, no problem.

[commit-bot: I haz the power, 2011-11-09 00:40:27]

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

[commit-bot: I haz the power, 2011-11-09 02:09:39]

Change committed as 109169