Bug 240056 - SharedMemory::IPCHandle is a redundant class
Summary: SharedMemory::IPCHandle is a redundant class
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kimmo Kinnunen
URL:
Keywords: InRadar
Depends on: 244118
Blocks: 244255
  Show dependency tree / graph
 
Reported: 2022-05-04 01:22 PDT by Kimmo Kinnunen
Modified: 2022-08-23 08:44 PDT (History)
11 users (show)

See Also:

Attachments
Patch (154.44 KB, patch)
2022-05-04 01:33 PDT, Kimmo Kinnunen 		ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (153.94 KB, patch)
2022-05-04 04:02 PDT, Kimmo Kinnunen 		no flags Details | Formatted Diff | Diff
Patch (153.94 KB, patch)
2022-05-04 10:56 PDT, Kimmo Kinnunen 		ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (153.93 KB, patch)
2022-05-04 11:54 PDT, Kimmo Kinnunen 		no flags Details | Formatted Diff | Diff
Patch (153.75 KB, patch)
2022-05-05 05:07 PDT, Kimmo Kinnunen 		no flags Details | Formatted Diff | Diff
Patch (156.58 KB, patch)
2022-05-13 00:07 PDT, Kimmo Kinnunen 		ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (157.13 KB, patch)
2022-05-13 04:51 PDT, Kimmo Kinnunen 		no flags Details | Formatted Diff | Diff
Patch (157.12 KB, patch)
2022-05-13 09:16 PDT, Kimmo Kinnunen 		Hironori.Fujii: review+
Details | Formatted Diff | Diff
Show Obsolete (7) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Kimmo Kinnunen 2022-05-04 01:22:44 PDT 
SharedMemory::IPCHandle is a redundant class
Comment 1 Kimmo Kinnunen 2022-05-04 01:33:17 PDT 
Created attachment 458776 [details]
Patch
Comment 2 Kimmo Kinnunen 2022-05-04 04:02:18 PDT 
Created attachment 458787 [details]
Patch
Comment 3 Kimmo Kinnunen 2022-05-04 10:56:25 PDT 
Created attachment 458810 [details]
Patch
Comment 4 Kimmo Kinnunen 2022-05-04 11:54:12 PDT 
Created attachment 458816 [details]
Patch
Comment 5 Kimmo Kinnunen 2022-05-05 05:07:23 PDT 
Created attachment 458866 [details]
Patch
Comment 6 Radar WebKit Bug Importer 2022-05-11 01:23:13 PDT 
<rdar://problem/93083601>
Comment 7 Fujii Hironori 2022-05-12 14:33:48 PDT 
Comment on attachment 458866 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=458866&action=review

Very nice!

> Source/WebKit/Platform/cocoa/SharedMemoryCocoa.cpp:263
> +    if (!roundedSize) {

How this condition can fail?
Handle::decode already checks the size.
Do you want to use UNLIKELY here?

> Source/WebKit/Platform/unix/SharedMemoryUnix.cpp:84
> +    encoder << const_cast<Handle*>(this)->releaseAttachment();

Why do you need const_cast here? releaseAttachment is a const method.
Comment 8 Fujii Hironori 2022-05-12 14:49:04 PDT 
Comment on attachment 458866 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=458866&action=review

> Source/WebKit/Shared/ShareableBitmap.cpp:51
> +    encoder << WTFMove(m_handle);

IUUC, WTFMove has no effect here. "encoder << m_handle;" works same here.

> Source/WebKit/Shared/ShareableResource.cpp:41
> +    encoder << WTFMove(m_handle);

Ditto.
Comment 9 Kimmo Kinnunen 2022-05-13 00:07:43 PDT 
Created attachment 459281 [details]
Patch
Comment 10 Kimmo Kinnunen 2022-05-13 00:12:25 PDT 
(In reply to Fujii Hironori from comment #8)
> Comment on attachment 458866 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=458866&action=review
> 
> > Source/WebKit/Shared/ShareableBitmap.cpp:51
> > +    encoder << WTFMove(m_handle);
> 
> IUUC, WTFMove has no effect here. "encoder << m_handle;" works same here.
> 


Thanks for looking!
Yeah, you are right. I was needlessly thinking ahead.
Added your suggestions.

For the slightly redundant safeRoundPage, it is just to preserve the existing logic. I'll change this and other similar cases after this refactor.
Comment 11 Fujii Hironori 2022-05-13 00:28:39 PDT 
Comment on attachment 459281 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=459281&action=review

> Source/WebKit/Shared/WebCompiledContentRuleListData.cpp:47
> +    encoder << WTFMove(handle);

encoder << handle;

> Source/WebKit/Shared/WebCoreArgumentCoders.cpp:193
> +    encoder << WTFMove(handle);

encoder << handle;

> Source/WebKit/Shared/WebHitTestResultData.cpp:130
> +    encoder << WTFMove(imageHandle);

encoder << imageHandle;

> Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm:700
>      });

WTFMove isn't needed for simple return statements.
return handle;
Comment 12 Kimmo Kinnunen 2022-05-13 04:51:53 PDT 
Created attachment 459291 [details]
Patch
Comment 13 Kimmo Kinnunen 2022-05-13 09:16:29 PDT 
Created attachment 459305 [details]
Patch
Comment 14 Fujii Hironori 2022-05-16 14:17:06 PDT 
Comment on attachment 459305 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=459305&action=review

> Source/WebKit/Shared/WebCompiledContentRuleListData.cpp:37
> +    return topURLFiltersBytecodeOffset + topURLFiltersBytecodeSize;

Do you think this addition needs overflow checking?

> Source/WebKit/Shared/WebCompiledContentRuleListData.cpp:112
> +    if (data->size() < ruleListDataSize(*topURLFiltersBytecodeOffset, *topURLFiltersBytecodeSize))

data->size() is a data size, not buffer size. I think it should (data->size() == ruleListDataSize(...)). If the condition fails, something wrong happens.

> Tools/MiniBrowser/mac/WK2BrowserWindowController.m:556
> +        title = @"";

Do you want to change MiniBrowser in this patch?
Comment 15 Kimmo Kinnunen 2022-08-15 05:55:48 PDT 
Pull request: https://github.com/WebKit/WebKit/pull/3304
Comment 16 EWS 2022-08-17 00:32:10 PDT 
Committed 253508@main (29dff38a6ecb): <https://commits.webkit.org/253508@main>

Reviewed commits have been landed. Closing PR #3304 and removing active labels.
Comment 17 Kimmo Kinnunen 2022-08-22 04:39:30 PDT 
Re-opening for pull request https://github.com/WebKit/WebKit/pull/3531
Comment 18 EWS 2022-08-23 03:21:12 PDT 
Committed 253680@main (5d61da99e4fe): <https://commits.webkit.org/253680@main>

Reviewed commits have been landed. Closing PR #3531 and removing active labels.