Chromium clang build is failing with the following error: > In file included from > third_party/WebKit/Source/WebCore/html/HTMLFrameSetElement.h:27: > third_party/WebKit/Source/JavaScriptCore/wtf/OwnArrayPtr.h:123:5:error: > use of undeclared identifier 'deleteOwnedPtr' > deleteOwnedPtr(oldPtr); > ^ > third_party/WebKit/Source/WebCore/html/HTMLFrameSetElement.cpp:78:26: > note: in instantiation of member function > 'WTF::OwnArrayPtr<WebCore::Length>::set' requested here > m_rowLengths.set(newLengthArray(attr->value().string(), > m_totalRows)); > ^ > 1 error generated. The problem is that OwnArrayPtr::set calls deleteOwnedPtr but doesn't include OwnPtrCommon.h. We should either include the header or should call deleteOwnedArrayPtr.
Created attachment 79693 [details] Include OwnPtrCommon.h in OwnArrayPtr.h Reading the comment: // Remove this once we make all WebKit code compatible with stricter rules about OwnArrayPtr. #define LOOSE_OWN_ARRAY_PTR the correct thing to do here seems to include OwnPtrCommon.h.
Comment on attachment 79693 [details] Include OwnPtrCommon.h in OwnArrayPtr.h r=me
(In reply to comment #2) > (From update of attachment 79693 [details]) > r=me Thanks for the review!
Comment on attachment 79693 [details] Include OwnPtrCommon.h in OwnArrayPtr.h Clearing flags on attachment: 79693 Committed r76334: <http://trac.webkit.org/changeset/76334>
All reviewed patches have been landed. Closing bug.
This was the wrong fix. OwnArrayPtr<T>::set needs to use deletedOwnedArrayPtr. Or it needs to be removed.
(In reply to comment #6) > This was the wrong fix. OwnArrayPtr<T>::set needs to use deletedOwnedArrayPtr. Or it needs to be removed. Ok, let's fix then!
(In reply to comment #7) > (In reply to comment #6) > > This was the wrong fix. OwnArrayPtr<T>::set needs to use deletedOwnedArrayPtr. Or it needs to be removed. > > Ok, let's fix then! My preference would be to remove it, if we can quickly find the callers using it and change them to use adoptPtr instead.
Let me do this over the weekend (hopefully).
Created attachment 79827 [details] get rid of set
Attachment 79827 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7521251
Attachment 79827 [details] did not build on win: Build output: http://queues.webkit.org/results/7622260
Created attachment 79830 [details] fixed platform code
Attachment 79830 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7510268
Attachment 79830 [details] did not build on win: Build output: http://queues.webkit.org/results/7596258
Created attachment 79834 [details] more port specific fixes
Attachment 79834 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7561293
Attachment 79834 [details] did not build on win: Build output: http://queues.webkit.org/results/7513275
Created attachment 79839 [details] another attempt
Attachment 79839 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7556269
Attachment 79839 [details] did not build on win: Build output: http://queues.webkit.org/results/7536279
Created attachment 79854 [details] yet another attempt
Attachment 79854 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7505249
Created attachment 79861 [details] fixed v8 binding
Attachment 79861 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7626241
Created attachment 81438 [details] fixed cr-mac build
Attachment 81438 [details] did not build on qt: Build output: http://queues.webkit.org/results/7700733
(In reply to comment #27) > Attachment 81438 [details] did not build on qt: > Build output: http://queues.webkit.org/results/7700733 This is ews flakinss. kling kindly verified that it builds on qt.
Comment on attachment 81438 [details] fixed cr-mac build View in context: https://bugs.webkit.org/attachment.cgi?id=81438&action=review Exciting to see this. Looking forward to it landing. review- because of the JavaScriptCore registers issues. > Source/JavaScriptCore/runtime/Arguments.h:232 > Register* registerArray = new Register[registerArraySize]; > memcpy(registerArray, d->registers - registerOffset, registerArraySize * sizeof(Register)); > - d->registerArray.set(registerArray); > + d->registerArray = registerArray; > d->registers = registerArray + registerOffset; Needs an adoptArrayPtr. Probably won’t compile. I think this is may be why EWS bots are failing to build. > Source/JavaScriptCore/runtime/JSVariableObject.h:165 > inline void JSVariableObject::setRegisters(Register* registers, Register* registerArray) > { > ASSERT(registerArray != d->registerArray.get()); > - d->registerArray.set(registerArray); > + d->registerArray = registerArray; > d->registers = registers; > } Needs PassOwnArrayPtr. Same as above.
(In reply to comment #29) > (From update of attachment 81438 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=81438&action=review > > Exciting to see this. Looking forward to it landing. review- because of the JavaScriptCore registers issues. Thanks for the review. > > Source/JavaScriptCore/runtime/Arguments.h:232 > > Register* registerArray = new Register[registerArraySize]; > > memcpy(registerArray, d->registers - registerOffset, registerArraySize * sizeof(Register)); > > - d->registerArray.set(registerArray); > > + d->registerArray = registerArray; > > d->registers = registerArray + registerOffset; > > Needs an adoptArrayPtr. Probably won’t compile. I think this is may be why EWS bots are failing to build. Sure enough. I don't understand why XCode didn't complain about this. Will fix.
Created attachment 81445 [details] fixed per Darin's comment
Comment on attachment 81445 [details] fixed per Darin's comment View in context: https://bugs.webkit.org/attachment.cgi?id=81445&action=review > Source/JavaScriptCore/runtime/JSVariableObject.h:165 > inline void JSVariableObject::setRegisters(Register* registers, Register* registerArray) > { > ASSERT(registerArray != d->registerArray.get()); > - d->registerArray.set(registerArray); > + d->registerArray = adoptArrayPtr(registerArray); > d->registers = registers; > } This is OK for landing, but really we need the adoptArrayPtr to be close to the actual allocation. In this case, setRegisters should be taking a PassOwnArrayPtr for registerArray. And JSGlobalObject::copyGlobalsFrom should be getting the register array in an OwnArrayPtr by calling JSVariableObject::copyRegisterArray. And inside copyRegisterArray, the adoptArrayPtr should be right there where it calls new.
(In reply to comment #32) > (From update of attachment 81445 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=81445&action=review > This is OK for landing, but really we need the adoptArrayPtr to be close to the actual allocation. In this case, setRegisters should be taking a PassOwnArrayPtr for registerArray. And JSGlobalObject::copyGlobalsFrom should be getting the register array in an OwnArrayPtr by calling JSVariableObject::copyRegisterArray. And inside copyRegisterArray, the adoptArrayPtr should be right there where it calls new. I totally agree. The only reason I didn't do was because this patch was already 47KB and adding that change seemed to bloat the patch.
Committed r77785: <http://trac.webkit.org/changeset/77785>
Attachment 81445 [details] did not build on qt: Build output: http://queues.webkit.org/results/7701754
http://trac.webkit.org/changeset/77785 might have broken Chromium Win Release