Bug 94671 – Implement UndoManager's item() method

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Sukolsak Sakshuwong]

Implement UndoManager's item() method

[Sukolsak Sakshuwong]

Created attachment 159871 [details]
Work in progress

[Sukolsak Sakshuwong]

Created attachment 160028 [details]
Patch

[Ryosuke Niwa]

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.

[Kentaro Hara]

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?

[Sukolsak Sakshuwong]

Created attachment 160061 [details]
Patch

[Ryosuke Niwa]

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.

[Sukolsak Sakshuwong]

Created attachment 160066 [details]
Patch

[Kentaro Hara]

What is your plan about introducing DOMTransaction C++ interface, as we discussed in IRC?

[Ryosuke Niwa]

(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.

[Kentaro Hara]

(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.

[WebKit Review Bot]

Comment on attachment 160066 [details]
Patch

Clearing flags on attachment: 160066

Committed r126392: <http://trac.webkit.org/changeset/126392>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.