Closed Bug 922924 Opened 11 years ago Closed 11 years ago

Centralize the code for network per-app metering

Categories

(Core :: Networking, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: johnshih.bugs, Assigned: johnshih.bugs)

References

Details

(Whiteboard: [qa-])

Attachments

(1 file, 5 obsolete files)

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.
Assignee: nobody → jshih
Depends on: 746073, 855949, 887699, 855948
Blocks: 925670
Attached patch (wip) patch (obsolete) — Splinter Review
move the getActiveNetwork function to nsNetUtil.h to reduce the duplicated code
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 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 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
(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.
yeah I guess this would be better as a const static uint64_t.  Thanks Arnaud!
use const static uint64_t instead of #define
Attachment #831999 - Attachment is obsolete: true
Attachment #832700 - Flags: review+
Keywords: checkin-needed
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
(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.
fix the cross-platform issue
Attachment #832700 - Attachment is obsolete: true
Attachment #832897 - Flags: review+
Keywords: checkin-needed
add header for NS_IsMainThread (only for gonk)
Attachment #832897 - Attachment is obsolete: true
Attachment #8334347 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9ae27d0a8566
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: