Closed
Bug 922924
Opened 11 years ago
Closed 11 years ago
Centralize the code for network per-app metering
Categories
(Core :: Networking, defect)
Tracking
()
RESOLVED
FIXED
mozilla28
People
(Reporter: johnshih.bugs, Assigned: johnshih.bugs)
References
Details
(Whiteboard: [qa-])
Attachments
(1 file, 5 obsolete files)
12.46 KB,
patch
|
johnshih.bugs
:
review+
|
Details | Diff | Splinter Review |
Currently, the network per-app metering is implemented on the protocol layer such as HTTP layer (746073), websocket (855949, 887699), FTP (855948, with low priority). Since the implementation in these protocols are in the similar way, we need to centralize the code in order to reduce the duplicated code in multiple places.
move the getActiveNetwork function to nsNetUtil.h to reduce the duplicated code
Comment 2•11 years ago
|
||
Comment on attachment 824570 [details] [diff] [review] (wip) patch Review of attachment 824570 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/base/public/nsNetUtil.h @@ +2378,5 @@ > + do_GetService("@mozilla.org/network/manager;1", &rv); > + > + if (NS_FAILED(rv) || !networkManager) { > + aNetworkInterface = nullptr; > + return false; might as well have this function return nsresult instead of true/false. Have it return NS_ERROR_NOT_IMPLEMENTED for the non-B2G case. Of course right now in the patch you're not checking the return value, so it doesn't matter too much :)
patch update
Attachment #824570 -
Attachment is obsolete: true
Attachment #826252 -
Flags: review?(jduell.mcbugs)
Comment 4•11 years ago
|
||
Comment on attachment 826252 [details] [diff] [review] Bug 922924 - Centralize the code for network per-app metering. Review of attachment 826252 [details] [diff] [review]: ----------------------------------------------------------------- Just a few minor things. No need to review again once you fixed them. Please do compile next patch before we land it. ::: netwerk/base/public/nsNetUtil.h @@ +2367,5 @@ > +// The following members are used for network per-app metering. > +#define NETWORK_STATS_THRESHOLD 65536 > + > +inline nsresult > +NS_GetActiveNetwork(nsCOMPtr<nsINetworkInterface> &aNetworkInterface) Rename to "NS_GetActiveNetworkInterface". @@ +2383,5 @@ > + } > + > + networkManager->GetActive(getter_AddRefs(aNetworkInterface)); > + > + return NO_OK; did you try to compile this patch? NS_OK. ::: netwerk/protocol/ftp/nsFtpConnectionThread.cpp @@ +1690,5 @@ > // initialize counter for network metering > mCountRecv = 0; > > +#ifdef MOZ_WIDGET_GONK > + // obatin active network "obtain" :) but I'm not sure you need the comment--the function name basically says it. ::: netwerk/protocol/http/nsHttpTransaction.cpp @@ +240,2 @@ > // obtain active connection type > + nsCOMPtr<nsINetworkInterface> activeNetwork; nit: might as well declare activeNetwork within the 'if' condition below. @@ +837,5 @@ > PR_Now(), 0, EmptyCString()); > } > > // we must no longer reference the connection! find out if the > + // connection was being reused before letting it g. accidentally deleted 'o' for 'go' ::: netwerk/protocol/websocket/WebSocketChannel.h @@ +16,5 @@ > #include "nsIDNSListener.h" > #include "nsIChannelEventSink.h" > #include "nsIHttpChannelInternal.h" > #include "BaseWebSocketChannel.h" > +#include "nsNetUtil.h" unless I'm mistaken, you can put the include of nsNetUtil.h in websocketchannel.cpp. Always better to put in the cpp file if you don't need it in the .h file, so other code that includes websocketchannel.h doesn't slurp in more files than needed.
Attachment #826252 -
Flags: review?(jduell.mcbugs) → review+
r=jduell refine based on Comment 4
Attachment #826252 -
Attachment is obsolete: true
Attachment #831999 -
Flags: review+
Keywords: checkin-needed
Comment 6•11 years ago
|
||
Comment on attachment 831999 [details] [diff] [review] Bug 922924 - Centralize the code for network per-app metering. Review of attachment 831999 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/base/public/nsNetUtil.h @@ +2364,5 @@ > return false; > } > > +// The following members are used for network per-app metering. > +#define NETWORK_STATS_THRESHOLD 65536 Any special reason for not keeping "const static uint64_t"? I guess this should work + allow us to keep type safety, etc.? (but maybe I missed something).
(In reply to Arnaud Bienner from comment #6) > Comment on attachment 831999 [details] [diff] [review] > Bug 922924 - Centralize the code for network per-app metering. > > Review of attachment 831999 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: netwerk/base/public/nsNetUtil.h > @@ +2364,5 @@ > > return false; > > } > > > > +// The following members are used for network per-app metering. > > +#define NETWORK_STATS_THRESHOLD 65536 > > Any special reason for not keeping "const static uint64_t"? > I guess this should work + allow us to keep type safety, etc.? (but maybe I > missed something). I just followed the style used in nsNetUtil.h to define the value, like http://dxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsNetUtil.h#1355 http://dxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsNetUtil.h#1356 http://dxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsNetUtil.h#1358 But if you really have safety concern, I can modify that. Thanks
Keywords: checkin-needed
Comment 8•11 years ago
|
||
(In reply to John Shih from comment #7) > But if you really have safety concern, I can modify that. > Thanks I was just wondering if there was a special reason. IMHO it's better to avoid #define when it's not needed and if it doesn't hurt to change this, even if it's not consistent with other declarations already written in this file. As you prefer.
Comment 9•11 years ago
|
||
yeah I guess this would be better as a const static uint64_t. Thanks Arnaud!
Assignee | ||
Comment 10•11 years ago
|
||
use const static uint64_t instead of #define
Attachment #831999 -
Attachment is obsolete: true
Attachment #832700 -
Flags: review+
Keywords: checkin-needed
Comment 11•11 years ago
|
||
https://hg.mozilla.org/integration/b2g-inbound/rev/f3c4d03ad677
Keywords: checkin-needed
Comment 12•11 years ago
|
||
Backed out for bustage on all platforms. Please confirm that this compiles at least locally before requesting checkin again. https://hg.mozilla.org/integration/b2g-inbound/rev/60a117ce0fab https://tbpl.mozilla.org/php/getParsedLog.php?id=30596598&tree=B2g-Inbound
Assignee | ||
Comment 13•11 years ago
|
||
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #12) > Backed out for bustage on all platforms. Please confirm that this compiles > at least locally before requesting checkin again. > https://hg.mozilla.org/integration/b2g-inbound/rev/60a117ce0fab > > https://tbpl.mozilla.org/php/getParsedLog.php?id=30596598&tree=B2g-Inbound Sorry, it's a cross-platform issue. I'll fix it and have a test on try-server before recheck-in.
Assignee | ||
Comment 14•11 years ago
|
||
fix the cross-platform issue
Attachment #832700 -
Attachment is obsolete: true
Attachment #832897 -
Flags: review+
Assignee | ||
Comment 15•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=c7990b742fe1
Keywords: checkin-needed
Comment 16•11 years ago
|
||
https://hg.mozilla.org/integration/b2g-inbound/rev/5d4d19ebcd70
Keywords: checkin-needed
Comment 17•11 years ago
|
||
Backed out for debug bustage. https://hg.mozilla.org/integration/b2g-inbound/rev/15fe6f678638 https://tbpl.mozilla.org/php/getParsedLog.php?id=30709696&tree=B2g-Inbound
Assignee | ||
Comment 18•11 years ago
|
||
add header for NS_IsMainThread (only for gonk)
Attachment #832897 -
Attachment is obsolete: true
Attachment #8334347 -
Flags: review+
Assignee | ||
Comment 19•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=2b31f831fb74
Keywords: checkin-needed
Comment 20•11 years ago
|
||
https://hg.mozilla.org/integration/b2g-inbound/rev/9ae27d0a8566
Keywords: checkin-needed
Comment 21•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/9ae27d0a8566
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Updated•11 years ago
|
Keywords: checkin-needed
Updated•10 years ago
|
Whiteboard: [qa-]
You need to log in
before you can comment on or make changes to this bug.
Description
•