Bug 155470

Summary: [GTK] Remove duplicate HashMap traversal and unneeded reference count churn in DataObjectGtk::forClipboard
Product: WebKit Reporter: Joonghun Park <jh718.park>
Component: WebKitGTKAssignee: Joonghun Park <jh718.park>
Status: RESOLVED FIXED    
Severity: Normal CC: bugs-noreply, commit-queue
Priority: P2    
Version: WebKit Local Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Joonghun Park
Reported 2016-03-14 15:11:49 PDT
Remove duplicate HashMap traversal and unneeded reference count churn in DataObjectGtk::forClipboard.
Attachments
Patch (1.87 KB, patch)
2016-03-15 05:25 PDT, Joonghun Park 		no flags
DetailsFormatted DiffDiff
Patch (1.87 KB, patch)
2016-03-15 05:39 PDT, Joonghun Park 		no flags
DetailsFormatted DiffDiff
Patch (1.77 KB, patch)
2016-03-15 06:28 PDT, Joonghun Park 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Joonghun Park
Comment 1 2016-03-15 05:25:08 PDT
Created attachment 274088 [details] Patch
Joonghun Park
Comment 2 2016-03-15 05:39:19 PDT
Created attachment 274090 [details] Patch
Carlos Garcia Campos
Comment 3 2016-03-15 05:56:51 PDT
Comment on attachment 274090 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=274090&action=review > Source/WebCore/platform/gtk/DataObjectGtk.cpp:163 > + auto it = objectMap.find(clipboard); > > - if (!objectMap.contains(clipboard)) { > - Ref<DataObjectGtk> dataObject = DataObjectGtk::create(); > - objectMap.set(clipboard, dataObject.copyRef()); > - return dataObject.ptr(); > + if (it == objectMap.end()) { > + auto dataObject = DataObjectGtk::create(); > + DataObjectGtk* dataObjectPtr = dataObject.ptr(); > + objectMap.set(clipboard, WTFMove(dataObject)); > + return dataObjectPtr; > } > > - HashMap<GtkClipboard*, RefPtr<DataObjectGtk> >::iterator it = objectMap.find(clipboard); > return it->value.get(); > } Or even better you could use HashMap::add and check the result. Something like this: auto addResult = objectMap.add(clipboard, nullptr); if (addResult.isNewEntry) addResult.iterator->value = DataObjectGtk::create(); return addResult.iterator->value.get();
Joonghun Park
Comment 4 2016-03-15 06:16:22 PDT
(In reply to comment #3) > Comment on attachment 274090 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=274090&action=review > > > Source/WebCore/platform/gtk/DataObjectGtk.cpp:163 > > + auto it = objectMap.find(clipboard); > > > > - if (!objectMap.contains(clipboard)) { > > - Ref<DataObjectGtk> dataObject = DataObjectGtk::create(); > > - objectMap.set(clipboard, dataObject.copyRef()); > > - return dataObject.ptr(); > > + if (it == objectMap.end()) { > > + auto dataObject = DataObjectGtk::create(); > > + DataObjectGtk* dataObjectPtr = dataObject.ptr(); > > + objectMap.set(clipboard, WTFMove(dataObject)); > > + return dataObjectPtr; > > } > > > > - HashMap<GtkClipboard*, RefPtr<DataObjectGtk> >::iterator it = objectMap.find(clipboard); > > return it->value.get(); > > } > > Or even better you could use HashMap::add and check the result. Something > like this: > > auto addResult = objectMap.add(clipboard, nullptr); > if (addResult.isNewEntry) > addResult.iterator->value = DataObjectGtk::create(); > return addResult.iterator->value.get(); Indeed it is. Thank you for your comment. I will change this as you wrote.
Joonghun Park
Comment 5 2016-03-15 06:28:22 PDT
Created attachment 274091 [details] Patch
WebKit Commit Bot
Comment 6 2016-03-15 07:21:36 PDT
Comment on attachment 274091 [details] Patch Clearing flags on attachment: 274091 Committed r198209: <http://trac.webkit.org/changeset/198209>
WebKit Commit Bot
Comment 7 2016-03-15 07:21:41 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.