|
|
Created:
3 years, 7 months ago by calamity Modified:
3 years, 6 months ago CC:
chromium-reviews, tfarina, browser-components-watch_chromium.org, arv+watch_chromium.org, michaelpg+watch-md-ui_chromium.org, chrome-apps-syd-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[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
Patch Set 1 : #
Total comments: 2
Patch Set 2 : fix shortcut #Patch Set 3 : fix test #
Total comments: 3
Patch Set 4 : address comment #Patch Set 5 : rebase #Patch Set 6 : fix DEPS #
Messages
Total messages: 47 (32 generated)
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 ==========
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
calamity@chromium.org changed reviewers: + tsergeant@chromium.org
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.
The CQ bit was checked by calamity@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by calamity@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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.
The CQ bit was checked by calamity@chromium.org
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")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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@chromium.org changed reviewers: + sky@chromium.org
+sky for chrome/browser/bookmarks OWNERS. PTAL, thanks!
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?
The CQ bit was checked by calamity@chromium.org to run a CQ dry run
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.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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-...)
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.
The CQ bit was checked by calamity@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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.
The CQ bit was unchecked by commit-bot@chromium.org
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...)
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.
The CQ bit was checked by calamity@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
SLGTM
The CQ bit was checked by calamity@chromium.org
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")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 140001, "attempt_start_ts": 1496025905917510, "parent_rev": "d93dbb23431e74ccc31aba4183fc2aef78a6553c", "commit_rev": "2d4b550b0a05e24998d550b8d070d170880283a2"}
Message was sent while issue was closed.
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... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:140001) as https://chromium.googlesource.com/chromium/src/+/2d4b550b0a05e24998d550b8d070... |