Bug 138212 - [WK2] Send origin & deletion requests to WebProcessess in additon to the DatabaseProcess.
Summary: [WK2] Send origin & deletion requests to WebProcessess in additon to the Data...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jer Noble
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-10-30 08:51 PDT by Jer Noble
Modified: 2014-10-31 16:07 PDT (History)
4 users (show)

See Also:


Attachments
Patch (15.33 KB, patch)
2014-10-30 09:08 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (15.58 KB, patch)
2014-10-31 10:27 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (13.76 KB, patch)
2014-10-31 14:36 PDT, Jer Noble
beidson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jer Noble 2014-10-30 08:51:57 PDT
[WK2] Send origin & deletion requests to WebProcessess in additon to the DatabaseProcess.
Comment 1 Jer Noble 2014-10-30 09:08:15 PDT
Created attachment 240674 [details]
Patch
Comment 2 WebKit Commit Bot 2014-10-30 09:10:04 PDT
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.
Comment 3 Radar WebKit Bug Importer 2014-10-30 09:10:36 PDT
<rdar://problem/18824931>
Comment 4 Brady Eidson 2014-10-30 16:46:11 PDT
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.
Comment 5 Jer Noble 2014-10-31 10:27:06 PDT
Created attachment 240741 [details]
Patch
Comment 6 WebKit Commit Bot 2014-10-31 10:28:30 PDT
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.
Comment 7 Brady Eidson 2014-10-31 11:03:28 PDT
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.
Comment 8 Jer Noble 2014-10-31 14:36:59 PDT
Created attachment 240756 [details]
Patch
Comment 9 WebKit Commit Bot 2014-10-31 14:39:31 PDT
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.
Comment 10 Brady Eidson 2014-10-31 15:43:16 PDT
Comment on attachment 240756 [details]
Patch

Why are the bots orange?  that's bad.
Comment 11 Jer Noble 2014-10-31 16:03:55 PDT
(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.
Comment 12 Jer Noble 2014-10-31 16:07:04 PDT
Committed r175430: <http://trac.webkit.org/changeset/175430>