Bug 27824 - [commit+] [chromium] fix v8 binding problem when locallyEntangledPort returns NULL
: [commit+] [chromium] fix v8 binding problem when locallyEntangledPort returns...
Status: RESOLVED FIXED
: WebKit
Platform
: 528+ (Nightly build)
: PC All
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2009-07-29 18:04 PST by
Modified: 2009-08-03 06:26 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2009-07-29 18:04:47 PST
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 From 2009-07-29 18:08:10 PST -------
Created an attachment (id=33757) [details]
Proposed patch
------- Comment #2 From 2009-07-29 18:36:29 PST -------
(From update of attachment 33757 [details])
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 From 2009-07-29 19:26:42 PST -------
(In reply to comment #2)
> (From update of attachment 33757 [details] [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 From 2009-07-30 09:28:48 PST -------
(From update of attachment 33757 [details])

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 From 2009-07-30 10:00:48 PST -------
(From update of attachment 33757 [details])
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 From 2009-07-30 13:04:46 PST -------
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 From 2009-07-30 13:05:34 PST -------
Created an attachment (id=33811) [details]
new patch
------- Comment #8 From 2009-07-30 13:07:42 PST -------
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 From 2009-07-30 13:15:57 PST -------
(From update of attachment 33811 [details])
Thanks John.
------- Comment #10 From 2009-08-01 00:37:23 PST -------
(From update of attachment 33811 [details])
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 From 2009-08-01 00:37:29 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #12 From 2009-08-03 06:26:39 PST -------
(In reply to comment #11)
> All reviewed patches have been landed.  Closing bug.

Thanks!