Get rid of PassRefPtr from Pasteboard, LegacyWebArchive and DataObjectGtk.
Created attachment 263915 [details] Patch
Attachment 263915 [details] did not pass style-queue: ERROR: Source/WebCore/loader/archive/cf/LegacyWebArchive.cpp:423: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/loader/archive/cf/LegacyWebArchive.cpp:489: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/loader/archive/cf/LegacyWebArchive.h:61: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 3 in 18 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 263916 [details] Patch
Attachment 263916 [details] did not pass style-queue: ERROR: Source/WebCore/loader/archive/cf/LegacyWebArchive.cpp:423: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/loader/archive/cf/LegacyWebArchive.cpp:489: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/loader/archive/cf/LegacyWebArchive.h:61: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 3 in 18 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 263977 [details] Patch
Attachment 263977 [details] did not pass style-queue: ERROR: Source/WebCore/loader/archive/cf/LegacyWebArchive.cpp:423: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/loader/archive/cf/LegacyWebArchive.cpp:489: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/loader/archive/cf/LegacyWebArchive.h:61: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 3 in 18 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 263978 [details] Patch
Attachment 263978 [details] did not pass style-queue: ERROR: Source/WebCore/loader/archive/cf/LegacyWebArchive.cpp:423: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/loader/archive/cf/LegacyWebArchive.cpp:489: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/loader/archive/cf/LegacyWebArchive.h:61: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 3 in 22 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 263978 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=263978&action=review As usual, there seem to be a lot of PassRefPtr arguments to functions that are not taking ownership of the thing that is passed. These should not be converted to RefPtr&& because there is no value to the move semantics. Instead they should be converted to raw pointers or raw references. I can quickly review all those functions returning RefPtr and Ref, but for the ones taking && I am not at all sure it’s correct. Even worse, we need to be sure we use WTF::move to take full advantage of those &&, which was not needed to take advantage of PassRefPtr. > Source/WebCore/editing/ios/EditorIOS.mm:403 > fragment = newFragment; Need WTF::move here. > Source/WebCore/editing/ios/EditorIOS.mm:406 > +bool Editor::WebContentReader::readWebArchive(RefPtr<SharedBuffer>&& buffer) This function doesn’t currently take advantage of the rvalue reference and so it could just take a SharedBuffer& instead. Or we could fix it to take advantage of the rvalue reference. > Source/WebCore/editing/ios/EditorIOS.mm:448 > +bool Editor::WebContentReader::readRTFD(RefPtr<SharedBuffer>&& buffer) This function doesn’t currently take advantage of the rvalue reference and so it could just take a SharedBuffer& instead. > Source/WebCore/editing/ios/EditorIOS.mm:454 > +bool Editor::WebContentReader::readRTF(RefPtr<SharedBuffer>&& buffer) This function doesn’t currently take advantage of the rvalue reference and so it could just take a SharedBuffer& instead. > Source/WebCore/editing/ios/EditorIOS.mm:460 > +bool Editor::WebContentReader::readImage(RefPtr<SharedBuffer>&& buffer, const String& type) There’s no WTF::move at the use of the buffer below, so I suspect we are not getting the value out of the rvalue reference. > Source/WebCore/editing/mac/EditorMac.mm:323 > + Ref<Range> range = selectedRange().releaseNonNull(); What guarantees this can never be null? Seems we need to fix the selectedRange() function to never return null (change its return type?), or handle the case where the range is null, or figure out why this is never called when the range is null. > Source/WebCore/editing/mac/EditorMac.mm:482 > +bool Editor::WebContentReader::readWebArchive(RefPtr<SharedBuffer>&& buffer) This function doesn’t currently take advantage of the rvalue reference and so it could just take a SharedBuffer& instead. Or we could fix it to take advantage of the rvalue reference. > Source/WebCore/editing/mac/EditorMac.mm:569 > +bool Editor::WebContentReader::readRTFD(RefPtr<SharedBuffer>&& buffer) This function doesn’t currently take advantage of the rvalue reference and so it could just take a SharedBuffer& instead. > Source/WebCore/editing/mac/EditorMac.mm:575 > +bool Editor::WebContentReader::readRTF(RefPtr<SharedBuffer>&& buffer) This function doesn’t currently take advantage of the rvalue reference and so it could just take a SharedBuffer& instead. > Source/WebCore/editing/mac/EditorMac.mm:581 > +bool Editor::WebContentReader::readImage(RefPtr<SharedBuffer>&& buffer, const String& type) There’s no WTF::move at the use of the buffer below, so I suspect we are not getting the value out of the rvalue reference. > Source/WebCore/editing/mac/EditorMac.mm:629 > +RefPtr<DocumentFragment> Editor::createFragmentForImageResourceAndAddResource(RefPtr<ArchiveResource>&& resource) There’s no WTF::move at the use of the resource below, so I suspect we are not getting the value out of the rvalue reference. > Source/WebCore/loader/archive/cf/LegacyWebArchive.cpp:243 > archive->setMainResource(mainResource); Need a WTF::move here. > Source/WebCore/platform/gtk/DataObjectGtk.cpp:157 > + Ref<DataObjectGtk> dataObject = DataObjectGtk::create(); > + objectMap.set(clipboard, WTF::move(dataObject)); > + return dataObject.ptr(); This is broken and will start always returning null. After calling WTF::move on dataObject, dataObject.ptr will always be null. It needs to be dataObject.copyRef(), not WTF::move(dataObject). > Source/WebCore/platform/gtk/PasteboardGtk.cpp:78 > : m_dataObject(dataObject) Missing WTF::move here. > Source/WebCore/platform/gtk/PasteboardGtk.cpp:96 > +RefPtr<DataObjectGtk> Pasteboard::dataObject() const I don’t understand why the return type for this function is RefPtr. It’s just going to churn the reference count for no reason. This never returns a new object.
Created attachment 271977 [details] Patch
Attachment 271977 [details] did not pass style-queue: ERROR: Source/WebCore/loader/archive/cf/LegacyWebArchive.cpp:423: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/loader/archive/cf/LegacyWebArchive.cpp:489: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/loader/archive/cf/LegacyWebArchive.h:61: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 3 in 49 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 271987 [details] Patch
Attachment 271987 [details] did not pass style-queue: ERROR: Source/WebCore/loader/archive/cf/LegacyWebArchive.cpp:423: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/loader/archive/cf/LegacyWebArchive.cpp:489: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/loader/archive/cf/LegacyWebArchive.h:61: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 3 in 49 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 271988 [details] Patch
Attachment 271988 [details] did not pass style-queue: ERROR: Source/WebCore/loader/archive/cf/LegacyWebArchive.cpp:423: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/loader/archive/cf/LegacyWebArchive.cpp:489: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/loader/archive/cf/LegacyWebArchive.h:61: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 3 in 49 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 272078 [details] Patch
Attachment 272078 [details] did not pass style-queue: ERROR: Source/WebCore/loader/archive/cf/LegacyWebArchive.cpp:423: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/loader/archive/cf/LegacyWebArchive.cpp:489: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/loader/archive/cf/LegacyWebArchive.h:61: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 3 in 57 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 272096 [details] Fix build break in Win port
Attachment 272096 [details] did not pass style-queue: ERROR: Source/WebCore/loader/archive/cf/LegacyWebArchive.cpp:423: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/loader/archive/cf/LegacyWebArchive.cpp:489: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/loader/archive/cf/LegacyWebArchive.h:61: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 3 in 57 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 272096 [details] Fix build break in Win port View in context: https://bugs.webkit.org/attachment.cgi?id=272096&action=review Generally looks good. A little refinement still might be worthwhile. A lot of the functions that say SharedBuffer* or RefPtr<SharedBuffer>&& probably don’t actually handle null correctly and should be taking a SharedBuffer& or a Ref<SharedBuffer>&&. We can double check how callers use these functions and their implementations and figure that out. > Source/JavaScriptCore/runtime/ArrayBuffer.h:152 > return ArrayBuffer::create(other->data(), other->byteLength()); No need for the "ArrayBuffer::" prefix here. > Source/WebCore/Modules/mediastream/RTCDataChannel.cpp:287 > if (m_binaryType == BinaryTypeArrayBuffer) { > RefPtr<ArrayBuffer> buffer = ArrayBuffer::create(data, dataLength); > - scheduleDispatchEvent(MessageEvent::create(buffer.release())); > + scheduleDispatchEvent(MessageEvent::create(WTFMove(buffer))); > return; > } Would read better as a one-liner without the WTFMove: scheduleDispatchEvent(MessageEvent::create(ArrayBuffer::create(data, dataLength))); > Source/WebCore/editing/VisiblePosition.cpp:710 > +RefPtr<Range> makeRange(const VisiblePosition &start, const VisiblePosition &end) While touching this line would be good to fix the incorrect formatting. Should be "VisiblePosition& x", not "VisiblePosition &x". > Source/WebCore/editing/VisibleUnits.cpp:1859 > - RefPtr<Range> range = Range::create(prevBoundary.deepEquivalent().deprecatedNode()->document(), prevBoundary, nextBoundary); > + Ref<Range> range = Range::create(prevBoundary.deepEquivalent().deprecatedNode()->document(), prevBoundary, nextBoundary); > > - return range; > + return WTFMove(range); Would read better without the local variable: return Range::create(prevBoundary.deepEquivalent().deprecatedNode()->document(), prevBoundary, nextBoundary); > Source/WebCore/editing/gtk/EditorGtk.cpp:56 > return fragment.release(); The use of "release()" here is an indirect way to use PassRefPtr, so should be removed. Should just use plain return or a return WTFMove. > Source/WebCore/editing/ios/EditorIOS.mm:447 > +bool Editor::WebContentReader::readRTFD(SharedBuffer* buffer) This function dereferences "buffer" without checking for null, so the argument should be a SharedBuffer&, not a SharedBuffer*. > Source/WebCore/editing/ios/EditorIOS.mm:453 > +bool Editor::WebContentReader::readRTF(SharedBuffer* buffer) This function dereferences "buffer" without checking for null, so the argument should be a SharedBuffer&, not a SharedBuffer*. > Source/WebCore/editing/ios/EditorIOS.mm:500 > RefPtr<DocumentFragment> newFragment = frame.document()->createDocumentFragment(); Local variable here has the wrong type, RefPtr instead of Ref. This is why I prefer auto for cases like this. > Source/WebCore/editing/ios/EditorIOS.mm:528 > + return reader.fragment; Can we save a little reference count churn by using WTFMove here? > Source/WebCore/editing/ios/EditorIOS.mm:594 > + Ref<DocumentFragment> fragment = m_frame.document()->createDocumentFragment(); I’d prefer auto to explicitly stating the type here. > Source/WebCore/editing/mac/EditorMac.mm:623 > + return reader.fragment; WTFMove? > Source/WebCore/editing/mac/EditorMac.mm:638 > RefPtr<DocumentFragment> fragment = document().createDocumentFragment(); Should be Ref instead of RefPtr. I suggest using auto. > Source/WebCore/loader/DocumentLoader.h:159 > + WEBCORE_EXPORT void addArchiveResource(RefPtr<ArchiveResource>&&); I don’t think null is a legal resource to add, so this should be Ref&&, not RefPtr&&. > Source/WebCore/loader/DocumentLoader.h:170 > + WEBCORE_EXPORT RefPtr<ArchiveResource> mainResource() const; This never returns null, so its return type should be Ref, not RefPtr. > Source/WebCore/loader/SubstituteResource.h:51 > ASSERT(m_data); This assert shows that the argument should be Ref<SharedBuffer>&&, not RefPtr<SharedBuffer>. > Source/WebCore/loader/archive/ArchiveResource.h:41 > + static RefPtr<ArchiveResource> create(RefPtr<SharedBuffer>&&, const URL&, const ResourceResponse&); > + WEBCORE_EXPORT static RefPtr<ArchiveResource> create(RefPtr<SharedBuffer>&&, const URL&, > const String& mimeType, const String& textEncoding, const String& frameName, > const ResourceResponse& = ResourceResponse()); Should be Ref<SharedBuffer>&&, not RefPtr. > Source/WebCore/loader/archive/ArchiveResource.h:51 > + ArchiveResource(RefPtr<SharedBuffer>&&, const URL&, const String& mimeType, const String& textEncoding, const String& frameName, const ResourceResponse&); Should be Ref<SharedBuffer>&&, not RefPtr. > Source/WebCore/loader/archive/ArchiveResourceCollection.cpp:62 > -void ArchiveResourceCollection::addResource(PassRefPtr<ArchiveResource> resource) > +void ArchiveResourceCollection::addResource(RefPtr<ArchiveResource>&& resource) Should take a look at callers and see if we change change this to be Ref&&, not RefPtr. This is one of those crazy functions that assert non-null but also do early return for non-null. Would be nice to clean it up if we can. > Source/WebCore/loader/archive/ArchiveResourceCollection.cpp:68 > const URL& url = resource->url(); // get before passing PassRefPtr (which sets it to 0) Comment explicitly mentions PassRefPtr and 0. Need to update the wording or remove the comment. > Source/WebCore/loader/archive/cf/LegacyWebArchive.cpp:425 > ASSERT(node); This assertion means that the create function should take a Node&, not a Node*. > Source/WebCore/loader/archive/cf/LegacyWebArchive.cpp:453 > ASSERT(frame); This assertion means that the create function should take a Frame&, not a Frame*. > Source/WebCore/loader/archive/cf/LegacyWebArchive.cpp:491 > ASSERT(frame); This assertion means that the create function should take a Frame&, not a Frame*. > Source/WebCore/platform/gtk/DataObjectGtk.h:34 > + return adoptRef(*new DataObjectGtk()); Normally we’d leave off the () after DataObjectGtk.
Created attachment 272486 [details] Patch
Attachment 272486 [details] did not pass style-queue: ERROR: Source/WebCore/loader/archive/cf/LegacyWebArchive.cpp:418: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/loader/archive/cf/LegacyWebArchive.cpp:478: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/loader/archive/cf/LegacyWebArchive.h:61: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 3 in 79 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 272490 [details] Patch
Attachment 272490 [details] did not pass style-queue: ERROR: Source/WebCore/loader/archive/cf/LegacyWebArchive.cpp:418: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/loader/archive/cf/LegacyWebArchive.cpp:478: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/loader/archive/cf/LegacyWebArchive.h:61: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 3 in 83 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 272096 [details] Fix build break in Win port View in context: https://bugs.webkit.org/attachment.cgi?id=272096&action=review I took r+ to this previously, but there are more changes here, so I think this change should be reviewed again. Would you please take a look this again, Darin? >> Source/JavaScriptCore/runtime/ArrayBuffer.h:152 >> return ArrayBuffer::create(other->data(), other->byteLength()); > > No need for the "ArrayBuffer::" prefix here. Done. >> Source/WebCore/Modules/mediastream/RTCDataChannel.cpp:287 >> } > > Would read better as a one-liner without the WTFMove: > > scheduleDispatchEvent(MessageEvent::create(ArrayBuffer::create(data, dataLength))); Done. >> Source/WebCore/editing/VisiblePosition.cpp:710 >> +RefPtr<Range> makeRange(const VisiblePosition &start, const VisiblePosition &end) > > While touching this line would be good to fix the incorrect formatting. Should be "VisiblePosition& x", not "VisiblePosition &x". Done. >> Source/WebCore/editing/VisibleUnits.cpp:1859 >> + return WTFMove(range); > > Would read better without the local variable: > > return Range::create(prevBoundary.deepEquivalent().deprecatedNode()->document(), prevBoundary, nextBoundary); Done. >> Source/WebCore/editing/gtk/EditorGtk.cpp:56 >> return fragment.release(); > > The use of "release()" here is an indirect way to use PassRefPtr, so should be removed. Should just use plain return or a return WTFMove. Done. >> Source/WebCore/editing/ios/EditorIOS.mm:447 >> +bool Editor::WebContentReader::readRTFD(SharedBuffer* buffer) > > This function dereferences "buffer" without checking for null, so the argument should be a SharedBuffer&, not a SharedBuffer*. Done. >> Source/WebCore/editing/ios/EditorIOS.mm:453 >> +bool Editor::WebContentReader::readRTF(SharedBuffer* buffer) > > This function dereferences "buffer" without checking for null, so the argument should be a SharedBuffer&, not a SharedBuffer*. Done. >> Source/WebCore/editing/ios/EditorIOS.mm:500 >> RefPtr<DocumentFragment> newFragment = frame.document()->createDocumentFragment(); > > Local variable here has the wrong type, RefPtr instead of Ref. This is why I prefer auto for cases like this. Done. >> Source/WebCore/editing/ios/EditorIOS.mm:528 >> + return reader.fragment; > > Can we save a little reference count churn by using WTFMove here? Yes, it's not the target of Return Value Optimization since it's not the local variable itself, but its member variable. I added WTFMove here. >> Source/WebCore/editing/ios/EditorIOS.mm:594 >> + Ref<DocumentFragment> fragment = m_frame.document()->createDocumentFragment(); > > I’d prefer auto to explicitly stating the type here. Done. >> Source/WebCore/editing/mac/EditorMac.mm:623 >> + return reader.fragment; > > WTFMove? I added WTFMove here. >> Source/WebCore/editing/mac/EditorMac.mm:638 >> RefPtr<DocumentFragment> fragment = document().createDocumentFragment(); > > Should be Ref instead of RefPtr. I suggest using auto. Done. >> Source/WebCore/loader/DocumentLoader.h:159 >> + WEBCORE_EXPORT void addArchiveResource(RefPtr<ArchiveResource>&&); > > I don’t think null is a legal resource to add, so this should be Ref&&, not RefPtr&&. Done. >> Source/WebCore/loader/DocumentLoader.h:170 >> + WEBCORE_EXPORT RefPtr<ArchiveResource> mainResource() const; > > This never returns null, so its return type should be Ref, not RefPtr. In mainResource()'s body, it could invokes ArchiveResource::create() in which null can be returned, it seems. So I let it well enough alone for now. >> Source/WebCore/loader/SubstituteResource.h:51 >> ASSERT(m_data); > > This assert shows that the argument should be Ref<SharedBuffer>&&, not RefPtr<SharedBuffer>. Done. >> Source/WebCore/loader/archive/ArchiveResource.h:41 >> const ResourceResponse& = ResourceResponse()); > > Should be Ref<SharedBuffer>&&, not RefPtr. ArchiveResource's ctor can be non-null, so I changed it to use Ref, while it seems that create() can return null so I left it as it is. >> Source/WebCore/loader/archive/ArchiveResource.h:51 >> + ArchiveResource(RefPtr<SharedBuffer>&&, const URL&, const String& mimeType, const String& textEncoding, const String& frameName, const ResourceResponse&); > > Should be Ref<SharedBuffer>&&, not RefPtr. Done. >> Source/WebCore/loader/archive/ArchiveResourceCollection.cpp:62 >> +void ArchiveResourceCollection::addResource(RefPtr<ArchiveResource>&& resource) > > Should take a look at callers and see if we change change this to be Ref&&, not RefPtr. This is one of those crazy functions that assert non-null but also do early return for non-null. Would be nice to clean it up if we can. I changed it to use Ref&& instead. >> Source/WebCore/loader/archive/ArchiveResourceCollection.cpp:68 >> const URL& url = resource->url(); // get before passing PassRefPtr (which sets it to 0) > > Comment explicitly mentions PassRefPtr and 0. Need to update the wording or remove the comment. I removed the comment since it is not vaild anymore. >> Source/WebCore/loader/archive/cf/LegacyWebArchive.cpp:425 >> ASSERT(node); > > This assertion means that the create function should take a Node&, not a Node*. In win port, at win/DOMCoreClasses.cpp, DOMNode::createInstance and DOMElement::createInstance ensures that node is non-null. In mac port, at WebDomOperations.mm, the caller is _initWithCoreLegacyWebArchive:LegacyWebArchive::create(core(self))] autorelease]; The self is DOMNode itself, so all the callers guarantees that Node* can be replaced with Node&, I think. >> Source/WebCore/loader/archive/cf/LegacyWebArchive.cpp:453 >> ASSERT(frame); > > This assertion means that the create function should take a Frame&, not a Frame*. Done. >> Source/WebCore/loader/archive/cf/LegacyWebArchive.cpp:491 >> ASSERT(frame); > > This assertion means that the create function should take a Frame&, not a Frame*. Done. >> Source/WebCore/platform/gtk/DataObjectGtk.h:34 >> + return adoptRef(*new DataObjectGtk()); > > Normally we’d leave off the () after DataObjectGtk. Done.
Comment on attachment 272490 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=272490&action=review > Source/WebCore/Modules/mediastream/RTCDataChannel.cpp:284 > + scheduleDispatchEvent(MessageEvent::create(ArrayBuffer::create(data, dataLength))); Strange that this calls create, which returns nullptr when it fails, but doesn’t check for null. Is that really the behavior we want here? I think we probably need to change ArrayBuffer::create to crash when it runs out of memory, and add a new ArrayBuffer::tryCreate for the call sites that want to check for null. I know that has little to do with PassRefPtr, but it’s obvious when reading this patch. > Source/WebCore/dom/MessageEvent.h:74 > + static Ref<MessageEvent> create(RefPtr<ArrayBuffer>&& data, const String& origin = String()) Not sure we want to support null for this. Tied up with the question I mentioned above. > Source/WebCore/editing/ios/EditorIOS.mm:410 > + RefPtr<LegacyWebArchive> archive = LegacyWebArchive::create(URL(), *buffer); This is assuming the buffer won’t be null. The old code just passed the null along to LegacyWebArchive. So this is a behavior change. We should either change the argument to this function to a SharedBuffer& or handle null here by returning false. > Source/WebCore/editing/mac/EditorMac.mm:488 > + RefPtr<LegacyWebArchive> archive = LegacyWebArchive::create(URL(), *buffer); Same problem here as above. > Source/WebCore/loader/SubstituteResource.h:41 > + SharedBuffer* data() const { return const_cast<SharedBuffer*>(m_data.ptr()); } Since this can’t be null, the return type should be SharedBuffer&, not SharedBuffer*. > Source/WebCore/loader/archive/cf/LegacyWebArchive.cpp:556 > + return create(WTFMove(mainResource).releaseNonNull(), WTFMove(subresources), WTFMove(subframeArchives)); It’s wrong to do both WTFMove and releaseNonNull on the same value. You only need one or the other. I think just WTFMove should work but if not, then just releaseNonNull. > Source/WebCore/loader/archive/mhtml/MHTMLParser.cpp:63 > - RefPtr<MIMEHeader> header = MIMEHeader::parseHeader(&m_lineReader); > + auto header = MIMEHeader::parseHeader(m_lineReader); > return parseArchiveWithHeader(header.get()); I’d suggest doing this as a one-liner: return parseArchiveWithHeader(MIMEHeader::parseHeader(m_lineReader).get()); > Source/WebCore/loader/cache/CachedImage.cpp:123 > - m_image->setData(m_data, true); > + auto data = m_data; > + m_image->setData(WTFMove(data), true); I don’t understand this change at all. Explanation?
(In reply to comment #26) > Comment on attachment 272490 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=272490&action=review > > > Source/WebCore/Modules/mediastream/RTCDataChannel.cpp:284 > > + scheduleDispatchEvent(MessageEvent::create(ArrayBuffer::create(data, dataLength))); > > Strange that this calls create, which returns nullptr when it fails, but > doesn’t check for null. Is that really the behavior we want here? > > I think we probably need to change ArrayBuffer::create to crash when it runs > out of memory, and add a new ArrayBuffer::tryCreate for the call sites that > want to check for null. I know that has little to do with PassRefPtr, but > it’s obvious when reading this patch. As can be seen from step 5 of https://fetch.spec.whatwg.org/#body-mixin, there may be cases where we might want to throw a JS exception if creating an array buffer fails. "tryCreate" might be useful for that purpose.
Joonghun, are you planning to handle the ArrayBuffer::create issue? If that helps, I can work on this specific issue as a separate bug. Wdyt?
(In reply to comment #28) > Joonghun, are you planning to handle the ArrayBuffer::create issue? > If that helps, I can work on this specific issue as a separate bug. > Wdyt? I added ArrayBuffer::tryCreate()s in which it can return null, and be planning to upload it some hours later. After that, maybe setDOMException() you commented could be added in a separate bug, I think.
(In reply to comment #29) > (In reply to comment #28) > > Joonghun, are you planning to handle the ArrayBuffer::create issue? > > If that helps, I can work on this specific issue as a separate bug. > > Wdyt? > > I added ArrayBuffer::tryCreate()s in which it can return null, and be > planning to upload it some hours later. Great :) > After that, maybe setDOMException() you commented could be added in a > separate bug, I think. The error handling might depend on the caller. Some callers may return null, some may throw JS exceptions, some may want to crash and therefore switch back to ArrayBuffer::create(). Let's handle that as follow-up bugs.
Created attachment 273560 [details] Add ArrayBuffer::tryCreate()s
Created attachment 273561 [details] Remove untracked git files to test in EWS
Attachment 273561 [details] did not pass style-queue: ERROR: Source/WebCore/loader/archive/cf/LegacyWebArchive.h:61: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/loader/archive/cf/LegacyWebArchive.cpp:416: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/loader/archive/cf/LegacyWebArchive.cpp:476: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 3 in 92 files If any of these errors are false positives, please file a bug against check-webkit-style.
As a side note, the current bug title is not totally accurate as it does not mention the tryCreate/create issue. I am also wondering whether, given its size, the patch could be split in smaller patches.
Created attachment 273565 [details] Fix build break
Attachment 273565 [details] did not pass style-queue: ERROR: Source/WebCore/loader/archive/cf/LegacyWebArchive.h:61: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/loader/archive/cf/LegacyWebArchive.cpp:416: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/loader/archive/cf/LegacyWebArchive.cpp:476: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 3 in 93 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 273565 [details] Fix build break Attachment 273565 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/954286 New failing tests: fast/canvas/webgl/array-unit-tests.html webgl/1.0.2/conformance/typedarrays/array-unit-tests.html
Created attachment 273567 [details] Archive of layout-test-results from ews101 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 273565 [details] Fix build break Attachment 273565 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/954281 New failing tests: fast/canvas/webgl/array-unit-tests.html webgl/1.0.2/conformance/typedarrays/array-unit-tests.html
Created attachment 273568 [details] Archive of layout-test-results from ews106 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Comment on attachment 273565 [details] Fix build break Attachment 273565 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/954297 New failing tests: fast/canvas/webgl/array-unit-tests.html webgl/1.0.2/conformance/typedarrays/array-unit-tests.html
Created attachment 273569 [details] Archive of layout-test-results from ews117 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews117 Port: mac-yosemite Platform: Mac OS X 10.10.5
(In reply to comment #34) > As a side note, the current bug title is not totally accurate as it does not > mention the tryCreate/create issue. > I am also wondering whether, given its size, the patch could be split in > smaller patches. Splitting purge PassRefPtrs and create/tryCreate patch could be done. I made a bug for handling tryCreate separately as below. I will upload tryCreate patch here. https://bugs.webkit.org/show_bug.cgi?id=155328 The purge PassRefPtr patch's size could be a little big, but it still just doing only purging PassRefPtr, so it seems ok. By the way, it seems that the 2 of layout test regressions are caused by that I added tryCreate, but not changed the callsites to trycreate where needed also. After fixing this thing(by splitting the patch), I will re-upload the patch.
Created attachment 273732 [details] Exclude adding ArrayBuffer::tryCreate from this patch
Created attachment 273769 [details] Rebase this patch
Attachment 273769 [details] did not pass style-queue: ERROR: Source/WebCore/loader/archive/cf/LegacyWebArchive.h:61: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/loader/archive/cf/LegacyWebArchive.cpp:416: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/loader/archive/cf/LegacyWebArchive.cpp:476: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 3 in 93 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 273783 [details] Purge PassRefPtr from ArchiveFactory additionally
Attachment 273783 [details] did not pass style-queue: ERROR: Source/WebCore/loader/archive/cf/LegacyWebArchive.h:61: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/loader/archive/cf/LegacyWebArchive.cpp:416: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/loader/archive/cf/LegacyWebArchive.cpp:476: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 3 in 94 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 272490 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=272490&action=review >>> Source/WebCore/Modules/mediastream/RTCDataChannel.cpp:284 >>> + scheduleDispatchEvent(MessageEvent::create(ArrayBuffer::create(data, dataLength))); >> >> Strange that this calls create, which returns nullptr when it fails, but doesn’t check for null. Is that really the behavior we want here? >> >> I think we probably need to change ArrayBuffer::create to crash when it runs out of memory, and add a new ArrayBuffer::tryCreate for the call sites that want to check for null. I know that has little to do with PassRefPtr, but it’s obvious when reading this patch. > > As can be seen from step 5 of https://fetch.spec.whatwg.org/#body-mixin, there may be cases where we might want to throw a JS exception if creating an array buffer fails. "tryCreate" might be useful for that purpose. I will add ArrayBuffer::tryCreate() versions to each ArrayBuffer::create() if it can return null originally, and if it encounters out of memory, it will meet ASSERT() that will be added, too. But this change will be dealt with in https://bugs.webkit.org/show_bug.cgi?id=155328 as separate bug as Youenn suggested. >> Source/WebCore/dom/MessageEvent.h:74 >> + static Ref<MessageEvent> create(RefPtr<ArrayBuffer>&& data, const String& origin = String()) > > Not sure we want to support null for this. Tied up with the question I mentioned above. I will change this to Ref, because ArrayBuffer::create's return type will be from RefPtr to Ref. This will be dealt with in https://bugs.webkit.org/show_bug.cgi?id=155328 as separate bug as Youenn suggested, so for now change this to RefPtr&&. >> Source/WebCore/editing/ios/EditorIOS.mm:410 >> + RefPtr<LegacyWebArchive> archive = LegacyWebArchive::create(URL(), *buffer); > > This is assuming the buffer won’t be null. The old code just passed the null along to LegacyWebArchive. So this is a behavior change. We should either change the argument to this function to a SharedBuffer& or handle null here by returning false. I added if (!buffer) return false; statement before here. >> Source/WebCore/editing/mac/EditorMac.mm:488 >> + RefPtr<LegacyWebArchive> archive = LegacyWebArchive::create(URL(), *buffer); > > Same problem here as above. The same as above. >> Source/WebCore/loader/SubstituteResource.h:41 >> + SharedBuffer* data() const { return const_cast<SharedBuffer*>(m_data.ptr()); } > > Since this can’t be null, the return type should be SharedBuffer&, not SharedBuffer*. I changed this from raw pointer to reference. >> Source/WebCore/loader/archive/cf/LegacyWebArchive.cpp:556 >> + return create(WTFMove(mainResource).releaseNonNull(), WTFMove(subresources), WTFMove(subframeArchives)); > > It’s wrong to do both WTFMove and releaseNonNull on the same value. You only need one or the other. I think just WTFMove should work but if not, then just releaseNonNull. I removed releaseNonNull. It's redundant call, ouch! >> Source/WebCore/loader/archive/mhtml/MHTMLParser.cpp:63 >> return parseArchiveWithHeader(header.get()); > > I’d suggest doing this as a one-liner: > > return parseArchiveWithHeader(MIMEHeader::parseHeader(m_lineReader).get()); I revised this one as you commented here. >> Source/WebCore/loader/cache/CachedImage.cpp:123 >> + m_image->setData(WTFMove(data), true); > > I don’t understand this change at all. Explanation? I changed this to m_image->setData(m_data.copyRef(), true); statement. Image::setData's signature now uses RefPtr<SharedBuffer>&&, instead of PassRefPtr. So to retain m_data of cachedImage I used copyRef. If just do WTFMove(data), m_data gonna be null unexpectedly.
Created attachment 273787 [details] Restore missing null buffer check at WebContentReader::readWebArchive in EditorMac.mm
Attachment 273787 [details] did not pass style-queue: ERROR: Source/WebCore/loader/archive/cf/LegacyWebArchive.h:61: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/loader/archive/cf/LegacyWebArchive.cpp:416: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/loader/archive/cf/LegacyWebArchive.cpp:476: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 3 in 94 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 273787 [details] Restore missing null buffer check at WebContentReader::readWebArchive in EditorMac.mm View in context: https://bugs.webkit.org/attachment.cgi?id=273787&action=review > Source/JavaScriptCore/runtime/ArrayBuffer.h:101 > + static inline RefPtr<ArrayBuffer> create(ArrayBuffer*); This needs to take a reference, not a pointer. It always dereferences the pointer. I’m not saying that must be done in this patch, but it does need to be done. Maybe fix that when we rename to tryCreate? > Source/WebCore/editing/gtk/EditorGtk.cpp:56 > if (dataObject->hasMarkup() && frame.document()) { > - if (RefPtr<DocumentFragment> fragment = createFragmentFromMarkup(*frame.document(), dataObject->markup(), emptyString(), DisallowScriptingAndPluginContent)) > - return fragment.release(); > + return createFragmentFromMarkup(*frame.document(), dataObject->markup(), emptyString(), DisallowScriptingAndPluginContent); > } In WebKit coding style, when an if statement’s body becomes a single line, we omit the parentheses. So that should be done here. > Source/WebCore/platform/Pasteboard.h:121 > + virtual bool readWebArchive(SharedBuffer*) = 0; Do callers really need to pass null? In the future I suggest we change this argument to a reference and move the null checks to the call sites. It’s strange that this one function takes a pointer while the others take references. > Source/WebCore/platform/SharedBuffer.h:58 > + static Ref<SharedBuffer> create(const unsigned char* c, unsigned i) { return adoptRef(*new SharedBuffer(c, i)); } When touching a line of code like this it’s best to change single letters to words to follow usual WebKit coding style. Here it would be "data" and "size", rather than "c" and "i". > Source/WebCore/platform/SharedBuffer.h:68 > + WEBCORE_EXPORT static Ref<SharedBuffer> wrapNSData(NSData*); Formatting here is wrong. It should be NSData *, with a space before the * given that this is an Objective-C class. > Source/WebCore/platform/gtk/DataObjectGtk.cpp:160 > if (!objectMap.contains(clipboard)) { > - RefPtr<DataObjectGtk> dataObject = DataObjectGtk::create(); > - objectMap.set(clipboard, dataObject); > - return dataObject.get(); > + Ref<DataObjectGtk> dataObject = DataObjectGtk::create(); > + objectMap.set(clipboard, dataObject.copyRef()); > + return dataObject.ptr(); > } > > HashMap<GtkClipboard*, RefPtr<DataObjectGtk> >::iterator it = objectMap.find(clipboard); This is written wrong, in a way that does a second hash table lookup and a one cycle of unneeded reference count churn. The change to make it optimal isn’t all that big a challenge, now that we have the ensure function, but I am not highly motivated to rewrite the code myself to fix it. > Source/WebCore/platform/win/ClipboardUtilitiesWin.cpp:652 > if (!cfhtml.isEmpty()) { > - if (RefPtr<DocumentFragment> fragment = fragmentFromCFHTML(doc, cfhtml)) > - return fragment.release(); > + return fragmentFromCFHTML(doc, cfhtml); > } WebKit coding style does not involve braces when it’s a single line like this. > Source/WebCore/platform/win/ClipboardUtilitiesWin.cpp:670 > if (getDataMapItem(data, htmlFormat(), stringData)) { > - if (RefPtr<DocumentFragment> fragment = fragmentFromCFHTML(document, stringData)) > - return fragment.release(); > + return fragmentFromCFHTML(document, stringData); > } Ditto. > Source/WebKit2/Shared/APIWebArchive.mm:91 > + auto buffer = SharedBuffer::create(data->bytes(), data->size()); > m_legacyWebArchive = LegacyWebArchive::create(buffer.get()); Would read better as a one-liner.
Comment on attachment 273787 [details] Restore missing null buffer check at WebContentReader::readWebArchive in EditorMac.mm View in context: https://bugs.webkit.org/attachment.cgi?id=273787&action=review >> Source/JavaScriptCore/runtime/ArrayBuffer.h:101 >> + static inline RefPtr<ArrayBuffer> create(ArrayBuffer*); > > This needs to take a reference, not a pointer. It always dereferences the pointer. I’m not saying that must be done in this patch, but it does need to be done. Maybe fix that when we rename to tryCreate? Ok, I will change this in tryCreate patch. >> Source/WebCore/editing/gtk/EditorGtk.cpp:56 >> } > > In WebKit coding style, when an if statement’s body becomes a single line, we omit the parentheses. So that should be done here. Done. >> Source/WebCore/platform/Pasteboard.h:121 >> + virtual bool readWebArchive(SharedBuffer*) = 0; > > Do callers really need to pass null? In the future I suggest we change this argument to a reference and move the null checks to the call sites. It’s strange that this one function takes a pointer while the others take references. Maybe this can be dealt with in ShareBuffer specific patch separately. >> Source/WebCore/platform/SharedBuffer.h:58 >> + static Ref<SharedBuffer> create(const unsigned char* c, unsigned i) { return adoptRef(*new SharedBuffer(c, i)); } > > When touching a line of code like this it’s best to change single letters to words to follow usual WebKit coding style. Here it would be "data" and "size", rather than "c" and "i". Done. >> Source/WebCore/platform/SharedBuffer.h:68 >> + WEBCORE_EXPORT static Ref<SharedBuffer> wrapNSData(NSData*); > > Formatting here is wrong. It should be NSData *, with a space before the * given that this is an Objective-C class. Done. >> Source/WebCore/platform/gtk/DataObjectGtk.cpp:160 >> HashMap<GtkClipboard*, RefPtr<DataObjectGtk> >::iterator it = objectMap.find(clipboard); > > This is written wrong, in a way that does a second hash table lookup and a one cycle of unneeded reference count churn. The change to make it optimal isn’t all that big a challenge, now that we have the ensure function, but I am not highly motivated to rewrite the code myself to fix it. Ok, I will do this in https://bugs.webkit.org/show_bug.cgi?id=155470 separately. >> Source/WebCore/platform/win/ClipboardUtilitiesWin.cpp:652 >> } > > WebKit coding style does not involve braces when it’s a single line like this. Done. >> Source/WebCore/platform/win/ClipboardUtilitiesWin.cpp:670 >> } > > Ditto. Done. >> Source/WebKit2/Shared/APIWebArchive.mm:91 >> m_legacyWebArchive = LegacyWebArchive::create(buffer.get()); > > Would read better as a one-liner. Done.
Created attachment 274036 [details] Patch for landing
Attachment 274036 [details] did not pass style-queue: ERROR: Source/WebCore/loader/archive/cf/LegacyWebArchive.h:61: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/loader/archive/cf/LegacyWebArchive.cpp:416: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/loader/archive/cf/LegacyWebArchive.cpp:476: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 3 in 94 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 274036 [details] Patch for landing Clearing flags on attachment: 274036 Committed r198177: <http://trac.webkit.org/changeset/198177>
All reviewed patches have been landed. Closing bug.
This broke paste of images on Mac and iOS. The changes to EditorMac and editoriOS reverted a fix in http://trac.webkit.org/changeset/167517. The resource needs to be added to the DocumentLoader before the src attribute is set to the image element. I have a fix and I will add a test.