Bug 27824 - [commit+] [chromium] fix v8 binding problem when locallyEntangledPort returns NULL
: [commit+] [chromium] fix v8 binding problem when locallyEntangledPort returns...
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: Platform
: 528+ (Nightly build)
: PC All
: P2 Normal
Assigned To: Nobody
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-07-29 18:04 PDT by John Abd-El-Malek
Modified: 2009-08-03 06:26 PDT (History)
5 users (show)

See Also:


Attachments
Proposed patch (6.74 KB, patch)
2009-07-29 18:08 PDT, John Abd-El-Malek
levin: review-
Details | Formatted Diff | Diff
new patch (1.77 KB, patch)
2009-07-30 13:05 PDT, John Abd-El-Malek
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description John Abd-El-Malek 2009-07-29 18:04:47 PDT
Chromium will need to use a different implementation of PlatforMessagePortChannel to support message ports being in different processes.  This is the WebKit side.  Please do NOT check this in until the Chromium submission is ready.  I'll work with Darin or Dmitry to time it right.
Comment 1 John Abd-El-Malek 2009-07-29 18:08:10 PDT
Created attachment 33757 [details]
Proposed patch
Comment 2 Eric Seidel 2009-07-29 18:36:29 PDT
Comment on attachment 33757 [details]
Proposed patch

Your changelog doesn't make clear what you're doing here.  Are you just re-indenting code?

Why no tests?
+        No new tests. (OOPS!)
Comment 3 John Abd-El-Malek 2009-07-29 19:26:42 PDT
(In reply to comment #2)
> (From update of attachment 33757 [details])
> Your changelog doesn't make clear what you're doing here.  Are you just
> re-indenting code?

The first section is just re-indenting code.
> 
> Why no tests?
> +        No new tests. (OOPS!)

The existing layout tests cover this.  However as our multi-process implementation lives in Chromium's repository, the tests will run under ui_tests.
Comment 4 David Levin 2009-07-30 09:28:48 PDT
Comment on attachment 33757 [details]
Proposed patch


The general consensus in WebKit land seems to be that pure style/formatting changes should be separated from other changes, so while I think the indentation change in WebCore/bindings/v8/V8GCController.cpp is good, it should be a separate patch.
Comment 5 David Levin 2009-07-30 10:00:48 PDT
Comment on attachment 33757 [details]
Proposed patch

Detailed comments about the remaining portions.

> Index: WebCore/ChangeLog
> +2009-07-29  John Abd-El-Malek  <jam@chromium.org>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        V8 binding and GYP changes for the Chromium port's multi process message port implementation 
> +
> +        https://bugs.webkit.org/show_bug.cgi?id=27824
> +

As Eric indicated, "Your changelog doesn't make clear what you're doing here."

There should be something here that explains what you are doing.


> +        No new tests. (OOPS!)

   > > Why no tests?
   > > +        No new tests. (OOPS!)
  > The existing layout tests cover this.  

Please change the line "No new tests. (OOPS!)" to say this.


> Index: WebCore/bindings/v8/V8GCController.cpp
> +        }
> +
> +        // Set back the weak bit that we cleared in GCPrologueVisitor.

This comment says what is being done (which I can tell from the code) but not why it is being done.

It took me a while but I think I figured out why. Is doing this so that the port will be attempted to 
be garbage collected again in the next round because the condition "if (port1->isEntangled() && !port2)" may change?

And without making this weak, the garbage collection of port is never done?


> +        if (type == V8ClassIndex::MESSAGEPORT) {
> +            MessagePort* port1 = static_cast<MessagePort*>(object);
> +            MessagePort* port2 = port1->locallyEntangledPort();
> +            if (port1->isEntangled() && !port2)
> +                wrapper.MakeWeak(port1, &DOMDataStore::weakActiveDOMObjectCallback);

PS If you don't want anyone to commit your patch, simply assign the bug to yourself, which I'll do for you now.  When you're ready for landing, coordinate with someone to land the patch. In general, people only automatically land patches assigned to "nodoby".
Comment 6 John Abd-El-Malek 2009-07-30 13:04:46 PDT
I found out I don't need to update the gypi file, since it should contain all the files and I can add an exclusion to the gyp file.

As for the code style changes, I think this is a little inconsistent with other patches that go through which fix minor style violations along the way.  The discussion on webkit-dev was related to big style fixups, which this one isn't.

Anyways, I've attached an updated version with no style fixup.
Comment 7 John Abd-El-Malek 2009-07-30 13:05:34 PDT
Created attachment 33811 [details]
new patch
Comment 8 John Abd-El-Malek 2009-07-30 13:07:42 PDT
note, this patch removes the two-sided change, so it can be committed anytime (sooner better than later, as the tests on the chrome side fail without it)
Comment 9 Adam Barth 2009-07-30 13:15:57 PDT
Comment on attachment 33811 [details]
new patch

Thanks John.
Comment 10 Adam Barth 2009-08-01 00:37:23 PDT
Comment on attachment 33811 [details]
new patch

Clearing review flag on attachment: 33811

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/bindings/v8/V8GCController.cpp
Committed r46666
	M	WebCore/ChangeLog
	M	WebCore/bindings/v8/V8GCController.cpp
r46666 = d575beb0b79d6a07653bad20c671069a6daba56b (trunk)
No changes between current HEAD and refs/remotes/trunk
Resetting to the latest refs/remotes/trunk
http://trac.webkit.org/changeset/46666
Comment 11 Adam Barth 2009-08-01 00:37:29 PDT
All reviewed patches have been landed.  Closing bug.
Comment 12 John Abd-El-Malek 2009-08-03 06:26:39 PDT
(In reply to comment #11)
> All reviewed patches have been landed.  Closing bug.

Thanks!