859046 - Implement filtering out certain types of requests

Status: RESOLVED FIXED

(DevTools :: Netmonitor, defect, P2)

Description

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

A classic "show only HTML, CSS, JS, XHR, Images, Media, Flash" type of filtering.

Comment 1

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

Moving into Developer Tools: Netmonitor component. Filter on NETMONITORAMA.

Comment 3

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

Ideally the implementation here would borrow the idea of using predicates to dictate what's visible and what's not. This way addons could easily extend the filtering mechanism.

Comment 4

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

Attachment: v1

WIP.

Comment 5

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

Attachment: v1.1

Everything works except filtering on XHR. Need to be smart about that.

Comment 6

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

Bug: sorting then filtering makes everything visible again.

Comment 7

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

Honza, how does Firebug filter on XHR? It sounds like it should be a bit tricky.

Comment 8

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Victor Porof [:vp] from comment #7)
> Honza, how does Firebug filter on XHR? It sounds like it should be a bit
> tricky.
Take a look at Http.isXHR() method here:
https://github.com/firebug/firebug/blob/master/extension/content/firebug/lib/http.js#L415

(just ignore the StackFrame.* calls)

There is no simple API so, yet the code is a bit tricky.

Honza

Comment 9

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

Thanks!

Comment 10

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

Attachment: v1.2

Moar WIP. Fixes most issues, adds font sorting.

Comment 11

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

I'm having a hard time deciding where to add an isXHR flag. It almost seems to me like grip of a networkEvent packet would be the best choice, if the extra transfer overhead can be ignored (I think the "overhead" is close to nothing if not actually nothing).

None of the networkEventUpdate packets seem to be satisfying and generic enough for my taste. "requestHeaders" and "responseContent" are the most plausible candidates IMHO, but using them may seem a bit awkward since isXHR has nothing to do with neither headers nor content.

Comment 12

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

In the meantime, here's a try run: https://tbpl.mozilla.org/?tree=Try&rev=f7f130c0c96f

Comment 13

[Mihai Sucan [:msucan]]

Agreed. Add isXHR to the networkEventActor grip.

Comment 14

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

Attachment: v2

Mihai, let me know if the webconsole backend changes are ok.

Comment 15

[Mihai Sucan [:msucan]]

Comment on attachment 749802 [details] [diff] [review]
v2

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

Web Console-related changes are good. Thank you!

Once this bug is fixed please also update the protocol documentation. [1] Thanks!

[1] https://developer.mozilla.org/en-US/docs/Tools/Web_Console/remoting

::: toolkit/devtools/webconsole/WebConsoleUtils.jsm
@@ +1933,5 @@
>      let event = {};
>      event.startedDateTime = new Date(Math.round(aTimestamp / 1000)).toISOString();
>      event.headersSize = aExtraStringData.length;
>      event.method = aChannel.requestMethod;
>      event.url = aChannel.URI.spec;

Please also add event.isXHR = false.

@@ +1938,5 @@
>  
> +    // Determine if this is an XHR request.
> +    try {
> +      var callbacks = aChannel.notificationCallbacks;
> +      var xhrRequest = callbacks ? callbacks.getInterface(Ci.nsIXMLHttpRequest) : null;

s/var/let/

Comment 16

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

Attachment: v2.1

Addressed comments. Need to write some tests and 'tis done.

Comment 17

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

Hi!

This bug is about a very important and needed feature in the Network Monitor, a new tool introduced in Firefox 23.

The feature is filtering ('errbody filters on XHR when writing webapps!). Work on this bug is pretty much finished, and I can't even begin to express how awesome it would be if it got into 23. However, unavoidably, it contains string additions (for new buttons, like HTLM, CSS, JS, XHR etc.).

I know the policy on uplifting patches that touch l10n is quite string, but what's your take on this? As I mentioned, there are only string *additions*, not *changes*.

Comment 18

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

s/string/strict in comment #17 :)

Comment 19

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

(In reply to Victor Porof [:vp] from comment #17)

After some discussion on IRC with dcamp and robcee, we concluded that this bug can wait a cycle. Although it'd be a nice feature, the Network Monitor is not unshippable without filtering.

Comment 20

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

Attachment: v3

Added tests. Going through try. Reviewable.

Comment 21

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

Green! https://tbpl.mozilla.org/?tree=Try&rev=d6a799669713

Comment 23

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 751037 [details] [diff] [review]
v3

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

Looks good. Would consider a radiogroup, but not sure how that would look for CSS. Get back to me about it, bro!

::: browser/devtools/netmonitor/netmonitor-view.js
@@ +102,5 @@
>     */
>    _initializePanes: function() {
>      dumpn("Initializing the NetMonitorView panes");
>  
> +    this._body = $("#body");

document.body? Feels like jquery in here.

@@ +330,2 @@
>     */
> +  addRequest: function(aId, aStartedDateTime, aMethod, aUrl, aIsXHR) {

aIsXHR: case where our 'aParam' notation is kinda ugly. But so be it!

@@ +361,5 @@
>    /**
> +   * Filters all network requests in this container by a specified type.
> +   *
> +   * @param string aType
> +   *        Either null, "html", "css", "js", "xhr", "images" or "media".

what about "flash"?

@@ +372,5 @@
> +      if (button != target) {
> +        button.removeAttribute("checked");
> +      } else {
> +        button.setAttribute("checked", "");
> +      }

should we be using radio buttons for this stuff?

@@ +377,5 @@
> +    }
> +
> +    // Filter on nothing.
> +    if (!target) {
> +      this.filterContents(() => true);

augh! stop being clever!

@@ +513,5 @@
> +
> +  _onImages: function({ attachment: { mimeType } })
> +    mimeType && mimeType.contains("image/"),
> +
> +  _onFonts: function({ attachment: { url, mimeType } }) // Fonts are a mess

lol

@@ +520,5 @@
> +      mimeType.contains("/font"))) ||
> +    url.contains(".eot") ||
> +    url.contains(".ttf") ||
> +    url.contains(".otf") ||
> +    url.contains(".woff"),

wow. neat.

@@ +527,5 @@
> +    mimeType && (
> +      mimeType.contains("audio/") ||
> +      mimeType.contains("video/") ||
> +      mimeType.contains("image/") ||
> +      mimeType.contains("model/")),

we talked about this a bit. Almost would be nice to split images out of this group since they have a dedicated grouping already.

Thinking about it a bit more, probably still makes sense to include it.

@@ +568,1 @@
>  

ok, for consistency's sake, I guess.

::: browser/devtools/netmonitor/netmonitor.xul
@@ +166,5 @@
> +        <button id="requests-menu-filter-media-button"
> +                class="requests-menu-footer-button"
> +                onclick="NetMonitorView.RequestsMenu.filterOn('media')">
> +          &netmonitorUI.footer.filterMedia;
> +        </button>

again, any reason not to use a radiogroup here?

::: browser/devtools/netmonitor/test/browser_net_filter-03.js
@@ +183,5 @@
> +      return deferred.promise;
> +    }
> +
> +    aDebuggee.performRequests("<p>" + new Array(10).join(Math.random()) + "</p>");
> +  });

I'm beginning to wonder if you have:
a) A script to magically generate these tests,
b) A mechanical turk operation to outsource the generation of said scripts to another planet.
c) AI.

::: toolkit/devtools/webconsole/WebConsoleUtils.jsm
@@ +1939,5 @@
> +    // Determine if this is an XHR request.
> +    try {
> +      let callbacks = aChannel.notificationCallbacks;
> +      let xhrRequest = callbacks ? callbacks.getInterface(Ci.nsIXMLHttpRequest) : null;
> +      event.isXHR = !!xhrRequest;

nice!

Comment 24

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

(In reply to Rob Campbell [:rc] (:robcee) from comment #23)
> Comment on attachment 751037 [details] [diff] [review]
> v3
> 
> Review of attachment 751037 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good. Would consider a radiogroup, but not sure how that would look
> for CSS. Get back to me about it, bro!
> 

Radiogroups are a viable option, but the stuff happening in bug 875138 because of XBL refactoring and anonymous content makes me think that the current implementation is *ok* and we shouldn't think about it that much. 

> @@ +361,5 @@
> >    /**
> > +   * Filters all network requests in this container by a specified type.
> > +   *
> > +   * @param string aType
> > +   *        Either null, "html", "css", "js", "xhr", "images" or "media".
> 
> what about "flash"?
> 

Ok, it's easy, I'll add this.

> 
> @@ +527,5 @@
> > +    mimeType && (
> > +      mimeType.contains("audio/") ||
> > +      mimeType.contains("video/") ||
> > +      mimeType.contains("image/") ||
> > +      mimeType.contains("model/")),
> 
> we talked about this a bit. Almost would be nice to split images out of this
> group since they have a dedicated grouping already.
> 
> Thinking about it a bit more, probably still makes sense to include it.
> 

I have no idea. Really, I can't think of any incredibly pertinent argument for either approaches. Chrome doesn't even have a media tab. Firebug doesn't show images in its media tab. I'm not sure if this is a great idea or not, but maybe we don't necessarily have to be on parity with Firebug for no good reason.

Anywhoo, I'll remove images for now. If anyone complains, we'll burn that bridge when we get to it.
 
> I'm beginning to wonder if you have:
> a) A script to magically generate these tests,
> b) A mechanical turk operation to outsource the generation of said scripts
> to another planet.
> c) AI.
>

AI.

Comment 25

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

Attachment: v3.1

Addressed comments and implemented support for filtering Flash requests. Also, consolidated tests and fixed some UI awkwardness on Linux and Windows.

Comment 26

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 753882 [details] [diff] [review]
v3.1

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

lookin' good.

::: browser/devtools/netmonitor/netmonitor-view.js
@@ +531,5 @@
> +      mimeType.contains("audio/") ||
> +      mimeType.contains("video/") ||
> +      mimeType.contains("model/")),
> +
> +  _onFlash: function({ attachment: { url, mimeType } }) // Flash is a mess.

oh boy!

@@ +536,5 @@
> +    (mimeType && (
> +      mimeType.contains("/x-flv") ||
> +      mimeType.contains("/x-shockwave-flash"))) ||
> +    url.contains(".swf") ||
> +    url.contains(".flv"),

good for a start.

Comment 27

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

https://hg.mozilla.org/integration/fx-team/rev/c74840cfe392

Comment 28

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/c74840cfe392