Bug 156877 – Modern IDB (Workers): Get the IDBConnectionProxy from the Document to the WorkerGlobalScope

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Brady Eidson]

Modern IDB (Workers): Get the IDBConnectionProxy from the Document to the WorkerGlobalScope

[Brady Eidson]

Created attachment 276981 [details]
Patch v1

[WebKit Commit Bot]

Comment on attachment 276981 [details]
Patch v1

Clearing flags on attachment: 276981

Committed r199853: <http://trac.webkit.org/changeset/199853>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

(In reply to comment #2)
> Comment on attachment 276981 [details]
> Patch v1
> 
> Clearing flags on attachment: 276981
> 
> Committed r199853: <http://trac.webkit.org/changeset/199853>

It broke the Apple Mac cmake build:

/Volumes/Data/slave/elcapitan-cmake-debug/build/Source/WebCore/workers/WorkerGlobalScope.cpp:66:219: warning: unused parameter 'connectionProxy' [-Wunused-parameter]
WorkerGlobalScope::WorkerGlobalScope(const URL& url, const String& userAgent, WorkerThread& thread, bool shouldBypassMainWorldContentSecurityPolicy, PassRefPtr<SecurityOrigin> topOrigin, IDBClient::IDBConnectionProxy* connectionProxy)
                                                                                                                                                                                                                          ^
/Volumes/Data/slave/elcapitan-cmake-debug/build/Source/WebCore/workers/WorkerMessagingProxy.cpp:81:246: error: no member named 'idbConnectionProxy' in 'WebCore::Document'
    RefPtr<DedicatedWorkerThread> thread = DedicatedWorkerThread::create(scriptURL, userAgent, sourceCode, *this, *this, startMode, contentSecurityPolicyResponseHeaders, shouldBypassMainWorldContentSecurityPolicy, document.topOrigin(), document.idbConnectionProxy());
                                                                                                                                                                                                                                            ~~~~~~~~ ^
/Volumes/Data/slave/elcapitan-cmake-debug/build/Source/WebCore/workers/WorkerThread.cpp:96:411: warning: unused parameter 'connectionProxy' [-Wunused-parameter]
WorkerThread::WorkerThread(const URL& scriptURL, const String& userAgent, const String& sourceCode, WorkerLoaderProxy& workerLoaderProxy, WorkerReportingProxy& workerReportingProxy, WorkerThreadStartMode startMode, const ContentSecurityPolicyResponseHeaders& contentSecurityPolicyResponseHeaders, bool shouldBypassMainWorldContentSecurityPolicy, const SecurityOrigin* topOrigin, IDBClient::IDBConnectionProxy* connectionProxy)
                                                                                                                                                                                                                                                                                                                                                                                                                          ^
1 error generated.
make[2]: *** [Source/WebCore/CMakeFiles/WebCore.dir/workers/WorkerMessagingProxy.cpp.o] Error 1

[Csaba Osztrogonác]

(In reply to comment #3)
> All reviewed patches have been landed.  Closing bug.

And it broke the Apple Windows build too:
C:\cygwin\home\buildbot\slave\win-release\build\Source\WebCore\workers\WorkerMessagingProxy.cpp(81): error C2039: 'idbConnectionProxy': is not a member of 'WebCore::Document' [C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Source\WebCore\WebCore.vcxproj]

[Csaba Osztrogonác]

Comment on attachment 276981 [details]
Patch v1

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

> Source/WebCore/workers/WorkerMessagingProxy.cpp:81
> +    RefPtr<DedicatedWorkerThread> thread = DedicatedWorkerThread::create(scriptURL, userAgent, sourceCode, *this, *this, startMode, contentSecurityPolicyResponseHeaders, shouldBypassMainWorldContentSecurityPolicy, document.topOrigin(), document.idbConnectionProxy());

Using document.idbConnectionProxy() is incorrect here for !ENABLE(INDEXED_DATABASE) ports:
http://trac.webkit.org/browser/trunk/Source/WebCore/dom/Document.h#L659

[Brady Eidson]

(In reply to comment #4)
> (In reply to comment #2)
> > Comment on attachment 276981 [details]
> > Patch v1
> > 
> > Clearing flags on attachment: 276981
> > 
> > Committed r199853: <http://trac.webkit.org/changeset/199853>
> 
> It broke the Apple Mac cmake build:

Once keeping that build working is a directive of the project and not just the side project of one engineer, we'll have bots that will catch such a thing.(In reply to comment #5)

Since we don't even have EWS for it, it's definitely allowed to break.

> And it broke the Apple Windows build too:

This was certainly sad. I've said this about a million times, but will say it again:
"Automation is helpful" seems to be the motto of the project, but *imperfect* automation is harmful.

If we're going to rely on EWS and CQ, then CQ can't land something that an EWS would catch.

Anyways, this was fixed earlier today in http://trac.webkit.org/changeset/199885

[Csaba Osztrogonác]

(In reply to comment #7)
> (In reply to comment #4)
> > (In reply to comment #2)
> > > Comment on attachment 276981 [details]
> > > Patch v1
> > > 
> > > Clearing flags on attachment: 276981
> > > 
> > > Committed r199853: <http://trac.webkit.org/changeset/199853>
> > 
> > It broke the Apple Mac cmake build:
> 
> Once keeping that build working is a directive of the project and not just
> the side project of one engineer, we'll have bots that will catch such a
> thing.(In reply to comment #5)
> 
> Since we don't even have EWS for it, it's definitely allowed to break.

I didn't say the opposite of it. Of course, you are allowed to break 
it whenever you want and you don't have to fix it if you don't want.

I just noticed the build break to let you and Alex know that
the Apple Mac cmake is broken due to this change. 
 
> > And it broke the Apple Windows build too:
> 
> This was certainly sad. I've said this about a million times, but will say
> it again:
> "Automation is helpful" seems to be the motto of the project, but
> *imperfect* automation is harmful.
> 
> If we're going to rely on EWS and CQ, then CQ can't land something that an
> EWS would catch.

If you check the build.webkit.org waterfall, you can see that the automation
was perfect here, but an other Apple employee break the Windows build before
your patch, that's why the EWS wasn't able to build the trunk at all that time.

> Anyways, this was fixed earlier today in
> http://trac.webkit.org/changeset/199885

Thanks for the fix.

[Brady Eidson]

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #4)
> > > And it broke the Apple Windows build too:
> > 
> > This was certainly sad. I've said this about a million times, but will say
> > it again:
> > "Automation is helpful" seems to be the motto of the project, but
> > *imperfect* automation is harmful.
> > 
> > If we're going to rely on EWS and CQ, then CQ can't land something that an
> > EWS would catch.
> 
> If you check the build.webkit.org waterfall, you can see that the automation
> was perfect here, but an other Apple employee break the Windows build before
> your patch, that's why the EWS wasn't able to build the trunk at all that
> time.

The "Perfect Automation" argument is that:
1 - The automated tools have to do 100% of what we need them to do, correctly.
2 - Nobody can land anything manually with using the tools

If somebody landed something with CQ that broke the Windows build ahead of time, that's a breakdown in automation because #1 was violated.
If somebody landed something *manually* to break the Windows build ahead of time, that's a breakdown in automation because #2 was violated.

</soapbox>