Summary: | Add state dumping facility | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Keith Rollin <krollin> | ||||||||
Component: | WebKit2 | Assignee: | Keith Rollin <krollin> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | andersca, benjamin, cdumez, cmarcelo, commit-queue, sam, webkit-bug-importer | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 154148 | ||||||||||
Attachments: |
|
Description
Keith Rollin
2016-03-02 13:55:31 PST
Created attachment 273345 [details]
Patch
Attachment 273345 [details] did not pass style-queue:
ERROR: Source/WebKit2/WebProcess/cocoa/WebProcessCocoa.mm:217: Place brace on its own line for function definitions. [whitespace/braces] [4]
Total errors found: 1 in 7 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 273345 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=273345&action=review Check if sam or anders can do a proper review for this one. > Source/WebKit2/WebProcess/WebPage/WebPage.h:938 > + std::chrono::system_clock::time_point lastPageLoadTime() const { return m_lastPageLoadTime; } The name "lastPageLoadTime" is a bit confusing. I would prefer something like "loadCommitTime". > Source/WebKit2/WebProcess/cocoa/WebProcessCocoa.mm:232 > + for (auto& page : this->m_pageMap.values()) { do we need the explicit this-> ? > Source/WebKit2/WebProcess/cocoa/WebProcessCocoa.mm:284 > + (void) os_state_add_handler(queue, block); Do we really need this (void) ? Comment on attachment 273345 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=273345&action=review Patch looks very good overall! > Source/WebKit2/WebProcess/cocoa/WebProcessCocoa.mm:217 > + os_state_block_t block = ^(os_state_hints_t hints) { I think you could just pass the block directly to os_state_add_handler here instead of declaring it and then calling os_state_add_handler. > Source/WebKit2/WebProcess/cocoa/WebProcessCocoa.mm:229 > + auto stateDict = [[[NSMutableDictionary alloc] init] autorelease]; Instead of using autorelease here, you can use a RetainPtr. Something like auto stateDictionary = adoptNS([[NSMutabledictionary alloc] init]); > Source/WebKit2/WebProcess/cocoa/WebProcessCocoa.mm:231 > + auto pageLoadTimes = [[[NSMutableArray alloc] init] autorelease]; Same thing here. >> Source/WebKit2/WebProcess/cocoa/WebProcessCocoa.mm:232 >> + for (auto& page : this->m_pageMap.values()) { > > do we need the explicit this-> ? I don't think so. > Source/WebKit2/WebProcess/cocoa/WebProcessCocoa.mm:238 > + time_t t = std::chrono::system_clock::to_time_t(page->lastPageLoadTime()); > + NSDate* asDate = [NSDate dateWithTimeIntervalSince1970:t]; > + [pageLoadTimes addObject:asDate]; I'd just fold t into the date here. I'd call "asDate" just date. > Source/WebKit2/WebProcess/cocoa/WebProcessCocoa.mm:257 > + NSError* error; Error should be initialized to nil here (I think). > Source/WebKit2/WebProcess/cocoa/WebProcessCocoa.mm:262 > + NSData* data = [NSPropertyListSerialization > + dataWithPropertyList: stateDict > + format: NSPropertyListBinaryFormat_v1_0 > + options: 0 > + error: &error]; We don't use this Objective-C method formatting anymore; these all go on one line. > Source/WebKit2/WebProcess/cocoa/WebProcessCocoa.mm:265 > + LOG_ALWAYS_ERROR("Error serializing process state information: %{public}@", error); I don't think it's useful to log the error here - the only time we'd fail to serialize the dictionary is if it contains objects that can't be serialized. Since we control the dictionary, I think you can just ASSERT(data) here. > Source/WebKit2/WebProcess/cocoa/WebProcessCocoa.mm:275 > + // os_state->osd_decoder = ...; Is this a FIXME comment? If so, it's better to say something like: // FIXME: Fill in osd_decoder with ... Created attachment 273368 [details]
Patch
Comment on attachment 273368 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=273368&action=review > Source/WebKit2/WebProcess/cocoa/WebProcessCocoa.mm:243 > + if (pageLoadTimes.get().count) Our coding style is to not use .get().count (too many dots) - instead we use [pageLoadTimes count]. > Source/WebKit2/WebProcess/cocoa/WebProcessCocoa.mm:244 > + stateDict.get()[@"Page Load Times"] = pageLoadTimes.get(); Same thing here - I"d just use [pageLoadTimes setObject: forKey:]. > Source/WebKit2/WebProcess/cocoa/WebProcessCocoa.mm:251 > + if (!stateDict.get().count) And same thing here. Created attachment 273490 [details]
Patch
Comment on attachment 273490 [details] Patch Clearing flags on attachment: 273490 Committed r197901: <http://trac.webkit.org/changeset/197901> All reviewed patches have been landed. Closing bug. |