Fix for http://code.google.com/p/chromium-os/issues/detail?id=13899

Fix for http://code.google.com/p/chromium-os/issues/detail?id=13899
Basically in this CL we reuse the existing web UI based bookmark editor
which is launched from the wrench menu. This address the WebUI part of
this issue. The ComboxBox still needs to be ported to Views. 
This change is enabled for touch only, however I am not aware of any
reason why we could not make this change for the general case.

BUG=13899
TEST=none


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

[Emmanuel Saint-loubert-Bié, 2011-04-08 02:54:16]

This is what I propose we do for touch to address
http://code.google.com/p/chromium-os/issues/detail?id=13899

I am wondering if we do not want this to be the general solution looking
forward. If you think so make it a comment and I can change the CL

Thanks,

-- E

[sky, 2011-04-08 15:33:33]

The bookmark editor is an editor designed around a single bookmark.
The bookmark manager is designed around editing all your bookmarks.
IMO we shouldn't replace the bookmark editor with the bookmark
manager. You should talk with the designers, Glen and Nicholas, for
their input.

  -Scott

On Thu, Apr 7, 2011 at 7:54 PM,  <saintlou@chromium.org> wrote:
> Reviewers: rjkroege, sky,
>
> Message:
>
> This is what I propose we do for touch to address
> http://code.google.com/p/chromium-os/issues/detail?id=13899
>
> I am wondering if we do not want this to be the general solution looking
> forward. If you think so make it a comment and I can change the CL
>
> Thanks,
>
> -- E
>
> Description:
> Fix for http://code.google.com/p/chromium-os/issues/detail?id=13899
> Basically in this CL we reuse the existing web UI based bookmark editor
> which is launched from the wrench menu. This address the WebUI part of
> this issue. The ComboxBox still needs to be ported to Views.
> This change is enabled for touch only, however I am not aware of any
> reason why we could not make this change for the general case.
>
> BUG=13899
> TEST=none
>
>
> Please review this at http://codereview.chromium.org/6814037/
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/src
>
> Affected files:
>  M chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc
>
>
> Index: chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc
> diff --git a/chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc
> b/chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc
> index
>
2b0db48d162c9c779efa1f8dac6a950376704a63..17a1ed89f68d00fabc06414e586a91e60b64317c
> 100644
> --- a/chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc
> +++ b/chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc
> @@ -13,6 +13,8 @@
>  #include "chrome/browser/bookmarks/bookmark_utils.h"
>  #include "chrome/browser/metrics/user_metrics.h"
>  #include "chrome/browser/profiles/profile.h"
> +#include "chrome/browser/ui/browser.h"
> +#include "chrome/browser/ui/browser_list.h"
>  #include "chrome/browser/ui/views/info_bubble.h"
>  #include "content/common/notification_service.h"
>  #include "grit/generated_resources.h"
> @@ -192,11 +194,6 @@ void BookmarkBubbleView::Init() {
>
>   edit_button_ = new NativeButton(
>       this,
> UTF16ToWide(l10n_util::GetStringUTF16(IDS_BOOMARK_BUBBLE_OPTIONS)));
> -#if defined(TOUCH_UI)
> -  // TODO(saintlou): We need to disable the Edit button for touch since we
> are
> -  // missing some of the controls for the dialog that would pop up next.
> -  edit_button_->SetEnabled(false);
> -#endif
>
>   close_button_ =
>       new NativeButton(this,
> UTF16ToWide(l10n_util::GetStringUTF16(IDS_DONE)));
> @@ -372,6 +369,21 @@ void
> BookmarkBubbleView::HandleButtonPressed(views::Button* sender) {
>  }
>
>  void BookmarkBubbleView::ShowEditor() {
> +#if defined(TOUCH_UI)
> +  // Commit any edits now.
> +  ApplyEdits();
> +
> +  // Close the InfoBubble
> +  Close();
> +
> +  // Open the Bookmark Manager
> +  Browser* browser = BrowserList::GetLastActiveWithProfile(profile_);
> +  if (browser)
> +    browser->OpenBookmarkManager();
> +  else
> +    NOTREACHED();
> +
> +#else
>   const BookmarkNode* node =
>       profile_->GetBookmarkModel()->GetMostRecentlyAddedNodeForURL(url_);
>
> @@ -405,6 +417,7 @@ void BookmarkBubbleView::ShowEditor() {
>                          BookmarkEditor::EditDetails(node),
>                          BookmarkEditor::SHOW_TREE);
>   }
> +#endif
>  }
>
>  void BookmarkBubbleView::ApplyEdits() {
>
>
>

[Glen Murphy, 2011-04-08 15:56:54]

I agree with Scott.

On Fri, Apr 8, 2011 at 8:33 AM, Scott Violet <sky@chromium.org> wrote:
> The bookmark editor is an editor designed around a single bookmark.
> The bookmark manager is designed around editing all your bookmarks.
> IMO we shouldn't replace the bookmark editor with the bookmark
> manager. You should talk with the designers, Glen and Nicholas, for
> their input.
>
>  -Scott
>
> On Thu, Apr 7, 2011 at 7:54 PM,  <saintlou@chromium.org> wrote:
>> Reviewers: rjkroege, sky,
>>
>> Message:
>>
>> This is what I propose we do for touch to address
>> http://code.google.com/p/chromium-os/issues/detail?id=13899
>>
>> I am wondering if we do not want this to be the general solution looking
>> forward. If you think so make it a comment and I can change the CL
>>
>> Thanks,
>>
>> -- E
>>
>> Description:
>> Fix for http://code.google.com/p/chromium-os/issues/detail?id=13899
>> Basically in this CL we reuse the existing web UI based bookmark editor
>> which is launched from the wrench menu. This address the WebUI part of
>> this issue. The ComboxBox still needs to be ported to Views.
>> This change is enabled for touch only, however I am not aware of any
>> reason why we could not make this change for the general case.
>>
>> BUG=13899
>> TEST=none
>>
>>
>> Please review this at http://codereview.chromium.org/6814037/
>>
>> SVN Base: svn://svn.chromium.org/chrome/trunk/src
>>
>> Affected files:
>>  M chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc
>>
>>
>> Index: chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc
>> diff --git a/chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc
>> b/chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc
>> index
>>
2b0db48d162c9c779efa1f8dac6a950376704a63..17a1ed89f68d00fabc06414e586a91e60b64317c
>> 100644
>> --- a/chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc
>> +++ b/chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc
>> @@ -13,6 +13,8 @@
>>  #include "chrome/browser/bookmarks/bookmark_utils.h"
>>  #include "chrome/browser/metrics/user_metrics.h"
>>  #include "chrome/browser/profiles/profile.h"
>> +#include "chrome/browser/ui/browser.h"
>> +#include "chrome/browser/ui/browser_list.h"
>>  #include "chrome/browser/ui/views/info_bubble.h"
>>  #include "content/common/notification_service.h"
>>  #include "grit/generated_resources.h"
>> @@ -192,11 +194,6 @@ void BookmarkBubbleView::Init() {
>>
>>   edit_button_ = new NativeButton(
>>       this,
>> UTF16ToWide(l10n_util::GetStringUTF16(IDS_BOOMARK_BUBBLE_OPTIONS)));
>> -#if defined(TOUCH_UI)
>> -  // TODO(saintlou): We need to disable the Edit button for touch since we
>> are
>> -  // missing some of the controls for the dialog that would pop up next.
>> -  edit_button_->SetEnabled(false);
>> -#endif
>>
>>   close_button_ =
>>       new NativeButton(this,
>> UTF16ToWide(l10n_util::GetStringUTF16(IDS_DONE)));
>> @@ -372,6 +369,21 @@ void
>> BookmarkBubbleView::HandleButtonPressed(views::Button* sender) {
>>  }
>>
>>  void BookmarkBubbleView::ShowEditor() {
>> +#if defined(TOUCH_UI)
>> +  // Commit any edits now.
>> +  ApplyEdits();
>> +
>> +  // Close the InfoBubble
>> +  Close();
>> +
>> +  // Open the Bookmark Manager
>> +  Browser* browser = BrowserList::GetLastActiveWithProfile(profile_);
>> +  if (browser)
>> +    browser->OpenBookmarkManager();
>> +  else
>> +    NOTREACHED();
>> +
>> +#else
>>   const BookmarkNode* node =
>>       profile_->GetBookmarkModel()->GetMostRecentlyAddedNodeForURL(url_);
>>
>> @@ -405,6 +417,7 @@ void BookmarkBubbleView::ShowEditor() {
>>                          BookmarkEditor::EditDetails(node),
>>                          BookmarkEditor::SHOW_TREE);
>>   }
>> +#endif
>>  }
>>
>>  void BookmarkBubbleView::ApplyEdits() {
>>
>>
>>
>

[sky, 2011-04-08 17:48:44]

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

http://codereview.chromium.org/6814037/diff/1/chrome/browser/ui/views/bookmar...
chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:374: ApplyEdits();
Close applys the edits, so you shouldn't need to invoke this.

http://codereview.chromium.org/6814037/diff/1/chrome/browser/ui/views/bookmar...
chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:383: else
This should be a DCHECK before 381.

[commit-bot: I haz the power, 2011-04-08 19:07:54]

No LGTM from valid reviewers yet.

[Emmanuel Saint-loubert-Bié, 2011-04-08 19:09:41]

Please LGTM when you have a chance. Thanks,
-- E

[sky, 2011-04-08 19:19:48]

LGTM

[commit-bot: I haz the power, 2011-04-08 21:04:36]

Change committed as 80986