Summary: | Add WKContext API to retrieve basic network process statistics | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Brady Eidson <beidson> | ||||||||
Component: | WebKit2 | Assignee: | Brady Eidson <beidson> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | ossy, sam, webkit-ews, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Attachments: |
|
Description
Brady Eidson
2013-02-08 15:47:07 PST
Created attachment 187374 [details]
Patch v1
Attachment 187374 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/NetworkProcess/HostRecord.cpp', u'Source/WebKit2/NetworkProcess/HostRecord.h', u'Source/WebKit2/NetworkProcess/NetworkProcess.cpp', u'Source/WebKit2/NetworkProcess/NetworkProcess.h', u'Source/WebKit2/NetworkProcess/NetworkProcess.messages.in', u'Source/WebKit2/NetworkProcess/NetworkResourceLoadScheduler.cpp', u'Source/WebKit2/NetworkProcess/NetworkResourceLoadScheduler.h', u'Source/WebKit2/Shared/Authentication/AuthenticationManager.h', u'Source/WebKit2/Shared/Downloads/DownloadManager.h', u'Source/WebKit2/UIProcess/API/C/WKContext.cpp', u'Source/WebKit2/UIProcess/API/C/WKContext.h', u'Source/WebKit2/UIProcess/WebContext.cpp', u'Source/WebKit2/UIProcess/WebContext.h', u'Source/WebKit2/UIProcess/WebContext.messages.in']" exit_code: 1
Source/WebKit2/NetworkProcess/NetworkProcess.cpp:233: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Total errors found: 1 in 15 files
If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #2) > Attachment 187374 [details] did not pass style-queue: > > Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', > Source/WebKit2/NetworkProcess/NetworkProcess.cpp:233: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] > Total errors found: 1 in 15 files > Weird... totally seems like 4 spaces locally. I deleted them and re-inserted them to make sure. Comment on attachment 187374 [details] Patch v1 Attachment 187374 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/16465021 Created attachment 187385 [details]
Patch v2 - Use LoaderStrategy
Maybe fixed the bizarre style error, definitely fixed the qt compile error.
Attachment 187385 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/NetworkProcess/HostRecord.cpp', u'Source/WebKit2/NetworkProcess/HostRecord.h', u'Source/WebKit2/NetworkProcess/NetworkProcess.cpp', u'Source/WebKit2/NetworkProcess/NetworkProcess.h', u'Source/WebKit2/NetworkProcess/NetworkProcess.messages.in', u'Source/WebKit2/NetworkProcess/NetworkResourceLoadScheduler.cpp', u'Source/WebKit2/NetworkProcess/NetworkResourceLoadScheduler.h', u'Source/WebKit2/Shared/Authentication/AuthenticationManager.h', u'Source/WebKit2/Shared/Downloads/DownloadManager.h', u'Source/WebKit2/UIProcess/API/C/WKContext.cpp', u'Source/WebKit2/UIProcess/API/C/WKContext.h', u'Source/WebKit2/UIProcess/WebContext.cpp', u'Source/WebKit2/UIProcess/WebContext.h', u'Source/WebKit2/UIProcess/WebContext.messages.in']" exit_code: 1
Source/WebKit2/NetworkProcess/NetworkProcess.cpp:233: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Total errors found: 1 in 15 files
If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #6) > Attachment 187385 [details] did not pass style-queue: > Source/WebKit2/NetworkProcess/NetworkProcess.cpp:233: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] > Total errors found: 1 in 15 files Aha, got it. Fixed locally. Comment on attachment 187385 [details] Patch v2 - Use LoaderStrategy View in context: https://bugs.webkit.org/attachment.cgi?id=187385&action=review > Source/WebKit2/NetworkProcess/NetworkResourceLoadScheduler.cpp:180 > + uint64_t count = m_nonHTTPProtocolHost->pendingRequestCount() ? 1 : 0; Extra space after the =. This code is also not very self explanatory. Why are you doing the ? 1 : 0 part? > Source/WebKit2/NetworkProcess/NetworkResourceLoadScheduler.cpp:193 > + uint64_t count = m_nonHTTPProtocolHost->pendingRequestCount(); Extra space after the =. > Source/WebKit2/NetworkProcess/NetworkResourceLoadScheduler.cpp:204 > + uint64_t count = m_nonHTTPProtocolHost->activeLoadCount() ? 1 : 0; Extra space after the =. > Source/WebKit2/NetworkProcess/NetworkResourceLoadScheduler.cpp:217 > + uint64_t count = m_nonHTTPProtocolHost->activeLoadCount(); Extra space after the =. > Source/WebKit2/Shared/Authentication/AuthenticationManager.h:66 > + uint64_t outstandingAuthenticationChallengeCount() const { return m_challenges.size(); } What makes them so outstanding? ;) > Source/WebKit2/UIProcess/API/C/WKContext.h:191 > +WK_EXPORT void WKContextGetNetworkingStatistics(WKContextRef context, void* functionContext, WKContextGetStatisticsFunction function); Why did you make the decision to use a new function, rather than just add the new stats to WKContextGetStatistics's dictionary? (In reply to comment #8) > (From update of attachment 187385 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=187385&action=review > > > Source/WebKit2/NetworkProcess/NetworkResourceLoadScheduler.cpp:180 > > + uint64_t count = m_nonHTTPProtocolHost->pendingRequestCount() ? 1 : 0; > > Extra space after the =. This code is also not very self explanatory. Why are you doing the ? 1 : 0 part? > If the host has any pending requests, then the total count of hosts with pending requests gets incremented. I'll make it more clear. I'll also fix all the bizarre extra spaces. > > Source/WebKit2/UIProcess/API/C/WKContext.h:191 > > +WK_EXPORT void WKContextGetNetworkingStatistics(WKContextRef context, void* functionContext, WKContextGetStatisticsFunction function); > > Why did you make the decision to use a new function, rather than just add the new stats to WKContextGetStatistics's dictionary? We can chat on IRC or you can look at the other patch related for this to see why it made sense to have networking stats be in a separate call. An alternative solution would be to change WKContextGetStatistics to take a mask of which statistics are wanted, but it's already shipped API... (In reply to comment #9) > (In reply to comment #8) > > > Source/WebKit2/UIProcess/API/C/WKContext.h:191 > > > +WK_EXPORT void WKContextGetNetworkingStatistics(WKContextRef context, void* functionContext, WKContextGetStatisticsFunction function); > > > > Why did you make the decision to use a new function, rather than just add the new stats to WKContextGetStatistics's dictionary? > > We can chat on IRC or you can look at the other patch related for this to see why it made sense to have networking stats be in a separate call. > > An alternative solution would be to change WKContextGetStatistics to take a mask of which statistics are wanted, but it's already shipped API... Well, you could always just add WKContextGetStatisticsWithOptions(), but this seems fine. Created attachment 187909 [details]
Patch v3 - Based on Sam's feedback
A bit more complicated, this adds WKContextGetStatisticsWithOptions that allows for gathering stats from multiple processes at once.
It is both a better API going forward, and will help separately resolve the "get statistics from multiple WebProcesses" bug.
Comment on attachment 187909 [details] Patch v3 - Based on Sam's feedback View in context: https://bugs.webkit.org/attachment.cgi?id=187909&action=review > Source/WebKit2/UIProcess/API/C/WKContext.h:150 > +enum { > + kWKStatisticsTypeWebContent = 0x00000001, > + kWKStatisticsTypeNetworking = 0x00000002 > +}; > +typedef uint32_t WKStatisticsType; Please use 0 << 1 and such. > Source/WebKit2/UIProcess/API/C/WKContext.h:197 > +WK_EXPORT void WKContextGetStatisticsWithOptions(WKContextRef context, WKStatisticsType statisticsMask, void* functionContext, WKContextGetStatisticsFunction function); This doesn't take options. Please rename. Comment on attachment 187385 [details] Patch v2 - Use LoaderStrategy Cleared Sam Weinig's review+ from obsolete attachment 187385 [details] so that this bug does not appear in http://webkit.org/pending-commit. It was already landed in http://trac.webkit.org/changeset/142651 |