Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2813503002:
MD Bookmarks: Prevent navigating to invalid folders (Closed)
DescriptionMD Bookmarks: Prevent navigating to invalid folders
This CL prevents items or non-existent IDs from being navigated to. In
order to achieve this, we add support for 'deferred actions', which
allow actions to be created asynchronously. Deferred actions are currently
only used for selecting folders from the Router, but will be used to
simplify search in a follow-up CL.
BUG=697706
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2813503002
Cr-Commit-Position: refs/heads/master@{#469290}
Committed: https://chromium.googlesource.com/chromium/src/+/c17b37ab36a216c3923da51e382d531efc61c35e
Patch Set 1 #Patch Set 2 : Add positive test case #
Total comments: 4
Patch Set 3 : Rebase & Implement deferred actions #
Total comments: 16
Patch Set 4 : Review comments #
Total comments: 9
Patch Set 5 : handleAction -> dispatch #Messages
Total messages: 47 (36 generated)
Description was changed from ========== MD Bookmarks: Prevent navigating to invalid folders This CL prevents items or non-existent IDs from being navigated to. It also adds additional tests for complex action creators. BUG=697706 ========== to ========== MD Bookmarks: Prevent navigating to invalid folders This CL prevents items or non-existent IDs from being navigated to. It also adds additional tests for complex action creators. BUG=697706 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: This issue passed the CQ dry run.
tsergeant@chromium.org changed reviewers: + calamity@chromium.org
Okay, I think I'm happier with continuing this pattern than doing the complicated other thing. WDYT?
https://codereview.chromium.org/2813503002/diff/20001/chrome/browser/resource... File chrome/browser/resources/md_bookmarks/actions.js (right): https://codereview.chromium.org/2813503002/diff/20001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/actions.js:100: * @param {NodeList} nodes Current node state. Can be ommitted in tests. Doesn't omitting this always result in a no-op? When would that be useful? https://codereview.chromium.org/2813503002/diff/20001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/actions.js:107: }; Make no-op a private action and early return in reduceAction.
Patchset #3 (id:40001) has been deleted
Patchset #3 (id:60001) 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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...
Patchset #4 (id:100001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #3 (id:80001) has been deleted
Patchset #3 (id:120001) has been deleted
Description was changed from ========== MD Bookmarks: Prevent navigating to invalid folders This CL prevents items or non-existent IDs from being navigated to. It also adds additional tests for complex action creators. BUG=697706 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Bookmarks: Prevent navigating to invalid folders This CL prevents items or non-existent IDs from being navigated to. In order to achieve this, we add support for 'deferred actions', which allow actions to be created asynchronously. Deferred actions are currently only used for selecting folders from the Router, but will be used to simplify search in a follow-up CL. BUG=697706 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Patchset #3 (id:140001) has been deleted
3 weeks later, the solution for this is ready. I've added a whole new concept to the Store to solve an annoying bug in the previous version of this CL. The CL description has been updated to explain this, and there's some inline comments as well. https://codereview.chromium.org/2813503002/diff/20001/chrome/browser/resource... File chrome/browser/resources/md_bookmarks/actions.js (right): https://codereview.chromium.org/2813503002/diff/20001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/actions.js:100: * @param {NodeList} nodes Current node state. Can be ommitted in tests. On 2017/04/11 04:13:37, calamity wrote: > Doesn't omitting this always result in a no-op? When would that be useful? If nodes is falsey, the check for validity will be skipped entirely, and a select-folder action will always be returned. This is useful in tests that don't care about the validity check. https://codereview.chromium.org/2813503002/diff/20001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/actions.js:107: }; On 2017/04/11 04:13:37, calamity wrote: > Make no-op a private action and early return in reduceAction. I've replace 'no-op' with returning null, which does exactly that.
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/2813503002/diff/160001/chrome/browser/resourc... File chrome/browser/resources/md_bookmarks/actions.js (right): https://codereview.chromium.org/2813503002/diff/160001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/actions.js:104: if (nodes && (id == ROOT_NODE_ID || !nodes[id] || nodes[id].url)) Hmm. Do you think it's reasonable to leave a console.warn() here? It would make it easier to catch when things are going wildly wrong. https://codereview.chromium.org/2813503002/diff/160001/chrome/browser/resourc... File chrome/browser/resources/md_bookmarks/router.js (right): https://codereview.chromium.org/2813503002/diff/160001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/router.js:67: }.bind(this)); So... If I understand correctly, the deferred action here prevents this.getState() from being called before the store is initialized by pushing the evaluation as a function into queuedActions? Neat. https://codereview.chromium.org/2813503002/diff/160001/chrome/browser/resourc... File chrome/browser/resources/md_bookmarks/store.js (right): https://codereview.chromium.org/2813503002/diff/160001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/store.js:18: /** @type {!Array<?Action|DeferredAction>} */ This is an abomination of a type. https://codereview.chromium.org/2813503002/diff/160001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/store.js:73: return; When does this happen? https://codereview.chromium.org/2813503002/diff/160001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/store.js:87: if (typeof action == 'function') { So one option is to make everything internally a DeferredAction, and wrap all normal Actions in a simple function that immediately calls this.reduce_(action). You can also add a separate handleAction and handleDeferredAction that makes things more explicit. Are there good reasons to make the Action vs DeferredAction (sort of) isomorphism implicit like this? I'm not sure what the general policy is on multi-type params. https://codereview.chromium.org/2813503002/diff/160001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/store.js:104: if (this.isInitialized()) // Batch notification until after all initialization queuedActions are resolved. https://codereview.chromium.org/2813503002/diff/160001/chrome/test/data/webui... File chrome/test/data/webui/md_bookmarks/router_test.js (left): https://codereview.chromium.org/2813503002/diff/160001/chrome/test/data/webui... chrome/test/data/webui/md_bookmarks/router_test.js:79: console.log(window.location.href); rebase?
https://codereview.chromium.org/2813503002/diff/160001/chrome/browser/resourc... File chrome/browser/resources/md_bookmarks/actions.js (right): https://codereview.chromium.org/2813503002/diff/160001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/actions.js:104: if (nodes && (id == ROOT_NODE_ID || !nodes[id] || nodes[id].url)) On 2017/05/02 08:27:15, calamity wrote: > Hmm. Do you think it's reasonable to leave a console.warn() here? It would make > it easier to catch when things are going wildly wrong. I've added the warning. There are some legitimate cases where this warning could fire, though, so we might want to remove it later if it gets annoying. https://codereview.chromium.org/2813503002/diff/160001/chrome/browser/resourc... File chrome/browser/resources/md_bookmarks/router.js (right): https://codereview.chromium.org/2813503002/diff/160001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/router.js:67: }.bind(this)); On 2017/05/02 08:27:15, calamity wrote: > So... If I understand correctly, the deferred action here prevents > this.getState() from being called before the store is initialized by pushing the > evaluation as a function into queuedActions? > > Neat. Yup, that's the idea. https://codereview.chromium.org/2813503002/diff/160001/chrome/browser/resourc... File chrome/browser/resources/md_bookmarks/store.js (right): https://codereview.chromium.org/2813503002/diff/160001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/store.js:18: /** @type {!Array<?Action|DeferredAction>} */ On 2017/05/02 08:27:15, calamity wrote: > This is an abomination of a type. Acknowledged. https://codereview.chromium.org/2813503002/diff/160001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/store.js:73: return; On 2017/05/02 08:27:15, calamity wrote: > When does this happen? It's the null check that replaces the no-op action. Looks like it was duplicated unnecessarily, though: the same check is below at line 100. I've removed this one. https://codereview.chromium.org/2813503002/diff/160001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/store.js:87: if (typeof action == 'function') { On 2017/05/02 08:27:15, calamity wrote: > So one option is to make everything internally a DeferredAction, and wrap all > normal Actions in a simple function that immediately calls this.reduce_(action). > You can also add a separate handleAction and handleDeferredAction that makes > things more explicit. > Are there good reasons to make the Action vs DeferredAction (sort of) > isomorphism implicit like this? I'm not sure what the general policy is on > multi-type params. Hmmm. There is some precedent for passing in parameters as "a thing, or a function which creates a thing". Eg: https://cs.chromium.org/chromium/src/ui/webui/resources/js/cr/ui.js?q=typeof.... I think that Closure also has a lot of trouble with "?Action|DeferredAction", so separating them out probably makes sense in that way too. I've made the changes you suggest. It's a bit weird that the common case bounces from handling an Action to a DeferredAction then back to Action, but other than that I think it's an improvement. https://codereview.chromium.org/2813503002/diff/160001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/store.js:104: if (this.isInitialized()) On 2017/05/02 08:27:15, calamity wrote: > // Batch notification until after all initialization queuedActions are resolved. Done. https://codereview.chromium.org/2813503002/diff/160001/chrome/test/data/webui... File chrome/test/data/webui/md_bookmarks/router_test.js (left): https://codereview.chromium.org/2813503002/diff/160001/chrome/test/data/webui... chrome/test/data/webui/md_bookmarks/router_test.js:79: console.log(window.location.href); On 2017/05/02 08:27:15, calamity wrote: > rebase? I'm not sure exactly what you mean, but this was landed (https://cs.chromium.org/chromium/src/chrome/test/data/webui/md_bookmarks/rout...) and is being removed in this CL.
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.
lgtm https://codereview.chromium.org/2813503002/diff/160001/chrome/browser/resourc... File chrome/browser/resources/md_bookmarks/actions.js (right): https://codereview.chromium.org/2813503002/diff/160001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/actions.js:104: if (nodes && (id == ROOT_NODE_ID || !nodes[id] || nodes[id].url)) On 2017/05/03 02:57:31, tsergeant wrote: > On 2017/05/02 08:27:15, calamity wrote: > > Hmm. Do you think it's reasonable to leave a console.warn() here? It would > make > > it easier to catch when things are going wildly wrong. > > I've added the warning. There are some legitimate cases where this warning could > fire, though, so we might want to remove it later if it gets annoying. Could you provide some examples? https://codereview.chromium.org/2813503002/diff/180001/chrome/browser/resourc... File chrome/browser/resources/md_bookmarks/store.js (right): https://codereview.chromium.org/2813503002/diff/180001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/store.js:63: * form `handleDeferredAction(function(dispatch) { ... })`). Inside that Should this really be called dispatch? It's a bit confusing because this directly calls reduce, and dispatch is an overloaded term due to StoreClient. https://codereview.chromium.org/2813503002/diff/180001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/store.js:65: * dispatch Actions directly to the Store. So clients can totally mess this up and send bogus actions if the state changes out from underneath them, right? I guess we need to be careful about what can happen before |dispatch| gets called. https://codereview.chromium.org/2813503002/diff/180001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/store.js:93: action(this.reduce_.bind(this)); nit: May as well inline this now.
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/2813503002/diff/160001/chrome/browser/resourc... File chrome/browser/resources/md_bookmarks/actions.js (right): https://codereview.chromium.org/2813503002/diff/160001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/actions.js:104: if (nodes && (id == ROOT_NODE_ID || !nodes[id] || nodes[id].url)) On 2017/05/03 07:49:46, calamity wrote: > On 2017/05/03 02:57:31, tsergeant wrote: > > On 2017/05/02 08:27:15, calamity wrote: > > > Hmm. Do you think it's reasonable to leave a console.warn() here? It would > > make > > > it easier to catch when things are going wildly wrong. > > > > I've added the warning. There are some legitimate cases where this warning > could > > fire, though, so we might want to remove it later if it gets annoying. > > Could you provide some examples? The main one is if you delete a folder that you had previously visited and then use the browser Back button to go back to chrome://bookmarks/?id=<deleted id> https://codereview.chromium.org/2813503002/diff/180001/chrome/browser/resourc... File chrome/browser/resources/md_bookmarks/store.js (right): https://codereview.chromium.org/2813503002/diff/180001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/store.js:63: * form `handleDeferredAction(function(dispatch) { ... })`). Inside that On 2017/05/03 07:49:46, calamity wrote: > Should this really be called dispatch? It's a bit confusing because this > directly calls reduce, and dispatch is an overloaded term due to StoreClient. If anything, I think it's confusing that the function names are different between Store and StoreClient. I would prefer to rename handleAction and handleDeferredAction to match the names from StoreClient. This way, 'dispatch' always refers to 'sending an action to the store', it's just that there's a few different contexts where that can happen. No matter where you are, if you call a function called 'dispatch', you're going to end up triggering 'reduce' and 'notifyObservers'. I've uploaded a new patchset with this change. WDYT? https://codereview.chromium.org/2813503002/diff/180001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/store.js:65: * dispatch Actions directly to the Store. On 2017/05/03 07:49:46, calamity wrote: > So clients can totally mess this up and send bogus actions if the state changes > out from underneath them, right? > > I guess we need to be careful about what can happen before |dispatch| gets > called. Yeah. This isn't a problem for any actions at the moment, but it could come up in the future. Redux gets around this by passing two functions to the DeferredAction: return function(dispatch, getState) { } where getState is a function which returns the current state. We could probably make that happen fairly easily if we need it. https://codereview.chromium.org/2813503002/diff/180001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/store.js:93: action(this.reduce_.bind(this)); On 2017/05/03 07:49:46, calamity wrote: > nit: May as well inline this now. It's still called from multiple places, so I'd prefer to leave it as a separate function.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2813503002/diff/180001/chrome/browser/resourc... File chrome/browser/resources/md_bookmarks/store.js (right): https://codereview.chromium.org/2813503002/diff/180001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/store.js:63: * form `handleDeferredAction(function(dispatch) { ... })`). Inside that On 2017/05/04 01:08:49, tsergeant wrote: > On 2017/05/03 07:49:46, calamity wrote: > > Should this really be called dispatch? It's a bit confusing because this > > directly calls reduce, and dispatch is an overloaded term due to StoreClient. > > If anything, I think it's confusing that the function names are different > between Store and StoreClient. I would prefer to rename handleAction and > handleDeferredAction to match the names from StoreClient. > > This way, 'dispatch' always refers to 'sending an action to the store', it's > just that there's a few different contexts where that can happen. No matter > where you are, if you call a function called 'dispatch', you're going to end up > triggering 'reduce' and 'notifyObservers'. > > I've uploaded a new patchset with this change. WDYT? Hmm. I guess now that all DeferredActions go through dispatchAsync, this feels better. It was worse when one version of dispatch took an Action|DeferredAction but the callback version only took an Action. Are there any possible future use cases where queuedActions gets used after initialization? Like, pausing the action queue? The callback dispatch will skip that. Is it unreasonable to expect clients to use StoreClient's dispatch rather than passing it? I don't actually mind here right now. This seems fine, but it just seems uncomfortable to have two callback-y things that do very subtly different things. https://codereview.chromium.org/2813503002/diff/180001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/store.js:65: * dispatch Actions directly to the Store. On 2017/05/04 01:08:50, tsergeant wrote: > On 2017/05/03 07:49:46, calamity wrote: > > So clients can totally mess this up and send bogus actions if the state > changes > > out from underneath them, right? > > > > I guess we need to be careful about what can happen before |dispatch| gets > > called. > > Yeah. This isn't a problem for any actions at the moment, but it could come up > in the future. > > Redux gets around this by passing two functions to the DeferredAction: > > return function(dispatch, getState) { > > } > > where getState is a function which returns the current state. > > We could probably make that happen fairly easily if we need it. Acknowledged.
https://codereview.chromium.org/2813503002/diff/180001/chrome/browser/resourc... File chrome/browser/resources/md_bookmarks/store.js (right): https://codereview.chromium.org/2813503002/diff/180001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/store.js:63: * form `handleDeferredAction(function(dispatch) { ... })`). Inside that On 2017/05/04 07:14:26, calamity wrote: > On 2017/05/04 01:08:49, tsergeant wrote: > > On 2017/05/03 07:49:46, calamity wrote: > > > Should this really be called dispatch? It's a bit confusing because this > > > directly calls reduce, and dispatch is an overloaded term due to > StoreClient. > > > > If anything, I think it's confusing that the function names are different > > between Store and StoreClient. I would prefer to rename handleAction and > > handleDeferredAction to match the names from StoreClient. > > > > This way, 'dispatch' always refers to 'sending an action to the store', it's > > just that there's a few different contexts where that can happen. No matter > > where you are, if you call a function called 'dispatch', you're going to end > up > > triggering 'reduce' and 'notifyObservers'. > > > > I've uploaded a new patchset with this change. WDYT? > > Hmm. I guess now that all DeferredActions go through dispatchAsync, this feels > better. It was worse when one version of dispatch took an Action|DeferredAction > but the callback version only took an Action. > > Are there any possible future use cases where queuedActions gets used after > initialization? Like, pausing the action queue? The callback dispatch will skip > that. Is it unreasonable to expect clients to use StoreClient's dispatch rather > than passing it? > I don't actually mind here right now. This seems fine, but it just seems > uncomfortable to have two callback-y things that do very subtly different > things. That's a fair point. Injecting the dispatch method is necessary to make Closure happy when writing deferred actions in actions.js. I'll leave it as it is for now, but it's something to keep in mind if we add any more functionality to the Store.
The CQ bit was checked by tsergeant@chromium.org
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": 1493882743723870,
"parent_rev": "3833c3878b778637ca7769f91e8a191a36379850", "commit_rev":
"c17b37ab36a216c3923da51e382d531efc61c35e"}
Message was sent while issue was closed.
Description was changed from ========== MD Bookmarks: Prevent navigating to invalid folders This CL prevents items or non-existent IDs from being navigated to. In order to achieve this, we add support for 'deferred actions', which allow actions to be created asynchronously. Deferred actions are currently only used for selecting folders from the Router, but will be used to simplify search in a follow-up CL. BUG=697706 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Bookmarks: Prevent navigating to invalid folders This CL prevents items or non-existent IDs from being navigated to. In order to achieve this, we add support for 'deferred actions', which allow actions to be created asynchronously. Deferred actions are currently only used for selecting folders from the Router, but will be used to simplify search in a follow-up CL. BUG=697706 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2813503002 Cr-Commit-Position: refs/heads/master@{#469290} Committed: https://chromium.googlesource.com/chromium/src/+/c17b37ab36a216c3923da51e382d... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:200001) as https://chromium.googlesource.com/chromium/src/+/c17b37ab36a216c3923da51e382d... |
