Summary: | [SOUP] Network Cache: run the IO completion handler in the given queue instead of the whole operation | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Carlos Garcia Campos <cgarcia> | ||||||
Component: | WebKit2 | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | commit-queue, koivisto, svillar, zan | ||||||
Priority: | P2 | Keywords: | Gtk, Soup | ||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Carlos Garcia Campos
2015-06-09 05:42:57 PDT
Created attachment 254561 [details]
Patch
Attachment 254561 [details] did not pass style-queue:
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:156: Extra space before ( in function call [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:178: Extra space before ( in function call [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:265: Extra space before ( in function call [whitespace/parens] [4]
Total errors found: 3 in 2 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 254561 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=254561&action=review > Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:119 > + auto asyncData = std::unique_ptr<ReadAsyncData>(asyncDataPtr); This can be just std::unique_ptr<ReadAsyncData> asyncData(asyncDataPtr); Same for the rest of such statements. > Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:188 > + Data data; > + completionHandler(data, -1); You could probably just construct the Data object in the completionHandler call: completionHandler(Data(), -1); > Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:191 > + g_cond_wait(&condition, &mutex); Can this function be called on the main thread, with a null queue? That would schedule the callback in the main context and continue to block the main thread in g_cond_wait(), with the callback never invoked. > Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:223 > g_cond_wait(&condition, &mutex); Same problem here. (In reply to comment #3) > Comment on attachment 254561 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=254561&action=review > > > Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:119 > > + auto asyncData = std::unique_ptr<ReadAsyncData>(asyncDataPtr); > > This can be just > > std::unique_ptr<ReadAsyncData> asyncData(asyncDataPtr); > > Same for the rest of such statements. Ok, no much difference anyway. > > Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:188 > > + Data data; > > + completionHandler(data, -1); > > You could probably just construct the Data object in the completionHandler > call: > > completionHandler(Data(), -1); No, that doesn't build. no known conversion for argument 1 from ‘WebKit::NetworkCache::Data’ to ‘WebKit::NetworkCache::Data&’ > > Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:191 > > + g_cond_wait(&condition, &mutex); > > Can this function be called on the main thread, with a null queue? No this, is always called from a thread. I'll add an assert to ensure it. > That would schedule the callback in the main context and continue to block > the main thread in g_cond_wait(), with the callback never invoked. > > > Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:223 > > g_cond_wait(&condition, &mutex); > > Same problem here. Yes, we definitely don't want to block the main thread ever. Created attachment 254570 [details]
Updated patch
Attachment 254570 [details] did not pass style-queue:
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:157: Extra space before ( in function call [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:179: Extra space before ( in function call [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:268: Extra space before ( in function call [whitespace/parens] [4]
Total errors found: 3 in 2 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 254570 [details]
Updated patch
Looks good.
Committed r185365: <http://trac.webkit.org/changeset/185365> |