Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
21163003:
DevTools: Implement undo, redo operations for the DOMStorage views. (Closed)
|
Created:
7 years, 4 months ago by vivekg_samsung Modified:
7 years, 4 months ago CC:
blink-reviews, caseq+blink_chromium.org, loislo+blink_chromium.org, eustas+blink_chromium.org, alph+blink_chromium.org, eae+blinkwatch, yurys+blink_chromium.org, lushnikov+blink_chromium.org, vsevik+blink_chromium.org, pfeldman+blink_chromium.org, dglazkov+blink, paulirish+reviews_chromium.org, apavlov+blink_chromium.org, devtools-reviews_chromium.org, aandrey+blink_chromium.org Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionDevTools: Implement undo, redo operations for the DOMStorage views.
R=apavlov, pfeldman
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=155573
Patch Set 1 #
Total comments: 14
Patch Set 2 : #
Total comments: 18
Patch Set 3 : #Patch Set 4 : #
Total comments: 12
Patch Set 5 : #
Total comments: 4
Patch Set 6 : #Patch Set 7 : #
Total comments: 11
Patch Set 8 : Patch for landing may be! :) #
Total comments: 8
Patch Set 9 : Patch for landing! #
Total comments: 8
Patch Set 10 : #
Total comments: 1
Patch Set 11 : Patch for landing after pfeldman's comment #Patch Set 12 : Patch for landing after arv's changes for ExceptionState #Messages
Total messages: 31 (0 generated)
I was not able to update the previous same issue as the user ID is different. Hence creating this new issue. Previous issue link: https://codereview.chromium.org/14877010/
https://codereview.chromium.org/21163003/diff/1/Source/core/inspector/Inspect... File Source/core/inspector/InspectorDOMStorageAgent.cpp (right): https://codereview.chromium.org/21163003/diff/1/Source/core/inspector/Inspect... Source/core/inspector/InspectorDOMStorageAgent.cpp:122: ec = 0; This is not required, since hadException() would have returned true above. https://codereview.chromium.org/21163003/diff/1/Source/devtools/front_end/DOM... File Source/devtools/front_end/DOMStorage.js (right): https://codereview.chromium.org/21163003/diff/1/Source/devtools/front_end/DOM... Source/devtools/front_end/DOMStorage.js:109: * @constructor This should be @interface instead https://codereview.chromium.org/21163003/diff/1/Source/devtools/front_end/DOM... Source/devtools/front_end/DOMStorage.js:118: perform: function(callback) callback should be annotated using @param https://codereview.chromium.org/21163003/diff/1/Source/devtools/front_end/DOM... Source/devtools/front_end/DOMStorage.js:133: * @param {WebInspector.DOMStorage} domStorage also add the @implements annotation https://codereview.chromium.org/21163003/diff/1/Source/devtools/front_end/DOM... Source/devtools/front_end/DOMStorage.js:135: * @extends {WebInspector.DOMStorageAction} this should be removed https://codereview.chromium.org/21163003/diff/1/Source/devtools/front_end/DOM... Source/devtools/front_end/DOMStorage.js:144: __proto__: WebInspector.DOMStorageAction.prototype, We usually put these last in the prototype definition https://codereview.chromium.org/21163003/diff/1/Source/devtools/front_end/DOM... Source/devtools/front_end/DOMStorage.js:170: * @constructor ditto re @implements https://codereview.chromium.org/21163003/diff/1/Source/devtools/front_end/DOM... Source/devtools/front_end/DOMStorage.js:185: perform: function(callback) JsDoc (perhaps just @inherit) https://codereview.chromium.org/21163003/diff/1/Source/devtools/front_end/DOM... Source/devtools/front_end/DOMStorage.js:214: WebInspector.DOMStorageHistory = function(domStorage) JsDoc for domStorage https://codereview.chromium.org/21163003/diff/1/Source/devtools/front_end/DOM... Source/devtools/front_end/DOMStorage.js:222: perform: function(action) JsDoc https://codereview.chromium.org/21163003/diff/1/Source/devtools/front_end/DOM... Source/devtools/front_end/DOMStorage.js:239: if (action) I believe you should console.assert(action) here, since no action at this point is an error https://codereview.chromium.org/21163003/diff/1/Source/devtools/front_end/DOM... Source/devtools/front_end/DOMStorage.js:250: if (action) I believe you should console.assert(action) here, since no action at this point is an error https://codereview.chromium.org/21163003/diff/1/Source/devtools/front_end/Res... File Source/devtools/front_end/ResourcesPanel.js (right): https://codereview.chromium.org/21163003/diff/1/Source/devtools/front_end/Res... Source/devtools/front_end/ResourcesPanel.js:139: * @param {WebInspector.Event} event This is not the web inspector event, this is a DOM Event (not sure if you can use KeyboardEvent here, but worth a try) https://codereview.chromium.org/21163003/diff/1/Source/devtools/protocol.json File Source/devtools/protocol.json (right): https://codereview.chromium.org/21163003/diff/1/Source/devtools/protocol.json... Source/devtools/protocol.json:1448: { "name": "value", "type": "string" } make this "optional": true and get rid of the "exists" flag.
@apavlov, thank you for the quick review. All the review comments have been addressed in the new patch. Please take a look.
https://codereview.chromium.org/21163003/diff/7001/LayoutTests/inspector/stor... File LayoutTests/inspector/storage-panel-dom-storage-undo-redo-expected.txt (right): https://codereview.chromium.org/21163003/diff/7001/LayoutTests/inspector/stor... LayoutTests/inspector/storage-panel-dom-storage-undo-redo-expected.txt:1: This test demonstrates the undo/redo operations performed on the storage views redo is not tested for the session storage. You should also test attempt to undo and redo actions beyond the stack bounds (like, undo with an empty stack and redo with no active undo's) https://codereview.chromium.org/21163003/diff/7001/LayoutTests/inspector/stor... LayoutTests/inspector/storage-panel-dom-storage-undo-redo-expected.txt:25: Running: undoLocalStorageModifications Should you test that each of the modifications is undone at each of the two steps? https://codereview.chromium.org/21163003/diff/7001/LayoutTests/inspector/stor... LayoutTests/inspector/storage-panel-dom-storage-undo-redo-expected.txt:28: Running: redoLocalStorageModifications Ditto for redo https://codereview.chromium.org/21163003/diff/7001/LayoutTests/inspector/stor... File LayoutTests/inspector/storage-panel-dom-storage-undo-redo.html (right): https://codereview.chromium.org/21163003/diff/7001/LayoutTests/inspector/stor... LayoutTests/inspector/storage-panel-dom-storage-undo-redo.html:19: entries.push(key + value); Are you sure you used this code to generate the expectations? I can see neither "=" nor "," as delimiters between key-value and between key-value pairs... https://codereview.chromium.org/21163003/diff/7001/LayoutTests/inspector/stor... LayoutTests/inspector/storage-panel-dom-storage-undo-redo.html:22: return "[" + entries.join() + "]"; This will just glue them together without "," in between https://codereview.chromium.org/21163003/diff/7001/LayoutTests/inspector/stor... LayoutTests/inspector/storage-panel-dom-storage-undo-redo.html:27: // Resources panel must be visible Comments are not required in tests (these are typical patterns we all know :)) https://codereview.chromium.org/21163003/diff/7001/LayoutTests/inspector/stor... LayoutTests/inspector/storage-panel-dom-storage-undo-redo.html:30: var LocalStorage; in order to follow the naming guidelines, these can be theLocalStorage and theSessionStorage https://codereview.chromium.org/21163003/diff/7001/LayoutTests/inspector/stor... LayoutTests/inspector/storage-panel-dom-storage-undo-redo.html:43: function storageEntriesReceived(entries) We have moved to the practice of putting callbacks AFTER the code that invokes them, so that the execution will follow the code layout. https://codereview.chromium.org/21163003/diff/7001/LayoutTests/inspector/stor... LayoutTests/inspector/storage-panel-dom-storage-undo-redo.html:228: <p>This test demonstrates the undo/redo operations performed on the storage views</p> tests do not demonstrate, they check that something is performed correctly (never to the full extent, though :)) https://codereview.chromium.org/21163003/diff/7001/Source/devtools/front_end/... File Source/devtools/front_end/DOMStorage.js (right): https://codereview.chromium.org/21163003/diff/7001/Source/devtools/front_end/... Source/devtools/front_end/DOMStorage.js:119: * @param {function(): void} callback we typically omit "void" in signatures. In fact, it is not mandatory for a callback to be void... https://codereview.chromium.org/21163003/diff/7001/Source/devtools/front_end/... Source/devtools/front_end/DOMStorage.js:138: * @extends {WebInspector.DOMStorageAction} @extends should immediately follow @constructor for clarity (this is a constructor of a class that extends this other class) https://codereview.chromium.org/21163003/diff/7001/Source/devtools/front_end/... Source/devtools/front_end/DOMStorage.js:149: * @param {function(): void} callback ditto re void (and more below) https://codereview.chromium.org/21163003/diff/7001/Source/devtools/front_end/... Source/devtools/front_end/DOMStorage.js:166: /* /** https://codereview.chromium.org/21163003/diff/7001/Source/devtools/front_end/... Source/devtools/front_end/DOMStorage.js:174: /* /** https://codereview.chromium.org/21163003/diff/7001/Source/devtools/front_end/... Source/devtools/front_end/DOMStorage.js:189: * @extends {WebInspector.DOMStorageAction} Please move next to @constructor https://codereview.chromium.org/21163003/diff/7001/Source/devtools/front_end/... Source/devtools/front_end/DOMStorage.js:223: /* /** https://codereview.chromium.org/21163003/diff/7001/Source/devtools/front_end/... Source/devtools/front_end/DOMStorage.js:234: /* /** https://codereview.chromium.org/21163003/diff/7001/Source/devtools/front_end/... Source/devtools/front_end/DOMStorage.js:257: /* /**
Thanks, addressed all the comments in patch set 4. Had missed moving one callback function below the call from patch 3. Please take a look.
Good work! A few nits, mostly programming style-wise, and we are ready to land! https://codereview.chromium.org/21163003/diff/25001/LayoutTests/inspector/sto... File LayoutTests/inspector/storage-panel-dom-storage-undo-redo-expected.txt (right): https://codereview.chromium.org/21163003/diff/25001/LayoutTests/inspector/sto... LayoutTests/inspector/storage-panel-dom-storage-undo-redo-expected.txt:25: Running: undoLocalStorageBeyondBounds This looks strange. At this point, you should still have a single undo entry for the a1=b1 addition in the stack... You are undoing 2 actions, right? Perhaps worth adding a message into the output? WDYT? https://codereview.chromium.org/21163003/diff/25001/LayoutTests/inspector/sto... LayoutTests/inspector/storage-panel-dom-storage-undo-redo-expected.txt:32: LocalStorage contents:[a1=b1,a2=b2] Same thing - 2 actions redone? https://codereview.chromium.org/21163003/diff/25001/LayoutTests/inspector/sto... LayoutTests/inspector/storage-panel-dom-storage-undo-redo-expected.txt:49: Running: modifySessionStorageEntriesKey Adding a test for undo of the key modifications would be nice. https://codereview.chromium.org/21163003/diff/25001/LayoutTests/inspector/sto... File LayoutTests/inspector/storage-panel-dom-storage-undo-redo.html (right): https://codereview.chromium.org/21163003/diff/25001/LayoutTests/inspector/sto... LayoutTests/inspector/storage-panel-dom-storage-undo-redo.html:63: InspectorTest.runAfterPendingDispatches(dumpDOMStorage.bind(this, next)); I know we generally avoid relying on InspectorTest.runAfterPendingDispatches() and add a sniffer (InspectorTest.addSniffer) for callbacks in the code instead, but this is not so important. https://codereview.chromium.org/21163003/diff/25001/LayoutTests/inspector/sto... LayoutTests/inspector/storage-panel-dom-storage-undo-redo.html:76: var creationNode = dataGrid.rootNode().children[dataGrid.rootNode().children.length - 1]; dataGrid.rootNode().children.peekLast() (defined in utilities.js) https://codereview.chromium.org/21163003/diff/25001/Source/devtools/front_end... File Source/devtools/front_end/DOMStorage.js (right): https://codereview.chromium.org/21163003/diff/25001/Source/devtools/front_end... Source/devtools/front_end/DOMStorage.js:138: * @param {!string} key ! is not required for non-object types (and is the default for them (see the Closure compiler JsDoc reference I sent in the previous review comments), so we omit it for strings, numbers, etc.) https://codereview.chromium.org/21163003/diff/25001/Source/devtools/front_end... Source/devtools/front_end/DOMStorage.js:149: * @param {function()} callback With @override, you can omit other annotations altogether: "If no other annotations are included, the method or property automatically inherits annotations from its superclass." https://codereview.chromium.org/21163003/diff/25001/Source/devtools/front_end... Source/devtools/front_end/DOMStorage.js:190: * @param {!string} key ditto for ! https://codereview.chromium.org/21163003/diff/25001/Source/devtools/front_end... Source/devtools/front_end/DOMStorage.js:203: * @param {function(): void} callback ditto for JsDoc https://codereview.chromium.org/21163003/diff/25001/Source/devtools/front_end... Source/devtools/front_end/DOMStorage.js:215: if (typeof value === 'undefined' || value === null) { double-quotes, please. Also, you can never get "null" here, since the protocol does not allow transmitting null in place of strings - remove this check. What you must do is check for the |error| (see protocol method callbacks elsewhere, e.g. in CSSStyleModel.js), something along the lines of: if (error) { callback(...something...); return; } Perhaps, the callback should accept a boolean indicating whether the operation has finished successfully (if that makes sense for your code to distinguish these situations). https://codereview.chromium.org/21163003/diff/25001/Source/devtools/front_end... Source/devtools/front_end/DOMStorage.js:216: this._exists = false; this can be "delete this._exists" https://codereview.chromium.org/21163003/diff/25001/Source/devtools/protocol.... File Source/devtools/protocol.json (right): https://codereview.chromium.org/21163003/diff/25001/Source/devtools/protocol.... Source/devtools/protocol.json:1441: "name": "getDOMStorageItem", It just occurred to me that the name is not quite correct. It should be getDOMStorageItemValue or something, since getDOMStorageItems below returns Array.<Item>, so, logically, getDOMStorageItem should return Item, while it returns a string.
Thank you, done all the changes. Please take a look!
not lgtm https://codereview.chromium.org/21163003/diff/33001/Source/devtools/front_end... File Source/devtools/front_end/DOMStorage.js (right): https://codereview.chromium.org/21163003/diff/33001/Source/devtools/front_end... Source/devtools/front_end/DOMStorage.js:216: if (typeof value === "undefined") { remove "{" for a single line https://codereview.chromium.org/21163003/diff/33001/Source/devtools/front_end... Source/devtools/front_end/DOMStorage.js:256: this._actions = []; plz add /** @type {!Array.<!WebInspector.DOMStorageAction>} */ https://codereview.chromium.org/21163003/diff/33001/Source/devtools/front_end... Source/devtools/front_end/DOMStorage.js:257: this._currentActionIndex = -1; consider adding a limit to the stack length to prevent unbound growth https://codereview.chromium.org/21163003/diff/33001/Source/devtools/front_end... Source/devtools/front_end/DOMStorage.js:272: this._actions.push(action); I think this will not work. Consider the following: perform A perform B undo // <-- will correctly undo B perform C undo // <-- FAIL: will try to undo B again Please write a test with the scenario above.
On 2013/08/02 11:21:33, aandrey wrote: > not lgtm > Thank you for the review. All the comments are done. Have some doubts below: https://codereview.chromium.org/21163003/diff/33001/Source/devtools/front_end... > Source/devtools/front_end/DOMStorage.js:257: this._currentActionIndex = -1; > consider adding a limit to the stack length to prevent unbound growth > The below snippet can be used to inform about the maximum undo/redo stack depth. * Is this the right way to inform the user? * This would restrict the developers from adding more entries than the depth using the DOM storage view. Is this ok? if (this._currentActionIndex + 1 == WebInspector.DOMStorageHistory.MAX_UNDO_REDO_DEPTH) { console.warn("Reached maximum Undo/Redo depth."); return; } > https://codereview.chromium.org/21163003/diff/33001/Source/devtools/front_end... > Source/devtools/front_end/DOMStorage.js:272: this._actions.push(action); > I think this will not work. Consider the following: > > perform A > perform B > undo // <-- will correctly undo B > perform C > undo // <-- FAIL: will try to undo B again > > Please write a test with the scenario above. The below snippet should fix this issue. Thanks for pointing it out. I will add the test for the same. function actionCompleted() { if (this._currentActionIndex + 1 < this._actions.length) this._actions.splice(this._currentActionIndex + 1); this._actions.push(action); ++this._currentActionIndex; }
Added new patch. Please take a look, thanks.
On 2013/08/02 13:28:46, vivekg wrote: > Added new patch. Please take a look, thanks. ping?
https://codereview.chromium.org/21163003/diff/56001/LayoutTests/inspector/sto... File LayoutTests/inspector/storage-panel-dom-storage-undo-redo-expected.txt (right): https://codereview.chromium.org/21163003/diff/56001/LayoutTests/inspector/sto... LayoutTests/inspector/storage-panel-dom-storage-undo-redo-expected.txt:70: LocalStorage contents:[a10=b10,a1=b1,a2=b2,a3=b3,a4=b4,a5=b5,a6=b6,a7=b7,a8=b8,a9=b9] How can we validate that this is the correct content of the storage after all the undo's? You set a1=b1 and a2=b2, while these key-value pairs are already present in the storage. Should we perhaps perform 40 actions to set xN=yN, so that they will be distinguishable from the pre-existing key-value pairs? https://codereview.chromium.org/21163003/diff/56001/LayoutTests/inspector/sto... File LayoutTests/inspector/storage-panel-dom-storage-undo-redo.html (right): https://codereview.chromium.org/21163003/diff/56001/LayoutTests/inspector/sto... LayoutTests/inspector/storage-panel-dom-storage-undo-redo.html:158: InspectorTest.runAfterPendingDispatches(undo.bind(theLocalStorage, 2, firstUndoDone.bind(next))); Semantically, ...bind(next) is not the right way of binding an argument (callback). This should be ...bind(null, next), and the signature should be function firstUndoDone(callback) { InspectorTest.runAfterPendingDispatches(dumpDOMStorage.bind(theLocalStorage, nextUndoDone.bind(null, callback))); } and so on. https://codereview.chromium.org/21163003/diff/56001/Source/devtools/front_end... File Source/devtools/front_end/DOMStorage.js (right): https://codereview.chromium.org/21163003/diff/56001/Source/devtools/front_end... Source/devtools/front_end/DOMStorage.js:262: WebInspector.DOMStorageHistory.MAX_UNDO_REDO_DEPTH = 20; MAX_UNDO_STACK_DEPTH (since the thing is called "undo stack" :)) https://codereview.chromium.org/21163003/diff/56001/Source/devtools/front_end... Source/devtools/front_end/DOMStorage.js:278: this._currentActionIndex--; We always use the prefix form: ++this._currentActionIndex. Also, should we perhaps rename it to "_redoableActionIndex"? Since "current" does not reflect the actual nature of the action index...
https://codereview.chromium.org/21163003/diff/56001/Source/devtools/front_end... File Source/devtools/front_end/DOMStorage.js (right): https://codereview.chromium.org/21163003/diff/56001/Source/devtools/front_end... Source/devtools/front_end/DOMStorage.js:278: this._currentActionIndex--; On 2013/08/05 09:07:15, apavlov wrote: > We always use the prefix form: ++this._currentActionIndex. > > Also, should we perhaps rename it to "_redoableActionIndex"? Since "current" > does not reflect the actual nature of the action index... Sorry, looked at the wrong place. Of course, it should be --this._currentActionIndex; https://codereview.chromium.org/21163003/diff/56001/Source/devtools/front_end... Source/devtools/front_end/DOMStorage.js:281: if (this._currentActionIndex + 1 < this._actions.length) this can become an "else if", I guess...
Thank you the review. Uploading a new patch with review comments incorporated. https://codereview.chromium.org/21163003/diff/56001/LayoutTests/inspector/sto... File LayoutTests/inspector/storage-panel-dom-storage-undo-redo-expected.txt (right): https://codereview.chromium.org/21163003/diff/56001/LayoutTests/inspector/sto... LayoutTests/inspector/storage-panel-dom-storage-undo-redo-expected.txt:70: LocalStorage contents:[a10=b10,a1=b1,a2=b2,a3=b3,a4=b4,a5=b5,a6=b6,a7=b7,a8=b8,a9=b9] On 2013/08/05 09:07:15, apavlov wrote: > How can we validate that this is the correct content of the storage after all > the undo's? You set a1=b1 and a2=b2, while these key-value pairs are already > present in the storage. Should we perhaps perform 40 actions to set xN=yN, so > that they will be distinguishable from the pre-existing key-value pairs? Done. https://codereview.chromium.org/21163003/diff/56001/LayoutTests/inspector/sto... File LayoutTests/inspector/storage-panel-dom-storage-undo-redo.html (right): https://codereview.chromium.org/21163003/diff/56001/LayoutTests/inspector/sto... LayoutTests/inspector/storage-panel-dom-storage-undo-redo.html:158: InspectorTest.runAfterPendingDispatches(undo.bind(theLocalStorage, 2, firstUndoDone.bind(next))); On 2013/08/05 09:07:15, apavlov wrote: > Semantically, ...bind(next) is not the right way of binding an argument > (callback). This should be ...bind(null, next), and the signature should be > > function firstUndoDone(callback) > { > InspectorTest.runAfterPendingDispatches(dumpDOMStorage.bind(theLocalStorage, > nextUndoDone.bind(null, callback))); > } > > and so on. Done. https://codereview.chromium.org/21163003/diff/56001/Source/devtools/front_end... File Source/devtools/front_end/DOMStorage.js (right): https://codereview.chromium.org/21163003/diff/56001/Source/devtools/front_end... Source/devtools/front_end/DOMStorage.js:262: WebInspector.DOMStorageHistory.MAX_UNDO_REDO_DEPTH = 20; On 2013/08/05 09:07:15, apavlov wrote: > MAX_UNDO_STACK_DEPTH (since the thing is called "undo stack" :)) Done. https://codereview.chromium.org/21163003/diff/56001/Source/devtools/front_end... Source/devtools/front_end/DOMStorage.js:278: this._currentActionIndex--; On 2013/08/05 09:07:15, apavlov wrote: > We always use the prefix form: ++this._currentActionIndex. > > Also, should we perhaps rename it to "_redoableActionIndex"? Since "current" > does not reflect the actual nature of the action index... Done. https://codereview.chromium.org/21163003/diff/56001/Source/devtools/front_end... Source/devtools/front_end/DOMStorage.js:281: if (this._currentActionIndex + 1 < this._actions.length) On 2013/08/05 09:11:04, apavlov wrote: > this can become an "else if", I guess... Done.
Let's brush up the error handling and naming, and the patch is good to land! https://codereview.chromium.org/21163003/diff/70001/Source/devtools/front_end... File Source/devtools/front_end/DOMStorage.js (right): https://codereview.chromium.org/21163003/diff/70001/Source/devtools/front_end... Source/devtools/front_end/DOMStorage.js:160: this._value = value; Ugh... hm... this doesn't handle the error, it seems. https://codereview.chromium.org/21163003/diff/70001/Source/devtools/front_end... Source/devtools/front_end/DOMStorage.js:214: return; So, you are leaving the callback to hang around forever in the event of error? I seem to have given a sample of code to handle errors. It included the callback invocation... https://codereview.chromium.org/21163003/diff/70001/Source/devtools/front_end... Source/devtools/front_end/DOMStorage.js:289: if (this._redoableActionIndex < 0) ugh... I got confused by the naming, sorry. This seems to actually be the _undoable_ action index, right? https://codereview.chromium.org/21163003/diff/70001/Source/devtools/front_end... Source/devtools/front_end/DOMStorage.js:303: var action = this._actions[++this._redoableActionIndex]; This confirms the previous comment. In order to redo an action, you increment the index first.
https://codereview.chromium.org/21163003/diff/70001/Source/devtools/front_end... File Source/devtools/front_end/DOMStorage.js (right): https://codereview.chromium.org/21163003/diff/70001/Source/devtools/front_end... Source/devtools/front_end/DOMStorage.js:160: this._value = value; On 2013/08/05 09:53:00, apavlov wrote: > Ugh... hm... this doesn't handle the error, it seems. Oops had missed this one. Sorry! Corrected now! https://codereview.chromium.org/21163003/diff/70001/Source/devtools/front_end... Source/devtools/front_end/DOMStorage.js:214: return; On 2013/08/05 09:53:00, apavlov wrote: > So, you are leaving the callback to hang around forever in the event of error? I > seem to have given a sample of code to handle errors. It included the callback > invocation... The history only processes the action only upon completion. No special error handling is done by the history object. Hence in case of error, simply returning! https://codereview.chromium.org/21163003/diff/70001/Source/devtools/front_end... Source/devtools/front_end/DOMStorage.js:289: if (this._redoableActionIndex < 0) On 2013/08/05 09:53:00, apavlov wrote: > ugh... I got confused by the naming, sorry. This seems to actually be the > _undoable_ action index, right? Done. https://codereview.chromium.org/21163003/diff/70001/Source/devtools/front_end... Source/devtools/front_end/DOMStorage.js:303: var action = this._actions[++this._redoableActionIndex]; On 2013/08/05 09:53:00, apavlov wrote: > This confirms the previous comment. In order to redo an action, you increment > the index first. Done.
https://codereview.chromium.org/21163003/diff/81001/Source/devtools/front_end... File Source/devtools/front_end/DOMStorage.js (right): https://codereview.chromium.org/21163003/diff/81001/Source/devtools/front_end... Source/devtools/front_end/DOMStorage.js:265: WebInspector.DOMStorageHistory.MAX_UNDO_STACK_DEPTH = 20; Let's bump it up. The limit is to prevent unbound growth (just in case), not to limit the functionality. 256 seems reasonable. https://codereview.chromium.org/21163003/diff/81001/Source/devtools/front_end... Source/devtools/front_end/DOMStorage.js:276: action.perform(actionCompleted.bind(this)); nit: would you plz move this line after the actionCompleted func declaration? https://codereview.chromium.org/21163003/diff/81001/Source/devtools/front_end... Source/devtools/front_end/DOMStorage.js:283: this._actions.splice(this._undoableActionIndex + 1); I think you should clean the redo stack (the tail of the array in your case) once you perform an action. Otherwise, having the following: perform A perform B undo perform C undo we'll end up with the [A,C,B] actions "history", which is not the history in the first place, and moreover, the B may not be necessarily redo-able after the C was performed, or it would be a no-op. At least, I'm sure we can find some scenarios that will confuse users. Let's end up with [A,C] in the example above. We do so in TextEditorModel undo/redo handling: https://chromium.googlesource.com/chromium/blink/+/master/Source/devtools/fro...
https://codereview.chromium.org/21163003/diff/81001/Source/devtools/front_end... File Source/devtools/front_end/DOMStorage.js (right): https://codereview.chromium.org/21163003/diff/81001/Source/devtools/front_end... Source/devtools/front_end/DOMStorage.js:265: WebInspector.DOMStorageHistory.MAX_UNDO_STACK_DEPTH = 20; On 2013/08/05 11:29:33, aandrey wrote: > Let's bump it up. The limit is to prevent unbound growth (just in case), not to > limit the functionality. 256 seems reasonable. Sure I will bump it to 256. https://codereview.chromium.org/21163003/diff/81001/Source/devtools/front_end... Source/devtools/front_end/DOMStorage.js:276: action.perform(actionCompleted.bind(this)); On 2013/08/05 11:29:33, aandrey wrote: > nit: would you plz move this line after the actionCompleted func declaration? This was done in response to comment from apavlov: "We have moved to the practice of putting callbacks AFTER the code that invokes them, so that the execution will follow the code layout." Please let me know which one should I follow. https://codereview.chromium.org/21163003/diff/81001/Source/devtools/front_end... Source/devtools/front_end/DOMStorage.js:283: this._actions.splice(this._undoableActionIndex + 1); On 2013/08/05 11:29:33, aandrey wrote: > I think you should clean the redo stack (the tail of the array in your case) > once you perform an action. Otherwise, having the following: > > perform A > perform B > undo > perform C > undo > > we'll end up with the [A,C,B] actions "history", which is not the history in the > first place, and moreover, the B may not be necessarily redo-able after the C > was performed, or it would be a no-op. At least, I'm sure we can find some > scenarios that will confuse users. > > Let's end up with [A,C] in the example above. > We do so in TextEditorModel undo/redo handling: > https://chromium.googlesource.com/chromium/blink/+/master/Source/devtools/fro... I am actually removing the entries with the splice call. So that we are exactly having [A, C]. Correct me if wrong!
https://codereview.chromium.org/21163003/diff/81001/Source/devtools/front_end... File Source/devtools/front_end/DOMStorage.js (right): https://codereview.chromium.org/21163003/diff/81001/Source/devtools/front_end... Source/devtools/front_end/DOMStorage.js:265: WebInspector.DOMStorageHistory.MAX_UNDO_STACK_DEPTH = 20; On 2013/08/05 11:29:33, aandrey wrote: > Let's bump it up. The limit is to prevent unbound growth (just in case), not to > limit the functionality. 256 seems reasonable. Agree. These objects are lightweight enough to allow big enough depths. Just override it in your test up front. https://codereview.chromium.org/21163003/diff/81001/Source/devtools/front_end... Source/devtools/front_end/DOMStorage.js:276: action.perform(actionCompleted.bind(this)); On 2013/08/05 11:29:33, aandrey wrote: > nit: would you plz move this line after the actionCompleted func declaration? Callbacks following their usages is the modern way of things, according to Pavel. Usage before - callback after. https://codereview.chromium.org/21163003/diff/81001/Source/devtools/front_end... Source/devtools/front_end/DOMStorage.js:283: this._actions.splice(this._undoableActionIndex + 1); On 2013/08/05 11:29:33, aandrey wrote: > I think you should clean the redo stack (the tail of the array in your case) > once you perform an action. Otherwise, having the following: > > perform A > perform B > undo > perform C > undo > > we'll end up with the [A,C,B] actions "history", which is not the history in the > first place, and moreover, the B may not be necessarily redo-able after the C > was performed, or it would be a no-op. At least, I'm sure we can find some > scenarios that will confuse users. > > Let's end up with [A,C] in the example above. Looks like this is what is actually happening, since this._actions.splice(this._undoableActionIndex + 1); (lacking the second argument, which is allowed as a SpiderMonkey extension, hmm... what about Blink?) will truncate the tail of the array. > We do so in TextEditorModel undo/redo handling: > https://chromium.googlesource.com/chromium/blink/+/master/Source/devtools/fro...
https://codereview.chromium.org/21163003/diff/81001/Source/devtools/front_end... File Source/devtools/front_end/DOMStorage.js (right): https://codereview.chromium.org/21163003/diff/81001/Source/devtools/front_end... Source/devtools/front_end/DOMStorage.js:276: action.perform(actionCompleted.bind(this)); On 2013/08/05 11:42:59, apavlov wrote: > On 2013/08/05 11:29:33, aandrey wrote: > > nit: would you plz move this line after the actionCompleted func declaration? > > Callbacks following their usages is the modern way of things, according to > Pavel. Usage before - callback after. ah, ok https://codereview.chromium.org/21163003/diff/81001/Source/devtools/front_end... Source/devtools/front_end/DOMStorage.js:283: this._actions.splice(this._undoableActionIndex + 1); On 2013/08/05 11:42:59, vivekg_ wrote: > On 2013/08/05 11:29:33, aandrey wrote: > > I think you should clean the redo stack (the tail of the array in your case) > > once you perform an action. Otherwise, having the following: > > > > perform A > > perform B > > undo > > perform C > > undo > > > > we'll end up with the [A,C,B] actions "history", which is not the history in > the > > first place, and moreover, the B may not be necessarily redo-able after the C > > was performed, or it would be a no-op. At least, I'm sure we can find some > > scenarios that will confuse users. > > > > Let's end up with [A,C] in the example above. > > We do so in TextEditorModel undo/redo handling: > > > https://chromium.googlesource.com/chromium/blink/+/master/Source/devtools/fro... > > I am actually removing the entries with the splice call. So that we are exactly > having [A, C]. Correct me if wrong! oh my bad! sorry.
I have bumped the number to 256. Also modified the test case to setup 20 as the limit just for the testing purposes. Thank you!
deferring to Alexander for further review
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vivek.vg@samsung.com/21163003/67002
Retried try job too often on blink_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=blink_pres...
https://codereview.chromium.org/21163003/diff/67002/Source/devtools/protocol.... File Source/devtools/protocol.json (right): https://codereview.chromium.org/21163003/diff/67002/Source/devtools/protocol.... Source/devtools/protocol.json:1441: "name": "getDOMStorageItemValue", I'd rename it to getValue. Otherwise lgtm.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vivek.vg@samsung.com/21163003/99001
On 2013/08/05 15:34:06, pfeldman wrote: > https://codereview.chromium.org/21163003/diff/67002/Source/devtools/protocol.... > File Source/devtools/protocol.json (right): > > https://codereview.chromium.org/21163003/diff/67002/Source/devtools/protocol.... > Source/devtools/protocol.json:1441: "name": "getDOMStorageItemValue", > I'd rename it to getValue. Otherwise lgtm. Done. Thank you Pavel.
Sorry for I got bad news for ya. Compile failed with a clobber build on linux_layout. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_layo... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vivek.vg@samsung.com/21163003/121001
Message was sent while issue was closed.
Change committed as 155573 |
