Last Comment Bug 673260 - HttpChannelChild::RecvAssociateApplicationCache needs to be buffered like other necko IPDL messages
: HttpChannelChild::RecvAssociateApplicationCache needs to be buffered like oth...
Status: RESOLVED FIXED
[good first bug][mentor=jdm]
:
Product: Core
Classification: Components
Component: Networking: HTTP (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla9
Assigned To: Han Chang
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-07-21 15:03 PDT by Josh Matthews [:jdm]
Modified: 2011-08-23 20:42 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Refactoring attempt #1 (4.96 KB, patch)
2011-08-22 01:18 PDT, Han Chang
jduell.mcbugs: review+
Details | Diff | Review
v2: minor fixups (4.97 KB, patch)
2011-08-22 14:15 PDT, Jason Duell [:jduell] (needinfo? me)
jduell.mcbugs: review+
Details | Diff | Review

Description Josh Matthews [:jdm] 2011-07-21 15:03:22 PDT
The body of RecvAssociateApplicationCache (http://hg.mozilla.org/mozilla-central/annotate/5d1799e16fe9/netwerk/protocol/http/HttpChannelChild.cpp#l196) needs to be moved into another function (AssociateApplicationCache) which is called from a runnable, exactly the same process as the surrounding Recv* functions in that file.
Comment 1 Han Chang 2011-08-22 01:18:02 PDT
Created attachment 554801 [details] [diff] [review]
Refactoring attempt #1

Refactored as described, compiled with success, ran the `make xpcshell-tests` in obj/netwerk/tests as well as `make check` in obj/netwerk/tests with all tests passing.

Not sure if additional tests need to be written / run though, please advise.
Comment 2 :Ms2ger 2011-08-22 01:41:23 PDT
Comment on attachment 554801 [details] [diff] [review]
Refactoring attempt #1

>+HttpChannelChild::RecvAssociateApplicationCache(const nsCString &groupID,
>+                                                const nsCString &clientID)
>+{
>+  if (mEventQ.ShouldEnqueue()) {
>+    mEventQ.Enqueue(new AssociateApplicationCacheEvent(this, groupID, clientID));
>+  }
>+  else {

Should be "} else {".
Comment 3 Jason Duell [:jduell] (needinfo? me) 2011-08-22 14:14:03 PDT
Comment on attachment 554801 [details] [diff] [review]
Refactoring attempt #1

Good stuff.

I've fixed some nits and will check in a new patch.  Here's what I changed, FYI:


>+class AssociateApplicationCacheEvent : public ChannelEvent
>+{
>+  public:
>+    AssociateApplicationCacheEvent(HttpChannelChild* child,
>+                                   const nsCString &groupID,
>+								   const nsCString &clientID)

For some reason the above line uses tabs instead of spaces--fixed and make sure it indents with other parameters.

>+    : mChild(child)
>+    , groupID(groupID)
>+    , clientID(clientID) 

minor nit:  the clientID line has an extra space at the end of the line.

>+    {}
>+
>+    void Run()
>+    {
>+      mChild->AssociateApplicationCache(groupID, clientID);
>+    }

I've been formatting these as one liners (see elsewhere in file), which is non-Mozilla-standard (and I'm not sorry!).  Might as well keep file consistent.

>+void
>+HttpChannelChild::AssociateApplicationCache(const nsCString &groupID,
>+                                            const nsCString &clientID)
>+{

Note that we don't need an AutoEventEnqueuer in this method: since we don't call into client code that might spin the event loop, we're guaranteed that regular IPDL queuing will handle serialization.

> 
>-bool
>-HttpChannelChild::RecvAssociateApplicationCache(const nsCString &groupID,
>-                                                const nsCString &clientID)
>-{

Thanks for moving this--don't know how it got stuck here in between OnStartReqEvent and RecvOnStartRequest.
Comment 4 Jason Duell [:jduell] (needinfo? me) 2011-08-22 14:15:41 PDT
Created attachment 554962 [details] [diff] [review]
v2: minor fixups
Comment 5 Jason Duell [:jduell] (needinfo? me) 2011-08-22 14:20:12 PDT
Han,

Thanks for the patch!  I've checked it into the "inbound" tree at Mozilla--if there are no errors detected by the test system, it will land in mozilla-central and make it into releases as of version 9.

  http://hg.mozilla.org/integration/mozilla-inbound/rev/c7c232adc3ec
Comment 6 Han Chang 2011-08-22 18:41:58 PDT
Crap, sorry for all the formatting problems, my .vimrc settings aren't quite compatible with Mozilla's. Is there someplace I can grab a Mozilla approved .vimrc file? Or if such a place doesn't exist, where can I create one?
Comment 7 Mounir Lamouri (:mounir) 2011-08-23 01:34:11 PDT
http://hg.mozilla.org/mozilla-central/rev/c7c232adc3ec
Comment 8 Jason Duell [:jduell] (needinfo? me) 2011-08-23 20:16:54 PDT
Han: Mozilla files generally have a modeline at the top (sometimes they only have an emacs one: we sadly have more emacs users than vi-ones).

The file you modified has a modeline already:

   /* vim: set sw=2 ts=8 et tw=80 : */

Somehow you must have overridden 'et' (expandtabs).

Not a big deal!
Comment 9 Han Chang 2011-08-23 20:42:48 PDT
Oh, that's super cool! Will read up more on modelines, should make things a lot easier. Thanks for the pointer!

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