Bug 70953 (crbug.com/101665) - Chromium ui_tests WorkerTest.WorkerMessagePort[GC] were broken by https://bugs.webkit.org/attachment.cgi?id=112342
Summary: Chromium ui_tests WorkerTest.WorkerMessagePort[GC] were broken by https://bug...
Status: RESOLVED FIXED
Alias: crbug.com/101665
Product: WebKit
Classification: Unclassified
Component: WebKit API (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL: http://crbug.com/101665
Keywords:
Depends on:
Blocks:
 
Reported: 2011-10-26 11:44 PDT by Dave Michael
Modified: 2011-10-27 15:40 PDT (History)
3 users (show)

See Also:


Attachments
Patch (1.52 KB, patch)
2011-10-26 11:53 PDT, Dave Michael
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dave Michael 2011-10-26 11:44:04 PDT
In http://trac.webkit.org/changeset/98381, I changed PlatformMessagePortChannel::hasPendingActivity thusly:
 bool PlatformMessagePortChannel::hasPendingActivity()
 {
     MutexLocker lock(m_mutex);
-    return m_localPort;
+    return m_localPort && m_localPort->hasPendingActivity();
 }

I suspect the new code is 'right' (in that I think it provides the originally _intended_ behavior), but existing code elsewhere must depend on the previous behavior.

I'm planning to undo this part of CS 98381, and hoping that somebody else more familiar with existing PlatformMessagePortChannels can fix the problem with this function and its caller(s) (if there is a problem).
Comment 1 Dave Michael 2011-10-26 11:53:52 PDT
Created attachment 112572 [details]
Patch
Comment 2 Dave Michael 2011-10-26 11:55:45 PDT
I verified locally that these ui tests pass with this change.
Comment 3 David Levin 2011-10-26 11:55:52 PDT
Comment on attachment 112572 [details]
Patch

hmm. I missed that change somehow. It is best not to do unrelated changes in your patch and it is also good to explain why a change was necessary in the changelog where the function is listed.

I should have mentioned this in your previous change.
Comment 4 David Levin 2011-10-26 11:57:27 PDT
Please re-enable the disabled tests where this gets into chromium.

(Also for changes of this sort, there should be some test to verify that the change is correct and stays in place, but that was for the last patch.)
Comment 5 WebKit Review Bot 2011-10-27 07:03:47 PDT
Comment on attachment 112572 [details]
Patch

Clearing flags on attachment: 112572

Committed r98566: <http://trac.webkit.org/changeset/98566>
Comment 6 WebKit Review Bot 2011-10-27 07:03:51 PDT
All reviewed patches have been landed.  Closing bug.