Bug 65778

Created attachment 103086 [details] Fixes.

Comment on attachment 103086 [details] Fixes. Attachment 103086 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9310020

Comment on attachment 103086 [details] Fixes. Attachment 103086 [details] did not pass cr-mac-ews (chromium): Output: http://queues.webkit.org/results/9323039

Created attachment 103102 [details] Release compilation fixed

Comment on attachment 103102 [details] Release compilation fixed View in context: https://bugs.webkit.org/attachment.cgi?id=103102&action=review Just a few things to clear/clean up. > Source/WebCore/ChangeLog:11 > + (WebCore::V8BindingPerIsolateData::lazyEventListenerToStringTemplate): Why are all of these items being moved? Ideally they would be moved in one change and if any changes were needed, then that would happen in another change. > Source/WebCore/bindings/v8/V8Binding.h:176 > + inline AllowAllocation() Isn't inline redundant here (since you put the function body in the class definition)? > Source/WebCore/bindings/v8/V8Binding.h:183 > + inline ~AllowAllocation() Ditto. > Source/WebCore/bindings/v8/V8Binding.h:199 > + static inline v8::Local<v8::Object> newInstance(v8::Handle<v8::Function>, int argc, v8::Handle<v8::Value> argv[]); The indentation is following Chromium style as opposed to WebKit style. > Source/WebCore/bindings/v8/V8LazyEventListener.cpp:143 > toStringTemplate = v8::Persistent<v8::FunctionTemplate>::New(v8::FunctionTemplate::New(V8LazyEventListenerToString)); I wonder why this isn't part of the method lazyEventListenerToStringTemplate. > Source/WebKit/chromium/ChangeLog:8 > + * src/BoundObject.cpp:Headers Would be nice to explain why this change was needed. (Now I see why bur it is still nice to add the info here to make it quicker for people to understand.)

Comment on attachment 103102 [details] Release compilation fixed View in context: https://bugs.webkit.org/attachment.cgi?id=103102&action=review >> Source/WebCore/ChangeLog:11 > > Why are all of these items being moved? > > Ideally they would be moved in one change and if any changes were needed, then that would happen in another change. You mean why AllowAllocation is moved between headers? This is because of the header dependencies (V8Proxy include V8Utilities include V8Bindings include V8Proxy). Do you insist on splitting class move between headers and change in class into two different patches? I see little reason for that. >> Source/WebCore/bindings/v8/V8Binding.h:176 >> + inline AllowAllocation() > > Isn't inline redundant here (since you put the function body in the class definition)? You are right - I'll remove this. >> Source/WebCore/bindings/v8/V8Binding.h:199 >> + static inline v8::Local<v8::Object> newInstance(v8::Handle<v8::Function>, int argc, v8::Handle<v8::Value> argv[]); > > The indentation is following Chromium style as opposed to WebKit style. Will fix >> Source/WebCore/bindings/v8/V8LazyEventListener.cpp:143 >> toStringTemplate = v8::Persistent<v8::FunctionTemplate>::New(v8::FunctionTemplate::New(V8LazyEventListenerToString)); > > I wonder why this isn't part of the method lazyEventListenerToStringTemplate. V8BindingPerIsolateData does not need to know anything about V8LazyEventListener, other than provide a container for its data. This logic really belongs here. >> Source/WebKit/chromium/ChangeLog:8 >> + * src/BoundObject.cpp:Headers > > Would be nice to explain why this change was needed. (Now I see why bur it is still nice to add the info here to make it quicker for people to understand.) True that) Will add comments.

Created attachment 103263 [details] CR feedback

(In reply to comment #6) > (From update of attachment 103102 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=103102&action=review > > >> Source/WebCore/ChangeLog:11 > > > > > Why are all of these items being moved? > > > > Ideally they would be moved in one change and if any changes were needed, then that would happen in another change. > > You mean why AllowAllocation is moved between headers? This is because of the header dependencies (V8Proxy include V8Utilities include V8Bindings include V8Proxy). > > Do you insist on splitting class move between headers and change in class into two different patches? I see little reason for that. I won't insist. However, doing this all at once makes it much harder to determine what is being moved vs what is being changed but at the moment and later when someone goes thru diffs to understand what was done.

Comment on attachment 103263 [details] CR feedback Clearing flags on attachment: 103263 Committed r92619: <http://trac.webkit.org/changeset/92619>

All reviewed patches have been landed. Closing bug.

(In reply to comment #8) > (In reply to comment #6) > > (From update of attachment 103102 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=103102&action=review > > > > >> Source/WebCore/ChangeLog:11 > > > > > > > > Why are all of these items being moved? > > > > > > Ideally they would be moved in one change and if any changes were needed, then that would happen in another change. > > > > You mean why AllowAllocation is moved between headers? This is because of the header dependencies (V8Proxy include V8Utilities include V8Bindings include V8Proxy). > > > > Do you insist on splitting class move between headers and change in class into two different patches? I see little reason for that. > > I won't insist. However, doing this all at once makes it much harder to determine what is being moved vs what is being changed but at the moment and later when someone goes thru diffs to understand what was done. Point taken.

Patch rolled out due to breaking chromium canary

Created attachment 103370 [details] Fixed field initialization to unbreak chromium canary.

Comment on attachment 103370 [details] Fixed field initialization to unbreak chromium canary. Clearing flags on attachment: 103370 Committed r92694: <http://trac.webkit.org/changeset/92694>

All reviewed patches have been landed. Closing bug.