Bug 27824 – [commit+] [chromium] fix v8 binding problem when locallyEntangledPort returns NULL

• Component: Platform
• Status: RESOLVED FIXED

[John Abd-El-Malek]

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.

[John Abd-El-Malek]

Created attachment 33757 [details]
Proposed patch

[Eric Seidel (no email)]

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!)

[John Abd-El-Malek]

(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.

[David Levin]

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.

[David Levin]

Comment on attachment 33757 [details]
Proposed patch

Detailed comments about the remaining portions.

> Index: WebCore/ChangeLog
> [phone]  John Abd-El-Malek  <[e-mail]>
> +
> +        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".

[John Abd-El-Malek]

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.

[John Abd-El-Malek]

Created attachment 33811 [details]
new patch

[John Abd-El-Malek]

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)

[Adam Barth]

Comment on attachment 33811 [details]
new patch

Thanks John.

[Adam Barth]

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

[Adam Barth]

All reviewed patches have been landed.  Closing bug.

[John Abd-El-Malek]

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

Thanks!