Summary: | [commit+] [chromium] fix v8 binding problem when locallyEntangledPort returns NULL | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | John Abd-El-Malek <jam> | ||||||
Component: | Platform | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | abarth, atwilson, dglazkov, fishd, levin | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | PC | ||||||||
OS: | All | ||||||||
Attachments: |
|
Description
John Abd-El-Malek
2009-07-29 18:04:47 PDT
Created attachment 33757 [details]
Proposed patch
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!)
(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 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 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". 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. Created attachment 33811 [details]
new patch
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 on attachment 33811 [details]
new patch
Thanks John.
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 All reviewed patches have been landed. Closing bug. (In reply to comment #11) > All reviewed patches have been landed. Closing bug. Thanks! |