949956 - [Network Metering] Move metering related code in necko to NetStaistics.h

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[John Shih]

Based on 940740, since we need to do the same thing as bug 746073. (i.e., to dispatch every save event to main thread in order to make sure AddRef() won't be called outside it.)
As a result the code of SaveNetworkStats will be duplicated (in Http, WebSocket, Ftp).
This bug is intended to move this shared code to nsNetUtils.h.

Comment 1

[John Shih]

Attachment: Bug 949956 - Move SaveNetworkStatsEvent to nsNetUtils.h. r=jduell

In Bug 940740, we found that SaveNetworkStatsEvent is needed in Http, WebSocket, and also Ftp in order to ensure smart pointer operation runs in the main thread. As a result the code of SaveNetworkEvent can now move to nsNetUtils.h and let Http, WebSocekt, Ftp call it directly. Some redundant codes can thus be removed.

Comment 2

[John Shih]

Attachment: Bug 949956 - Move SaveNetworkStatsEvent to nsNetUtils.h. r=jduell

Comment 3

[John Shih]

try server result: https://tbpl.mozilla.org/?tree=Try&rev=b2eba0357ac3

Comment 4

[John Shih]

Comment on attachment 8360330 [details] [diff] [review]
Bug 949956 - Move SaveNetworkStatsEvent to nsNetUtils.h. r=jduell

Patrick, could you take a look if you are available? Thanks

Comment 5

[Patrick McManus [:mcmanus]]

Comment on attachment 8360330 [details] [diff] [review]
Bug 949956 - Move SaveNetworkStatsEvent to nsNetUtils.h. r=jduell

Review of attachment 8360330 [details] [diff] [review]:
-----------------------------------------------------------------

I support the general direction of this patch. Thanks!

I'd rather you made a new .h file rather than adding more junk to nsNetUtil.. maybe NetStatistics.h? Also - don't name the new class NS_SaveNetworkStatsEvent just mozilla::net::SaveNetworkStatsEvent should be fine.

Comment 6

[John Shih]

Attachment: Bug 949956: Move duplicated code to nsNetStatistics.h r=mcmanus

Patrick,
Thanks for your suggestion! The patch is updated, please take look on it when you're available.

Comment 7

[John Shih]

Attachment: Bug 949956: Move duplicated code to nsNetStatistics.h r=mcmanus

sorry for uploading a wrong patch.

Comment 8

[Patrick McManus [:mcmanus]]

Comment on attachment 8398384 [details] [diff] [review]
Bug 949956: Move duplicated code to nsNetStatistics.h r=mcmanus

Review of attachment 8398384 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/base/public/nsNetStatistics.h
@@ +3,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef nsNetStatistics_h__

name this file and guard NetStatistics.h (no ns prefix) and place it in the mozilla::net namespace. otherwise its fine.

::: netwerk/protocol/websocket/WebSocketChannel.h
@@ +20,5 @@
>  #include "BaseWebSocketChannel.h"
>  
>  #ifdef MOZ_WIDGET_GONK
>  #include "nsINetworkManager.h"
> +#include "nsProxyRelease.h"

is proxy release still needed here?

Comment 9

[John Shih]

Comment on attachment 8398384 [details] [diff] [review]
Bug 949956: Move duplicated code to nsNetStatistics.h r=mcmanus

Review of attachment 8398384 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/protocol/websocket/WebSocketChannel.h
@@ +20,5 @@
>  #include "BaseWebSocketChannel.h"
>  
>  #ifdef MOZ_WIDGET_GONK
>  #include "nsINetworkManager.h"
> +#include "nsProxyRelease.h"

Yes, it is needed for declaring nsMainThreadPtrHandle [1].

[1] http://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/websocket/WebSocketChannel.h?from=websocketchannel#272

Comment 10

[John Shih]

Attachment: Bug 949956: Move duplicated code to NetStatistics.h r=mcmanus

patch update.

Comment 11

[John Shih]

Attachment: Bug 949956: Move duplicated code to NetStatistics.h r=mcmanus

fix typo

Comment 12

[Patrick McManus [:mcmanus]]

Comment on attachment 8399864 [details] [diff] [review]
Bug 949956: Move duplicated code to NetStatistics.h r=mcmanus

Review of attachment 8399864 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm thanks. please add a simple try run to the bug before commiting.

Comment 13

[John Shih]

try result: https://tbpl.mozilla.org/?tree=Try&rev=eed252181096

There is no fails related to this bug, I think we can move on!

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a4d6df64006b

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a4d6df64006b