See bug 1312782 comment #5. We have to store the top outer windowID in nsHttpConnectionMgr.

From bug 1312782 comment#0: > 1. notification of an active tab change or navigation start (probably from > [2], ideally should happen solely on the parent process) ; each channel has > "root load group id" assigned [3], the notification should tell us root load > group id of the active tab > In the case there are many subdocuments in the active tab, only the root load group id is not enough. IIRC, a subdocument's load group id is not the same as the top level document's load group id. I think maybe we should store the top level window ID in nsIRequestContext so that we can know whether transactions are coming from the active tab or not by comparing the window ID in nsHttpConnectionMgr.

Honza, Could you perhaps take a look at this patch? Do you think merely storing top level window ID is enough for our purpose? BTW, the patch to update the current top level window ID will be happened in bug 1309653.

(In reply to Kershaw Chang [:kershaw] from comment #2) > Created attachment 8825278 [details] [diff] [review] > Store top level window ID in nsHttpConnectionMgr and nsHttpTransaction > > Honza, > > Could you perhaps take a look at this patch? > Do you think merely storing top level window ID is enough for our purpose? I think this what we want. > > BTW, the patch to update the current top level window ID will be happened in > bug 1309653. Good!

Comment on attachment 8825278 [details] [diff] [review] Store top level window ID in nsHttpConnectionMgr and nsHttpTransaction Review of attachment 8825278 [details] [diff] [review]: ----------------------------------------------------------------- Thanks! ::: netwerk/protocol/http/HttpBaseChannel.cpp @@ +3659,5 @@ > + nsCOMPtr<mozIDOMWindowProxy> topWindow; > + loadContext->GetTopWindow(getter_AddRefs(topWindow)); > + nsCOMPtr<nsIDOMWindowUtils> windowUtils = do_GetInterface(topWindow); > + if (windowUtils) { > + windowUtils->GetOuterWindowID(&mTopLevelOuterWindowId); I think Boris should double check this (just to be sure we are doing the right thing). ::: netwerk/protocol/http/HttpChannelParent.cpp @@ +380,5 @@ > > // Set the channelId allocated in child to the parent instance > mChannel->SetChannelId(aChannelId); > mChannel->SetTopLevelContentWindowId(aContentWindowId); > + mChannel->SetTopLevelOuterWindowId(aTopLevelOuterWindowId); maybe TopLevel*Chrome*WindowId? Or just TabId? Not sure...

(In reply to Honza Bambas (:mayhemer) from comment #3) > I think this what we want. I think this *IS* what we want :)

(In reply to Honza Bambas (:mayhemer) from comment #4) > Comment on attachment 8825278 [details] [diff] [review] > Store top level window ID in nsHttpConnectionMgr and nsHttpTransaction > > Review of attachment 8825278 [details] [diff] [review]: > ----------------------------------------------------------------- > > Thanks! > > ::: netwerk/protocol/http/HttpBaseChannel.cpp > @@ +3659,5 @@ > > + nsCOMPtr<mozIDOMWindowProxy> topWindow; > > + loadContext->GetTopWindow(getter_AddRefs(topWindow)); > > + nsCOMPtr<nsIDOMWindowUtils> windowUtils = do_GetInterface(topWindow); > > + if (windowUtils) { > > + windowUtils->GetOuterWindowID(&mTopLevelOuterWindowId); > > I think Boris should double check this (just to be sure we are doing the > right thing). > @bz, could you take a quick look at this patch? We may need your advice. We would like to know whether the Http requests are coming from the active tab or not, so my idea is to store the top level outer content window id in the channel. Not sure if this is a right approach. Thanks. > ::: netwerk/protocol/http/HttpChannelParent.cpp > @@ +380,5 @@ > > > > // Set the channelId allocated in child to the parent instance > > mChannel->SetChannelId(aChannelId); > > mChannel->SetTopLevelContentWindowId(aContentWindowId); > > + mChannel->SetTopLevelOuterWindowId(aTopLevelOuterWindowId); > > maybe TopLevel*Chrome*WindowId? Or just TabId? Not sure... IIRC, in content process, we are not able to get the chrome window id, since the chrome window should live in parent.

So just to be clear... Do you care about the active/inactive state at some point later than asyncOpen, or just at the point when asyncOpen happens? If you need it at some later point, then storing the toplevel outer window id sounds right. If you just care about it up front, you could just grab the active boolean then. One other side note: there's no need to get windowutils involved. If you have a mozIDOMWindowProxy* topWindow, you should be able to do: mTopLevelOuterWindowId = nsPIDOMWindowOuter::From(topWindow)->WindowId();

Thanks for your comment. (In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #7) > So just to be clear... Do you care about the active/inactive state at some > point later than asyncOpen, or just at the point when asyncOpen happens? > The active state later than asyncOpen is exactly what I care about. The usage of the top level outer window id stored in http transaction is for comparing with the current active top level window id in nsHttpConnectionMgr. If they are the same, we want to serve those http transactions more quickly. > If you need it at some later point, then storing the toplevel outer window > id sounds right. If you just care about it up front, you could just grab > the active boolean then. > > One other side note: there's no need to get windowutils involved. If you > have a mozIDOMWindowProxy* topWindow, you should be able to do: > > mTopLevelOuterWindowId = nsPIDOMWindowOuter::From(topWindow)->WindowId(); Good catch! I'll fix this in my next patch.

Summary of change: Get top level window id as bz suggested. > > ::: netwerk/protocol/http/HttpChannelParent.cpp > @@ +380,5 @@ > > > > // Set the channelId allocated in child to the parent instance > > mChannel->SetChannelId(aChannelId); > > mChannel->SetTopLevelContentWindowId(aContentWindowId); > > + mChannel->SetTopLevelOuterWindowId(aTopLevelOuterWindowId); > > maybe TopLevel*Chrome*WindowId? Or just TabId? Not sure... Actually, the full name should be TopLevelOuter"Content"WindowId, but I am not sure if I should use this name. I think this is too long. What do you think?

(In reply to Kershaw Chang [:kershaw] from comment #9) > Created attachment 8842397 [details] [diff] [review] > Store top level window ID in nsHttpConnectionMgr and nsHttpTransaction - v2 > > Summary of change: > Get top level window id as bz suggested. Also add |TopLevelOuterWindowId| in nsAHttpTransaction and NullHttpTransaction. > > > > > ::: netwerk/protocol/http/HttpChannelParent.cpp > > @@ +380,5 @@ > > > > > > // Set the channelId allocated in child to the parent instance > > > mChannel->SetChannelId(aChannelId); > > > mChannel->SetTopLevelContentWindowId(aContentWindowId); > > > + mChannel->SetTopLevelOuterWindowId(aTopLevelOuterWindowId); > > > > maybe TopLevel*Chrome*WindowId? Or just TabId? Not sure... > > Actually, the full name should be TopLevelOuter"Content"WindowId, but I am > not sure if I should use this name. I think this is too long. What do you > think?

(In reply to Kershaw Chang [:kershaw] from comment #9) > Actually, the full name should be TopLevelOuter"Content"WindowId, but I am > not sure if I should use this name. I think this is too long. What do you > think? Yep, the better description of the function the better. Feel free to use the "full" long name.

Comment on attachment 8842397 [details] [diff] [review] Store top level window ID in nsHttpConnectionMgr and nsHttpTransaction - v2 Review of attachment 8842397 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/protocol/http/nsAHttpTransaction.h @@ +226,5 @@ > + > + virtual uint64_t TopLevelOuterWindowId() { > + MOZ_ASSERT(false); > + return 0; > + } does this belong to here or to bug 1312782?

(In reply to Honza Bambas (:mayhemer) from comment #11) > (In reply to Kershaw Chang [:kershaw] from comment #9) > > Actually, the full name should be TopLevelOuter"Content"WindowId, but I am > > not sure if I should use this name. I think this is too long. What do you > > think? > > Yep, the better description of the function the better. Feel free to use > the "full" long name. Thanks. I'll cancel the review and update the patch. (In reply to Honza Bambas (:mayhemer) from comment #12) > Comment on attachment 8842397 [details] [diff] [review] > Store top level window ID in nsHttpConnectionMgr and nsHttpTransaction - v2 > > Review of attachment 8842397 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: netwerk/protocol/http/nsAHttpTransaction.h > @@ +226,5 @@ > > + > > + virtual uint64_t TopLevelOuterWindowId() { > > + MOZ_ASSERT(false); > > + return 0; > > + } > > does this belong to here or to bug 1312782? Not sure... But it seems this should belong to bug 1312782, because the related discussion are not in this bug. I'll also move this part away.

Summary: - Use the long full name: "TopLevelOuterContentWindowId"

Comment on attachment 8842822 [details] [diff] [review] Store top level content window ID in nsHttpConnectionMgr and nsHttpTransaction - v3 Review of attachment 8842822 [details] [diff] [review]: ----------------------------------------------------------------- lgtm, thanks. ::: netwerk/protocol/http/HttpBaseChannel.cpp @@ +3815,5 @@ > > void > +HttpBaseChannel::EnsureTopLevelOuterContentWindowId() > +{ > + if (!mTopLevelOuterContentWindowId) { having the style as: if (mTopLevel...) { return; } if (!loadContext) { return; } etc.. is generally more preferred when possible. More for next time, feel free to leave it as is. ::: netwerk/protocol/http/nsHttpConnectionMgr.cpp @@ +2792,5 @@ > + MOZ_ASSERT(PR_GetCurrentThread() == gSocketThread); > + mCurrentTopLevelOuterContentWindowId = > + static_cast<UINT64Wrapper*>(param)->GetValue(); > + LOG(("nsHttpConnectionMgr::OnMsgUpdateCurrentTopLevelOuterContentWindowId" > + " id=%llu\n", don't we need the new fancy PR* stuff here? ::: netwerk/protocol/http/nsHttpTransaction.h @@ +87,5 @@ > nsIEventTarget *consumerTarget, > nsIInterfaceRequestor *callbacks, > nsITransportEventSink *eventsink, > + nsIAsyncInputStream **responseBody, > + uint64_t topLevelOuterContentWindowId); nit: please let the |out| param - here responseBody - be the last one.

Thanks for the review. (In reply to Honza Bambas (:mayhemer) from comment #15) > Comment on attachment 8842822 [details] [diff] [review] > Store top level content window ID in nsHttpConnectionMgr and > nsHttpTransaction - v3 > > Review of attachment 8842822 [details] [diff] [review]: > ----------------------------------------------------------------- > > lgtm, thanks. > > ::: netwerk/protocol/http/HttpBaseChannel.cpp > @@ +3815,5 @@ > > > > void > > +HttpBaseChannel::EnsureTopLevelOuterContentWindowId() > > +{ > > + if (!mTopLevelOuterContentWindowId) { > > having the style as: > > if (mTopLevel...) { > return; > } > > if (!loadContext) { > return; > } > > etc.. is generally more preferred when possible. More for next time, feel > free to leave it as is. > I see. I'll update it. > ::: netwerk/protocol/http/nsHttpConnectionMgr.cpp > @@ +2792,5 @@ > > + MOZ_ASSERT(PR_GetCurrentThread() == gSocketThread); > > + mCurrentTopLevelOuterContentWindowId = > > + static_cast<UINT64Wrapper*>(param)->GetValue(); > > + LOG(("nsHttpConnectionMgr::OnMsgUpdateCurrentTopLevelOuterContentWindowId" > > + " id=%llu\n", > > don't we need the new fancy PR* stuff here? > Yes. This will be changed. > ::: netwerk/protocol/http/nsHttpTransaction.h > @@ +87,5 @@ > > nsIEventTarget *consumerTarget, > > nsIInterfaceRequestor *callbacks, > > nsITransportEventSink *eventsink, > > + nsIAsyncInputStream **responseBody, > > + uint64_t topLevelOuterContentWindowId); > > nit: please let the |out| param - here responseBody - be the last one. Got it.

Summary: - Carry reviewer's name. - Fix review comments. https://treeherder.mozilla.org/#/jobs?repo=try&revision=3731b9402fa0a153396f2998ed06be99530da6c0&selectedJob=82663197

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/379ac27cbef7 Store top level outer content window id in http transaction and connMgr. r=mayhemer