Bug 138212 – [WK2] Send origin & deletion requests to WebProcessess in additon to the DatabaseProcess.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jer Noble]

[WK2] Send origin & deletion requests to WebProcessess in additon to the DatabaseProcess.

[Jer Noble]

Created attachment 240674 [details]
Patch

[WebKit Commit Bot]

Attachment 240674 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/WebOriginDataManagerProxy.cpp:126:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/WebOriginDataManagerProxy.cpp:136:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebKit2/UIProcess/WebOriginDataManagerProxy.cpp:140:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/WebOriginDataManagerProxy.cpp:194:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/WebOriginDataManagerProxy.cpp:235:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/WebOriginDataManagerProxy.cpp:278:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 6 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Radar WebKit Bug Importer]

<rdar://problem/18824931>

[Brady Eidson]

Comment on attachment 240674 [details]
Patch

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

I think the naming needs work.  It's hard to "read the code" when the naming is as weird as it is.

> Source/WebKit2/ChangeLog:14
> +        Add a simple class, GroupSynchronizer, which will keep track of outstanding process callbacks
> +        and which will trigger the final callback once all of them finish.

I don't think GroupSynchronizer is a good name.

If I didn't read this comment and was asked what I think "GroupSynchronizer" does, I wouldn't have a clue.

> Source/WebKit2/UIProcess/WebOriginDataManagerProxy.cpp:108
> +    void escalateError(const CallbackBase::Error& error)

Not a fan of the naming "escalateError".  Not sure what that's supposed to mean.  Especially when sometimes the error is "not an error", and especially when it is always called.

> Source/WebKit2/UIProcess/WebOriginDataManagerProxy.cpp:121
> +    std::atomic<unsigned> m_count;

Are we expecting calls to be from multiple threads?

Is there a good reason to allow them to be?

Unless there's a good reason, I'd rather we protect against it instead of explicitly allow it.

[Jer Noble]

Created attachment 240741 [details]
Patch

[WebKit Commit Bot]

Attachment 240741 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/WebOriginDataManagerProxy.cpp:125:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/WebOriginDataManagerProxy.cpp:135:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebKit2/UIProcess/WebOriginDataManagerProxy.cpp:139:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/WebOriginDataManagerProxy.cpp:194:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/WebOriginDataManagerProxy.cpp:236:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/WebOriginDataManagerProxy.cpp:280:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 6 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Brady Eidson]

Comment on attachment 240741 [details]
Patch

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

> Source/WebKit2/UIProcess/WebOriginDataManagerProxy.cpp:137
> +    RefPtr<CallbackSynchronizer> synchronizer = CallbackSynchronizer::create();
> +
> +    synchronizer->setCallback([finalOrigins, callbackFunction](const CallbackBase::Error& error){
> +        callbackFunction(finalOrigins.get(), error);
> +    });

Can't the CallbackSynchronizer just take the callback in its creator?  It should be invalid to create one without a callback.

> Source/WebKit2/UIProcess/WebOriginDataManagerProxy.cpp:192
> +    RefPtr<CallbackSynchronizer> synchronizer = CallbackSynchronizer::create();
>  
> -    if (!context()) {
> -        callback->invalidate();
> -        return;
> -    }
> +    synchronizer->setCallback([callbackFunction](const CallbackBase::Error& error) {
> +        callbackFunction(error);
> +    });

Can't the CallbackSynchronizer just take the callback in its creator?  It should be invalid to create one without a callback.

> Source/WebKit2/UIProcess/WebOriginDataManagerProxy.cpp:234
> +    RefPtr<CallbackSynchronizer> synchronizer = CallbackSynchronizer::create();
>  
> -    if (!context()) {
> -        callback->invalidate();
> -        return;
> -    }
> +    synchronizer->setCallback([callbackFunction](const CallbackBase::Error& error) {
> +        callbackFunction(error);
> +    });

Can't the CallbackSynchronizer just take the callback in its creator?  It should be invalid to create one without a callback.

> Source/WebKit2/UIProcess/WebOriginDataManagerProxy.cpp:278
> +    RefPtr<CallbackSynchronizer> synchronizer = CallbackSynchronizer::create();
>  
> -    if (!context()) {
> -        callback->invalidate();
> -        return;
> -    }
> +    synchronizer->setCallback([callbackFunction](const CallbackBase::Error& error) {
> +        callbackFunction(error);
> +    });

Can't the CallbackSynchronizer just take the callback in its creator?  It should be invalid to create one without a callback.

[Jer Noble]

Created attachment 240756 [details]
Patch

[WebKit Commit Bot]

Attachment 240756 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/WebOriginDataManagerProxy.cpp:135:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/WebOriginDataManagerProxy.cpp:149:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/WebOriginDataManagerProxy.cpp:190:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 3 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Brady Eidson]

Comment on attachment 240756 [details]
Patch

Why are the bots orange?  that's bad.

[Jer Noble]

(In reply to comment #10)
> Comment on attachment 240756 [details]
> Patch
> 
> Why are the bots orange?  that's bad.

GTK: ninja: error: '../../Source/WebKit2/NetworkProcess/cocoa/NetworkCacheStorage.h', needed by 'WebKit2-forwarding-headers.stamp', missing and no known rule to make it

Seems unrelated.

mac-wk2: flakey transitions/ tests.

[Jer Noble]

Committed r175430: <http://trac.webkit.org/changeset/175430>