Bug 94671 - Implement UndoManager's item() method
Summary: Implement UndoManager's item() method
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sukolsak Sakshuwong
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-08-21 23:22 PDT by Sukolsak Sakshuwong
Modified: 2012-08-22 22:57 PDT (History)
8 users (show)

See Also:


Attachments
Work in progress (7.60 KB, patch)
2012-08-21 23:41 PDT, Sukolsak Sakshuwong
no flags Details | Formatted Diff | Diff
Patch (12.48 KB, patch)
2012-08-22 16:06 PDT, Sukolsak Sakshuwong
no flags Details | Formatted Diff | Diff
Patch (12.49 KB, patch)
2012-08-22 18:51 PDT, Sukolsak Sakshuwong
no flags Details | Formatted Diff | Diff
Patch (12.93 KB, patch)
2012-08-22 19:18 PDT, Sukolsak Sakshuwong
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sukolsak Sakshuwong 2012-08-21 23:22:27 PDT
Implement UndoManager's item() method
Comment 1 Sukolsak Sakshuwong 2012-08-21 23:41:19 PDT
Created attachment 159871 [details]
Work in progress
Comment 2 Sukolsak Sakshuwong 2012-08-22 16:06:32 PDT
Created attachment 160028 [details]
Patch
Comment 3 Ryosuke Niwa 2012-08-22 16:19:34 PDT
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 4 Kentaro Hara 2012-08-22 16:58:20 PDT
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?
Comment 5 Sukolsak Sakshuwong 2012-08-22 18:51:25 PDT
Created attachment 160061 [details]
Patch
Comment 6 Ryosuke Niwa 2012-08-22 18:52:33 PDT
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.
Comment 7 Sukolsak Sakshuwong 2012-08-22 19:18:15 PDT
Created attachment 160066 [details]
Patch
Comment 8 Kentaro Hara 2012-08-22 21:04:54 PDT
What is your plan about introducing DOMTransaction C++ interface, as we discussed in IRC?
Comment 9 Ryosuke Niwa 2012-08-22 21:21:04 PDT
(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.
Comment 10 Kentaro Hara 2012-08-22 21:29:15 PDT
(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 11 WebKit Review Bot 2012-08-22 22:57:48 PDT
Comment on attachment 160066 [details]
Patch

Clearing flags on attachment: 160066

Committed r126392: <http://trac.webkit.org/changeset/126392>
Comment 12 WebKit Review Bot 2012-08-22 22:57:52 PDT
All reviewed patches have been landed.  Closing bug.