Implement Bookmark All Tabs Dialog with WebUI.

Implement Bookmark All Tabs Dialog with WebUI. 

Currently, this does not work with use_aura=1. 

BUG=97839
TEST=manual on Win and Linux with toolkit_views=1


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

[yoshiki, 2011-11-08 09:38:49]

Please take a look.

[mazda, 2011-11-09 04:49:43]

http://codereview.chromium.org/8497008/diff/1/chrome/app/generated_resources.grd
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/8497008/diff/1/chrome/app/generated_resources....
chrome/app/generated_resources.grd:10063: <message
name="IDS_BOOKMARK_MANAGER_BOOKMARK_ALL_TABS" desc="Title of the window the
bookmark all tabs dialog.">
Can't we use the existing message used by the native implementation?

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

http://codereview.chromium.org/8497008/diff/1/chrome/browser/bookmarks/bookma...
chrome/browser/bookmarks/bookmark_editor.h:90: #if defined(OS_WIN) ||
defined(TOOLKIT_VIEWS)
I think it's better to switch the implementation with --use-more-webui flag
instead of ifdef.

http://codereview.chromium.org/8497008/diff/1/chrome/browser/resources/bookma...
File chrome/browser/resources/bookmark_manager/bookmark_all_tabs.html (right):

http://codereview.chromium.org/8497008/diff/1/chrome/browser/resources/bookma...
chrome/browser/resources/bookmark_manager/bookmark_all_tabs.html:61: <button
id="ok" i18n-content="ok">OK</button>
This button should be labeled with "Save".
http://code.google.com/p/chromium/issues/detail?id=20053#c8

http://codereview.chromium.org/8497008/diff/1/chrome/browser/resources/bookma...
File chrome/browser/resources/bookmark_manager/js/bmm.js (right):

http://codereview.chromium.org/8497008/diff/1/chrome/browser/resources/bookma...
chrome/browser/resources/bookmark_manager/js/bmm.js:74: for (var i = 0, item;
item = items[i]; i++) {
for (var i = 0; i < items.length; ++i) {

http://codereview.chromium.org/8497008/diff/1/chrome/browser/resources/bookma...
chrome/browser/resources/bookmark_manager/js/bmm.js:138: //
console.info('handleCreated', id, bookmarkNode);
Please delete this.

http://codereview.chromium.org/8497008/diff/1/chrome/browser/resources/bookma...
chrome/browser/resources/bookmark_manager/js/bmm.js:146: function
handleMoved(id, moveInfo) {
Please add comment.

http://codereview.chromium.org/8497008/diff/1/chrome/browser/resources/bookma...
chrome/browser/resources/bookmark_manager/js/bmm.js:147: //
console.info('handleMoved', id, moveInfo);
Please delete this.

http://codereview.chromium.org/8497008/diff/1/chrome/browser/resources/bookma...
chrome/browser/resources/bookmark_manager/js/bmm.js:158: function
handleRemoved(id, removeInfo) {
Please add comment.

http://codereview.chromium.org/8497008/diff/1/chrome/browser/resources/bookma...
chrome/browser/resources/bookmark_manager/js/bmm.js:159: //
console.info('handleRemoved', id, removeInfo);
Please delete this.

http://codereview.chromium.org/8497008/diff/1/chrome/browser/resources/bookma...
chrome/browser/resources/bookmark_manager/js/bmm.js:169: function
handleImportBegan() {
Please add comment.

http://codereview.chromium.org/8497008/diff/1/chrome/browser/resources/bookma...
chrome/browser/resources/bookmark_manager/js/bmm.js:174: function
handleImportEnded() {
Please add comment.

http://codereview.chromium.org/8497008/diff/1/chrome/browser/resources/bookma...
chrome/browser/resources/bookmark_manager/js/bmm.js:222: 
Unnecessary empty line.

http://codereview.chromium.org/8497008/diff/1/chrome/browser/resources/bookma...
File chrome/browser/resources/bookmark_manager/js/bookmark_all_tabs.js (right):

http://codereview.chromium.org/8497008/diff/1/chrome/browser/resources/bookma...
chrome/browser/resources/bookmark_manager/js/bookmark_all_tabs.js:91: //
console.info('navigateTo', 'from', window.location.hash, 'to', id);
Please delete this.

http://codereview.chromium.org/8497008/diff/1/chrome/browser/resources/bookma...
chrome/browser/resources/bookmark_manager/js/bookmark_all_tabs.js:119: }
Please add return for the case that parentNode is not found.

http://codereview.chromium.org/8497008/diff/1/chrome/browser/resources/bookma...
chrome/browser/resources/bookmark_manager/js/bookmark_all_tabs.js:122: // Checks
disabilities of the buttons.
Please format the comment.

http://codereview.chromium.org/8497008/diff/1/chrome/browser/resources/bookma...
chrome/browser/resources/bookmark_manager/js/bookmark_all_tabs.js:155: function
cancel() {
Please add comment.

http://codereview.chromium.org/8497008/diff/1/chrome/browser/resources/bookma...
chrome/browser/resources/bookmark_manager/js/bookmark_all_tabs.js:179: function
callbackAllTabs(tabs) {
Ditto.

http://codereview.chromium.org/8497008/diff/1/chrome/browser/resources/bookma...
chrome/browser/resources/bookmark_manager/js/bookmark_all_tabs.js:196: if
(parentId == 0 || newFolderName == "")
if (parentId == 0 || !newFolderName)

http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml?showone...

http://codereview.chromium.org/8497008/diff/1/chrome/browser/ui/views/extensi...
File chrome/browser/ui/views/extensions/extension_dialog.cc (right):

http://codereview.chromium.org/8497008/diff/1/chrome/browser/ui/views/extensi...
chrome/browser/ui/views/extensions/extension_dialog.cc:67: std::string title,
const string16& title

http://codereview.chromium.org/8497008/diff/1/chrome/browser/ui/views/extensi...
chrome/browser/ui/views/extensions/extension_dialog.cc:135: return
(window_title_ == "") ? false : true;
return !window_title_.empty();

http://codereview.chromium.org/8497008/diff/1/chrome/browser/ui/views/extensi...
File chrome/browser/ui/views/extensions/extension_dialog.h (right):

http://codereview.chromium.org/8497008/diff/1/chrome/browser/ui/views/extensi...
chrome/browser/ui/views/extensions/extension_dialog.h:45: std::string title,
const string16& title

http://codereview.chromium.org/8497008/diff/1/chrome/browser/ui/views/extensi...
chrome/browser/ui/views/extensions/extension_dialog.h:56: void
set_title(std::string title) { window_title_ = title; }
const string16& title

http://codereview.chromium.org/8497008/diff/1/chrome/browser/ui/views/extensi...
chrome/browser/ui/views/extensions/extension_dialog.h:88: std::string
window_title_;
string16 window_title_;

http://codereview.chromium.org/8497008/diff/1/chrome/browser/ui/webui/bookmar...
File chrome/browser/ui/webui/bookmark_all_tabs_dialog.cc (right):

http://codereview.chromium.org/8497008/diff/1/chrome/browser/ui/webui/bookmar...
chrome/browser/ui/webui/bookmark_all_tabs_dialog.cc:47:
BookmarkAllTabsDialogBuilder::BookmarkAllTabsDialogBuilder(Profile* profile) {
BookmarkAllTabsDialogBuilder::BookmarkAllTabsDialogBuilder(Profile* profile)
    : profile_(profile) {

http://codereview.chromium.org/8497008/diff/1/chrome/browser/ui/webui/bookmar...
chrome/browser/ui/webui/bookmark_all_tabs_dialog.cc:60: NOTREACHED() << "Can't
find owning browser";
LOG(ERROR) is more appropriate?

http://codereview.chromium.org/8497008/diff/1/chrome/browser/ui/webui/bookmar...
chrome/browser/ui/webui/bookmark_all_tabs_dialog.cc:88: = new
BookmarkAllTabsDialogBuilder(profile);
Is this deleted?

[yoshiki, 2011-11-09 15:06:38]

http://codereview.chromium.org/8497008/diff/1/chrome/app/generated_resources.grd
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/8497008/diff/1/chrome/app/generated_resources....
chrome/app/generated_resources.grd:10063: <message
name="IDS_BOOKMARK_MANAGER_BOOKMARK_ALL_TABS" desc="Title of the window the
bookmark all tabs dialog.">
On 2011/11/09 04:49:43, mazda wrote:
> Can't we use the existing message used by the native implementation?

There are IDS_BOOKMARK_ALL_TABS_MAC and IDS_BOOKMARK_ALL_TABS_LINUX, which are
depended on platform. On windows, dialog title is "Edit Bookmark"
(IDS_BOOKMARK_EDITOR_TITLE).
I think it is good to create a new resource string.

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

http://codereview.chromium.org/8497008/diff/1/chrome/browser/bookmarks/bookma...
chrome/browser/bookmarks/bookmark_editor.h:90: #if defined(OS_WIN) ||
defined(TOOLKIT_VIEWS)
On 2011/11/09 04:49:43, mazda wrote:
> I think it's better to switch the implementation with --use-more-webui flag
> instead of ifdef.

Done.

http://codereview.chromium.org/8497008/diff/1/chrome/browser/resources/bookma...
File chrome/browser/resources/bookmark_manager/bookmark_all_tabs.html (right):

http://codereview.chromium.org/8497008/diff/1/chrome/browser/resources/bookma...
chrome/browser/resources/bookmark_manager/bookmark_all_tabs.html:61: <button
id="ok" i18n-content="ok">OK</button>
On 2011/11/09 04:49:43, mazda wrote:
> This button should be labeled with "Save".
> http://code.google.com/p/chromium/issues/detail?id=20053#c8

Done.

http://codereview.chromium.org/8497008/diff/1/chrome/browser/resources/bookma...
File chrome/browser/resources/bookmark_manager/js/bmm.js (right):

http://codereview.chromium.org/8497008/diff/1/chrome/browser/resources/bookma...
chrome/browser/resources/bookmark_manager/js/bmm.js:74: for (var i = 0, item;
item = items[i]; i++) {
On 2011/11/09 04:49:43, mazda wrote:
> for (var i = 0; i < items.length; ++i) {

Done.

http://codereview.chromium.org/8497008/diff/1/chrome/browser/resources/bookma...
chrome/browser/resources/bookmark_manager/js/bmm.js:138: //
console.info('handleCreated', id, bookmarkNode);
On 2011/11/09 04:49:43, mazda wrote:
> Please delete this.

Done.

http://codereview.chromium.org/8497008/diff/1/chrome/browser/resources/bookma...
chrome/browser/resources/bookmark_manager/js/bmm.js:146: function
handleMoved(id, moveInfo) {
On 2011/11/09 04:49:43, mazda wrote:
> Please add comment.

Done.

http://codereview.chromium.org/8497008/diff/1/chrome/browser/resources/bookma...
chrome/browser/resources/bookmark_manager/js/bmm.js:147: //
console.info('handleMoved', id, moveInfo);
On 2011/11/09 04:49:43, mazda wrote:
> Please delete this.

Done.

http://codereview.chromium.org/8497008/diff/1/chrome/browser/resources/bookma...
chrome/browser/resources/bookmark_manager/js/bmm.js:158: function
handleRemoved(id, removeInfo) {
On 2011/11/09 04:49:43, mazda wrote:
> Please add comment.

Done.

http://codereview.chromium.org/8497008/diff/1/chrome/browser/resources/bookma...
chrome/browser/resources/bookmark_manager/js/bmm.js:159: //
console.info('handleRemoved', id, removeInfo);
On 2011/11/09 04:49:43, mazda wrote:
> Please delete this.

Done.

http://codereview.chromium.org/8497008/diff/1/chrome/browser/resources/bookma...
chrome/browser/resources/bookmark_manager/js/bmm.js:169: function
handleImportBegan() {
On 2011/11/09 04:49:43, mazda wrote:
> Please add comment.

Done.

http://codereview.chromium.org/8497008/diff/1/chrome/browser/resources/bookma...
chrome/browser/resources/bookmark_manager/js/bmm.js:174: function
handleImportEnded() {
On 2011/11/09 04:49:43, mazda wrote:
> Please add comment.

Done.

http://codereview.chromium.org/8497008/diff/1/chrome/browser/resources/bookma...
chrome/browser/resources/bookmark_manager/js/bmm.js:222: 
On 2011/11/09 04:49:43, mazda wrote:
> Unnecessary empty line.

Done.

http://codereview.chromium.org/8497008/diff/1/chrome/browser/resources/bookma...
File chrome/browser/resources/bookmark_manager/js/bookmark_all_tabs.js (right):

http://codereview.chromium.org/8497008/diff/1/chrome/browser/resources/bookma...
chrome/browser/resources/bookmark_manager/js/bookmark_all_tabs.js:91: //
console.info('navigateTo', 'from', window.location.hash, 'to', id);
On 2011/11/09 04:49:43, mazda wrote:
> Please delete this.

Done.

http://codereview.chromium.org/8497008/diff/1/chrome/browser/resources/bookma...
chrome/browser/resources/bookmark_manager/js/bookmark_all_tabs.js:119: }
This function is no longer used here. Deleted.

On 2011/11/09 04:49:43, mazda wrote:
> Please add return for the case that parentNode is not found.

http://codereview.chromium.org/8497008/diff/1/chrome/browser/resources/bookma...
chrome/browser/resources/bookmark_manager/js/bookmark_all_tabs.js:122: // Checks
disabilities of the buttons.
On 2011/11/09 04:49:43, mazda wrote:
> Please format the comment.

Done.

http://codereview.chromium.org/8497008/diff/1/chrome/browser/resources/bookma...
chrome/browser/resources/bookmark_manager/js/bookmark_all_tabs.js:155: function
cancel() {
On 2011/11/09 04:49:43, mazda wrote:
> Please add comment.

Done.

http://codereview.chromium.org/8497008/diff/1/chrome/browser/resources/bookma...
chrome/browser/resources/bookmark_manager/js/bookmark_all_tabs.js:179: function
callbackAllTabs(tabs) {
This function is only used once. Made it inline.

On 2011/11/09 04:49:43, mazda wrote:
> Ditto.

http://codereview.chromium.org/8497008/diff/1/chrome/browser/resources/bookma...
chrome/browser/resources/bookmark_manager/js/bookmark_all_tabs.js:196: if
(parentId == 0 || newFolderName == "")
On 2011/11/09 04:49:43, mazda wrote:
> if (parentId == 0 || !newFolderName)
> 
>
http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml?showone...

Done.

http://codereview.chromium.org/8497008/diff/1/chrome/browser/ui/views/extensi...
File chrome/browser/ui/views/extensions/extension_dialog.cc (right):

http://codereview.chromium.org/8497008/diff/1/chrome/browser/ui/views/extensi...
chrome/browser/ui/views/extensions/extension_dialog.cc:67: std::string title,
On 2011/11/09 04:49:43, mazda wrote:
> const string16& title

Done.

http://codereview.chromium.org/8497008/diff/1/chrome/browser/ui/views/extensi...
chrome/browser/ui/views/extensions/extension_dialog.cc:135: return
(window_title_ == "") ? false : true;
On 2011/11/09 04:49:43, mazda wrote:
> return !window_title_.empty();

Done.

http://codereview.chromium.org/8497008/diff/1/chrome/browser/ui/views/extensi...
File chrome/browser/ui/views/extensions/extension_dialog.h (right):

http://codereview.chromium.org/8497008/diff/1/chrome/browser/ui/views/extensi...
chrome/browser/ui/views/extensions/extension_dialog.h:45: std::string title,
On 2011/11/09 04:49:43, mazda wrote:
> const string16& title

Done.

http://codereview.chromium.org/8497008/diff/1/chrome/browser/ui/views/extensi...
chrome/browser/ui/views/extensions/extension_dialog.h:56: void
set_title(std::string title) { window_title_ = title; }
On 2011/11/09 04:49:43, mazda wrote:
> const string16& title

Done.

http://codereview.chromium.org/8497008/diff/1/chrome/browser/ui/views/extensi...
chrome/browser/ui/views/extensions/extension_dialog.h:88: std::string
window_title_;
On 2011/11/09 04:49:43, mazda wrote:
> string16 window_title_;

Done.

http://codereview.chromium.org/8497008/diff/1/chrome/browser/ui/webui/bookmar...
File chrome/browser/ui/webui/bookmark_all_tabs_dialog.cc (right):

http://codereview.chromium.org/8497008/diff/1/chrome/browser/ui/webui/bookmar...
chrome/browser/ui/webui/bookmark_all_tabs_dialog.cc:47:
BookmarkAllTabsDialogBuilder::BookmarkAllTabsDialogBuilder(Profile* profile) {
On 2011/11/09 04:49:43, mazda wrote:
> BookmarkAllTabsDialogBuilder::BookmarkAllTabsDialogBuilder(Profile* profile)
>     : profile_(profile) {

Done.

http://codereview.chromium.org/8497008/diff/1/chrome/browser/ui/webui/bookmar...
chrome/browser/ui/webui/bookmark_all_tabs_dialog.cc:60: NOTREACHED() << "Can't
find owning browser";
On 2011/11/09 04:49:43, mazda wrote:
> LOG(ERROR) is more appropriate?

Done.

http://codereview.chromium.org/8497008/diff/1/chrome/browser/ui/webui/bookmar...
chrome/browser/ui/webui/bookmark_all_tabs_dialog.cc:88: = new
BookmarkAllTabsDialogBuilder(profile);
On 2011/11/09 04:49:43, mazda wrote:
> Is this deleted?

Done.

[mazda, 2011-11-10 06:03:46]

http://codereview.chromium.org/8497008/diff/8020/chrome/browser/bookmarks/boo...
File chrome/browser/bookmarks/bookmark_editor.cc (right):

http://codereview.chromium.org/8497008/diff/8020/chrome/browser/bookmarks/boo...
chrome/browser/bookmarks/bookmark_editor.cc:85: BookmarkModel* model =
profile->GetBookmarkModel();
It's nice to make a function to do things in else-block to reduce code
duplication.

http://codereview.chromium.org/8497008/diff/8020/chrome/browser/bookmarks/boo...
File chrome/browser/bookmarks/bookmark_editor.h (right):

http://codereview.chromium.org/8497008/diff/8020/chrome/browser/bookmarks/boo...
chrome/browser/bookmarks/bookmark_editor.h:97: static void
ShowBookmarkAllTabsDialog(Browser* browser);
Please add comment.

http://codereview.chromium.org/8497008/diff/8020/chrome/browser/resources/boo...
File chrome/browser/resources/bookmark_manager/js/bookmark_all_tabs.js (right):

http://codereview.chromium.org/8497008/diff/8020/chrome/browser/resources/boo...
chrome/browser/resources/bookmark_manager/js/bookmark_all_tabs.js:94:
navigateTo.timer_ = setTimeout(updateHash, 250);
Please make a constant variable.

http://codereview.chromium.org/8497008/diff/8020/chrome/browser/resources/boo...
chrome/browser/resources/bookmark_manager/js/bookmark_all_tabs.js:130: }, 50);
Please make a constant variable.

http://codereview.chromium.org/8497008/diff/8020/chrome/browser/resources/com...
File chrome/browser/resources/component_extension_resources.grd (right):

http://codereview.chromium.org/8497008/diff/8020/chrome/browser/resources/com...
chrome/browser/resources/component_extension_resources.grd:20: <include
name="IDR_BOOKMARK_MANAGER_BOOKMARK_ALL_TABS_JS"
file="bookmark_manager/js/bookmark_all_tabs.js" type="BINDATA" />
Is this necessary?

http://codereview.chromium.org/8497008/diff/8020/chrome/browser/ui/views/exte...
File chrome/browser/ui/views/extensions/extension_dialog.cc (right):

http://codereview.chromium.org/8497008/diff/8020/chrome/browser/ui/views/exte...
chrome/browser/ui/views/extensions/extension_dialog.cc:7: #include
"base/utf_string_conversions.h"
This is no longer necessary.

http://codereview.chromium.org/8497008/diff/8020/chrome/browser/ui/webui/book...
File chrome/browser/ui/webui/bookmark_all_tabs_dialog.cc (right):

http://codereview.chromium.org/8497008/diff/8020/chrome/browser/ui/webui/book...
chrome/browser/ui/webui/bookmark_all_tabs_dialog.cc:7: #include <string>
Unnecessary?

http://codereview.chromium.org/8497008/diff/8020/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/8497008/diff/8020/chrome/chrome_browser.gypi#n...
chrome/chrome_browser.gypi:3660: 'browser/ui/webui/bookmark_all_tabs_dialog.h',
.cc is first

[yoshiki, 2011-11-10 07:54:38]

http://codereview.chromium.org/8497008/diff/8020/chrome/browser/bookmarks/boo...
File chrome/browser/bookmarks/bookmark_editor.cc (right):

http://codereview.chromium.org/8497008/diff/8020/chrome/browser/bookmarks/boo...
chrome/browser/bookmarks/bookmark_editor.cc:85: BookmarkModel* model =
profile->GetBookmarkModel();
On 2011/11/10 06:03:46, mazda wrote:
> It's nice to make a function to do things in else-block to reduce code
> duplication.

Done.

http://codereview.chromium.org/8497008/diff/8020/chrome/browser/bookmarks/boo...
File chrome/browser/bookmarks/bookmark_editor.h (right):

http://codereview.chromium.org/8497008/diff/8020/chrome/browser/bookmarks/boo...
chrome/browser/bookmarks/bookmark_editor.h:97: static void
ShowBookmarkAllTabsDialog(Browser* browser);
On 2011/11/10 06:03:46, mazda wrote:
> Please add comment.

Done.

http://codereview.chromium.org/8497008/diff/8020/chrome/browser/resources/boo...
File chrome/browser/resources/bookmark_manager/js/bookmark_all_tabs.js (right):

http://codereview.chromium.org/8497008/diff/8020/chrome/browser/resources/boo...
chrome/browser/resources/bookmark_manager/js/bookmark_all_tabs.js:94:
navigateTo.timer_ = setTimeout(updateHash, 250);
On 2011/11/10 06:03:46, mazda wrote:
> Please make a constant variable.

Done.

http://codereview.chromium.org/8497008/diff/8020/chrome/browser/resources/boo...
chrome/browser/resources/bookmark_manager/js/bookmark_all_tabs.js:130: }, 50);
On 2011/11/10 06:03:46, mazda wrote:
> Please make a constant variable.

Done.

http://codereview.chromium.org/8497008/diff/8020/chrome/browser/resources/com...
File chrome/browser/resources/component_extension_resources.grd (right):

http://codereview.chromium.org/8497008/diff/8020/chrome/browser/resources/com...
chrome/browser/resources/component_extension_resources.grd:20: <include
name="IDR_BOOKMARK_MANAGER_BOOKMARK_ALL_TABS_JS"
file="bookmark_manager/js/bookmark_all_tabs.js" type="BINDATA" />
This is necessary, because JavaScript files are loaded externally due to
Content-Security-Policy.

On 2011/11/10 06:03:46, mazda wrote:
> Is this necessary?

http://codereview.chromium.org/8497008/diff/8020/chrome/browser/ui/views/exte...
File chrome/browser/ui/views/extensions/extension_dialog.cc (right):

http://codereview.chromium.org/8497008/diff/8020/chrome/browser/ui/views/exte...
chrome/browser/ui/views/extensions/extension_dialog.cc:7: #include
"base/utf_string_conversions.h"
On 2011/11/10 06:03:46, mazda wrote:
> This is no longer necessary.

Done.

http://codereview.chromium.org/8497008/diff/8020/chrome/browser/ui/webui/book...
File chrome/browser/ui/webui/bookmark_all_tabs_dialog.cc (right):

http://codereview.chromium.org/8497008/diff/8020/chrome/browser/ui/webui/book...
chrome/browser/ui/webui/bookmark_all_tabs_dialog.cc:7: #include <string>
On 2011/11/10 06:03:46, mazda wrote:
> Unnecessary?

Done.

http://codereview.chromium.org/8497008/diff/8020/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/8497008/diff/8020/chrome/chrome_browser.gypi#n...
chrome/chrome_browser.gypi:3660: 'browser/ui/webui/bookmark_all_tabs_dialog.h',
On 2011/11/10 06:03:46, mazda wrote:
> .cc is first

Done.

[mazda, 2011-11-10 12:47:53]

lgtm

[commit-bot: I haz the power, 2011-11-14 16:22:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoshiki@chromium.org/8497008/8026

[commit-bot: I haz the power, 2011-11-14 16:22:07]

Can't process patch for file chrome/chrome_browser.gypi.
File's status is None, patchset upload is incomplete.

[tfarina, 2011-11-14 17:33:04]

http://codereview.chromium.org/8497008/diff/15001/chrome/browser/ui/webui/boo...
File chrome/browser/ui/webui/bookmark_all_tabs_dialog.cc (right):

http://codereview.chromium.org/8497008/diff/15001/chrome/browser/ui/webui/boo...
chrome/browser/ui/webui/bookmark_all_tabs_dialog.cc:16: const char*
kBookmarkAllTabsURL =
nit: const char kBookmarkAllTabsURL[]

http://codereview.chromium.org/8497008/diff/15001/chrome/browser/ui/webui/boo...
chrome/browser/ui/webui/bookmark_all_tabs_dialog.cc:18:
"eemcgdkfndhakfknompkggombfjjjeno/"
can you save one line and fold this into the above line? I think it fits;

http://codereview.chromium.org/8497008/diff/15001/chrome/browser/ui/webui/boo...
chrome/browser/ui/webui/bookmark_all_tabs_dialog.cc:33: virtual void
ExtensionDialogIsClosing(ExtensionDialog* dialog);
Add OVERRIDE and compiler_specific.h

http://codereview.chromium.org/8497008/diff/15001/chrome/browser/ui/webui/boo...
chrome/browser/ui/webui/bookmark_all_tabs_dialog.cc:87: //
****************************************************
weird :(

http://codereview.chromium.org/8497008/diff/15001/chrome/browser/ui/webui/boo...
chrome/browser/ui/webui/bookmark_all_tabs_dialog.cc:96: 
remove this blank line.

[tfarina, 2011-11-14 17:34:20]

http://codereview.chromium.org/8497008/diff/15001/chrome/browser/ui/webui/boo...
File chrome/browser/ui/webui/bookmark_all_tabs_dialog.h (right):

http://codereview.chromium.org/8497008/diff/15001/chrome/browser/ui/webui/boo...
chrome/browser/ui/webui/bookmark_all_tabs_dialog.h:11: class
BookmarkAllTabsDialog {
Can you convert this into a function instead?

void ShowBookmarkAllTabsDialog(Profile* profile);

http://codereview.chromium.org/8497008/diff/15001/chrome/browser/ui/webui/boo...
chrome/browser/ui/webui/bookmark_all_tabs_dialog.h:13: static void Show(Profile*
profile);
forward declare Profile.

[yoshiki, 2011-11-17 04:09:37]

tfarina: Thanks!

http://codereview.chromium.org/8497008/diff/15001/chrome/browser/ui/webui/boo...
File chrome/browser/ui/webui/bookmark_all_tabs_dialog.cc (right):

http://codereview.chromium.org/8497008/diff/15001/chrome/browser/ui/webui/boo...
chrome/browser/ui/webui/bookmark_all_tabs_dialog.cc:16: const char*
kBookmarkAllTabsURL =
On 2011/11/14 17:33:05, tfarina wrote:
> nit: const char kBookmarkAllTabsURL[]

Done.

http://codereview.chromium.org/8497008/diff/15001/chrome/browser/ui/webui/boo...
chrome/browser/ui/webui/bookmark_all_tabs_dialog.cc:18:
"eemcgdkfndhakfknompkggombfjjjeno/"
On 2011/11/14 17:33:05, tfarina wrote:
> can you save one line and fold this into the above line? I think it fits;

Done.

http://codereview.chromium.org/8497008/diff/15001/chrome/browser/ui/webui/boo...
chrome/browser/ui/webui/bookmark_all_tabs_dialog.cc:33: virtual void
ExtensionDialogIsClosing(ExtensionDialog* dialog);
On 2011/11/14 17:33:05, tfarina wrote:
> Add OVERRIDE and compiler_specific.h

Done.

http://codereview.chromium.org/8497008/diff/15001/chrome/browser/ui/webui/boo...
File chrome/browser/ui/webui/bookmark_all_tabs_dialog.h (right):

http://codereview.chromium.org/8497008/diff/15001/chrome/browser/ui/webui/boo...
chrome/browser/ui/webui/bookmark_all_tabs_dialog.h:11: class
BookmarkAllTabsDialog {
On 2011/11/14 17:34:21, tfarina wrote:
> Can you convert this into a function instead?
> 
> void ShowBookmarkAllTabsDialog(Profile* profile);

Done.

http://codereview.chromium.org/8497008/diff/15001/chrome/browser/ui/webui/boo...
chrome/browser/ui/webui/bookmark_all_tabs_dialog.h:13: static void Show(Profile*
profile);
On 2011/11/14 17:34:21, tfarina wrote:
> forward declare Profile.

Done.

[commit-bot: I haz the power, 2011-11-18 03:48:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoshiki@chromium.org/8497008/21001

[commit-bot: I haz the power, 2011-11-18 05:45:30]

Change committed as 110659