Summary: | [WK2] Add support for Network Information API | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||||||||||||
Component: | WebKit2 | Assignee: | Chris Dumez <cdumez> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | andersca, cmarcelo, gyuyoung.kim, joone, kenneth, menard, rakuco, sam, vestbo, webkit.review.bot, zoltan | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Bug Depends on: | 89959 | ||||||||||||||||
Bug Blocks: | |||||||||||||||||
Attachments: |
|
Description
Chris Dumez
2012-06-25 05:31:15 PDT
Created attachment 149326 [details]
Patch
Comment on attachment 149326 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=149326&action=review > Source/WebKit2/WebProcess/NetworkInfo/WebNetworkInfoManager.cpp:75 > + unsigned int bandwidth = true; Why do you initialize *unsigned int* with boolean type ? In addition, why true ? According to specification, bandwidth returns below values. - 0 if the user is currently offline. - Infinity if the bandwidth is unknown. - an estimation of the current bandwidth in MB/s (Megabytes per seconds) available for communication with the browsing context active document's domain. > Source/WebKit2/WebProcess/NetworkInfo/WebNetworkInfoManager.cpp:82 > + bool metered = true; Basically, metered means operator limits network usage. So, I think it is good to set 'false' by default. Comment on attachment 149326 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=149326&action=review >> Source/WebKit2/WebProcess/NetworkInfo/WebNetworkInfoManager.cpp:75 >> + unsigned int bandwidth = true; > > Why do you initialize *unsigned int* with boolean type ? In addition, why true ? > > According to specification, bandwidth returns below values. > > - 0 if the user is currently offline. > - Infinity if the bandwidth is unknown. > - an estimation of the current bandwidth in MB/s (Megabytes per seconds) available for communication with the browsing context active document's domain. Bad copy/paste, thanks for catching it. >> Source/WebKit2/WebProcess/NetworkInfo/WebNetworkInfoManager.cpp:82 >> + bool metered = true; > > Basically, metered means operator limits network usage. So, I think it is good to set 'false' by default. Yes, it should be false. It is already set to false by default on Proxy side but it should be consistent. Created attachment 149458 [details]
Patch
Take Gyuyoung's feedback into consideration.
Comment on attachment 149458 [details]
Patch
Almost looks good to me.
(In reply to comment #5) > (From update of attachment 149458 [details]) > Almost looks good to me. Almost? ;) Then please tell me what is wrong so that I can make it look good to you in its entirety. (In reply to comment #6) > (In reply to comment #5) > > (From update of attachment 149458 [details] [details]) > > Almost looks good to me. > > Almost? ;) Then please tell me what is wrong so that I can make it look good to you in its entirety. As we said on irc, return type of bandwidth needs to be consistent with all places. I file a bug. Created attachment 149512 [details]
Patch
Call sendSync() on m_process->connection() instead of m_page, in WebNetworkInfoManager.cpp.
C APIs will be provided in a separate bug report / patch to decrease patch size (in case someone is wondering). Comment on attachment 149512 [details]
Patch
Clearing flags as the patch will need updating once the dependency lands.
Comment on attachment 149512 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=149512&action=review > Source/WebKit2/Shared/WebNetworkInfo.h:46 > + unsigned int bandwidth; bandwidth type is changed to double data type. This patch also needs to change type of bandwidth. Created attachment 149680 [details]
Patch
Update patch to use "double" instead of "unsigned int" for the bandwidth, now that the dependency has landed.
Comment on attachment 149680 [details]
Patch
LGTM.
Created attachment 150782 [details]
Patch
Rebase on master and add new files to GNUMakefile and Qt project.
Comment on attachment 150782 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=150782&action=review > Source/WebKit2/UIProcess/WebNetworkInfoProvider.h:45 > + bool metered(WebNetworkInfoManagerProxy*) const; why not isMetered here? > Source/WebKit2/WebProcess/NetworkInfo/WebNetworkInfoManager.cpp:77 > + // The spec indicates that we should return "infinity" if the bandwidth is unknown. > + double bandwidth = std::numeric_limits<double>::infinity(); Isnt there a smarter way to emit "Infinity" in JS? Comment on attachment 150782 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=150782&action=review >> Source/WebKit2/UIProcess/WebNetworkInfoProvider.h:45 >> + bool metered(WebNetworkInfoManagerProxy*) const; > > why not isMetered here? Right, this is better, I'll fix it. For some reason it is called metered() in WebCore. >> Source/WebKit2/WebProcess/NetworkInfo/WebNetworkInfoManager.cpp:77 >> + double bandwidth = std::numeric_limits<double>::infinity(); > > Isnt there a smarter way to emit "Infinity" in JS? I don't know of any other. I know this works though: Source/JavaScriptCore/runtime/NumberConstructor.cpp: return jsNumber(std::numeric_limits<double>::infinity()); Created attachment 150883 [details]
Patch
Take Kenneth's feedback into consideration.
Comment on attachment 150883 [details] Patch Clearing flags on attachment: 150883 Committed r121989: <http://trac.webkit.org/changeset/121989> All reviewed patches have been landed. Closing bug. |