[Bug 38742] [chromium] Implement canEstablishDatabase call for workers.
0 views
Skip to first unread message
bugzill...@webkit.org
May 12, 2010, 11:34:31 AM5/12/10
to webkit-bo...@googlegroups.com
https://bugs.webkit.org/show_bug.cgi?id=38742
--- Comment #26 from joc...@chromium.org 2010-05-12 08:34:29 PST ---
(In reply to comment #23)
> (In reply to comment #21)
> > Dmitry stated in Comment #7 that this was not possible
> I don't think that's correct. What he was saying is that you are never going to call this code outside of worker context, which I agree with. However, you could certainly call this code while the worker is shutting down, which will also cause controllerForContext() to return 0, I believe. Dmitry is indeed the authority here, but I'd like him to confirm that not checking for 0 is OK.
I've added the check for 0 back in. It surely won't hurt.
>
> >
> > e.g. createApplicationCacheHost should not be called either, I'd rather add ASSERT_NOT_REACHED to all methods that should not be invoked (but I cannot judge which these are), or none.
> >
> I understand your desire for consistency. If you want to change those other lines to be ASSERT_NOT_REACHED() also (and run the appropriate tests to make sure it doesn't break anything) I'm fine with that. The worker interface is confusing enough with various interfaces that are implemented but never called on certain sides of the renderer/worker divide, that I feel pretty strongly that we should be using ASSERT_NOT_REACHED() wherever possible, even if past authors haven't been rigorous in this regard.
I've added ASSERT_NOT_REACHED() to allowDatabase then
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.
--- Comment #26 from joc...@chromium.org 2010-05-12 08:34:29 PST ---
(In reply to comment #23)
> (In reply to comment #21)
> > Dmitry stated in Comment #7 that this was not possible
> I don't think that's correct. What he was saying is that you are never going to call this code outside of worker context, which I agree with. However, you could certainly call this code while the worker is shutting down, which will also cause controllerForContext() to return 0, I believe. Dmitry is indeed the authority here, but I'd like him to confirm that not checking for 0 is OK.
I've added the check for 0 back in. It surely won't hurt.
>
> >
> > e.g. createApplicationCacheHost should not be called either, I'd rather add ASSERT_NOT_REACHED to all methods that should not be invoked (but I cannot judge which these are), or none.
> >
> I understand your desire for consistency. If you want to change those other lines to be ASSERT_NOT_REACHED() also (and run the appropriate tests to make sure it doesn't break anything) I'm fine with that. The worker interface is confusing enough with various interfaces that are implemented but never called on certain sides of the renderer/worker divide, that I feel pretty strongly that we should be using ASSERT_NOT_REACHED() wherever possible, even if past authors haven't been rigorous in this regard.
I've added ASSERT_NOT_REACHED() to allowDatabase then
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.
bugzill...@webkit.org
May 13, 2010, 2:35:25 PM5/13/10
to webkit-bo...@googlegroups.com
https://bugs.webkit.org/show_bug.cgi?id=38742
--- Comment #27 from Dmitry Titov <dim...@chromium.org> 2010-05-13 11:35:22 PST ---
(In reply to comment #24)
> The traversing into and out of and back into (etc) in various processes really makes it challenging to grok the worker interfaces, especially when the same interfaces are used for very different purposes in different places. Interfaces like WebSharedWorker and WebWorker are difficult to get a handle on because they are both used by chrome to call into webkit and used by webkit to call out to chrome, and some of the methods are only applicable on one side or the other.
>
> What may be nice is if the worker interfaces where more segregated than they are. One programming interface for use in the process that creates a javascript worker object. Another for use in the process that realizes the worker thread. Could possibly help to avoid NOT_REACHED methods altogether.
This is certainly a painful issue. In Chromium setup, the number of layers, objects and interfaces that simply deliver a call from Worker object to WorkerContext and back clearly exceeds is the number of things the brain can keep simultaneously :-)
I think we will have an opportunity to re-visit this once we can run workers in the same process. Dedicated workers could be simply switched back to the renderer process and we might need a setup for shared workers that can support both in-process and IPC connections w/o much code required specifically for the latter.
In any case, patches that improve readability and simplify code are very welcome!
--- Comment #27 from Dmitry Titov <dim...@chromium.org> 2010-05-13 11:35:22 PST ---
(In reply to comment #24)
> The traversing into and out of and back into (etc) in various processes really makes it challenging to grok the worker interfaces, especially when the same interfaces are used for very different purposes in different places. Interfaces like WebSharedWorker and WebWorker are difficult to get a handle on because they are both used by chrome to call into webkit and used by webkit to call out to chrome, and some of the methods are only applicable on one side or the other.
>
> What may be nice is if the worker interfaces where more segregated than they are. One programming interface for use in the process that creates a javascript worker object. Another for use in the process that realizes the worker thread. Could possibly help to avoid NOT_REACHED methods altogether.
This is certainly a painful issue. In Chromium setup, the number of layers, objects and interfaces that simply deliver a call from Worker object to WorkerContext and back clearly exceeds is the number of things the brain can keep simultaneously :-)
I think we will have an opportunity to re-visit this once we can run workers in the same process. Dedicated workers could be simply switched back to the renderer process and we might need a setup for shared workers that can support both in-process and IPC connections w/o much code required specifically for the latter.
In any case, patches that improve readability and simplify code are very welcome!
bugzill...@webkit.org
May 13, 2010, 2:58:30 PM5/13/10
to webkit-bo...@googlegroups.com
https://bugs.webkit.org/show_bug.cgi?id=38742
Dmitry Titov <dim...@chromium.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #55815|review? |review-
Flag| |
--- Comment #28 from Dmitry Titov <dim...@chromium.org> 2010-05-13 11:58:28 PST ---
(From update of attachment 55815)
Looks great, would r+ with "change on landing" but since cq is needed, we need a perfect patch.
One small thing:
WebKit/chromium/src/WebWorkerBase.cpp:217
+ // The controller might be 0 when the worker context is in the process of shutting down.
I see the discussion in the comments about this check. It is not really needed. There is another method which returns a V8 proxy (WorkerScriptController::proxy()) which indeed returns 0 when the termination of JS in the worker is requested. The WorkerScriptController::controllerForContext() always returns a non-null controller in worker context.
See http://trac.webkit.org/changeset/56580. That change split the old function that could return 0 if either the context is not WorkerContext or the termination was requested, to be able to make these checks separately. This is why I think there is no real case when we need an "if(!controller)" here and rather could just have an assert, or even better no check at all since the very next statement is going to crash reliably anyways.
Dmitry Titov <dim...@chromium.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #55815|review? |review-
Flag| |
--- Comment #28 from Dmitry Titov <dim...@chromium.org> 2010-05-13 11:58:28 PST ---
(From update of attachment 55815)
Looks great, would r+ with "change on landing" but since cq is needed, we need a perfect patch.
One small thing:
WebKit/chromium/src/WebWorkerBase.cpp:217
+ // The controller might be 0 when the worker context is in the process of shutting down.
I see the discussion in the comments about this check. It is not really needed. There is another method which returns a V8 proxy (WorkerScriptController::proxy()) which indeed returns 0 when the termination of JS in the worker is requested. The WorkerScriptController::controllerForContext() always returns a non-null controller in worker context.
See http://trac.webkit.org/changeset/56580. That change split the old function that could return 0 if either the context is not WorkerContext or the termination was requested, to be able to make these checks separately. This is why I think there is no real case when we need an "if(!controller)" here and rather could just have an assert, or even better no check at all since the very next statement is going to crash reliably anyways.
bugzill...@webkit.org
May 13, 2010, 3:24:20 PM5/13/10
to webkit-bo...@googlegroups.com
https://bugs.webkit.org/show_bug.cgi?id=38742
--- Comment #29 from Michael Nordman <mich...@google.com> 2010-05-13 12:24:18 PST ---
--- Comment #29 from Michael Nordman <mich...@google.com> 2010-05-13 12:24:18 PST ---
> I think we will have an opportunity to re-visit this once we can run workers in the
> same process. Dedicated workers could be simply switched back to the renderer process
> and we might need a setup for shared workers that can support both in-process and IPC
> connections w/o much code required specifically for the latter.
Interesting point about the difference a multithreaded V8 could make. There would be a cost to making that switch, but it could be worth it.
> same process. Dedicated workers could be simply switched back to the renderer process
> and we might need a setup for shared workers that can support both in-process and IPC
> connections w/o much code required specifically for the latter.
bugzill...@webkit.org
May 14, 2010, 3:46:40 AM5/14/10
to webkit-bo...@googlegroups.com
https://bugs.webkit.org/show_bug.cgi?id=38742
joc...@chromium.org changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #55815|0 |1
is obsolete| |
Attachment #55815|review-, commit-queue? |
Flag| |
joc...@chromium.org changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #55815|0 |1
is obsolete| |
Attachment #55815|review-, commit-queue? |
Flag| |
bugzill...@webkit.org
May 14, 2010, 3:46:49 AM5/14/10
to webkit-bo...@googlegroups.com
https://bugs.webkit.org/show_bug.cgi?id=38742
joc...@chromium.org changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #56060| |review?
joc...@chromium.org changed:
What |Removed |Added
----------------------------------------------------------------------------
Flag| |
--- Comment #30 from joc...@chromium.org 2010-05-14 00:46:46 PST ---
Created an attachment (id=56060)
--> (https://bugs.webkit.org/attachment.cgi?id=56060)
Patch
bugzill...@webkit.org
May 14, 2010, 3:47:10 AM5/14/10
to webkit-bo...@googlegroups.com
https://bugs.webkit.org/show_bug.cgi?id=38742
joc...@chromium.org changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #56060| |commit-queue?
joc...@chromium.org changed:
What |Removed |Added
----------------------------------------------------------------------------
Flag| |
bugzill...@webkit.org
May 14, 2010, 3:48:07 AM5/14/10
to webkit-bo...@googlegroups.com
https://bugs.webkit.org/show_bug.cgi?id=38742
--- Comment #31 from joc...@chromium.org 2010-05-14 00:48:05 PST ---
(In reply to comment #28)
> (From update of attachment 55815 [details])
--- Comment #31 from joc...@chromium.org 2010-05-14 00:48:05 PST ---
(In reply to comment #28)
> (From update of attachment 55815 [details])
> Looks great, would r+ with "change on landing" but since cq is needed, we need a perfect patch.
>
> One small thing:
>
> WebKit/chromium/src/WebWorkerBase.cpp:217
> + // The controller might be 0 when the worker context is in the process of shutting down.
> I see the discussion in the comments about this check. It is not really needed. There is another method which returns a V8 proxy (WorkerScriptController::proxy()) which indeed returns 0 when the termination of JS in the worker is requested. The WorkerScriptController::controllerForContext() always returns a non-null controller in worker context.
> See http://trac.webkit.org/changeset/56580. That change split the old function that could return 0 if either the context is not WorkerContext or the termination was requested, to be able to make these checks separately. This is why I think there is no real case when we need an "if(!controller)" here and rather could just have an assert, or even better no check at all since the very next statement is going to crash reliably anyways.
hope it's perfect now :)
>
> One small thing:
>
> WebKit/chromium/src/WebWorkerBase.cpp:217
> + // The controller might be 0 when the worker context is in the process of shutting down.
> I see the discussion in the comments about this check. It is not really needed. There is another method which returns a V8 proxy (WorkerScriptController::proxy()) which indeed returns 0 when the termination of JS in the worker is requested. The WorkerScriptController::controllerForContext() always returns a non-null controller in worker context.
> See http://trac.webkit.org/changeset/56580. That change split the old function that could return 0 if either the context is not WorkerContext or the termination was requested, to be able to make these checks separately. This is why I think there is no real case when we need an "if(!controller)" here and rather could just have an assert, or even better no check at all since the very next statement is going to crash reliably anyways.
bugzill...@webkit.org
May 14, 2010, 2:33:15 PM5/14/10
to webkit-bo...@googlegroups.com
https://bugs.webkit.org/show_bug.cgi?id=38742
Dmitry Titov <dim...@chromium.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #56060|review?, commit-queue? |review+, commit-queue+
Flag| |
--- Comment #32 from Dmitry Titov <dim...@chromium.org> 2010-05-14 11:33:13 PST ---
(From update of attachment 56060)
r=me
Dmitry Titov <dim...@chromium.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #56060|review?, commit-queue? |review+, commit-queue+
Flag| |
--- Comment #32 from Dmitry Titov <dim...@chromium.org> 2010-05-14 11:33:13 PST ---
(From update of attachment 56060)
r=me
bugzill...@webkit.org
May 14, 2010, 9:02:59 PM5/14/10
to webkit-bo...@googlegroups.com
https://bugs.webkit.org/show_bug.cgi?id=38742
Eric U. <er...@chromium.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Blocks| |39145
Eric U. <er...@chromium.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Blocks| |39145
bugzill...@webkit.org
May 15, 2010, 12:23:44 PM5/15/10
to webkit-bo...@googlegroups.com
https://bugs.webkit.org/show_bug.cgi?id=38742
WebKit Commit Bot <ese...@chromium.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #56060|commit-queue+ |commit-queue-
Flag| |
--- Comment #33 from WebKit Commit Bot <ese...@chromium.org> 2010-05-15 09:23:42 PST ---
Failed to run "[u'git', u'svn', u'dcommit']" exit_code: 1
Last 500 characters of output:
WebKit/chromium/src/WebWorkerClientImpl.h
M WebKitTools/ChangeLog
M WebKitTools/DumpRenderTree/chromium/TestWebWorker.h
A repository hook failed: MERGE request failed on '/repository/webkit/trunk': Commit blocked by pre-commit hook (exit code 1) with output:
svnlook: Can't write to stream: Broken pipe
The following ChangeLog files contain OOPS:
trunk/WebKitTools/ChangeLog
Please don't ever say "OOPS" in a ChangeLog file.
at /usr/local/git/libexec/git-core/git-svn line 570
Full output: http://webkit-commit-queue.appspot.com/results/2313110
WebKit Commit Bot <ese...@chromium.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #56060|commit-queue+ |commit-queue-
Flag| |
--- Comment #33 from WebKit Commit Bot <ese...@chromium.org> 2010-05-15 09:23:42 PST ---
(From update of attachment 56060)
Rejecting patch 56060 from commit-queue.
Failed to run "[u'git', u'svn', u'dcommit']" exit_code: 1
Last 500 characters of output:
WebKit/chromium/src/WebWorkerClientImpl.h
M WebKitTools/ChangeLog
M WebKitTools/DumpRenderTree/chromium/TestWebWorker.h
A repository hook failed: MERGE request failed on '/repository/webkit/trunk': Commit blocked by pre-commit hook (exit code 1) with output:
svnlook: Can't write to stream: Broken pipe
The following ChangeLog files contain OOPS:
trunk/WebKitTools/ChangeLog
Please don't ever say "OOPS" in a ChangeLog file.
at /usr/local/git/libexec/git-core/git-svn line 570
Full output: http://webkit-commit-queue.appspot.com/results/2313110
bugzill...@webkit.org
May 15, 2010, 1:07:22 PM5/15/10
to webkit-bo...@googlegroups.com
https://bugs.webkit.org/show_bug.cgi?id=38742
--- Comment #34 from Adam Barth <aba...@webkit.org> 2010-05-15 10:07:18 PST ---
"Need a short description and bug URL (OOPS!)" was left in the second ChangeLog. I'll clean it up for you.
--- Comment #34 from Adam Barth <aba...@webkit.org> 2010-05-15 10:07:18 PST ---
"Need a short description and bug URL (OOPS!)" was left in the second ChangeLog. I'll clean it up for you.
bugzill...@webkit.org
May 15, 2010, 1:08:38 PM5/15/10
to webkit-bo...@googlegroups.com
https://bugs.webkit.org/show_bug.cgi?id=38742
Adam Barth <aba...@webkit.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #56060|0 |1
is obsolete| |
Attachment #56060|review+, commit-queue- |
Flag| |
Adam Barth <aba...@webkit.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #56060|0 |1
is obsolete| |
Attachment #56060|review+, commit-queue- |
Flag| |
bugzill...@webkit.org
May 15, 2010, 1:08:46 PM5/15/10
to webkit-bo...@googlegroups.com
https://bugs.webkit.org/show_bug.cgi?id=38742
Adam Barth <aba...@webkit.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #56158| |commit-queue+
Adam Barth <aba...@webkit.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Flag| |
--- Comment #35 from Adam Barth <aba...@webkit.org> 2010-05-15 10:08:44 PST ---
Created an attachment (id=56158)
--> (https://bugs.webkit.org/attachment.cgi?id=56158)
Patch for landing
bugzill...@webkit.org
May 15, 2010, 6:29:56 PM5/15/10
to webkit-bo...@googlegroups.com
https://bugs.webkit.org/show_bug.cgi?id=38742
WebKit Commit Bot <ese...@chromium.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #56158|commit-queue+ |
Flag| |
--- Comment #36 from WebKit Commit Bot <ese...@chromium.org> 2010-05-15 15:29:54 PST ---
(From update of attachment 56158)
Clearing flags on attachment: 56158
Committed r59555: <http://trac.webkit.org/changeset/59555>
WebKit Commit Bot <ese...@chromium.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #56158|commit-queue+ |
Flag| |
(From update of attachment 56158)
Clearing flags on attachment: 56158
Committed r59555: <http://trac.webkit.org/changeset/59555>
bugzill...@webkit.org
May 15, 2010, 6:30:04 PM5/15/10
to webkit-bo...@googlegroups.com
https://bugs.webkit.org/show_bug.cgi?id=38742
WebKit Commit Bot <ese...@chromium.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Status|UNCONFIRMED |RESOLVED
WebKit Commit Bot <ese...@chromium.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Resolution| |FIXED
--- Comment #37 from WebKit Commit Bot <ese...@chromium.org> 2010-05-15 15:30:02 PST ---
All reviewed patches have been landed. Closing bug.
Reply all
Reply to author
Forward
0 new messages