Last Comment Bug 757882 - remove a 200+ redundant lines from nsAHttpConnection implementations
: remove a 200+ redundant lines from nsAHttpConnection implementations
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Networking: HTTP (show other bugs)
: unspecified
: x86_64 Linux
: -- enhancement (vote)
: mozilla15
Assigned To: Patrick McManus [:mcmanus]
:
Mentors:
Depends on: 737470
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-23 09:50 PDT by Patrick McManus [:mcmanus]
Modified: 2012-05-26 05:30 PDT (History)
4 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch 0 (20.78 KB, patch)
2012-05-23 09:52 PDT, Patrick McManus [:mcmanus]
jduell.mcbugs: review+
Details | Diff | Splinter Review

Description Patrick McManus [:mcmanus] 2012-05-23 09:50:22 PDT
use macros to delete 200+ lines of code. yea!

depends on spdy3 (737470) because that patch uses some of the members impacted by this patch.
Comment 1 Patrick McManus [:mcmanus] 2012-05-23 09:52:53 PDT
Created attachment 626483 [details] [diff] [review]
patch 0
Comment 2 Jason Duell [:jduell] (needinfo me) 2012-05-25 10:56:48 PDT
Comment on attachment 626483 [details] [diff] [review]
patch 0

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

A bit gross to do this all with the preprocessor (an alternative could be to have all these classes implement some sort of "nsAHttpConnectionForwarder<class Cxn>" class, with methods that call "Cxn->ResumeSend()", etc.  But that's just an aesthetic, newfangled C++ for newfangled C++'s sake preference.  And you're piggybacking on the existing preprocessor stuff, so I guess it's fine...

::: netwerk/protocol/http/nsAHttpConnection.h
@@ +173,4 @@
>      PRUint32 CancelPipeline(nsresult originalReason);   \
> +    nsAHttpTransaction::Classifier Classification();      \
> +    /*                                                    \
> +       Thes methods below have automatic definitions that just forward the \

s/Thes/These/

@@ +192,5 @@
> +      }                                                   \
> +      return (fwdObject)->GetSecurityInfo(result);        \
> +    }                                                     \
> +    nsresult ResumeSend()                  \
> +    {                                      \

If you're going to use fixed-column position backslashes, I suggest you always go with putting them in column 80, so if a change to the function makes one line longer, you don't have to either reindent everything (and lose hg blame), or leave it looking ugly.  I believe the 80-column rule is the convention in the JS engine, FWIW.   OTOH, the rest of this macro just uses even uglier "put the slash just after the last character", so really, anything's an improvement :)
Comment 3 Patrick McManus [:mcmanus] 2012-05-25 14:55:08 PDT
(In reply to Jason Duell (:jduell) from comment #2)
> Comment on attachment 626483 [details] [diff] [review]
> patch 0
> 
> Review of attachment 626483 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> A bit gross to do this all with the preprocessor (an alternative could be to

Man, I really thought you were the perfect audience for a remove code patch :) !

Anyhow I put those back slashes all in line. They won't hurt anybody now.

Thanks!
Comment 4 Patrick McManus [:mcmanus] 2012-05-25 15:03:38 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/2cfcbe649545
Comment 5 Ryan VanderMeulen [:RyanVM] 2012-05-26 05:30:36 PDT
https://hg.mozilla.org/mozilla-central/rev/2cfcbe649545

Note You need to log in before you can comment on or make changes to this bug.