Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2977523002:
MD Bookmarks: Scroll and select items that are added to the main list (Closed)
DescriptionMD Bookmarks: Scroll and select items that are added to the main list
This 'highlights' bookmark items in the following situations:
* A new item is created in the list
* Item(s) are pasted into the list
* Item(s) are dropped into the list
* 'Show in folder' is selected on a search result
The list of items which will be created is not always known ahead
of time. Therefore, each time the user performs an action which can
create items, we track all items that are created and updated, and
highlight those items once the action has finished processing.
BUG=738958
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2977523002
Cr-Commit-Position: refs/heads/master@{#490223}
Committed: https://chromium.googlesource.com/chromium/src/+/f42530c8a51162f2b510188de04752edf06a1345
Patch Set 1 #Patch Set 2 : Rebase #Patch Set 3 : Fix test #
Total comments: 15
Patch Set 4 : Review comments #Patch Set 5 : Finish highlight on API callback #
Total comments: 5
Patch Set 6 : Review comments, change how highlighting triggers #
Total comments: 2
Patch Set 7 : Add debouncer, rebase past DND change #
Total comments: 10
Patch Set 8 : Review comments #
Total comments: 6
Patch Set 9 : Reformat json schema #Messages
Total messages: 67 (47 generated)
Description was changed from ========== MD Bookmarks: Scroll and select items that are added to the main list This 'highlights' bookmark items in the following situations: * A new item is created in the list * Item(s) are pasted into the list * Item(s) are dropped into the list * 'Show in folder' is selected on a search result Selecting these items correctly is a little bit complicated because: * It is not always known ahead of time what items will be added to the list (eg, paste). * Items are added to the list in batches. * Even after the batch of items are added, items will only be created (and therefore scrollable to) asynchronously BUG=738958 ========== to ========== MD Bookmarks: Scroll and select items that are added to the main list This 'highlights' bookmark items in the following situations: * A new item is created in the list * Item(s) are pasted into the list * Item(s) are dropped into the list * 'Show in folder' is selected on a search result Selecting these items correctly is a little bit complicated because: * It is not always known ahead of time what items will be added to the list (eg, paste). * Items are added to the list in batches. * Even after the batch of items are added, items will only be created (and therefore scrollable to) asynchronously BUG=738958 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by tsergeant@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: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by tsergeant@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: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by tsergeant@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: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== MD Bookmarks: Scroll and select items that are added to the main list This 'highlights' bookmark items in the following situations: * A new item is created in the list * Item(s) are pasted into the list * Item(s) are dropped into the list * 'Show in folder' is selected on a search result Selecting these items correctly is a little bit complicated because: * It is not always known ahead of time what items will be added to the list (eg, paste). * Items are added to the list in batches. * Even after the batch of items are added, items will only be created (and therefore scrollable to) asynchronously BUG=738958 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Bookmarks: Scroll and select items that are added to the main list This 'highlights' bookmark items in the following situations: * A new item is created in the list * Item(s) are pasted into the list * Item(s) are dropped into the list * 'Show in folder' is selected on a search result Selecting these items correctly is difficult because: * It is not always known ahead of time what items will be added to the list (eg, paste). * Items are added to the list in batches. * Even after the batch of items are added, items will only be created (and therefore scrollable to) asynchronously BUG=738958 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
tsergeant@chromium.org changed reviewers: + calamity@chromium.org
PTAL. Note that at the moment, focusing the items gives them a focus ring. I'm not sure how this will interact with your update to the mouse focus system.
This is pretty noice! Any possibility of something similar for undo/redo? https://codereview.chromium.org/2977523002/diff/40001/chrome/browser/resource... File chrome/browser/resources/md_bookmarks/list.js (right): https://codereview.chromium.org/2977523002/diff/40001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/list.js:177: (e.detail.filter((item) => this.displayedIds_.indexOf(item) != -1)); Comment plx. What are the cases where this comes up? https://codereview.chromium.org/2977523002/diff/40001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/list.js:183: bookmarks.actions.selectAll(toHighlight, this.getState(), leadId)); leadId is selected as the anchor here and in the previous statement. https://codereview.chromium.org/2977523002/diff/40001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/list.js:183: bookmarks.actions.selectAll(toHighlight, this.getState(), leadId)); So a batch of operations from a move isn't _guaranteed_ to be a single batch. Shouldn't this be additive in case the highlight event fires early? Or will everything be totally borked in that case anyway? https://codereview.chromium.org/2977523002/diff/40001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/list.js:185: // Allow iron-list time to render changes to the list. What changes need to be rendered here? The adds/removes from the batchUpdate? Is this guaranteed to happen in a single task? https://codereview.chromium.org/2977523002/diff/40001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/list.js:190: this.$.bookmarksCard.focusItem(leadIndex); tangent: I would love to rename bookmarksCard to like, ironList or something. https://codereview.chromium.org/2977523002/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/md_bookmarks/command_manager_test.js (right): https://codereview.chromium.org/2977523002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/md_bookmarks/command_manager_test.js:143: var bmpPasteFunction = chrome.bookmarkManagerPrivate.paste; Can we just do this in the setup/teardown like for copy?
https://codereview.chromium.org/2977523002/diff/40001/chrome/browser/resource... File chrome/browser/resources/md_bookmarks/list.js (right): https://codereview.chromium.org/2977523002/diff/40001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/list.js:177: (e.detail.filter((item) => this.displayedIds_.indexOf(item) != -1)); On 2017/07/14 04:14:39, calamity wrote: > Comment plx. What are the cases where this comes up? This is just being defensive, but I think it's a reasonable check to have: This array accepts any list of IDs, and the current caller (ApiListener) doesn't make any exact guarantees that the creations it sees are the ones we care about. https://codereview.chromium.org/2977523002/diff/40001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/list.js:183: bookmarks.actions.selectAll(toHighlight, this.getState(), leadId)); On 2017/07/14 04:14:39, calamity wrote: > leadId is selected as the anchor here and in the previous statement. Done. https://codereview.chromium.org/2977523002/diff/40001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/list.js:183: bookmarks.actions.selectAll(toHighlight, this.getState(), leadId)); On 2017/07/14 04:14:39, calamity wrote: > So a batch of operations from a move isn't _guaranteed_ to be a single batch. > Shouldn't this be additive in case the highlight event fires early? Or will > everything be totally borked in that case anyway? Yeah, this is the thing that was hard about this CL. Without having callbacks from the APIs to definitively tell when an operation is complete, we just can't be certain when the batch of create calls is finished. Making this additive wouldn't necessarily be sufficient, because we still need to decide when to turn off trackUpdatedItems in ApiListener. One alternative solution is to be more like the old bookmark manager, which sets a timeout for 300ms and highlights everything that was created and moved during that time. The problem there is that our processing time for big batches could potentially be more than 300ms on a slow device anyway. In short, I don't really know what the best solution is, but this seems to work well while I've been testing it. https://codereview.chromium.org/2977523002/diff/40001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/list.js:185: // Allow iron-list time to render changes to the list. On 2017/07/14 04:14:39, calamity wrote: > What changes need to be rendered here? The adds/removes from the batchUpdate? Is > this guaranteed to happen in a single task? Yeah, additions to the list are what's important. iron-list gives no formal guarantees on when things will be done rendering. But in the current version, it renders with microtask timing. In the latest version with Polymer 2.0, it will render with animation frame timing, welp: https://github.com/PolymerElements/iron-list/blob/master/iron-list.html#L1104 https://codereview.chromium.org/2977523002/diff/40001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/list.js:190: this.$.bookmarksCard.focusItem(leadIndex); On 2017/07/14 04:14:39, calamity wrote: > tangent: I would love to rename bookmarksCard to like, ironList or something. Renamed to list. https://codereview.chromium.org/2977523002/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/md_bookmarks/command_manager_test.js (right): https://codereview.chromium.org/2977523002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/md_bookmarks/command_manager_test.js:143: var bmpPasteFunction = chrome.bookmarkManagerPrivate.paste; On 2017/07/14 04:14:40, calamity wrote: > Can we just do this in the setup/teardown like for copy? Done.
https://codereview.chromium.org/2977523002/diff/40001/chrome/browser/resource... File chrome/browser/resources/md_bookmarks/list.js (right): https://codereview.chromium.org/2977523002/diff/40001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/list.js:177: (e.detail.filter((item) => this.displayedIds_.indexOf(item) != -1)); On 2017/07/14 06:39:20, tsergeant wrote: > On 2017/07/14 04:14:39, calamity wrote: > > Comment plx. What are the cases where this comes up? > > This is just being defensive, but I think it's a reasonable check to have: This > array accepts any list of IDs, and the current caller (ApiListener) doesn't make > any exact guarantees that the creations it sees are the ones we care about. Acknowledged. https://codereview.chromium.org/2977523002/diff/40001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/list.js:183: bookmarks.actions.selectAll(toHighlight, this.getState(), leadId)); On 2017/07/14 06:39:19, tsergeant wrote: > On 2017/07/14 04:14:39, calamity wrote: > > So a batch of operations from a move isn't _guaranteed_ to be a single batch. > > Shouldn't this be additive in case the highlight event fires early? Or will > > everything be totally borked in that case anyway? > > Yeah, this is the thing that was hard about this CL. Without having callbacks > from the APIs to definitively tell when an operation is complete, we just can't > be certain when the batch of create calls is finished. > > Making this additive wouldn't necessarily be sufficient, because we still need > to decide when to turn off trackUpdatedItems in ApiListener. > > One alternative solution is to be more like the old bookmark manager, which sets > a timeout for 300ms and highlights everything that was created and moved during > that time. The problem there is that our processing time for big batches could > potentially be more than 300ms on a slow device anyway. > > In short, I don't really know what the best solution is, but this seems to work > well while I've been testing it. Can we add a completion callback to Drop and Paste to guarantee completion of batched operations?
Patchset #5 (id:80001) has been deleted
The CQ bit was checked by tsergeant@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...
Description was changed from ========== MD Bookmarks: Scroll and select items that are added to the main list This 'highlights' bookmark items in the following situations: * A new item is created in the list * Item(s) are pasted into the list * Item(s) are dropped into the list * 'Show in folder' is selected on a search result Selecting these items correctly is difficult because: * It is not always known ahead of time what items will be added to the list (eg, paste). * Items are added to the list in batches. * Even after the batch of items are added, items will only be created (and therefore scrollable to) asynchronously BUG=738958 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Bookmarks: Scroll and select items that are added to the main list This 'highlights' bookmark items in the following situations: * A new item is created in the list * Item(s) are pasted into the list * Item(s) are dropped into the list * 'Show in folder' is selected on a search result The list of items which will be created is not always known ahead of time. Therefore, each time the user performs an action which can create items, we track all items that are created and updated, and highlight those items once the action has finished processing. BUG=738958 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
https://codereview.chromium.org/2977523002/diff/40001/chrome/browser/resource... File chrome/browser/resources/md_bookmarks/list.js (right): https://codereview.chromium.org/2977523002/diff/40001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/list.js:183: bookmarks.actions.selectAll(toHighlight, this.getState(), leadId)); On 2017/07/17 05:09:49, calamity wrote: > On 2017/07/14 06:39:19, tsergeant wrote: > > On 2017/07/14 04:14:39, calamity wrote: > > > So a batch of operations from a move isn't _guaranteed_ to be a single > batch. > > > Shouldn't this be additive in case the highlight event fires early? Or will > > > everything be totally borked in that case anyway? > > > > Yeah, this is the thing that was hard about this CL. Without having callbacks > > from the APIs to definitively tell when an operation is complete, we just > can't > > be certain when the batch of create calls is finished. > > > > Making this additive wouldn't necessarily be sufficient, because we still need > > to decide when to turn off trackUpdatedItems in ApiListener. > > > > One alternative solution is to be more like the old bookmark manager, which > sets > > a timeout for 300ms and highlights everything that was created and moved > during > > that time. The problem there is that our processing time for big batches could > > potentially be more than 300ms on a slow device anyway. > > > > In short, I don't really know what the best solution is, but this seems to > work > > well while I've been testing it. > > Can we add a completion callback to Drop and Paste to guarantee completion of > batched operations? Done. https://codereview.chromium.org/2977523002/diff/100001/chrome/test/data/webui... File chrome/test/data/webui/md_bookmarks/command_manager_test.js (right): https://codereview.chromium.org/2977523002/diff/100001/chrome/test/data/webui... chrome/test/data/webui/md_bookmarks/command_manager_test.js:175: test('Show In Folder is only available during search', function() { oops
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2977523002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/md_bookmarks/api_listener.js (right): https://codereview.chromium.org/2977523002/diff/100001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/api_listener.js:49: assert(trackUpdates); Try mashing Ctrl+V while you drop something. I get the feeling that this might fire. Probably not a huge deal either way. https://codereview.chromium.org/2977523002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/md_bookmarks/dnd_manager.js (right): https://codereview.chromium.org/2977523002/diff/100001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/dnd_manager.js:363: dropInfo.parentId, index, function() { shouldHighlight ? bookmarks.ApiListener.highlightUpdatedItems : undefined?
The CQ bit was checked by tsergeant@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...
https://codereview.chromium.org/2977523002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/md_bookmarks/api_listener.js (right): https://codereview.chromium.org/2977523002/diff/100001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/api_listener.js:49: assert(trackUpdates); On 2017/07/19 05:25:14, calamity wrote: > Try mashing Ctrl+V while you drop something. I get the feeling that this might > fire. Probably not a huge deal either way. Yeah, I think I was able to get this to trigger. I've removed the assert and shuffled around the logic so that it is called more directly after the batch update. This fixes all the cases I can think of with unusual event timing (although, to be honest, this is pretty hard to reason about). https://codereview.chromium.org/2977523002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/md_bookmarks/dnd_manager.js (right): https://codereview.chromium.org/2977523002/diff/100001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/dnd_manager.js:363: dropInfo.parentId, index, function() { On 2017/07/19 05:25:14, calamity wrote: > shouldHighlight ? bookmarks.ApiListener.highlightUpdatedItems : undefined? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: closure_compilation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)
The CQ bit was checked by tsergeant@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/2977523002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/md_bookmarks/api_listener.js (right): https://codereview.chromium.org/2977523002/diff/120001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/api_listener.js:19: var shouldHighlightAfterBatch = false; As discussed, it would be nice to have a resettable, single-task, promise-based debouncer here instead. Then we can have function batchUIUpdates() { if (debouncer.done()) { debouncer = new Debouncer(bookmarks.Store.getInstance().endBatchUpdate); bookmarks.Store.getInstance().beginBatchUpdate(); } debouncer.debounce(); } function highlightUpdatedItems() { debouncer.promise.then(highlightUpdatedItemsImpl); }
The CQ bit was checked by tsergeant@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/2977523002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/md_bookmarks/api_listener.js (right): https://codereview.chromium.org/2977523002/diff/120001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/api_listener.js:19: var shouldHighlightAfterBatch = false; On 2017/07/20 05:49:03, calamity wrote: > As discussed, it would be nice to have a resettable, single-task, promise-based > debouncer here instead. Then we can have > > function batchUIUpdates() { > if (debouncer.done()) { > debouncer = new Debouncer(bookmarks.Store.getInstance().endBatchUpdate); > bookmarks.Store.getInstance().beginBatchUpdate(); > } > > debouncer.debounce(); > } > > function highlightUpdatedItems() { > debouncer.promise.then(highlightUpdatedItemsImpl); > } Done. https://codereview.chromium.org/2977523002/diff/140001/chrome/browser/resourc... File chrome/browser/resources/md_bookmarks/timer_proxy.js (right): https://codereview.chromium.org/2977523002/diff/140001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/timer_proxy.js:35: function Debouncer(callback) { I'm happy enough with this for now, but one unanswered question is how we make this testable, both in terms of getting a 'TestDebouncer' instance, and how we inject a TestTimerProxy into it.
Basically LG. https://codereview.chromium.org/2977523002/diff/140001/chrome/browser/resourc... File chrome/browser/resources/md_bookmarks/dnd_manager.js (right): https://codereview.chromium.org/2977523002/diff/140001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/dnd_manager.js:483: bookmarks.ApiListener.highlightUpdatedItems(); optional: Consider using an array of PromiseResolvers and then Promise.all()ing them. https://codereview.chromium.org/2977523002/diff/140001/chrome/browser/resourc... File chrome/browser/resources/md_bookmarks/timer_proxy.js (right): https://codereview.chromium.org/2977523002/diff/140001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/timer_proxy.js:31: * A timer which will fire on the next task after the last time it is reset. nit: This isn't strictly true. It gets added to the task list each time, so the last reset can have a bunch of tasks between itself and the actual callback firing. How about: A one-shot debouncer which fires the given callback after a delay. The delay can be refreshed by calling resetTimer. Resetting with no delay moves the callback to the end of the task queue. https://codereview.chromium.org/2977523002/diff/140001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/timer_proxy.js:35: function Debouncer(callback) { On 2017/07/25 00:13:00, tsergeant wrote: > I'm happy enough with this for now, but one unanswered question is how we make > this testable, both in terms of getting a 'TestDebouncer' instance, and how we > inject a TestTimerProxy into it. Acknowledged. https://codereview.chromium.org/2977523002/diff/140001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/timer_proxy.js:41: this.promiseResolver_ = new PromiseResolver(); nit: Couple of types? https://codereview.chromium.org/2977523002/diff/140001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/timer_proxy.js:45: resetTimer: function() { nit: resetTimeout perhaps? This will be given a delay param in my auto expander patch. (Or just do it now to align with the class comment I suggested)
https://codereview.chromium.org/2977523002/diff/140001/chrome/browser/resourc... File chrome/browser/resources/md_bookmarks/dnd_manager.js (right): https://codereview.chromium.org/2977523002/diff/140001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/dnd_manager.js:483: bookmarks.ApiListener.highlightUpdatedItems(); On 2017/07/26 05:36:06, calamity wrote: > optional: Consider using an array of PromiseResolvers and then Promise.all()ing > them. Done, it works out a little bit nicer. https://codereview.chromium.org/2977523002/diff/140001/chrome/browser/resourc... File chrome/browser/resources/md_bookmarks/timer_proxy.js (right): https://codereview.chromium.org/2977523002/diff/140001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/timer_proxy.js:31: * A timer which will fire on the next task after the last time it is reset. On 2017/07/26 05:36:06, calamity wrote: > nit: This isn't strictly true. It gets added to the task list each time, so the > last reset can have a bunch of tasks between itself and the actual callback > firing. > > How about: > > A one-shot debouncer which fires the given callback after a delay. The delay can > be refreshed by calling resetTimer. Resetting with no delay moves the callback > to the end of the task queue. Done. https://codereview.chromium.org/2977523002/diff/140001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/timer_proxy.js:41: this.promiseResolver_ = new PromiseResolver(); On 2017/07/26 05:36:06, calamity wrote: > nit: Couple of types? Done. https://codereview.chromium.org/2977523002/diff/140001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/timer_proxy.js:45: resetTimer: function() { On 2017/07/26 05:36:06, calamity wrote: > nit: resetTimeout perhaps? This will be given a delay param in my auto expander > patch. (Or just do it now to align with the class comment I suggested) Done.
lgtm
tsergeant@chromium.org changed reviewers: + dpapad@chromium.org, rdevlin.cronin@chromium.org
+rdevlin.cronin@ for chrome/common/extensions/api/bookmark_manager_private.json +dpapad@ for third_party/closure_compiler/externs/bookmark_manager_private.js PTAL, thanks!
https://codereview.chromium.org/2977523002/diff/160001/chrome/common/extensio... File chrome/common/extensions/api/bookmark_manager_private.json (right): https://codereview.chromium.org/2977523002/diff/160001/chrome/common/extensio... chrome/common/extensions/api/bookmark_manager_private.json:175: {"type": "function", "name": "callback", "optional": true, "parameters": []} nit: wrap these: { "type": "function", "name": "callback", "parameters": [], "optional": true } https://codereview.chromium.org/2977523002/diff/160001/third_party/closure_co... File third_party/closure_compiler/externs/bookmark_manager_private.js (right): https://codereview.chromium.org/2977523002/diff/160001/third_party/closure_co... third_party/closure_compiler/externs/bookmark_manager_private.js:93: * @param {Function=} callback Did you regenerate these using the script? We have more specific typing for function params now.
Patchset #9 (id:180001) has been deleted
https://codereview.chromium.org/2977523002/diff/160001/chrome/common/extensio... File chrome/common/extensions/api/bookmark_manager_private.json (right): https://codereview.chromium.org/2977523002/diff/160001/chrome/common/extensio... chrome/common/extensions/api/bookmark_manager_private.json:175: {"type": "function", "name": "callback", "optional": true, "parameters": []} On 2017/07/26 16:10:36, Devlin wrote: > nit: wrap these: > { > "type": "function", > "name": "callback", > "parameters": [], > "optional": true > } Done. https://codereview.chromium.org/2977523002/diff/160001/third_party/closure_co... File third_party/closure_compiler/externs/bookmark_manager_private.js (right): https://codereview.chromium.org/2977523002/diff/160001/third_party/closure_co... third_party/closure_compiler/externs/bookmark_manager_private.js:93: * @param {Function=} callback On 2017/07/26 16:10:37, Devlin wrote: > Did you regenerate these using the script? We have more specific typing for > function params now. I did this by hand. I'm having trouble with the json_schema_compiler, where it takes the type 'bookmarks.BookmarkTreeNode' (used in getSubtree), and rewrites it as the type 'chrome.bookmarkManagerPrivate.bookmarks.BookmarkTreeNode', which Closure is not happy with. Do you know if there's a way around this? Or if this is a bug in json_schema_compiler that we'd need to work around.
https://codereview.chromium.org/2977523002/diff/160001/third_party/closure_co... File third_party/closure_compiler/externs/bookmark_manager_private.js (right): https://codereview.chromium.org/2977523002/diff/160001/third_party/closure_co... third_party/closure_compiler/externs/bookmark_manager_private.js:93: * @param {Function=} callback On 2017/07/26 23:36:39, tsergeant wrote: > On 2017/07/26 16:10:37, Devlin wrote: > > Did you regenerate these using the script? We have more specific typing for > > function params now. > > I did this by hand. I'm having trouble with the json_schema_compiler, where it > takes the type 'bookmarks.BookmarkTreeNode' (used in getSubtree), and rewrites > it as the type 'chrome.bookmarkManagerPrivate.bookmarks.BookmarkTreeNode', which > Closure is not happy with. > > Do you know if there's a way around this? Or if this is a bug in > json_schema_compiler that we'd need to work around. Ah, here's a bug for this problem: https://crbug.com/543822
lgtm (note: I also own externs, so dpapad might not be necessary on this CL? Though of course welcome to review it) https://codereview.chromium.org/2977523002/diff/160001/third_party/closure_co... File third_party/closure_compiler/externs/bookmark_manager_private.js (right): https://codereview.chromium.org/2977523002/diff/160001/third_party/closure_co... third_party/closure_compiler/externs/bookmark_manager_private.js:93: * @param {Function=} callback On 2017/07/27 03:06:19, tsergeant wrote: > On 2017/07/26 23:36:39, tsergeant wrote: > > On 2017/07/26 16:10:37, Devlin wrote: > > > Did you regenerate these using the script? We have more specific typing for > > > function params now. > > > > I did this by hand. I'm having trouble with the json_schema_compiler, where it > > takes the type 'bookmarks.BookmarkTreeNode' (used in getSubtree), and rewrites > > it as the type 'chrome.bookmarkManagerPrivate.bookmarks.BookmarkTreeNode', > which > > Closure is not happy with. > > > > Do you know if there's a way around this? Or if this is a bug in > > json_schema_compiler that we'd need to work around. > > Ah, here's a bug for this problem: https://crbug.com/543822 Oh, bummer. Okay, thanks for checking!
The CQ bit was checked by tsergeant@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: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by tsergeant@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.
Ah, didn't realise you were an owner for both. I'm going to go ahead and land it now. Thanks for reviewing!
The CQ bit was checked by tsergeant@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from calamity@chromium.org Link to the patchset: https://codereview.chromium.org/2977523002/#ps200001 (title: "Reformat json schema")
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": 200001, "attempt_start_ts": 1501209435026490,
"parent_rev": "1fa8d19c60e8f878547f98feb6c04e8cdc1faa12", "commit_rev":
"f42530c8a51162f2b510188de04752edf06a1345"}
Message was sent while issue was closed.
Description was changed from ========== MD Bookmarks: Scroll and select items that are added to the main list This 'highlights' bookmark items in the following situations: * A new item is created in the list * Item(s) are pasted into the list * Item(s) are dropped into the list * 'Show in folder' is selected on a search result The list of items which will be created is not always known ahead of time. Therefore, each time the user performs an action which can create items, we track all items that are created and updated, and highlight those items once the action has finished processing. BUG=738958 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Bookmarks: Scroll and select items that are added to the main list This 'highlights' bookmark items in the following situations: * A new item is created in the list * Item(s) are pasted into the list * Item(s) are dropped into the list * 'Show in folder' is selected on a search result The list of items which will be created is not always known ahead of time. Therefore, each time the user performs an action which can create items, we track all items that are created and updated, and highlight those items once the action has finished processing. BUG=738958 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2977523002 Cr-Commit-Position: refs/heads/master@{#490223} Committed: https://chromium.googlesource.com/chromium/src/+/f42530c8a51162f2b510188de047... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:200001) as https://chromium.googlesource.com/chromium/src/+/f42530c8a51162f2b510188de047... |
