Implement UndoManager's item() method
Created attachment 159871 [details] Work in progress
Created attachment 160028 [details] Patch
Comment on attachment 160028 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=160028&action=review > Source/WebCore/bindings/v8/custom/V8UndoManagerCustom.cpp:82 > + v8::Handle<v8::Object> object = v8::Object::New(); > + object->ForceSet(v8::String::NewSymbol("label"), v8::String::NewSymbol("[Editing command]")); We shouldn't be creating new object each time we return. e.g. if you do undoManager.item(0)[0].myProperty = 'foo'; then I should be able to get 'foo' on undoManager.item(0)[0].myProperty even if item(0)[0] was an object added for an automatic transaction.
Comment on attachment 160028 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=160028&action=review It might make sense to use custom binding... > Source/WebCore/bindings/v8/custom/V8UndoManagerCustom.cpp:70 > + return v8::Null(args.GetIsolate()); I guess we normally throw some exception in such a case. (e.g. JavaScript RangeError or INDEX_SIZE_ERR of DOM exception etc). We might want to update the spec if needed. > Source/WebCore/bindings/v8/custom/V8UndoManagerCustom.cpp:76 > + for (size_t i = 0; i < entry.size(); ++i) { Nit: i => index >> Source/WebCore/bindings/v8/custom/V8UndoManagerCustom.cpp:82 >> + object->ForceSet(v8::String::NewSymbol("label"), v8::String::NewSymbol("[Editing command]")); > > We shouldn't be creating new object each time we return. > e.g. if you do undoManager.item(0)[0].myProperty = 'foo'; > then I should be able to get 'foo' on > undoManager.item(0)[0].myProperty > even if item(0)[0] was an object added for an automatic transaction. We don't want to use ForceSet() unless we have a strong reason to use it. Why isn't Set() enough?
Created attachment 160061 [details] Patch
Comment on attachment 160061 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=160061&action=review > Source/WebCore/ChangeLog:7 > + You need to describe what you're doing here. > Source/WebCore/bindings/v8/custom/V8UndoManagerCustom.cpp:81 > + v8::Handle<v8::Object> object = v8::Object::New(); Please add a FIXME here.
Created attachment 160066 [details] Patch
What is your plan about introducing DOMTransaction C++ interface, as we discussed in IRC?
(In reply to comment #8) > What is your plan about introducing DOMTransaction C++ interface, as we discussed in IRC? I think we need to discusss that on public-webapps first as it requires a spec change. But it could also be that adding the C++ backing object may not dramatically simplify the binding code as we still have callback methods on that object. Really, we need to move to event model to get the most out of it.
(In reply to comment #9) > (In reply to comment #8) > > What is your plan about introducing DOMTransaction C++ interface, as we discussed in IRC? > > I think we need to discusss that on public-webapps first as it requires a spec change. But it could also be that adding the C++ backing object may not dramatically simplify the binding code as we still have callback methods on that object. OK. I don't have a strong objection to the current patch, but IMHO we might want to sophisticate the design before implementing it. I'd like to hear opinions of others. > Really, we need to move to event model to get the most out of it. This will simplify everything.
Comment on attachment 160066 [details] Patch Clearing flags on attachment: 160066 Committed r126392: <http://trac.webkit.org/changeset/126392>
All reviewed patches have been landed. Closing bug.