Bug 240056 – SharedMemory::IPCHandle is a redundant class

• Component: WebKit2
• Status: RESOLVED FIXED

[Kimmo Kinnunen]

SharedMemory::IPCHandle is a redundant class

[Kimmo Kinnunen]

Created attachment 458776 [details]
Patch

[Kimmo Kinnunen]

Created attachment 458787 [details]
Patch

[Kimmo Kinnunen]

Created attachment 458810 [details]
Patch

[Kimmo Kinnunen]

Created attachment 458816 [details]
Patch

[Kimmo Kinnunen]

Created attachment 458866 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/93083601>

[Fujii Hironori]

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.

[Fujii Hironori]

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.

[Kimmo Kinnunen]

Created attachment 459281 [details]
Patch

[Kimmo Kinnunen]

(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.

[Fujii Hironori]

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;

[Kimmo Kinnunen]

Created attachment 459291 [details]
Patch

[Kimmo Kinnunen]

Created attachment 459305 [details]
Patch

[Fujii Hironori]

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?

[Kimmo Kinnunen]

Pull request: https://github.com/WebKit/WebKit/pull/3304

[EWS]

Committed 253508@main (29dff38a6ecb): <https://commits.webkit.org/253508@main>

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

[Kimmo Kinnunen]

Re-opening for pull request https://github.com/WebKit/WebKit/pull/3531

[EWS]

Committed 253680@main (5d61da99e4fe): <https://commits.webkit.org/253680@main>

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