Bug 240056

Created attachment 458776 [details]
Patch

Created attachment 458787 [details]
Patch

Created attachment 458810 [details]
Patch

Created attachment 458816 [details]
Patch

Created attachment 458866 [details]
Patch

<rdar://problem/93083601>

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

Created attachment 459281 [details]
Patch

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

Created attachment 459291 [details]
Patch

Created attachment 459305 [details]
Patch

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?

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

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

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

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

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

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