Bug 74653 - [Chromium] Implement layoutTestController.workerThreadCount in DRT
Summary: [Chromium] Implement layoutTestController.workerThreadCount in DRT
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dmitry Lomov
URL:
Keywords:
Depends on:
Blocks: 74449
  Show dependency treegraph
 
Reported: 2011-12-15 15:18 PST by Dmitry Lomov
Modified: 2012-01-24 10:11 PST (History)
6 users (show)

See Also:


Attachments
Fix (5.26 KB, patch)
2012-01-10 16:52 PST, Dmitry Lomov
no flags Details | Formatted Diff | Diff
More tests pass now - updating expectations (5.59 KB, patch)
2012-01-10 17:00 PST, Dmitry Lomov
no flags Details | Formatted Diff | Diff
CR feedback (10.17 KB, patch)
2012-01-13 15:30 PST, Dmitry Lomov
fishd: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Rebased (10.22 KB, patch)
2012-01-23 10:57 PST, Dmitry Lomov
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dmitry Lomov 2011-12-15 15:18:28 PST
It might be either DumpRenderTree-related issue or actual bug in product
Comment 1 Dmitry Lomov 2011-12-20 17:48:15 PST
The underlying cause is that layoutTestController.workerThreadCount  is not implemented in chromium DRT
Comment 2 Dmitry Lomov 2012-01-10 16:52:37 PST
Created attachment 121941 [details]
Fix
Comment 3 David Levin 2012-01-10 16:54:38 PST
Comment on attachment 121941 [details]
Fix

Looks good to me. Needs Darin's ok of course.
Comment 4 WebKit Review Bot 2012-01-10 16:56:24 PST
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
Comment 5 Dmitry Lomov 2012-01-10 17:00:47 PST
Created attachment 121943 [details]
More tests pass now - updating expectations
Comment 6 Darin Fisher (:fishd, Google) 2012-01-11 21:23:46 PST
Comment on attachment 121943 [details]
More tests pass now - updating expectations

View in context: https://bugs.webkit.org/attachment.cgi?id=121943&action=review

> Source/WebKit/chromium/src/WebWorkerClientImpl.cpp:229
> +unsigned workerThreadCount() 

is this returning the count of dedicated worker threads?  or does this also include shared workers?

if this only returns the number of dedicated worker threads, then i'd make this a static function
on WebWorker.  WebWorker::count() would probably work.  There'd be no need to repeat the word
"worker", and no real need to say "thread" since worker implies a thread.
Comment 7 Dmitry Lomov 2012-01-11 21:55:29 PST
(In reply to comment #6)
> (From update of attachment 121943 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=121943&action=review
> 
> > Source/WebKit/chromium/src/WebWorkerClientImpl.cpp:229
> > +unsigned workerThreadCount() 
> 
> is this returning the count of dedicated worker threads?  or does this also include shared workers?

It is only dedicated workers.

> 
> if this only returns the number of dedicated worker threads, then i'd make this a static function
> on WebWorker.  WebWorker::count() would probably work.  There'd be no need to repeat the word
> "worker", and no real need to say "thread" since worker implies a thread.

Class WebWorker is going away soo (there are no real usages of it now). Point taken about Thread. How about function with one of these names: 
1. dedicatedWorkerCount 
2. webWorkerCount
3. WorkerCount.
?
Comment 8 Darin Fisher (:fishd, Google) 2012-01-12 22:04:11 PST
One idea is to put a method on WebView since all dedicated workers are associated with WebViews.

  class WebView {
  public:
      ...
      WEBKIT_EXPORT static unsigned dedicatedWorkerCount();
  };

Another idea is to introduce a new class that just exists to scope this method (and maybe other worker related methods in the future):

  class WebWorkerInfo {
  public:
      WEBKIT_EXPORT static unsigned dedicatedCount();
  };
Comment 9 Dmitry Lomov 2012-01-13 15:30:32 PST
Created attachment 122509 [details]
CR feedback
Comment 10 WebKit Review Bot 2012-01-20 22:08:22 PST
Comment on attachment 122509 [details]
CR feedback

Rejecting attachment 122509 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
s to file Source/WebKit/chromium/WebKit.gyp.rej
patching file Source/WebKit/chromium/public/WebWorkerInfo.h
patching file Source/WebKit/chromium/src/WebWorkerInfo.cpp
patching file Tools/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Tools/DumpRenderTree/chromium/LayoutTestController.cpp
patching file Tools/DumpRenderTree/chromium/LayoutTestController.h

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Darin Fisher', u'--for..." exit_code: 1

Full output: http://queues.webkit.org/results/11310334
Comment 11 Dmitry Lomov 2012-01-23 10:57:39 PST
Created attachment 123575 [details]
Rebased
Comment 12 WebKit Review Bot 2012-01-23 20:08:12 PST
Comment on attachment 123575 [details]
Rebased

Clearing flags on attachment: 123575

Committed r105684: <http://trac.webkit.org/changeset/105684>
Comment 13 WebKit Review Bot 2012-01-23 20:08:18 PST
All reviewed patches have been landed.  Closing bug.
Comment 14 Scott Graham 2012-01-24 10:11:11 PST
It looks like the rebase got messed up. The removal of WebWorker.h from WebKit.gyp here https://bugs.webkit.org/show_bug.cgi?id=76512 got reverted.