[MD Bookmarks] Add undo/redo functionality to MD Bookmarks.

[MD Bookmarks] Add undo/redo functionality to MD Bookmarks.

This CL adds undo/redo functionality to the bookmark manager by enabling
the BookmarkUndoService when MD Bookmarks is enabled.

BUG=692847
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2901463002
Cr-Commit-Position: refs/heads/master@{#475283}
Committed: https://chromium.googlesource.com/chromium/src/+/2d4b550b0a05e24998d550b8d070d170880283a2

[calamity, 2017-05-22 05:14:37]

Description was changed from

==========
[MD Bookmarks] Add undo/redo functionality to MD Bookmarks.

This CL adds undo/redo functionality to the bookmark manager by enabling
the BookmarkUndoService when MD Bookmarks is enabled.

BUG=692847
==========

to

==========
[MD Bookmarks] Add undo/redo functionality to MD Bookmarks.

This CL adds undo/redo functionality to the bookmark manager by enabling
the BookmarkUndoService when MD Bookmarks is enabled.

BUG=692847
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[calamity, 2017-05-23 05:51:11]

Patchset #1 (id:1) has been deleted

[calamity, 2017-05-23 05:52:27]

Patchset #1 (id:20001) has been deleted

[calamity, 2017-05-23 05:52:40]

calamity@chromium.org changed reviewers:
+ tsergeant@chromium.org

[tsergeant, 2017-05-23 07:34:02]

lgtm

https://codereview.chromium.org/2901463002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/command_manager.js (right):

https://codereview.chromium.org/2901463002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/command_manager.js:62: cr.isMac ? 'meta+z'
: 'ctrl+z ctrl+shift+z';
Is ctrl-shift-z meant to be REDO? That's what happens elsewhere in Chrome.

[calamity, 2017-05-24 04:15:10]

The CQ bit was checked by calamity@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-24 04:15:55]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-24 05:21:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-24 05:21:18]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[calamity, 2017-05-24 05:50:40]

The CQ bit was checked by calamity@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-24 05:51:21]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-24 07:33:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-24 07:33:10]

Dry run: This issue passed the CQ dry run.

[calamity, 2017-05-24 07:44:14]

https://codereview.chromium.org/2901463002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/command_manager.js (right):

https://codereview.chromium.org/2901463002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/command_manager.js:62: cr.isMac ? 'meta+z'
: 'ctrl+z ctrl+shift+z';
On 2017/05/23 07:34:02, tsergeant wrote:
> Is ctrl-shift-z meant to be REDO? That's what happens elsewhere in Chrome.

Oops, fixed.

[calamity, 2017-05-24 07:44:31]

The CQ bit was checked by calamity@chromium.org

[calamity, 2017-05-24 07:44:32]

The patchset sent to the CQ was uploaded after l-g-t-m from
tsergeant@chromium.org
Link to the patchset: https://codereview.chromium.org/2901463002/#ps80001
(title: "fix test")

[commit-bot: I haz the power, 2017-05-24 07:44:54]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-24 07:52:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-24 07:52:46]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[calamity, 2017-05-24 08:12:08]

calamity@chromium.org changed reviewers:
+ sky@chromium.org

[calamity, 2017-05-24 08:12:09]

+sky for chrome/browser/bookmarks OWNERS.

PTAL, thanks!

[sky, 2017-05-24 16:57:16]

https://codereview.chromium.org/2901463002/diff/80001/chrome/browser/bookmark...
File chrome/browser/bookmarks/bookmark_model_factory.cc (right):

https://codereview.chromium.org/2901463002/diff/80001/chrome/browser/bookmark...
chrome/browser/bookmarks/bookmark_model_factory.cc:78:
MdBookmarksUI::IsEnabled();
Where do we make use of the undo in non-android? For example,
chrome/browser/ui/bookmarks/bookmark_context_menu_controller.cc doesn't look at
this conditional. How about a single function for testing if undo is enabled so
it's easier to update all places?

[calamity, 2017-05-25 08:53:55]

The CQ bit was checked by calamity@chromium.org to run a CQ dry run

[calamity, 2017-05-25 08:54:02]

https://codereview.chromium.org/2901463002/diff/80001/chrome/browser/bookmark...
File chrome/browser/bookmarks/bookmark_model_factory.cc (right):

https://codereview.chromium.org/2901463002/diff/80001/chrome/browser/bookmark...
chrome/browser/bookmarks/bookmark_model_factory.cc:78:
MdBookmarksUI::IsEnabled();
On 2017/05/24 16:57:16, sky wrote:
> Where do we make use of the undo in non-android? For example,
> chrome/browser/ui/bookmarks/bookmark_context_menu_controller.cc doesn't look
at
> this conditional. How about a single function for testing if undo is enabled
so
> it's easier to update all places?

It's behind a flag for the bookmarks bar context menu, but that's all I think.
I've made the BookmarkUndoService::IsEnabled() function but I don't think I
should hook the Bookmarks Bar context menu up to that function because it's a
product change and should probably go through a PM/UX once-over first.

I think we'll aim to remove the kEnableBookmarkUndo flag one way or the other.
We'll either enable it for the context menu always, or remove the context menu
code and have it locked to MD Bookmarks.

[commit-bot: I haz the power, 2017-05-25 08:54:04]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-25 09:00:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-25 09:00:37]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)

[sky, 2017-05-25 18:09:06]

LGTM

https://codereview.chromium.org/2901463002/diff/80001/chrome/browser/bookmark...
File chrome/browser/bookmarks/bookmark_model_factory.cc (right):

https://codereview.chromium.org/2901463002/diff/80001/chrome/browser/bookmark...
chrome/browser/bookmarks/bookmark_model_factory.cc:78:
MdBookmarksUI::IsEnabled();
On 2017/05/25 08:54:02, calamity wrote:
> On 2017/05/24 16:57:16, sky wrote:
> > Where do we make use of the undo in non-android? For example,
> > chrome/browser/ui/bookmarks/bookmark_context_menu_controller.cc doesn't look
> at
> > this conditional. How about a single function for testing if undo is enabled
> so
> > it's easier to update all places?
> 
> It's behind a flag for the bookmarks bar context menu, but that's all I think.
> I've made the BookmarkUndoService::IsEnabled() function but I don't think I
> should hook the Bookmarks Bar context menu up to that function because it's a
> product change and should probably go through a PM/UX once-over first.
> 
> I think we'll aim to remove the kEnableBookmarkUndo flag one way or the other.
> We'll either enable it for the context menu always, or remove the context menu
> code and have it locked to MD Bookmarks.

If you don't enable undo in the menu how is it possible to use undo?

Given your in a different timezone I'm going to approve this assuming you have
it wired up somewhere.

[calamity, 2017-05-26 02:45:14]

The CQ bit was checked by calamity@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-26 02:46:24]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[calamity, 2017-05-26 02:47:54]

On 2017/05/25 18:09:06, sky wrote:
> LGTM
> 
>
https://codereview.chromium.org/2901463002/diff/80001/chrome/browser/bookmark...
> File chrome/browser/bookmarks/bookmark_model_factory.cc (right):
> 
>
https://codereview.chromium.org/2901463002/diff/80001/chrome/browser/bookmark...
> chrome/browser/bookmarks/bookmark_model_factory.cc:78:
> MdBookmarksUI::IsEnabled();
> On 2017/05/25 08:54:02, calamity wrote:
> > On 2017/05/24 16:57:16, sky wrote:
> > > Where do we make use of the undo in non-android? For example,
> > > chrome/browser/ui/bookmarks/bookmark_context_menu_controller.cc doesn't
look
> > at
> > > this conditional. How about a single function for testing if undo is
enabled
> > so
> > > it's easier to update all places?
> > 
> > It's behind a flag for the bookmarks bar context menu, but that's all I
think.
> > I've made the BookmarkUndoService::IsEnabled() function but I don't think I
> > should hook the Bookmarks Bar context menu up to that function because it's
a
> > product change and should probably go through a PM/UX once-over first.
> > 
> > I think we'll aim to remove the kEnableBookmarkUndo flag one way or the
other.
> > We'll either enable it for the context menu always, or remove the context
menu
> > code and have it locked to MD Bookmarks.
> 
> If you don't enable undo in the menu how is it possible to use undo?
> 
> Given your in a different timezone I'm going to approve this assuming you have
> it wired up somewhere.

It's for use in the Bookmark Manager, rather than the Bookmarks Bar, which calls
through via Javascript. It gets hooked up through the BookmarkPrivateAPI which
calls the BookmarkUndoService, which needs to be attached to the BookmarkModel.

[commit-bot: I haz the power, 2017-05-26 02:56:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-26 02:56:09]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[sky, 2017-05-26 03:11:31]

Makes sense. Thanks.

On Thu, May 25, 2017 at 7:47 PM, <calamity@chromium.org> wrote:

> On 2017/05/25 18:09:06, sky wrote:
> > LGTM
> >
> >
> https://codereview.chromium.org/2901463002/diff/80001/
> chrome/browser/bookmarks/bookmark_model_factory.cc
> > File chrome/browser/bookmarks/bookmark_model_factory.cc (right):
> >
> >
> https://codereview.chromium.org/2901463002/diff/80001/
> chrome/browser/bookmarks/bookmark_model_factory.cc#newcode78
> > chrome/browser/bookmarks/bookmark_model_factory.cc:78:
> > MdBookmarksUI::IsEnabled();
> > On 2017/05/25 08:54:02, calamity wrote:
> > > On 2017/05/24 16:57:16, sky wrote:
> > > > Where do we make use of the undo in non-android? For example,
> > > > chrome/browser/ui/bookmarks/bookmark_context_menu_controller.cc
> doesn't
> look
> > > at
> > > > this conditional. How about a single function for testing if undo is
> enabled
> > > so
> > > > it's easier to update all places?
> > >
> > > It's behind a flag for the bookmarks bar context menu, but that's all I
> think.
> > > I've made the BookmarkUndoService::IsEnabled() function but I don't
> think I
> > > should hook the Bookmarks Bar context menu up to that function because
> it's
> a
> > > product change and should probably go through a PM/UX once-over first.
> > >
> > > I think we'll aim to remove the kEnableBookmarkUndo flag one way or the
> other.
> > > We'll either enable it for the context menu always, or remove the
> context
> menu
> > > code and have it locked to MD Bookmarks.
> >
> > If you don't enable undo in the menu how is it possible to use undo?
> >
> > Given your in a different timezone I'm going to approve this assuming
> you have
> > it wired up somewhere.
>
> It's for use in the Bookmark Manager, rather than the Bookmarks Bar, which
> calls
> through via Javascript. It gets hooked up through the BookmarkPrivateAPI
> which
> calls the BookmarkUndoService, which needs to be attached to the
> BookmarkModel.
>
> https://codereview.chromium.org/2901463002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[calamity, 2017-05-26 04:06:40]

The CQ bit was checked by calamity@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-26 04:07:11]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[calamity, 2017-05-26 05:02:42]

Hey, there were some DEPS issues with the build, I've moved the undo service
gating back to the bookmark model factory. I think it makes sense for this to
live in Chrome anyhow, since a component shouldn't need to worry about how it
gets enabled and is a layering violation.

Let me know if you want something different. Thanks!

[commit-bot: I haz the power, 2017-05-26 05:46:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-26 05:46:41]

Dry run: This issue passed the CQ dry run.

[sky, 2017-05-26 13:42:22]

SLGTM

[calamity, 2017-05-29 02:45:05]

The CQ bit was checked by calamity@chromium.org

[calamity, 2017-05-29 02:45:06]

The patchset sent to the CQ was uploaded after l-g-t-m from
tsergeant@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2901463002/#ps140001
(title: "fix DEPS")

[commit-bot: I haz the power, 2017-05-29 02:45:13]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-29 03:58:04]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1496025905917510,
"parent_rev": "d93dbb23431e74ccc31aba4183fc2aef78a6553c", "commit_rev":
"2d4b550b0a05e24998d550b8d070d170880283a2"}

[commit-bot: I haz the power, 2017-05-29 03:58:15]

Description was changed from

==========
[MD Bookmarks] Add undo/redo functionality to MD Bookmarks.

This CL adds undo/redo functionality to the bookmark manager by enabling
the BookmarkUndoService when MD Bookmarks is enabled.

BUG=692847
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
[MD Bookmarks] Add undo/redo functionality to MD Bookmarks.

This CL adds undo/redo functionality to the bookmark manager by enabling
the BookmarkUndoService when MD Bookmarks is enabled.

BUG=692847
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2901463002
Cr-Commit-Position: refs/heads/master@{#475283}
Committed:
https://chromium.googlesource.com/chromium/src/+/2d4b550b0a05e24998d550b8d070...
==========

[commit-bot: I haz the power, 2017-05-29 03:58:17]

Committed patchset #6 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/2d4b550b0a05e24998d550b8d070...