1158264 - Show ServiceWorkers intercepted network requests in the netmonitor panel

Status: RESOLVED FIXED

(DevTools :: Netmonitor, defect)

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Are we sure they don't show up already?  I seemed to get something in the network panel when I looked a month or so ago.

Comment 2

[Andrea Marchesini [:baku]]

From the network panel we don't have any way to see what is controlled by a ServiceWorker and what's not. This is a very important feature for debugging.

Comment 3

[Andrea Marchesini [:baku]]

Attachment: patch 1

With this patch we show something in the devtools network panel. It doesn't work fully because of bug 1158533.
Plus would be nice to show that the request has been intercepted by a ServiceWorker.

Comment 4

[Josh Matthews [:jdm]]

Where do these notifications come from for non-intercepted channels? This feels like the wrong place for them to live.

Comment 5

[Andrea Marchesini [:baku]]

nsHttpTransaction is what we use for non-intercepted channels.
This is created by SetupTransaction() in nsHttpChannel but, when we read from cache we go into:

https://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpChannel.cpp#400

and we do not create the nsHttpTransaction, we don't register it, and so we don't notify the observers.

I can change how nsHttpChannel works when we retrieve data fully from the cache, but with this patch we have also timing stats for what happens between FetchEvent and the loading of the body.

Comment 6

[Josh Matthews [:jdm]]

Comment on attachment 8597635 [details] [diff] [review]
patch 1

I'm not a fan of this patch for a couple reasons:
* I don't think the FetchEvent/ServiceWorkerManager code should need to be concerned about these notifications
* All of the activity distributor stuff feels like we're duplicating a bunch of private implementation from netwerk/ into dom/
* It's pretty verbose, and the FetchEvent-related code is already a bit sprawling

My counter-proposal is that we move all of this (the activity distributor, the notifications, etc.) into InterceptedChannel.cpp and add any APIs necessary for FetchEvent-related code to announce that certain things have occurred. That doesn't solve the duplication problem, but it does contain it within netwerk/, at least.

Comment 7

[Andrea Marchesini [:baku]]

makes sense.

Comment 8

[Andrea Marchesini [:baku]]

bug 764958 already fixed this issue. nice!

Comment 9

[Josh Matthews [:jdm]]

Is that true for e10s as well? We don't use the cache in child processes.

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Also, it seems like maybe we should show "ServiceWorker" or something instead.  The ServiceWorker could have synthesized the response instead of pulling from Cache API.

Comment 11

[Josh Matthews [:jdm]]

It wasn't clear this morning that this is actually something that's been tested yet.

Comment 12

[Noemí Freire (:noemi)]

Hi,

I've just checked the information shown under "Transferred" field within Network tab to see from which cache the content has been served. Please find below my findings.


*Being e10s enabled
when content is served from Offline cache nothing appears under "Transferred" field within Network tab. It does, when the content is served from network or local cache. STRs:
1- load http://jaoo.github.io/service-worker-testing/index.html (being "Enable Service Workers over HTTP" flag and also Service Workers(prefs) enabled)
2- register a service worker
3- re-load the page (contents appear to be served from cache - Transferred field is showing it)
4- go off-line
5- re-load the page. The contents are shown but there is no information under "Transferred" field within Network tab

*Being e10s disabled, I'm not seeing any information under "Transferred" field, even when the contents are served from network or local cache (neither when the contents are served from offline cache)

Because of this, re-opening this bug. Thanks!

Comment 13

[Andrea Marchesini [:baku]]

I'm not working on this. I want to speak with somebody from the devtools team for a proper fix.

Comment 14

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(In reply to Andrea Marchesini (:baku) from comment #13)
> I'm not working on this. I want to speak with somebody from the devtools
> team for a proper fix.

I'd really like to talk more about this - perhaps in Whistler next week or if you aren't attending perhaps email your questions to the devtools list: https://lists.mozilla.org/listinfo/dev-developer-tools

Comment 15

[Alexandre Poirot [:ochameau]]

:baku, I would like to see that move forward again.
I'm wondering what information you were expecting from devtools?

I just started looking at this story and would like to understand why bug 764958, about cache support, isn't enough? Couldn't we pass along some useful metadata to/from netwerk/ to identify service worker things? I imagine no.
I will first try to figure out why we only see SW request in non-e10s tabs.

Comment 16

[Alexandre Poirot [:ochameau]]

Ok, so the reasone why devtools don't show service worker requests on e10s (even as "cached"),
is that http-on-examine-cached-response is fired in the parent process, but with missing information.
This isn't the case in non-e10s tabs.
We are missing the nsILoadContext.topFrameElement
http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsILoadContext.idl#46
We retrieve the nsILoadContext interface like this:
  request.notificationCallbacks.getInterface(Ci.nsILoadContext);
`request` being the nsIHttpChannel passed as subject of the http-on-examine-cached-response notification.

We could tweak devtools, but we need a way to identify the request as coming from the given tab.
We could listen to some events in the child process if that can help, or continue listening to some in the parent, but we would need to expose or identify existing field that help us filter the request as coming from the debugged tab.

Comment 17

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Alexandre Poirot [:ochameau] from comment #16)
> Ok, so the reasone why devtools don't show service worker requests on e10s
> (even as "cached"),
> is that http-on-examine-cached-response is fired in the parent process, but
> with missing information.

This surprises me.  When a channel is intercepted by a service worker we never even create the parent actor.  If devtools expects the parent side of necko to fire events, then that would explain why nothing shows up.  I just don't understand why you see an event firing.

For this particular e10s issue, I think maybe it would be good to wait to do anything.  At Mozlando we are planning to re-design our SW e10s code and our current thoughts might fix this particular issue.  We are thinking about doing interception in the parent process which would probably automatically solve this.

It would still be nice to know why we don't get any markers in the network panel at all for non-e10s, though.  Maybe thats a separate bug.

Thanks for investigating!

Comment 18

[Alexandre Poirot [:ochameau]]

Yes, I was wrong, I spent some more time the head in gdb and figured it out.
I got confused by some requests being reported in the parent for the worker script itself,
but they isn't fetch requests.

Hum, regarding non-e10s, it seems to work for me? Do you miss some requests?
They are labeled as "cached", but they do show up.

Handling everything in the parent may help. But I don't want to wait for a refactoring to have devtools.
Service workers are here. I see most developers using chrome and that makes me sad.
So if we can figure out something somewhat easy in the meantime, let's go for it.
But if you do refactor, we have to plan devtools from day one and have them working while you make progress on it!! It shouldn't be too hard, it is mostly matter of having the right event and piping things together.

Comment 19

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Alexandre Poirot [:ochameau] from comment #18)
> Hum, regarding non-e10s, it seems to work for me? Do you miss some requests?
> They are labeled as "cached", but they do show up.

I get entries that show cached, but nothing in the bar chart on the right of the panel.  It seems we should at least get start/complete markers for the operation.

> Handling everything in the parent may help. But I don't want to wait for a
> refactoring to have devtools.

I think fixing this would require adding new IPC methods.  We've added a ton of complexity trying to do interception in e10s with our current design.  I'm a bit hesitant to add even more commplexity when we are about to rip it out.

> But if you do refactor, we have to plan devtools from day one and have them
> working while you make progress on it!! It shouldn't be too hard, it is
> mostly matter of having the right event and piping things together.

I believe our plan will make it so anything that works in non-e10s will work in e10s as well.  So fixing the non-e10s case (which is actually shipping in release channels) would help our long term e10s plans as well.

Josh, do you have any ideas on how to do something short term thats not too complex here?

Comment 20

[Alexandre Poirot [:ochameau]]

(In reply to Ben Kelly [:bkelly] from comment #19)
> (In reply to Alexandre Poirot [:ochameau] from comment #18)
> > Hum, regarding non-e10s, it seems to work for me? Do you miss some requests?
> > They are labeled as "cached", but they do show up.
> 
> I get entries that show cached, but nothing in the bar chart on the right of
> the panel.  It seems we should at least get start/complete markers for the
> operation.

Ah ok, yes, that makes sense. We need http activity event for that.
But it doesn't sound as important as having request to show up on e10s/b2g.
Also, do these markers make any sense when you hook a request via service workers?!
If they do, it looks like these markers will be very specific to how service workers handle intercepted channel and so it sounds like it would make sense to fires some http activity events (or something similar) directory from service workers. And so, live with something that looks like code duplication with netwerk/

> > Handling everything in the parent may help. But I don't want to wait for a
> > refactoring to have devtools.
> 
> I think fixing this would require adding new IPC methods.  We've added a ton
> of complexity trying to do interception in e10s with our current design. 
> I'm a bit hesitant to add even more commplexity when we are about to rip it
> out.

Hey I'm not saying we should support existing implementation at all cost,
but we should try to figure it out. I'm convinced we could do something somewhat easily.

> Josh, do you have any ideas on how to do something short term thats not too
> complex here?

I do have a patch to at least hand over the nsIHttpChannel to devtools in e10s.
That patch looks somewhat sane and simple.
I do have another patch to pass along some service worker metadata on the channel,
but that one looks worse.

Comment 21

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Alexandre Poirot [:ochameau] from comment #20)
> (In reply to Ben Kelly [:bkelly] from comment #19)
> > I get entries that show cached, but nothing in the bar chart on the right of
> > the panel.  It seems we should at least get start/complete markers for the
> > operation.
> 
> Ah ok, yes, that makes sense. We need http activity event for that.
> But it doesn't sound as important as having request to show up on e10s/b2g.

Its definitely more important than b2g right now (IMO).  I also think its more important than e10s because we're not shipping that on release channels yet.

> Also, do these markers make any sense when you hook a request via service
> workers?!

Surely start and stop markers make sense.  It would be nice to see how the time is divided up, but that could come later.

> I do have a patch to at least hand over the nsIHttpChannel to devtools in
> e10s.
> That patch looks somewhat sane and simple.
> I do have another patch to pass along some service worker metadata on the
> channel,
> but that one looks worse.


If you have a sane way to handle this in devtools, that would be great.  I just don't want to further complicate the already complex necko channel implementations.

Comment 22

[Alexandre Poirot [:ochameau]]

Attachment: patches

Here is various tweaks, that are not enough, but it feels close.

1) Receive a reference of the intercepted channel in devtools

"service-worker-request" observer service notification.
It looks gross but it works great!
On non-e10s, we end up calling InterceptedChannelChrome::FinishSynthesizedResponse
which calls nsHttpChannel::ContinueConnect() 
which ends up firing a http-on-examine-cached-response notification with the channel.
On e10s, we don't receive anything, we call InterceptedChannelContent::FinishSynthesizedResponse
which doesn't seem to emit anything devtools could catch.

 > I just put an observer service notification in an arbitrary location, that looks gross,
   I send it from FinishResponse::Run(), after the call to InterceptedChannel::FinishSynthesizedResponse
 > May be we could fire one or two http activity event(s) at some smart locations

2) Identify request coming from service workers

nsIHttpChannelInternal.serviceWorkerSpec. I'm not really proud of that,
but I verified, and I correctly get that field set on channels received in devtools.
That would allow saying more than "cached" and explicitely say it comes from a service worker
and which one.

 > There might be better host interface for such info?
 > Better info itself, is the script spec the best on only ID for a SW?

3) Relates the requests to a given tab

 3a) Service worker script requests
 
 No matter if we have e10s or not, requests made to the worker script miss some metadata
 to be able to relate the request to a given tab.
   
   > Given that service worker are not necessarely related to a precise given tab,
     it may be a dead end here??

 3b) Service worker intercepted channels on e10s

 On e10s, the channels I forward from FinishResponse::Run(),
 I still have issue with its loadContext being empty.
   channel.notificationCallbacks.getInterface(Ci.nsILoadContext).topFrameElement
 is undefined and disallow me to relate the request to a given tab from devtools codebase.

   > It feels different than worker script. The request should be initiated by a precise document?
     It looks like we just miss piping some info on the context?

4) The rest of the patch is mostly tweaking devtools codebase

I tweaked the devtools codebase to also listen in the child process,
for the "service-worker-request" notification.
I haven't updated it to use "serviceWorkerSpec", but I should.

Comment 23

[Alexandre Poirot [:ochameau]]

ping, any update after orlando meetings?

Comment 24

[Ben Kelly [:bkelly, not reviewing]]

We didn't discuss this bug specifically at orlando, as far as I know.

Our plan, though, is to try to fix critical e10s issues in a short term way for 45 and then pursue our long term strategy.  In the long term interception will occur in the parent process, so devtools should see roughly the same behavior for intercepted and non-intercepted in e10s mode.  Modulo actually testing how it works, etc.

Comment 25

[Alexandre Poirot [:ochameau]]

Great.

What about also fixing network support in devtools for 45 with tweaks like the one in comment 22?
I would need feedback sooner than later.

Comment 26

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8695328 [details] [diff] [review]
patches

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

::: dom/workers/ServiceWorkerEvents.cpp
@@ +211,5 @@
>      NS_WARN_IF_FALSE(NS_SUCCEEDED(rv), "Failed to finish synthesized response");
> +
> +    nsCOMPtr<nsIObserverService> obsService = services::GetObserverService();
> +    if (obsService)
> +      obsService->NotifyObservers(underlyingChannel, "service-worker-request", nullptr);

nit: Enclosing curly braces.

Also, maybe calling the topic something more like "service-worker-synthesized-response" would be better.  It would be more consistent with terminology used elsewhere.

::: dom/workers/ServiceWorkerPrivate.cpp
@@ +1108,5 @@
>        mRequestMode = InternalRequest::MapChannelToRequestMode(channel);
>  
> +      // Pass some information to identify the request has been
> +      // through a service worker
> +      internalChannel->SetServiceWorkerSpec(mScriptSpec);

So, are you trying to capture if a request was looked at by a service worker or if a service worker actually synthesized the response?

This part of the code is just for dispatching the fetch event, but the service worker can ignore it.  It may not actually synthesize a response.

Also, I think it might be a bit nicer to actually set the ServiceWorkerInfo here instead of the script spec.  That way devtools could potentially show more information about the service worker.

Comment 27

[Alexandre Poirot [:ochameau]]

(In reply to Ben Kelly [:bkelly] from comment #26)
> Comment on attachment 8695328 [details] [diff] [review]
> 
> ::: dom/workers/ServiceWorkerPrivate.cpp
> @@ +1108,5 @@
> >        mRequestMode = InternalRequest::MapChannelToRequestMode(channel);
> >  
> > +      // Pass some information to identify the request has been
> > +      // through a service worker
> > +      internalChannel->SetServiceWorkerSpec(mScriptSpec);
> 
> So, are you trying to capture if a request was looked at by a service worker
> or if a service worker actually synthesized the response?
> 
> This part of the code is just for dispatching the fetch event, but the
> service worker can ignore it.  It may not actually synthesize a response.

Wait, do you think it is going to flag request as being processed by a Service worker, even if it has been ignored by it? That's not would I would expect.
But isn't that a plain new nsIHttpChannel instance here, that would just be completely dropped if SW ignores it?
I imagine that the request will timeout or something alike if the SW recent a fetch event and do not call respondWith. Isn't it what happen?

> Also, I think it might be a bit nicer to actually set the ServiceWorkerInfo
> here instead of the script spec.  That way devtools could potentially show
> more information about the service worker.

Good idea, I can give that a try if that's reasonably easy to pass such object around.

Comment 28

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Alexandre Poirot [:ochameau] from comment #27)
> Wait, do you think it is going to flag request as being processed by a
> Service worker, even if it has been ignored by it? That's not would I would
> expect.

I agree thats not good behavior.  I was just trying to understand why you wanted to flag the channel as being associated with the service worker even if the service worker ignored it.

> But isn't that a plain new nsIHttpChannel instance here, that would just be
> completely dropped if SW ignores it?
> I imagine that the request will timeout or something alike if the SW recent
> a fetch event and do not call respondWith. Isn't it what happen?

If a service worker ignores the fetch event the nsIChannel loads from the network just like it normally does.

> > Also, I think it might be a bit nicer to actually set the ServiceWorkerInfo
> > here instead of the script spec.  That way devtools could potentially show
> > more information about the service worker.
> 
> Good idea, I can give that a try if that's reasonably easy to pass such
> object around.

It should work.  We already have an nsIServiceWorkerInfo interface that about:debugging uses.

Comment 29

[Alexandre Poirot [:ochameau]]

If setting the ServiceWorkerSpec in FetchEventEunnable::Init ends up being a bad spot,
I can try to do it from FinishResponse::Run, where I dispatch the observer service notification.
But I only have access to the ScriptSrc here, not the ServiceWorkerInfo.

Thanks for your help!

Comment 30

[Ben Kelly [:bkelly, not reviewing]]

I think we have a couple options:

1) Set it in FetchEventRunnable::Init() and make it clear that it was just the SW that was consulted, but does not imply interception.  You need the observer event to signal that.
2) Or set it in FetchEventRunnable::Init() and then remove it if interception does not actually take place.

Comment 31

[Alexandre Poirot [:ochameau]]

Attachment: Pass ServiceWorkerInfo* instead of just ScriptSrc in event classes - v1

Ok. Here is some clean patches.

A first one to pass along ServiceWorkerInfo instead of ScriptSrc string.

Comment 32

[Alexandre Poirot [:ochameau]]

Attachment: Set ServiceWorkerInfo on nsIHttpInternalChannel when a request is intercepted by a service worker - v1

Now that we have ServiceWorkerInfo, I can set it in the internal channel.
I'm still wondering if that's the right thing to do...
Who would review such patch on http channels?

Comment 33

[Alexandre Poirot [:ochameau]]

Attachment: Send an observer service event when a service worker intercept a request - v1

And also send the message for e10s codepath.

Comment 34

[Alexandre Poirot [:ochameau]]

Attachment: devtools patch

And the devtools patch that still need some tweaks to display service worker infos,
but at least the requests are displayed on e10s/non-e10s.

Comment 35

[Alexandre Poirot [:ochameau]]

Ok. So. Updated status. I've been confused in comment 22.

Everything looks good for synthesized request e10s/non-e10s.
Open questions are:

 - is nsIHttpInternalChannel.serviceWorkerInfo relevant?
   Is there a better host interface for this information?
   Should we even set this information? Would it be better to just pass along the ServiceWorkerInfo via the event (observer service notification?

 - is the observer service notification relevant?
   Shouldn't I use nsIHttpActivityObserver instead?

 - we still miss requests being made to the service worker *scrit* itself.
   I get notified about requests to it, but the load context is empty.
   No topFrameElement, no associatedWindow.
   The request appears in the browser toolbox network inspector (which inspect everything that happens in firefox), but we can't make it appear in the tab toolbox as we have no way to make the link between the two.

Comment 36

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8699468 [details] [diff] [review]
Pass ServiceWorkerInfo* instead of just ScriptSrc in event classes - v1

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

::: dom/workers/ServiceWorkerEvents.h
@@ +110,5 @@
>    uint32_t mPreventDefaultLineNumber;
>    uint32_t mPreventDefaultColumnNumber;
>    bool mIsReload;
>    bool mWaitToRespond;
> +  ServiceWorkerInfo *mInfo;

The weak reference here does not look safe.  Unfortunately I don't think we can just use RefPtr<ServiceWorkerInfo> because the object is not threadsafe.

Can you use nsMainThreadPtrHandle<ServiceWorkerInfo>?

Comment 37

[Ben Kelly [:bkelly, not reviewing]]

Actually, can you pass the uint64_t serviceWorkerID instead:

  https://dxr.mozilla.org/mozilla-central/source/dom/workers/ServiceWorkerManager.h?from=ServiceWorkerInfo#202

Then in devtools you could get the registration for the page and compare the id to the .active worker.

Also, I think it would be best to just pass it in the observer event if possible.  That would be better than adding more cruft to nsIHttpChannelInternal.

I don't really know what nsIHttpActivityObserver is, but it sounds like it might be more appropriate.  Does it work?

Comment 38

[Ben Kelly [:bkelly, not reviewing]]

I think you would have to expose the id as a readonly attribute on the nsIServiceWorkerInfo interface as well:

  https://dxr.mozilla.org/mozilla-central/source/dom/interfaces/base/nsIServiceWorkerManager.idl#27

Comment 39

[Josh Matthews [:jdm]]

Seems like my input isn't required right now.

Comment 40

[Alexandre Poirot [:ochameau]]

I'll scope this bug down to:
 - display intercepted requests no matter if e10s is turned on or not
  (today, we don't see anything in e10s)
 - explicitely flag requests as being over a service worker
  (today, on non-e10s it appears as 'cached')

What I'm removing is:
 - be able to associate a given request to a given service worker.
It looks like Chrome doesn't expose that in their devtools. I'm not sure it is very important to know from which JS file a request has been crafted. It would just be cool to have.

So in term of code we just need:
 - a notification observer in order to see requests with e10s turned on
 - tweaks in devtools to flag service worker requests

Then we can followup with cool things, if we think it is useful.

Comment 41

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=470da57d1102

Comment 42

[Alexandre Poirot [:ochameau]]

Attachment: Send an observer service event when a service worker intercept a request - v2

Rebased to just land that for now.
We just need this to be able to fix e10s.
On non-e10s, it will also help us distinguish cached requests from service worker ones.

Ben, Does that sound reasonable to proceed with just this?
Then, we can followup to see if there is value in identifying which service worker is used,
and how to implement that in our c++.

Comment 43

[Alexandre Poirot [:ochameau]]

Attachment: Flag requests coming from service workers in devtools - v1

First concrete devtools patch without debug, with l10n and tests.
It just need the previous platform patch to flag all SW requests correctly
in e10s as well as non-e10s.

Comment 44

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8703776 [details] [diff] [review]
Send an observer service event when a service worker intercept a request - v2

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

Nice and minimal.  I agree this is a good place to start.  Thanks!  r=me

Comment 45

[Alexandre Poirot [:ochameau]]

Comment on attachment 8703778 [details] [diff] [review]
Flag requests coming from service workers in devtools - v1

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

Ryan, I feel like I'm spamming you and Brian with reviews, feel free to redirect to whoever makes sense!

This important piece of this patch lives in network-monitor.js,
where I'm listening for the new observer service notification I introduced in the other platform patch.
This notification is fired in both e10s/non-e10s and allows to identify requests going through a SW.
It also allows to workaround the fact that there is no 'http-on-examine-cached-response' notification
for SW requests on e10s (nor another other kind of event).
Then there is some tweaks in webconsole.js to setup a NetworkMonitor instance also from the child process (where this new notification is sent).
The rest is a lot of piping in order to propagate the "fromServiceworker" field. I just looked for all "fromCache" usages and duplicate with a fromServiceWorker field.
netmonitor-view.js is the final place where we actually display request as coming from a SW.
The most visible change is in the Transfered column, we display "service worker", like we do for cache (we display "cached" there).
There is also mention of SW on the status icon tooltip.

Comment 46

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(In reply to Alexandre Poirot [:ochameau] from comment #40)
...
> What I'm removing is:
>  - be able to associate a given request to a given service worker.
> It looks like Chrome doesn't expose that in their devtools. I'm not sure it
> is very important to know from which JS file a request has been crafted. It
> would just be cool to have.

We don't have source js files for any network requests, and I consider it to be a huge gap between us and chrome / safari. If an xhr is made from the page or some related file like a service worker, I'd love to be able to know the file name / line # of the code that made the request. 

Can you log a follow up for this? I don't think it should block shipping but I think it should get fixed.

Comment 47

[Alexandre Poirot [:ochameau]]

I'm not sure chrome / safari does it for service worker?
So I would rather start with XHR, which are a very different story. I imagine we already have a bug for this? Or do we only have a "break on xhr" one?

But yes, I'll file a followup specific to SW. I already have most of the work done. At least for saying which SW. (i.e. I don't have the line, and not necessarely the right file if the SW uses import)

Comment 48

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8703778 [details] [diff] [review]
Flag requests coming from service workers in devtools - v1

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

:victorporof or :jsantell are probably a better fit, let's try :victorporof.

Comment 49

[Alexandre Poirot [:ochameau]]

Victor, if you don't have time to review, could you forward this to someone who does?

Comment 50

[Victor Porof [:vporof][:vp]]

Comment on attachment 8703778 [details] [diff] [review]
Flag requests coming from service workers in devtools - v1

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

Not perfectly comfortable reviewing the backend changes, although they LGTM.

::: devtools/client/netmonitor/netmonitor-view.js
@@ +539,5 @@
>     * @param boolean aFromCache
>     *        Indicates if the result came from the browser cache
>     */
> +  addRequest: function(aId, aStartedDateTime, aMethod, aUrl, aIsXHR, aFromCache, aFromServiceWorker) {
> +    this._addQueue.push([aId, aStartedDateTime, aMethod, aUrl, aIsXHR, aFromCache, aFromServiceWorker]);

Please add documentation about the extra param.

@@ +1481,5 @@
>                          requestItem.attachment.statusText);
>              if(requestItem.attachment.fromCache) {
>                text += " (cached)";
> +            } else if(requestItem.attachment.fromServiceWorker) {
> +              text += " (service worker)";

Should we localize these strings? ("cached" and "service worker").

::: devtools/client/netmonitor/test/browser_net_service-worker-status.js
@@ +19,5 @@
> +  let { document, L10N, NetMonitorView } = monitor.panelWin;
> +  let { RequestsMenu, NetworkDetails } = NetMonitorView;
> +
> +  RequestsMenu.lazyUpdate = false;
> +  NetworkDetails._params.lazyEmpty = false;

Are those `lazyUpdate` and `lazyEmpty` really necessary? If so, can you add a comment as to why?

Comment 51

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9bd65916db78

Comment 52

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=df62d930e017

Comment 53

[Alexandre Poirot [:ochameau]]

(In reply to Victor Porof [:vporof][:vp] from comment #50)
> Comment on attachment 8703778 [details] [diff] [review]
> Flag requests coming from service workers in devtools - v1
> 
> Review of attachment 8703778 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Not perfectly comfortable reviewing the backend changes, although they LGTM.
> 
> ::: devtools/client/netmonitor/netmonitor-view.js
> @@ +539,5 @@
> >     * @param boolean aFromCache
> >     *        Indicates if the result came from the browser cache
> >     */
> > +  addRequest: function(aId, aStartedDateTime, aMethod, aUrl, aIsXHR, aFromCache, aFromServiceWorker) {
> > +    this._addQueue.push([aId, aStartedDateTime, aMethod, aUrl, aIsXHR, aFromCache, aFromServiceWorker]);
> 
> Please add documentation about the extra param.

done.

> 
> @@ +1481,5 @@
> >                          requestItem.attachment.statusText);
> >              if(requestItem.attachment.fromCache) {
> >                text += " (cached)";
> > +            } else if(requestItem.attachment.fromServiceWorker) {
> > +              text += " (service worker)";
> 
> Should we localize these strings? ("cached" and "service worker").

We should. I introduced two new l10n keys for that.


> 
> ::: devtools/client/netmonitor/test/browser_net_service-worker-status.js
> @@ +19,5 @@
> > +  let { document, L10N, NetMonitorView } = monitor.panelWin;
> > +  let { RequestsMenu, NetworkDetails } = NetMonitorView;
> > +
> > +  RequestsMenu.lazyUpdate = false;
> > +  NetworkDetails._params.lazyEmpty = false;
> 
> Are those `lazyUpdate` and `lazyEmpty` really necessary? If so, can you add
> a comment as to why?

I just copy-pasted them, I removed them and tests still pass.

Comment 54

[Alexandre Poirot [:ochameau]]

Attachment: Flag requests coming from service workers - v2

Comment 55

[Alexandre Poirot [:ochameau]]

https://hg.mozilla.org/integration/fx-team/rev/21fc0dde38a0524ecdd36e61cf17f4bc5accef37
Bug 1158264 - Send an observer service event when a service worker intercept a request. r=bkelly

https://hg.mozilla.org/integration/fx-team/rev/9c080ff935ef3d6df353f024c1dcad420ab64c38
bug 1158264 - Flag requests coming from service workers. r=vporof

Comment 56

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/21fc0dde38a0
https://hg.mozilla.org/mozilla-central/rev/9c080ff935ef

Comment 57

[Mayur Patil]

[bugday-20160323]

Status: RESOLVED,FIXED -> UNVERIFIED

Comments:
STR: Not clear.
Developer specific testing

Component: 
Name 			Firefox
Version 		46.0b4
Build ID 		20160322075646
Update Channel 	        beta
User Agent 		Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
OS                      Windows 7 SP1 x86_64

Expected Results: 
No Clear STR. Please provide one.

Actual Results: 
As expected