Use bookmark manager to add/edit bookmark pages with webui_dialogs=1.

Use bookmark manager to add/edit bookmark pages with webui_dialogs=1.

This is the first phase towards being able to replace Bookmark Editor with the WebUI Bookmark Manager as part of the GTK removal effort on ChromeOS. It does not yet have a means of selecting a folder to bookmark all of your tabs in and as such can't yet replace all calls to BookmarkEditor::Show.

BUG=None
TEST=Tested editing bookmarks from the popup bubble and bookmark bar in Linux and TouchUI builds.


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

[Rick Byers, 2011-08-04 18:26:07]

http://codereview.chromium.org/7572022/diff/1/build/common.gypi
File build/common.gypi (right):

http://codereview.chromium.org/7572022/diff/1/build/common.gypi#newcode38
build/common.gypi:38: # Disable webui bookmark editor until completion.
Perhaps we should consider combining these now into a general "webui dialogs"
flag?  Is there value to you in keeping them separate?  If so, feel free - but I
think we should avoid unnecessary flag-pollution.

http://codereview.chromium.org/7572022/diff/1/build/common.gypi#newcode82
build/common.gypi:82: # Use WebUI certificate viewer in Touch case
update comment

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

http://codereview.chromium.org/7572022/diff/1/chrome/browser/resources/bookma...
chrome/browser/resources/bookmark_manager/js/main.js:164: var editId =
id.slice(2);
This 'slice' line is duplicated in 3 places now.  I also find it confusing that
id is sometimes the hash string and sometimes a bookmark ID.  I'd suggest
changing this (above) to something that immediately tests the hash string for
the presence of '=' an sets 'id' to the RHS, etc.

http://codereview.chromium.org/7572022/diff/1/chrome/browser/resources/bookma...
chrome/browser/resources/bookmark_manager/js/main.js:172: var f = function(e) {
This add/remove pattern is duplicated often enough that I'd suggest adding a
little helper function somewhere, eg:
function addOneShotEventListener(node, name, handler) {
  var f = function(e) {
    handler(e);
    node.removeEventListener(name, f);
  }
  node.addEventListener(name, f);
}

http://codereview.chromium.org/7572022/diff/1/chrome/browser/resources/bookma...
chrome/browser/resources/bookmark_manager/js/main.js:172: var f = function(e) {
Should probably use a variable name more descriptive than 'f'.  How about
editBookmark or something?

http://codereview.chromium.org/7572022/diff/1/chrome/browser/resources/bookma...
chrome/browser/resources/bookmark_manager/js/main.js:183: // Navigate to the
parent folder of the node to edit and call function.
I find this a little confusing (especially given the "after the list reload"
comment above the declaration of f) - the control flow (immediate verses load
event callback) wasn't clear to me initially.  If you have the above helper,
then this could be simplified as:
 if (list.parentId == bookmarkNode.parentId)
    editBookmark();
 else {
    // Navigate to the parent folder, and once it's loaded edit the bookmark
    addOneShotEventListener(list, 'load', editBookmark);
    updateParentId(bookmarkNode.parentId);
  }

http://codereview.chromium.org/7572022/diff/1/chrome/browser/resources/bookma...
chrome/browser/resources/bookmark_manager/js/main.js:189: // We need more
information before we know which folder to navigate to.
What is this comment referring to?  The delayed call to 'f' above?  Seems like
it's in the wrong place.  I'd just say something like 'we handle the two cases
of navigating to the bookmark to be edited above, don't run the standard
navigation code below'.

http://codereview.chromium.org/7572022/diff/1/chrome/browser/resources/bookma...
chrome/browser/resources/bookmark_manager/js/main.js:207: 
Since this is now non-trivial, add a comment like:
// Navigate to bookmark 'id'.

http://codereview.chromium.org/7572022/diff/1/chrome/browser/resources/bookma...
chrome/browser/resources/bookmark_manager/js/main.js:390: window.onhashchange();
Technically I'd consider it invalid to call this event handler without any
arguments.  Instead I'd suggest writing a common (argument-less) helper function
("processHash" or something indicating it's for both load and change) that you
call here directly, and which onhashchange just delegates to.

http://codereview.chromium.org/7572022/diff/1/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/7572022/diff/1/chrome/browser/ui/browser.cc#ne...
chrome/browser/ui/browser.cc:1959:
UserMetrics::RecordAction(UserMetricsAction("ShowBookmarkManager"));
Avoid the duplicated code here - make all 3 functions delegate to some helper
function which takes the hash prefix as an extra argument.

http://codereview.chromium.org/7572022/diff/1/chrome/browser/ui/gtk/bookmarks...
File chrome/browser/ui/gtk/bookmarks/bookmark_bubble_gtk.cc (right):

http://codereview.chromium.org/7572022/diff/1/chrome/browser/ui/gtk/bookmarks...
chrome/browser/ui/gtk/bookmarks/bookmark_bubble_gtk.cc:311: #if not
defined(WEBUI_BOOKMARK_EDITOR)
I don't think 'not' is valid, use #if !defined instead, or #ifndef (the latter
appears to be more common in the chrome code base).

http://codereview.chromium.org/7572022/diff/1/chrome/browser/ui/views/bookmar...
File chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc (right):

http://codereview.chromium.org/7572022/diff/1/chrome/browser/ui/views/bookmar...
chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:367: #endif
I think you should probably remove this whole #if block (I'm guessing setting
fade_away was just because of the immediate close, not something we want in
touchui in general).

http://codereview.chromium.org/7572022/diff/1/chrome/browser/ui/views/bookmar...
File chrome/browser/ui/views/bookmarks/bookmark_context_menu_controller_views.cc
(right):

http://codereview.chromium.org/7572022/diff/1/chrome/browser/ui/views/bookmar...
chrome/browser/ui/views/bookmarks/bookmark_context_menu_controller_views.cc:132:
#if defined(WEBUI_BOOKMARK_EDITOR)
You've really had to sprinkle this #ifdef in a lot of places.  Maybe you could
come up with a nice helper function for launching the bookmark editor (with some
param for add/edit/etc) that you call everywhere?  Not sure what this would look
like exactly, but in general sprinkling a bunch of ifdefs sucks...

[arv (Not doing code reviews), 2011-08-11 22:32:22]

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

http://codereview.chromium.org/7572022/diff/1/chrome/browser/resources/bookma...
chrome/browser/resources/bookmark_manager/js/main.js:197: }
missing semicolon

http://codereview.chromium.org/7572022/diff/1/chrome/browser/resources/bookma...
chrome/browser/resources/bookmark_manager/js/main.js:390: window.onhashchange();
Yeah, don't do this. Just do what Rick suggested.

http://codereview.chromium.org/7572022/diff/1/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/7572022/diff/1/chrome/browser/ui/browser.cc#ne...
chrome/browser/ui/browser.cc:1962:
GetSingletonTabNavigateParams(GURL(chrome::kChromeUIBookmarksURL).Resolve(
fix indentation

http://codereview.chromium.org/7572022/diff/1/chrome/browser/ui/browser.cc#ne...
chrome/browser/ui/browser.cc:1970:
GetSingletonTabNavigateParams(GURL(chrome::kChromeUIBookmarksURL).Resolve(
here too

[flackr, 2011-08-12 18:11:21]

As we discussed, I think the refactoring of BookmarkEditor::Show will make this
change rather large so I left a couple TODO's with respect to this and will work
on it in a separate change.

http://codereview.chromium.org/7572022/diff/1/build/common.gypi
File build/common.gypi (right):

http://codereview.chromium.org/7572022/diff/1/build/common.gypi#newcode38
build/common.gypi:38: # Disable webui bookmark editor until completion.
On 2011/08/04 18:26:07, Rick Byers wrote:
> Perhaps we should consider combining these now into a general "webui dialogs"
> flag?  Is there value to you in keeping them separate?  If so, feel free - but
I
> think we should avoid unnecessary flag-pollution.

Done.

http://codereview.chromium.org/7572022/diff/1/build/common.gypi#newcode82
build/common.gypi:82: # Use WebUI certificate viewer in Touch case
On 2011/08/04 18:26:07, Rick Byers wrote:
> update comment

Done.

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

http://codereview.chromium.org/7572022/diff/1/chrome/browser/resources/bookma...
chrome/browser/resources/bookmark_manager/js/main.js:164: var editId =
id.slice(2);
On 2011/08/04 18:26:07, Rick Byers wrote:
> This 'slice' line is duplicated in 3 places now.  I also find it confusing
that
> id is sometimes the hash string and sometimes a bookmark ID.  I'd suggest
> changing this (above) to something that immediately tests the hash string for
> the presence of '=' an sets 'id' to the RHS, etc.

Done. In the case of q= the id is not changed.

http://codereview.chromium.org/7572022/diff/1/chrome/browser/resources/bookma...
chrome/browser/resources/bookmark_manager/js/main.js:172: var f = function(e) {
On 2011/08/04 18:26:07, Rick Byers wrote:
> Should probably use a variable name more descriptive than 'f'.  How about
> editBookmark or something?

Done.

http://codereview.chromium.org/7572022/diff/1/chrome/browser/resources/bookma...
chrome/browser/resources/bookmark_manager/js/main.js:172: var f = function(e) {
On 2011/08/04 18:26:07, Rick Byers wrote:
> This add/remove pattern is duplicated often enough that I'd suggest adding a
> little helper function somewhere, eg:
> function addOneShotEventListener(node, name, handler) {
>   var f = function(e) {
>     handler(e);
>     node.removeEventListener(name, f);
>   }
>   node.addEventListener(name, f);
> }

Done.

http://codereview.chromium.org/7572022/diff/1/chrome/browser/resources/bookma...
chrome/browser/resources/bookmark_manager/js/main.js:183: // Navigate to the
parent folder of the node to edit and call function.
On 2011/08/04 18:26:07, Rick Byers wrote:
> I find this a little confusing (especially given the "after the list reload"
> comment above the declaration of f) - the control flow (immediate verses load
> event callback) wasn't clear to me initially.  If you have the above helper,
> then this could be simplified as:
>  if (list.parentId == bookmarkNode.parentId)
>     editBookmark();
>  else {
>     // Navigate to the parent folder, and once it's loaded edit the bookmark
>     addOneShotEventListener(list, 'load', editBookmark);
>     updateParentId(bookmarkNode.parentId);
>   }

Done.

http://codereview.chromium.org/7572022/diff/1/chrome/browser/resources/bookma...
chrome/browser/resources/bookmark_manager/js/main.js:189: // We need more
information before we know which folder to navigate to.
On 2011/08/04 18:26:07, Rick Byers wrote:
> What is this comment referring to?  The delayed call to 'f' above?  Seems like
> it's in the wrong place.  I'd just say something like 'we handle the two cases
> of navigating to the bookmark to be edited above, don't run the standard
> navigation code below'.

Done.

http://codereview.chromium.org/7572022/diff/1/chrome/browser/resources/bookma...
chrome/browser/resources/bookmark_manager/js/main.js:197: }
On 2011/08/11 22:32:22, arv wrote:
> missing semicolon

Done.

http://codereview.chromium.org/7572022/diff/1/chrome/browser/resources/bookma...
chrome/browser/resources/bookmark_manager/js/main.js:207: 
On 2011/08/04 18:26:07, Rick Byers wrote:
> Since this is now non-trivial, add a comment like:
> // Navigate to bookmark 'id'.

Done.

http://codereview.chromium.org/7572022/diff/1/chrome/browser/resources/bookma...
chrome/browser/resources/bookmark_manager/js/main.js:390: window.onhashchange();
On 2011/08/04 18:26:07, Rick Byers wrote:
> Technically I'd consider it invalid to call this event handler without any
> arguments.  Instead I'd suggest writing a common (argument-less) helper
function
> ("processHash" or something indicating it's for both load and change) that you
> call here directly, and which onhashchange just delegates to.

Done.

http://codereview.chromium.org/7572022/diff/1/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/7572022/diff/1/chrome/browser/ui/browser.cc#ne...
chrome/browser/ui/browser.cc:1959:
UserMetrics::RecordAction(UserMetricsAction("ShowBookmarkManager"));
On 2011/08/04 18:26:07, Rick Byers wrote:
> Avoid the duplicated code here - make all 3 functions delegate to some helper
> function which takes the hash prefix as an extra argument.

Done.

http://codereview.chromium.org/7572022/diff/1/chrome/browser/ui/browser.cc#ne...
chrome/browser/ui/browser.cc:1962:
GetSingletonTabNavigateParams(GURL(chrome::kChromeUIBookmarksURL).Resolve(
On 2011/08/11 22:32:22, arv wrote:
> fix indentation

Done.

http://codereview.chromium.org/7572022/diff/1/chrome/browser/ui/browser.cc#ne...
chrome/browser/ui/browser.cc:1970:
GetSingletonTabNavigateParams(GURL(chrome::kChromeUIBookmarksURL).Resolve(
On 2011/08/11 22:32:22, arv wrote:
> here too

Done.

http://codereview.chromium.org/7572022/diff/1/chrome/browser/ui/gtk/bookmarks...
File chrome/browser/ui/gtk/bookmarks/bookmark_bubble_gtk.cc (right):

http://codereview.chromium.org/7572022/diff/1/chrome/browser/ui/gtk/bookmarks...
chrome/browser/ui/gtk/bookmarks/bookmark_bubble_gtk.cc:311: #if not
defined(WEBUI_BOOKMARK_EDITOR)
On 2011/08/04 18:26:07, Rick Byers wrote:
> I don't think 'not' is valid, use #if !defined instead, or #ifndef (the latter
> appears to be more common in the chrome code base).

Done.

http://codereview.chromium.org/7572022/diff/1/chrome/browser/ui/views/bookmar...
File chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc (right):

http://codereview.chromium.org/7572022/diff/1/chrome/browser/ui/views/bookmar...
chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:367: #endif
On 2011/08/04 18:26:07, Rick Byers wrote:
> I think you should probably remove this whole #if block (I'm guessing setting
> fade_away was just because of the immediate close, not something we want in
> touchui in general).

Done.

http://codereview.chromium.org/7572022/diff/1/chrome/browser/ui/views/bookmar...
File chrome/browser/ui/views/bookmarks/bookmark_context_menu_controller_views.cc
(right):

http://codereview.chromium.org/7572022/diff/1/chrome/browser/ui/views/bookmar...
chrome/browser/ui/views/bookmarks/bookmark_context_menu_controller_views.cc:132:
#if defined(WEBUI_BOOKMARK_EDITOR)
On 2011/08/04 18:26:07, Rick Byers wrote:
> You've really had to sprinkle this #ifdef in a lot of places.  Maybe you could
> come up with a nice helper function for launching the bookmark editor (with
some
> param for add/edit/etc) that you call everywhere?  Not sure what this would
look
> like exactly, but in general sprinkling a bunch of ifdefs sucks...

I completely agree. As we discussed I think a slight refactoring of the base
BookmarkEditor::Show method will be the best way to handle this. I left a TODO
regarding this in that class (and in the common.gypi file with regards to using
a runtime flag) and will work on it in a subsequent CL.

[Rick Byers, 2011-08-15 14:59:40]

LGTM as a good interim step.  I'm looking forward to the refactoring CL :-)

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

http://codereview.chromium.org/7572022/diff/1/chrome/browser/resources/bookma...
chrome/browser/resources/bookmark_manager/js/main.js:164: var editId =
id.slice(2);
On 2011/08/12 18:11:24, flackr wrote:
> On 2011/08/04 18:26:07, Rick Byers wrote:
> > This 'slice' line is duplicated in 3 places now.  I also find it confusing
> that
> > id is sometimes the hash string and sometimes a bookmark ID.  I'd suggest
> > changing this (above) to something that immediately tests the hash string
for
> > the presence of '=' an sets 'id' to the RHS, etc.
> 
> Done. In the case of q= the id is not changed.

Ah, I missed this subtlety.  Thanks for adding comments - certainly better now.

http://codereview.chromium.org/7572022/diff/6001/chrome/browser/resources/boo...
chrome/browser/resources/bookmark_manager/js/main.js:120: * @param {string} id
The ID to navigate to.
Use !Element or !EventTarget for the type
(HTMLElement is an HTML5 sub-class of Element)

http://codereview.chromium.org/7572022/diff/6001/chrome/browser/resources/boo...
chrome/browser/resources/bookmark_manager/js/main.js:190: return;
since you have the line anyway, just move the initialization of index up

[flackr, 2011-08-17 02:06:01]

http://codereview.chromium.org/7572022/diff/6001/chrome/browser/resources/boo...
File chrome/browser/resources/bookmark_manager/js/main.js (right):

http://codereview.chromium.org/7572022/diff/6001/chrome/browser/resources/boo...
chrome/browser/resources/bookmark_manager/js/main.js:120: * @param {HtmlElement}
node The DOM node to add the listener to.
On 2011/08/15 14:59:41, Rick Byers wrote:
> Use !Element or !EventTarget for the type
> (HTMLElement is an HTML5 sub-class of Element)

Done.

http://codereview.chromium.org/7572022/diff/6001/chrome/browser/resources/boo...
chrome/browser/resources/bookmark_manager/js/main.js:190: if ((index =
list.dataModel.findIndexById(bookmarkNode.id)) != -1) {
On 2011/08/15 14:59:41, Rick Byers wrote:
> since you have the line anyway, just move the initialization of index up

Done.

[arv (Not doing code reviews), 2011-08-17 02:31:09]

LGTM

http://codereview.chromium.org/7572022/diff/11001/chrome/browser/resources/bo...
File chrome/browser/resources/bookmark_manager/js/main.js (right):

http://codereview.chromium.org/7572022/diff/11001/chrome/browser/resources/bo...
chrome/browser/resources/bookmark_manager/js/main.js:237: }
missing semicolon

[flackr, 2011-08-17 13:31:35]

http://codereview.chromium.org/7572022/diff/11001/chrome/browser/resources/bo...
File chrome/browser/resources/bookmark_manager/js/main.js (right):

http://codereview.chromium.org/7572022/diff/11001/chrome/browser/resources/bo...
chrome/browser/resources/bookmark_manager/js/main.js:237: }
On 2011/08/17 02:31:09, arv wrote:
> missing semicolon

Done.

[commit-bot: I haz the power, 2011-08-17 19:51:38]

Change committed as 97186