Bookmark Manager Drag and Drop backend....

Bookmark Manager Drag and Drop backend.

This adds the following methods to chrome.experimental.bookmarkManager:

startDrag(idList)
drop(parentId, opt_index)

as well as the following events:

onDragEnter(function(BookmarkDragData))
onDragLeave(function(BookmarkDragData))
onDrop(function(BookmarkDragData))

BUG=32194
TEST=None


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

[sky, 2010-02-18 16:39:30]

http://codereview.chromium.org/596105/diff/15001/15018
File chrome/browser/renderer_host/render_view_host_delegate.h (right):

http://codereview.chromium.org/596105/diff/15001/15018#newcode63
chrome/browser/renderer_host/render_view_host_delegate.h:63: typedef
OSExchangeData TabContentsBookmarkDragManagerDragData;
You should consider moving this typedef into BookmarkDrag and naming it Data or
DragData. That way usage inside the class is simple and externally you qualify
with the class name for clarity.

http://codereview.chromium.org/596105/diff/15001/15018#newcode413
chrome/browser/renderer_host/render_view_host_delegate.h:413: class Observer {
Do we really need the observer here too? Couldn't
ExtensionBookmarkManagerEventRoute just set itself as the delegate?

[arv (Not doing code reviews), 2010-02-18 19:55:07]

http://codereview.chromium.org/596105/diff/15001/15018
File chrome/browser/renderer_host/render_view_host_delegate.h (right):

http://codereview.chromium.org/596105/diff/15001/15018#newcode63
chrome/browser/renderer_host/render_view_host_delegate.h:63: typedef
OSExchangeData TabContentsBookmarkDragManagerDragData;
On 2010/02/18 16:39:30, sky wrote:
> You should consider moving this typedef into BookmarkDrag and naming it Data
or
> DragData. That way usage inside the class is simple and externally you qualify
> with the class name for clarity.

Done.

http://codereview.chromium.org/596105/diff/15001/15018#newcode413
chrome/browser/renderer_host/render_view_host_delegate.h:413: class Observer {
On 2010/02/18 16:39:30, sky wrote:
> Do we really need the observer here too? Couldn't
> ExtensionBookmarkManagerEventRoute just set itself as the delegate?
> 

My only concern is that providing a setter would result in any possible
listeners would be removed. However, since there is only one of these at the
moment and this whole thing is a hack I'm removing the observer.

[sky, 2010-02-18 20:38:48]

Did you update the patch?

  -Scott

On Thu, Feb 18, 2010 at 11:55 AM,  <arv@chromium.org> wrote:
> Reviewers: sky,
>
>
> http://codereview.chromium.org/596105/diff/15001/15018
> File chrome/browser/renderer_host/render_view_host_delegate.h (right):
>
> http://codereview.chromium.org/596105/diff/15001/15018#newcode63
> chrome/browser/renderer_host/render_view_host_delegate.h:63: typedef
> OSExchangeData TabContentsBookmarkDragManagerDragData;
> On 2010/02/18 16:39:30, sky wrote:
>>
>> You should consider moving this typedef into BookmarkDrag and naming
>
> it Data or
>>
>> DragData. That way usage inside the class is simple and externally you
>
> qualify
>>
>> with the class name for clarity.
>
> Done.
>
> http://codereview.chromium.org/596105/diff/15001/15018#newcode413
> chrome/browser/renderer_host/render_view_host_delegate.h:413: class
> Observer {
> On 2010/02/18 16:39:30, sky wrote:
>>
>> Do we really need the observer here too? Couldn't
>> ExtensionBookmarkManagerEventRoute just set itself as the delegate?
>
>
> My only concern is that providing a setter would result in any possible
> listeners would be removed. However, since there is only one of these at
> the moment and this whole thing is a hack I'm removing the observer.
>
> Description:
> Bookmark Manager Drag and Drop backend.
>
> This is work in progress. DO NOT SUBMIT!
>
> This is working code of exposing enough DnD hooks to the bookmark manager
> extension to allow dragging out and into the bookmark manager. It correctly
> handles multiple bookmarks for the old bookmark manager as well as from
> other
> instances of Chrome.
>
> The code is a mess and breaks isolation-of-responsibilies.
>
> I'm sending this out early to get feedback.
>
> BUG=32194
> TEST=None yet
>
>
> Please review this at http://codereview.chromium.org/596105
>
> SVN Base: svn://chrome-svn/chrome/trunk/src/
>
> Affected files:
>  M     chrome/browser/bookmarks/bookmark_utils.h
>  M     chrome/browser/bookmarks/bookmark_utils.cc
>  M     chrome/browser/extensions/extension_bookmark_manager_api.h
>  M     chrome/browser/extensions/extension_bookmark_manager_api.cc
>  M     chrome/browser/extensions/extension_bookmarks_module_constants.h
>  M     chrome/browser/extensions/extension_bookmarks_module_constants.cc
>  M     chrome/browser/extensions/extension_dom_ui.h
>  M     chrome/browser/extensions/extension_dom_ui.cc
>  M     chrome/browser/extensions/extension_function_dispatcher.cc
>  M     chrome/browser/renderer_host/render_view_host_delegate.h
>  M     chrome/browser/renderer_host/render_view_host_delegate.cc
>  M     chrome/browser/tab_contents/tab_contents.h
>  M     chrome/browser/tab_contents/tab_contents.cc
>  A     chrome/browser/tab_contents/tab_contents_bookmark_drag_manager.h
>  A     chrome/browser/tab_contents/tab_contents_bookmark_drag_manager.cc
>  M     chrome/browser/tab_contents/web_drop_target_win.cc
>  M     chrome/chrome_browser.gypi
>  M     chrome/common/extensions/api/extension_api.json
>
>
>

[arv (Not doing code reviews), 2010-02-18 21:12:54]

Not yet. I'll ping you when done.

erik



On Thu, Feb 18, 2010 at 12:38, Scott Violet <sky@chromium.org> wrote:
> Did you update the patch?
>
>  -Scott
>
> On Thu, Feb 18, 2010 at 11:55 AM,  <arv@chromium.org> wrote:
>> Reviewers: sky,
>>
>>
>> http://codereview.chromium.org/596105/diff/15001/15018
>> File chrome/browser/renderer_host/render_view_host_delegate.h (right):
>>
>> http://codereview.chromium.org/596105/diff/15001/15018#newcode63
>> chrome/browser/renderer_host/render_view_host_delegate.h:63: typedef
>> OSExchangeData TabContentsBookmarkDragManagerDragData;
>> On 2010/02/18 16:39:30, sky wrote:
>>>
>>> You should consider moving this typedef into BookmarkDrag and naming
>>
>> it Data or
>>>
>>> DragData. That way usage inside the class is simple and externally you
>>
>> qualify
>>>
>>> with the class name for clarity.
>>
>> Done.
>>
>> http://codereview.chromium.org/596105/diff/15001/15018#newcode413
>> chrome/browser/renderer_host/render_view_host_delegate.h:413: class
>> Observer {
>> On 2010/02/18 16:39:30, sky wrote:
>>>
>>> Do we really need the observer here too? Couldn't
>>> ExtensionBookmarkManagerEventRoute just set itself as the delegate?
>>
>>
>> My only concern is that providing a setter would result in any possible
>> listeners would be removed. However, since there is only one of these at
>> the moment and this whole thing is a hack I'm removing the observer.
>>
>> Description:
>> Bookmark Manager Drag and Drop backend.
>>
>> This is work in progress. DO NOT SUBMIT!
>>
>> This is working code of exposing enough DnD hooks to the bookmark manager
>> extension to allow dragging out and into the bookmark manager. It correctly
>> handles multiple bookmarks for the old bookmark manager as well as from
>> other
>> instances of Chrome.
>>
>> The code is a mess and breaks isolation-of-responsibilies.
>>
>> I'm sending this out early to get feedback.
>>
>> BUG=32194
>> TEST=None yet
>>
>>
>> Please review this at http://codereview.chromium.org/596105
>>
>> SVN Base: svn://chrome-svn/chrome/trunk/src/
>>
>> Affected files:
>>  M     chrome/browser/bookmarks/bookmark_utils.h
>>  M     chrome/browser/bookmarks/bookmark_utils.cc
>>  M     chrome/browser/extensions/extension_bookmark_manager_api.h
>>  M     chrome/browser/extensions/extension_bookmark_manager_api.cc
>>  M     chrome/browser/extensions/extension_bookmarks_module_constants.h
>>  M     chrome/browser/extensions/extension_bookmarks_module_constants.cc
>>  M     chrome/browser/extensions/extension_dom_ui.h
>>  M     chrome/browser/extensions/extension_dom_ui.cc
>>  M     chrome/browser/extensions/extension_function_dispatcher.cc
>>  M     chrome/browser/renderer_host/render_view_host_delegate.h
>>  M     chrome/browser/renderer_host/render_view_host_delegate.cc
>>  M     chrome/browser/tab_contents/tab_contents.h
>>  M     chrome/browser/tab_contents/tab_contents.cc
>>  A     chrome/browser/tab_contents/tab_contents_bookmark_drag_manager.h
>>  A     chrome/browser/tab_contents/tab_contents_bookmark_drag_manager.cc
>>  M     chrome/browser/tab_contents/web_drop_target_win.cc
>>  M     chrome/chrome_browser.gypi
>>  M     chrome/common/extensions/api/extension_api.json
>>
>>
>>
>

[arv (Not doing code reviews), 2010-02-18 22:27:16]

Updated. Please take another look.

[sky, 2010-02-19 00:08:37]

Be sure and get one of the extensions guys to review the extension related
changes.

http://codereview.chromium.org/596105/diff/17026/16019
File chrome/browser/bookmarks/bookmark_utils.cc (right):

http://codereview.chromium.org/596105/diff/17026/16019#newcode340
chrome/browser/bookmarks/bookmark_utils.cc:340: DCHECK(nodes.size() > 0);
!nodes.empty()

http://codereview.chromium.org/596105/diff/17026/16019#newcode356
chrome/browser/bookmarks/bookmark_utils.cc:356:
DoDragDrop(OSExchangeDataProviderWin::GetIDataObject(data), drag_source.get(),
You should be able to use RootView::StartDragForViewFromMouseEvent, you can
supply a NULL view.

http://codereview.chromium.org/596105/diff/17026/16019#newcode364
chrome/browser/bookmarks/bookmark_utils.cc:364: views::WidgetGtk* widget =
views::WidgetGtk::GetViewForNative(root);
Only TOOLKITS_VIEWS & OS_LINUX has widgetgtk.

http://codereview.chromium.org/596105/diff/17026/16020
File chrome/browser/bookmarks/bookmark_utils.h (right):

http://codereview.chromium.org/596105/diff/17026/16020#newcode74
chrome/browser/bookmarks/bookmark_utils.h:74: // Begins the dragging a group of
bookmarks. |view| is only required for
nit: nuke the

http://codereview.chromium.org/596105/diff/16032/17037
File chrome/browser/extensions/extension_bookmark_manager_api.cc (right):

http://codereview.chromium.org/596105/diff/16032/17037#newcode77
chrome/browser/extensions/extension_bookmark_manager_api.cc:77:
dict->SetString(keys::kParentIdKey, parent_id_string);
Do you really need the parent id? You don't use in AddElementToList.

http://codereview.chromium.org/596105/diff/16032/17037#newcode96
chrome/browser/extensions/extension_bookmark_manager_api.cc:96: const
BookmarkDragData::Element* element) {
Any reason you went with const* instead of const&?

http://codereview.chromium.org/596105/diff/16032/17037#newcode114
chrome/browser/extensions/extension_bookmark_manager_api.cc:114: void
BookmarkDragDataToJSON(Profile* profile, const BookmarkDragData* data,
I would make this to a const BookmarkDragData&.

http://codereview.chromium.org/596105/diff/16032/17037#newcode154
chrome/browser/extensions/extension_bookmark_manager_api.cc:154: Profile*
profile, TabContents* tab_contents) : profile_(profile),
: profile_ should be on it's own line.

http://codereview.chromium.org/596105/diff/16032/17037#newcode160
chrome/browser/extensions/extension_bookmark_manager_api.cc:160:
tab_contents_->SetBookmarkDragDelegate(NULL);
What's the life time here? Is the router always deleted before the tabcontents?

http://codereview.chromium.org/596105/diff/16032/17037#newcode409
chrome/browser/extensions/extension_bookmark_manager_api.cc:409:
EXTENSION_FUNCTION_VALIDATE(drop_parent);
Do these macros return if they get invalid data, like null?

http://codereview.chromium.org/596105/diff/16032/17036
File chrome/browser/extensions/extension_bookmark_manager_api.h (right):

http://codereview.chromium.org/596105/diff/16032/17036#newcode21
chrome/browser/extensions/extension_bookmark_manager_api.h:21: typedef
RenderViewHostDelegate::BookmarkDrag::DragData DragData;
How come you put this in an anonymous namespace? Also, do even need the typedef?
Shouldn't ExtensionBookmarkManagerEventRouter see the name by virtue of
extending BookmarkDrag?

http://codereview.chromium.org/596105/diff/16032/17036#newcode29
chrome/browser/extensions/extension_bookmark_manager_api.h:29: explicit
ExtensionBookmarkManagerEventRouter(Profile* profile,
no explicit

http://codereview.chromium.org/596105/diff/16032/17036#newcode47
chrome/browser/extensions/extension_bookmark_manager_api.h:47: 
nit: nuke this line

http://codereview.chromium.org/596105/diff/16032/17041
File chrome/browser/tab_contents/tab_contents.h (right):

http://codereview.chromium.org/596105/diff/16032/17041#newcode660
chrome/browser/tab_contents/tab_contents.h:660: virtual
RenderViewHostDelegate::BookmarkDrag* GetBookmarkDragDelegate();
You should add a new header above this, also, be sure and mention ownership.

http://codereview.chromium.org/596105/diff/16032/17041#newcode1013
chrome/browser/tab_contents/tab_contents.h:1013: // Drag and drop manager,
lazily created.
This isn't lazily created, right?

[arv (Not doing code reviews), 2010-02-19 02:06:08]

All done. I hope.

Erik, do you mind looking at the extension parts of this?

Also, unit testing... These extension methods seems to require that we expose
DnD automation to extension unit tests. Do you have any idea of how to test this
without introducing a whole new experimental extension API?

http://codereview.chromium.org/596105/diff/17026/16019
File chrome/browser/bookmarks/bookmark_utils.cc (right):

http://codereview.chromium.org/596105/diff/17026/16019#newcode340
chrome/browser/bookmarks/bookmark_utils.cc:340: DCHECK(nodes.size() > 0);
On 2010/02/19 00:08:37, sky wrote:
> !nodes.empty()

Done.

http://codereview.chromium.org/596105/diff/17026/16019#newcode356
chrome/browser/bookmarks/bookmark_utils.cc:356:
DoDragDrop(OSExchangeDataProviderWin::GetIDataObject(data), drag_source.get(),
On 2010/02/19 00:08:37, sky wrote:
> You should be able to use RootView::StartDragForViewFromMouseEvent, you can
> supply a NULL view.

Done.

This allows me to use Views instead of using the GTK/Win32 specifics. Thanks.

This requires that the NativeView is non null but that should not be a problem.

http://codereview.chromium.org/596105/diff/17026/16019#newcode364
chrome/browser/bookmarks/bookmark_utils.cc:364: views::WidgetGtk* widget =
views::WidgetGtk::GetViewForNative(root);
On 2010/02/19 00:08:37, sky wrote:
> Only TOOLKITS_VIEWS & OS_LINUX has widgetgtk.

It is already in a TOOLKIT_VIEWS check.

http://codereview.chromium.org/596105/diff/17026/16020
File chrome/browser/bookmarks/bookmark_utils.h (right):

http://codereview.chromium.org/596105/diff/17026/16020#newcode74
chrome/browser/bookmarks/bookmark_utils.h:74: // Begins the dragging a group of
bookmarks. |view| is only required for
On 2010/02/19 00:08:37, sky wrote:
> nit: nuke the

Done.

http://codereview.chromium.org/596105/diff/16032/17037
File chrome/browser/extensions/extension_bookmark_manager_api.cc (right):

http://codereview.chromium.org/596105/diff/16032/17037#newcode77
chrome/browser/extensions/extension_bookmark_manager_api.cc:77:
dict->SetString(keys::kParentIdKey, parent_id_string);
On 2010/02/19 00:08:37, sky wrote:
> Do you really need the parent id? You don't use in AddElementToList.

Yes, it is needed because when dragging from the same profile we need to know
the parent ID so we can prevent dropping on the current parent folder.
AddElementToList is used when dragging from a different profile in which case
the id is not meaningful. I'll add some more comments.

http://codereview.chromium.org/596105/diff/16032/17037#newcode96
chrome/browser/extensions/extension_bookmark_manager_api.cc:96: const
BookmarkDragData::Element* element) {
On 2010/02/19 00:08:37, sky wrote:
> Any reason you went with const* instead of const&?

Nope, changing.

http://codereview.chromium.org/596105/diff/16032/17037#newcode114
chrome/browser/extensions/extension_bookmark_manager_api.cc:114: void
BookmarkDragDataToJSON(Profile* profile, const BookmarkDragData* data,
On 2010/02/19 00:08:37, sky wrote:
> I would make this to a const BookmarkDragData&.

Done.

http://codereview.chromium.org/596105/diff/16032/17037#newcode154
chrome/browser/extensions/extension_bookmark_manager_api.cc:154: Profile*
profile, TabContents* tab_contents) : profile_(profile),
On 2010/02/19 00:08:37, sky wrote:
> : profile_ should be on it's own line.

Done.

http://codereview.chromium.org/596105/diff/16032/17037#newcode160
chrome/browser/extensions/extension_bookmark_manager_api.cc:160:
tab_contents_->SetBookmarkDragDelegate(NULL);
On 2010/02/19 00:08:37, sky wrote:
> What's the life time here? Is the router always deleted before the
tabcontents?

This took some trials to get right.

The tab contents create the DOMUI which creates the router.

http://codereview.chromium.org/596105/diff/16032/17037#newcode409
chrome/browser/extensions/extension_bookmark_manager_api.cc:409:
EXTENSION_FUNCTION_VALIDATE(drop_parent);
On 2010/02/19 00:08:37, sky wrote:
> Do these macros return if they get invalid data, like null?

Yup, they do something like

DCHECK(value);
if (!value) return false;

http://codereview.chromium.org/596105/diff/16032/17036
File chrome/browser/extensions/extension_bookmark_manager_api.h (right):

http://codereview.chromium.org/596105/diff/16032/17036#newcode21
chrome/browser/extensions/extension_bookmark_manager_api.h:21: typedef
RenderViewHostDelegate::BookmarkDrag::DragData DragData;
On 2010/02/19 00:08:37, sky wrote:
> How come you put this in an anonymous namespace? Also, do even need the
typedef?
> Shouldn't ExtensionBookmarkManagerEventRouter see the name by virtue of
> extending BookmarkDrag?

Noob.

http://codereview.chromium.org/596105/diff/16032/17036#newcode29
chrome/browser/extensions/extension_bookmark_manager_api.h:29: explicit
ExtensionBookmarkManagerEventRouter(Profile* profile,
On 2010/02/19 00:08:37, sky wrote:
> no explicit

Done.

Strange that the compiler does not warn/error on this?

http://codereview.chromium.org/596105/diff/16032/17036#newcode47
chrome/browser/extensions/extension_bookmark_manager_api.h:47: 
On 2010/02/19 00:08:37, sky wrote:
> nit: nuke this line

Done.

http://codereview.chromium.org/596105/diff/16032/17041
File chrome/browser/tab_contents/tab_contents.h (right):

http://codereview.chromium.org/596105/diff/16032/17041#newcode660
chrome/browser/tab_contents/tab_contents.h:660: virtual
RenderViewHostDelegate::BookmarkDrag* GetBookmarkDragDelegate();
On 2010/02/19 00:08:37, sky wrote:
> You should add a new header above this, also, be sure and mention ownership.

Done. I think.

http://codereview.chromium.org/596105/diff/16032/17041#newcode1013
chrome/browser/tab_contents/tab_contents.h:1013: // Drag and drop manager,
lazily created.
On 2010/02/19 00:08:37, sky wrote:
> This isn't lazily created, right?

Old comment. Updated.

[sky, 2010-02-19 16:33:29]

LGTM

[Erik does not do reviews, 2010-02-19 18:32:44]

On Thu, Feb 18, 2010 at 6:06 PM, <arv@chromium.org> wrote:

> All done. I hope.
>
> Erik, do you mind looking at the extension parts of this?
>

Looking now.


> Also, unit testing... These extension methods seems to require that we
> expose
> DnD automation to extension unit tests. Do you have any idea of how to test
> this
> without introducing a whole new experimental extension API?


Ick.  We have chrome.test already for test stuff which we could use to hold
anything new we might need, but I'm not excited about DnD automation apis.
 Let me think about this a bit.



>
>
>
> http://codereview.chromium.org/596105/diff/17026/16019
> File chrome/browser/bookmarks/bookmark_utils.cc (right):
>
> http://codereview.chromium.org/596105/diff/17026/16019#newcode340
> chrome/browser/bookmarks/bookmark_utils.cc:340: DCHECK(nodes.size() >
> 0);
> On 2010/02/19 00:08:37, sky wrote:
>
>> !nodes.empty()
>>
>
> Done.
>
>
> http://codereview.chromium.org/596105/diff/17026/16019#newcode356
> chrome/browser/bookmarks/bookmark_utils.cc:356:
> DoDragDrop(OSExchangeDataProviderWin::GetIDataObject(data),
> drag_source.get(),
> On 2010/02/19 00:08:37, sky wrote:
>
>> You should be able to use RootView::StartDragForViewFromMouseEvent,
>>
> you can
>
>> supply a NULL view.
>>
>
> Done.
>
> This allows me to use Views instead of using the GTK/Win32 specifics.
> Thanks.
>
> This requires that the NativeView is non null but that should not be a
> problem.
>
>
> http://codereview.chromium.org/596105/diff/17026/16019#newcode364
> chrome/browser/bookmarks/bookmark_utils.cc:364: views::WidgetGtk* widget
> = views::WidgetGtk::GetViewForNative(root);
> On 2010/02/19 00:08:37, sky wrote:
>
>> Only TOOLKITS_VIEWS & OS_LINUX has widgetgtk.
>>
>
> It is already in a TOOLKIT_VIEWS check.
>
>
> http://codereview.chromium.org/596105/diff/17026/16020
> File chrome/browser/bookmarks/bookmark_utils.h (right):
>
> http://codereview.chromium.org/596105/diff/17026/16020#newcode74
> chrome/browser/bookmarks/bookmark_utils.h:74: // Begins the dragging a
> group of bookmarks. |view| is only required for
> On 2010/02/19 00:08:37, sky wrote:
>
>> nit: nuke the
>>
>
> Done.
>
>
> http://codereview.chromium.org/596105/diff/16032/17037
> File chrome/browser/extensions/extension_bookmark_manager_api.cc
> (right):
>
> http://codereview.chromium.org/596105/diff/16032/17037#newcode77
> chrome/browser/extensions/extension_bookmark_manager_api.cc:77:
> dict->SetString(keys::kParentIdKey, parent_id_string);
> On 2010/02/19 00:08:37, sky wrote:
>
>> Do you really need the parent id? You don't use in AddElementToList.
>>
>
> Yes, it is needed because when dragging from the same profile we need to
> know the parent ID so we can prevent dropping on the current parent
> folder. AddElementToList is used when dragging from a different profile
> in which case the id is not meaningful. I'll add some more comments.
>
>
> http://codereview.chromium.org/596105/diff/16032/17037#newcode96
> chrome/browser/extensions/extension_bookmark_manager_api.cc:96: const
> BookmarkDragData::Element* element) {
> On 2010/02/19 00:08:37, sky wrote:
>
>> Any reason you went with const* instead of const&?
>>
>
> Nope, changing.
>
>
> http://codereview.chromium.org/596105/diff/16032/17037#newcode114
> chrome/browser/extensions/extension_bookmark_manager_api.cc:114: void
> BookmarkDragDataToJSON(Profile* profile, const BookmarkDragData* data,
> On 2010/02/19 00:08:37, sky wrote:
>
>> I would make this to a const BookmarkDragData&.
>>
>
> Done.
>
>
> http://codereview.chromium.org/596105/diff/16032/17037#newcode154
> chrome/browser/extensions/extension_bookmark_manager_api.cc:154:
> Profile* profile, TabContents* tab_contents) : profile_(profile),
> On 2010/02/19 00:08:37, sky wrote:
>
>> : profile_ should be on it's own line.
>>
>
> Done.
>
>
> http://codereview.chromium.org/596105/diff/16032/17037#newcode160
> chrome/browser/extensions/extension_bookmark_manager_api.cc:160:
> tab_contents_->SetBookmarkDragDelegate(NULL);
> On 2010/02/19 00:08:37, sky wrote:
>
>> What's the life time here? Is the router always deleted before the
>>
> tabcontents?
>
> This took some trials to get right.
>
> The tab contents create the DOMUI which creates the router.
>
>
> http://codereview.chromium.org/596105/diff/16032/17037#newcode409
> chrome/browser/extensions/extension_bookmark_manager_api.cc:409:
> EXTENSION_FUNCTION_VALIDATE(drop_parent);
> On 2010/02/19 00:08:37, sky wrote:
>
>> Do these macros return if they get invalid data, like null?
>>
>
> Yup, they do something like
>
> DCHECK(value);
> if (!value) return false;
>
>
> http://codereview.chromium.org/596105/diff/16032/17036
> File chrome/browser/extensions/extension_bookmark_manager_api.h (right):
>
> http://codereview.chromium.org/596105/diff/16032/17036#newcode21
> chrome/browser/extensions/extension_bookmark_manager_api.h:21: typedef
> RenderViewHostDelegate::BookmarkDrag::DragData DragData;
> On 2010/02/19 00:08:37, sky wrote:
>
>> How come you put this in an anonymous namespace? Also, do even need
>>
> the typedef?
>
>> Shouldn't ExtensionBookmarkManagerEventRouter see the name by virtue
>>
> of
>
>> extending BookmarkDrag?
>>
>
> Noob.
>
>
> http://codereview.chromium.org/596105/diff/16032/17036#newcode29
> chrome/browser/extensions/extension_bookmark_manager_api.h:29: explicit
> ExtensionBookmarkManagerEventRouter(Profile* profile,
> On 2010/02/19 00:08:37, sky wrote:
>
>> no explicit
>>
>
> Done.
>
> Strange that the compiler does not warn/error on this?
>
>
> http://codereview.chromium.org/596105/diff/16032/17036#newcode47
> chrome/browser/extensions/extension_bookmark_manager_api.h:47:
> On 2010/02/19 00:08:37, sky wrote:
>
>> nit: nuke this line
>>
>
> Done.
>
>
> http://codereview.chromium.org/596105/diff/16032/17041
> File chrome/browser/tab_contents/tab_contents.h (right):
>
> http://codereview.chromium.org/596105/diff/16032/17041#newcode660
> chrome/browser/tab_contents/tab_contents.h:660: virtual
> RenderViewHostDelegate::BookmarkDrag* GetBookmarkDragDelegate();
> On 2010/02/19 00:08:37, sky wrote:
>
>> You should add a new header above this, also, be sure and mention
>>
> ownership.
>
> Done. I think.
>
>
> http://codereview.chromium.org/596105/diff/16032/17041#newcode1013
> chrome/browser/tab_contents/tab_contents.h:1013: // Drag and drop
> manager, lazily created.
> On 2010/02/19 00:08:37, sky wrote:
>
>> This isn't lazily created, right?
>>
>
> Old comment. Updated.
>
>
> http://codereview.chromium.org/596105
>

[Erik does not do reviews, 2010-02-19 21:51:35]

http://codereview.chromium.org/596105/diff/19001/19004
File chrome/browser/extensions/extension_bookmark_manager_api.cc (right):

http://codereview.chromium.org/596105/diff/19001/19004#newcode88
chrome/browser/extensions/extension_bookmark_manager_api.cc:88: for (int i = 0;
i < node.GetChildCount(); ++i) {
nit: remove {} (multiple places)

http://codereview.chromium.org/596105/diff/19001/19004#newcode97
chrome/browser/extensions/extension_bookmark_manager_api.cc:97: //
|BookmarkDragDataToJSON| when the data comes from a different profile. When
I'm surprised that we have all of this cross profile support.  Is there some new
multi-profile use-case coming?

http://codereview.chromium.org/596105/diff/19001/19004#newcode171
chrome/browser/extensions/extension_bookmark_manager_api.cc:171:
base::JSONWriter::Write(args, false, &json_args);
you could move this inside the if block

http://codereview.chromium.org/596105/diff/19001/19004#newcode182
chrome/browser/extensions/extension_bookmark_manager_api.cc:182: if
(::GetBookmarkDragData(data, &bookmark_drag_data)) {
when this fails, is there anything else we should do?  is it possible for the
client to get in a stuck state if the drag end event doesn't get sent?

http://codereview.chromium.org/596105/diff/19001/19004#newcode218
chrome/browser/extensions/extension_bookmark_manager_api.cc:218:
bookmark_drag_data_ = BookmarkDragData();
This looks like an odd construct to me.  Are you depending n the built-in copy
constructor to do the clear?  In general, we tend to dislike these constructors.
 Why not just have an explicit clear() method?

http://codereview.chromium.org/596105/diff/19001/19004#newcode401
chrome/browser/extensions/extension_bookmark_manager_api.cc:401: // LAME, the
arguments change between a list and a value depending on the
why not file a bug and reference here?

http://codereview.chromium.org/596105/diff/19001/19004#newcode414
chrome/browser/extensions/extension_bookmark_manager_api.cc:414:
EXTENSION_FUNCTION_VALIDATE(drop_parent);
I don't think this should be checked with EXTENSION_FUNCTION_VALIDATE since it's
legitimately possible for this id to have gone away.  EFV is only meant to be
used in cases where the data *can't* be generated by the renderer (or if it did
it would be a bug in the code).  When it fails, it kills the extension, assuming
it's a security violation.  Instead, this should be an error (error_ =
"message";) and an early return.

http://codereview.chromium.org/596105/diff/19001/19005
File chrome/browser/extensions/extension_dom_ui.h (right):

http://codereview.chromium.org/596105/diff/19001/19005#newcode53
chrome/browser/extensions/extension_dom_ui.h:53: virtual
ExtensionBookmarkManagerEventRouter*
why does this need to be here?  I think we've put other event routers on
ExtensionsService.

http://codereview.chromium.org/596105/diff/19001/19008
File chrome/browser/tab_contents/tab_contents.h (right):

http://codereview.chromium.org/596105/diff/19001/19008#newcode663
chrome/browser/tab_contents/tab_contents.h:663: // It is up to callers to call
SetBookmarkDragDelegate(NULL) when
nit: newline above comment

http://codereview.chromium.org/596105/diff/19001/19009
File chrome/browser/tab_contents/web_drop_target_win.cc (right):

http://codereview.chromium.org/596105/diff/19001/19009#newcode121
chrome/browser/tab_contents/web_drop_target_win.cc:121: if
(tab_contents_->GetBookmarkDragDelegate()) {
just so I understand this, this should be non-null if and only if the drag
operation involves bookmarks, right?

http://codereview.chromium.org/596105/diff/19001/19012
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/596105/diff/19001/19012#newcode1960
chrome/common/extensions/api/extension_api.json:1960: "id":
"BookmarkDragDataElement",
isn't this just the same as BookmarkNode?  Why have a different type?

[arv (Not doing code reviews), 2010-02-19 23:28:21]

Thanks for the feedback.

I resolved all the issues except moving things to ExtensionHost. I'll look into
that to see if that makes sense and ping you once it is done (or proven to not
be worth it)/

http://codereview.chromium.org/596105/diff/19001/19004
File chrome/browser/extensions/extension_bookmark_manager_api.cc (right):

http://codereview.chromium.org/596105/diff/19001/19004#newcode88
chrome/browser/extensions/extension_bookmark_manager_api.cc:88: for (int i = 0;
i < node.GetChildCount(); ++i) {
On 2010/02/19 21:51:35, Erik Kay wrote:
> nit: remove {} (multiple places)

Done.

http://codereview.chromium.org/596105/diff/19001/19004#newcode97
chrome/browser/extensions/extension_bookmark_manager_api.cc:97: //
|BookmarkDragDataToJSON| when the data comes from a different profile. When
On 2010/02/19 21:51:35, Erik Kay wrote:
> I'm surprised that we have all of this cross profile support.  Is there some
new
> multi-profile use-case coming?

The bookmark drag data can come from other sources. I'll rephrase it to make
this clearer.

http://codereview.chromium.org/596105/diff/19001/19004#newcode171
chrome/browser/extensions/extension_bookmark_manager_api.cc:171:
base::JSONWriter::Write(args, false, &json_args);
On 2010/02/19 21:51:35, Erik Kay wrote:
> you could move this inside the if block

Done.

http://codereview.chromium.org/596105/diff/19001/19004#newcode182
chrome/browser/extensions/extension_bookmark_manager_api.cc:182: if
(::GetBookmarkDragData(data, &bookmark_drag_data)) {
On 2010/02/19 21:51:35, Erik Kay wrote:
> when this fails, is there anything else we should do?  is it possible for the
> client to get in a stuck state if the drag end event doesn't get sent?

In practice, the drag data does not change during drag and drop so the client
will either get all or none of the extension DnD events.

Another thing is that this does not screw with the system DnD so the drag
session will not break if this fails. An application could still get *stuck* if
programmed wrong and the data changed during the drag and drop session.

http://codereview.chromium.org/596105/diff/19001/19004#newcode218
chrome/browser/extensions/extension_bookmark_manager_api.cc:218:
bookmark_drag_data_ = BookmarkDragData();
On 2010/02/19 21:51:35, Erik Kay wrote:
> This looks like an odd construct to me.  Are you depending n the built-in copy
> constructor to do the clear?  In general, we tend to dislike these
constructors.
>  Why not just have an explicit clear() method?

My intention was to not hold on to a possibly large struct.

I added a Clear method to BookmarkDragData.

http://codereview.chromium.org/596105/diff/19001/19004#newcode401
chrome/browser/extensions/extension_bookmark_manager_api.cc:401: // LAME, the
arguments change between a list and a value depending on the
On 2010/02/19 21:51:35, Erik Kay wrote:
> why not file a bug and reference here?

That was my intention but it fell of my radar.

Done.

http://codereview.chromium.org/596105/diff/19001/19004#newcode414
chrome/browser/extensions/extension_bookmark_manager_api.cc:414:
EXTENSION_FUNCTION_VALIDATE(drop_parent);
On 2010/02/19 21:51:35, Erik Kay wrote:
> I don't think this should be checked with EXTENSION_FUNCTION_VALIDATE since
it's
> legitimately possible for this id to have gone away.  EFV is only meant to be
> used in cases where the data *can't* be generated by the renderer (or if it
did
> it would be a bug in the code).  When it fails, it kills the extension,
assuming
> it's a security violation.  Instead, this should be an error (error_ =
> "message";) and an early return.

I went through this file and replaced a couple EFV macros with error_ messages
instead.

http://codereview.chromium.org/596105/diff/19001/19005
File chrome/browser/extensions/extension_dom_ui.h (right):

http://codereview.chromium.org/596105/diff/19001/19005#newcode53
chrome/browser/extensions/extension_dom_ui.h:53: virtual
ExtensionBookmarkManagerEventRouter*
On 2010/02/19 21:51:35, Erik Kay wrote:
> why does this need to be here?  I think we've put other event routers on
> ExtensionsService.

I'll try to move it.

http://codereview.chromium.org/596105/diff/19001/19008
File chrome/browser/tab_contents/tab_contents.h (right):

http://codereview.chromium.org/596105/diff/19001/19008#newcode663
chrome/browser/tab_contents/tab_contents.h:663: // It is up to callers to call
SetBookmarkDragDelegate(NULL) when
On 2010/02/19 21:51:35, Erik Kay wrote:
> nit: newline above comment

Done.

http://codereview.chromium.org/596105/diff/19001/19009
File chrome/browser/tab_contents/web_drop_target_win.cc (right):

http://codereview.chromium.org/596105/diff/19001/19009#newcode121
chrome/browser/tab_contents/web_drop_target_win.cc:121: if
(tab_contents_->GetBookmarkDragDelegate()) {
On 2010/02/19 21:51:35, Erik Kay wrote:
> just so I understand this, this should be non-null if and only if the drag
> operation involves bookmarks, right?

I'll add comments.

This is non-null if tab_contents_ is showing an ExtensionDOMUI and with support
for (at the moment experimental) drag and drop extensions.

http://codereview.chromium.org/596105/diff/19001/19012
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/596105/diff/19001/19012#newcode1960
chrome/common/extensions/api/extension_api.json:1960: "id":
"BookmarkDragDataElement",
On 2010/02/19 21:51:35, Erik Kay wrote:
> isn't this just the same as BookmarkNode?  Why have a different type?

No, the difference is that the BookmarkDragDataElement has an optional ID since
the BookmarkDragDataElement might come from non bookmark manager sources. Maybe
I should use a subtype?

[Erik does not do reviews, 2010-02-19 23:47:16]

LGTM

http://codereview.chromium.org/596105/diff/19001/19012
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/596105/diff/19001/19012#newcode1960
chrome/common/extensions/api/extension_api.json:1960: "id":
"BookmarkDragDataElement",
On 2010/02/19 23:28:21, arv wrote:
> On 2010/02/19 21:51:35, Erik Kay wrote:
> > isn't this just the same as BookmarkNode?  Why have a different type?
> 
> No, the difference is that the BookmarkDragDataElement has an optional ID
since
> the BookmarkDragDataElement might come from non bookmark manager sources.
Maybe
> I should use a subtype?

Nah.  I think this is fine.

http://codereview.chromium.org/596105/diff/20010/19027
File chrome/browser/extensions/extension_dom_ui.cc (right):

http://codereview.chromium.org/596105/diff/20010/19027#newcode54
chrome/browser/extensions/extension_dom_ui.cc:54: // TODO(arv): Move to
ExtensionHost?
ExtensionsService - but maybe I misunderstood what was going on here.  Does each
DOMUI need its own router?  And is it specific to tab contents?  I was used to
the other event routers which were more global.  If that's the case, then I
think it's OK to keep it here.

[arv (Not doing code reviews), 2010-02-20 00:04:31]

http://codereview.chromium.org/596105/diff/20010/19027
File chrome/browser/extensions/extension_dom_ui.cc (right):

http://codereview.chromium.org/596105/diff/20010/19027#newcode54
chrome/browser/extensions/extension_dom_ui.cc:54: // TODO(arv): Move to
ExtensionHost?
On 2010/02/19 23:47:17, Erik Kay wrote:
> ExtensionsService - but maybe I misunderstood what was going on here.  Does
each
> DOMUI need its own router?  And is it specific to tab contents?  I was used to
> the other event routers which were more global.  If that's the case, then I
> think it's OK to keep it here.

Yes, each ExtensionDOMUI tab contents needs its own router. I was thinking that
maybe the extension service could be used to only dispatch to the view that the
user was dragging over.

After looking at ExtensionsService I think the current solution is clearer.