1274162 - Fix -Wshadow warnings in netwerk/protocol/http/ directory

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Chris Peterson [:cpeterson]]

Attachment: Wshadow_http.patch

I hope to soon enable clang and gcc's -Wshadow warnings in bug 1272513. This patch fixes all the -Wshadow warnings in the netwerk/protocol/http/ directory. The nsHttpHeaderArray.h header file's warning is #included in other source directories, preventing those directories from enabling -Wshadow warnings.

netwerk/protocol/http/nsHttpHeaderArray.h:96:63 [-Wshadow] declaration shadows a field of 'mozilla::net::nsHttpHeaderArray::nsEntry'
netwerk/protocol/http/Http2Session.cpp:2766:14 [-Wshadow] declaration shadows a local variable
netwerk/protocol/http/HttpBaseChannel.cpp:1886:14 [-Wshadow] declaration shadows a local variable
netwerk/protocol/http/HttpChannelChild.cpp:265:53 [-Wshadow] declaration shadows a field of 'mozilla::net::AssociateApplicationCacheEvent'
netwerk/protocol/http/HttpChannelChild.cpp:266:53 [-Wshadow] declaration shadows a field of 'mozilla::net::AssociateApplicationCacheEvent'
netwerk/protocol/http/HttpChannelChild.cpp:2566:14 [-Wshadow] declaration shadows a local variable
netwerk/protocol/http/HttpChannelParent.cpp:397:17 [-Wshadow] declaration shadows a local variable
netwerk/protocol/http/InterceptedChannel.cpp:276:14 [-Wshadow] declaration shadows a local variable
netwerk/protocol/http/InterceptedChannel.cpp:285:14 [-Wshadow] declaration shadows a local variable
netwerk/protocol/http/SpdySession31.cpp:2177:14 [-Wshadow] declaration shadows a local variable
netwerk/protocol/http/nsCORSListenerProxy.cpp:304:30 [-Wshadow] declaration shadows a local variable
netwerk/protocol/http/nsHttpChannel.cpp:796:17 [-Wshadow] declaration shadows a local variable
netwerk/protocol/http/nsHttpChannel.cpp:4474:35 [-Wshadow] declaration shadows a local variable
netwerk/protocol/http/nsHttpChannel.cpp:5915:18 [-Wshadow] declaration shadows a local variable
netwerk/protocol/http/nsHttpConnection.cpp:982:21 [-Wshadow] declaration shadows a local variable
netwerk/protocol/http/nsHttpConnectionMgr.cpp:1220:43 [-Wshadow] declaration shadows a local variable
netwerk/protocol/http/nsHttpConnectionMgr.cpp:1240:43 [-Wshadow] declaration shadows a local variable
netwerk/protocol/http/nsHttpConnectionMgr.cpp:2247:27 [-Wshadow] declaration shadows a local variable
netwerk/protocol/http/nsHttpConnectionMgr.cpp:2758:23 [-Wshadow] declaration shadows a local variable
netwerk/protocol/http/nsHttpPipeline.cpp:709:30 [-Wshadow] declaration shadows a local variable

Comment 1

[Chris Peterson [:cpeterson]]

Comment on attachment 8754236 [details] [diff] [review]
Wshadow_http.patch

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

Some notes on name changes:

::: netwerk/protocol/http/Http2Session.cpp
@@ -2762,5 @@
>  
>    if (mDownstreamState == DISCARDING_DATA_FRAME ||
>        mDownstreamState == DISCARDING_DATA_FRAME_PADDING) {
>      char trash[4096];
> -    uint32_t count = std::min(4096U, mInputFrameDataSize - mInputFrameDataRead);

The name `count` is used in another local variable above.

::: netwerk/protocol/http/Http2Session.h
@@ -75,5 @@
>    |                     Frame Data (0...)                       ...
>    +---------------------------------------------------------------+
>  */
>  
> -  enum frameType {

Some variables are named `frameType`, shadowing this type name. The `FrameType` enum type name is not actually referenced anywhere else.

::: netwerk/protocol/http/HttpBaseChannel.cpp
@@ -1911,5 @@
>      if (!util) {
>        return NS_ERROR_NOT_AVAILABLE;
>      }
>      nsCOMPtr<mozIDOMWindowProxy> win;
> -    nsresult rv = util->GetTopWindowForChannel(this, getter_AddRefs(win));

This `rv` definition shadows another `rv` variable, the actual return value from this function. Thus an error returned from GetTopWindowForChannel() will be masked and GetTopWindowURI() will silently return NS_OK.

Was that the intention?

@@ -1917,5 @@
>        rv = util->GetURIFromWindow(win, getter_AddRefs(mTopWindowURI));
>  #if DEBUG
>        if (mTopWindowURI) {
>          nsCString spec;
> -        rv = mTopWindowURI->GetSpec(spec);

If we want GetTopWindowURI() to return errors from GetTopWindowForChannel(), then we shouldn't overwrite `rv` with the return value from GetSpec().

::: netwerk/protocol/http/nsCORSListenerProxy.cpp
@@ -268,5 @@
>      NS_WARNING("Invalid cache key!");
>      return nullptr;
>    }
>  
> -  CacheEntry* entry;

The name `entry` is used as a loop variable below. Giving this `entry` variable a more descriptive name seems more useful and reduces the number of lines of code changed.

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ -4516,5 @@
>          responseHead->GetHeader(nsHttp::Vary, buf);
>          if (!buf.IsEmpty()) {
>              NS_NAMED_LITERAL_CSTRING(prefix, "request-");
>  
> -            char *val = buf.BeginWriting(); // going to munge buf

The name `val` is used to parse HTTP headers inside the while loop below. The name `val` makes more sense in that context, so I instead renamed this pointer to the internal buffer of nsAutoCString buf.

Comment 2

[Patrick McManus [:mcmanus]]

Comment on attachment 8754236 [details] [diff] [review]
Wshadow_http.patch

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

I appreciate the 'pre-review' comments inline. very helpful style.

will I get -Wshadow in normal clang builds or will this need to rely on the CI to keep it up to date?

::: netwerk/protocol/http/HttpBaseChannel.cpp
@@ -1911,5 @@
>      if (!util) {
>        return NS_ERROR_NOT_AVAILABLE;
>      }
>      nsCOMPtr<mozIDOMWindowProxy> win;
> -    nsresult rv = util->GetTopWindowForChannel(this, getter_AddRefs(win));

your change is right and fixes a bug. thanks.

::: netwerk/protocol/http/HttpChannelChild.cpp
@@ -264,5 @@
> -    AssociateApplicationCacheEvent(HttpChannelChild* child,
> -                                   const nsCString &groupID,
> -                                   const nsCString &clientID)
> -    : mChild(child)
> -    , groupID(groupID)

wow. wouldn't have thought that would compile.

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ -1229,5 @@
>  
>      // We need to make a new connection. If that is going to exceed the
>      // global connection limit then try and free up some room by closing
>      // an idle connection to another host. We know it won't select "ent"
> -    // beacuse we have already determined there are no idle connections

and free typo fixes too! thanks.

Comment 3

[Chris Peterson [:cpeterson]]

(In reply to Patrick McManus [:mcmanus] from comment #2)
> will I get -Wshadow in normal clang builds or will this need to rely on the
> CI to keep it up to date?

The -Wshadow warnings will be reported on all clang and gcc builds: OS X, Linux, and Android.

Comment 4

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4cd4c9499c47

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4cd4c9499c47