Last Comment Bug 859046 - Implement filtering out certain types of requests
: Implement filtering out certain types of requests
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Netmonitor (show other bugs)
: unspecified
: All All
: P2 normal (vote)
: Firefox 24
Assigned To: Victor Porof [:vporof][:vp]
:
:
Mentors:
: 862907 873761 (view as bug list)
Depends on: 879185
Blocks: 859047
  Show dependency treegraph
 
Reported: 2013-04-07 02:40 PDT by Victor Porof [:vporof][:vp]
Modified: 2013-06-04 02:49 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (24.81 KB, patch)
2013-05-13 05:16 PDT, Victor Porof [:vporof][:vp]
no flags Details | Diff | Splinter Review
v1.1 (30.88 KB, patch)
2013-05-13 05:19 PDT, Victor Porof [:vporof][:vp]
no flags Details | Diff | Splinter Review
v1.2 (42.07 KB, patch)
2013-05-14 11:21 PDT, Victor Porof [:vporof][:vp]
no flags Details | Diff | Splinter Review
v2 (42.10 KB, patch)
2013-05-15 04:03 PDT, Victor Porof [:vporof][:vp]
mihai.sucan: feedback+
Details | Diff | Splinter Review
v2.1 (42.07 KB, patch)
2013-05-15 14:22 PDT, Victor Porof [:vporof][:vp]
no flags Details | Diff | Splinter Review
v3 (76.22 KB, patch)
2013-05-17 07:52 PDT, Victor Porof [:vporof][:vp]
no flags Details | Diff | Splinter Review
v3.1 (81.10 KB, patch)
2013-05-24 10:51 PDT, Victor Porof [:vporof][:vp]
rcampbell: review+
Details | Diff | Splinter Review

Description Victor Porof [:vporof][:vp] 2013-04-07 02:40:07 PDT
A classic "show only HTML, CSS, JS, XHR, Images, Media, Flash" type of filtering.
Comment 1 Victor Porof [:vporof][:vp] 2013-04-10 03:10:29 PDT
Moving into Developer Tools: Netmonitor component. Filter on NETMONITORAMA.
Comment 2 Victor Porof [:vporof][:vp] 2013-04-17 09:45:46 PDT
*** Bug 862907 has been marked as a duplicate of this bug. ***
Comment 3 Victor Porof [:vporof][:vp] 2013-05-02 03:40:35 PDT
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] 2013-05-13 05:16:47 PDT
Created attachment 748765 [details] [diff] [review]
v1

WIP.
Comment 5 Victor Porof [:vporof][:vp] 2013-05-13 05:19:31 PDT
Created attachment 748767 [details] [diff] [review]
v1.1

Everything works except filtering on XHR. Need to be smart about that.
Comment 6 Victor Porof [:vporof][:vp] 2013-05-13 05:30:27 PDT
Bug: sorting then filtering makes everything visible again.
Comment 7 Victor Porof [:vporof][:vp] 2013-05-13 05:33:04 PDT
Honza, how does Firebug filter on XHR? It sounds like it should be a bit tricky.
Comment 8 Jan Honza Odvarko [:Honza] 2013-05-13 05:53:06 PDT
(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] 2013-05-13 05:56:55 PDT
Thanks!
Comment 10 Victor Porof [:vporof][:vp] 2013-05-14 11:21:01 PDT
Created attachment 749386 [details] [diff] [review]
v1.2

Moar WIP. Fixes most issues, adds font sorting.
Comment 11 Victor Porof [:vporof][:vp] 2013-05-15 02:21:45 PDT
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] 2013-05-15 02:23:01 PDT
In the meantime, here's a try run: https://tbpl.mozilla.org/?tree=Try&rev=f7f130c0c96f
Comment 13 Mihai Sucan [:msucan] 2013-05-15 03:18:34 PDT
Agreed. Add isXHR to the networkEventActor grip.
Comment 14 Victor Porof [:vporof][:vp] 2013-05-15 04:03:15 PDT
Created attachment 749802 [details] [diff] [review]
v2

Mihai, let me know if the webconsole backend changes are ok.
Comment 15 Mihai Sucan [:msucan] 2013-05-15 04:52:00 PDT
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] 2013-05-15 14:22:22 PDT
Created attachment 750072 [details] [diff] [review]
v2.1

Addressed comments. Need to write some tests and 'tis done.
Comment 17 Victor Porof [:vporof][:vp] 2013-05-16 10:21:55 PDT
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] 2013-05-16 10:22:44 PDT
s/string/strict in comment #17 :)
Comment 19 Victor Porof [:vporof][:vp] 2013-05-16 10:26:02 PDT
(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] 2013-05-17 07:52:47 PDT
Created attachment 751037 [details] [diff] [review]
v3

Added tests. Going through try. Reviewable.
Comment 21 Victor Porof [:vporof][:vp] 2013-05-18 01:19:44 PDT
Green! https://tbpl.mozilla.org/?tree=Try&rev=d6a799669713
Comment 22 Victor Porof [:vporof][:vp] 2013-05-21 11:47:23 PDT
*** Bug 873761 has been marked as a duplicate of this bug. ***
Comment 23 Rob Campbell [:rc] (:robcee) 2013-05-23 12:34:39 PDT
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] 2013-05-24 06:18:55 PDT
(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] 2013-05-24 10:51:22 PDT
Created attachment 753882 [details] [diff] [review]
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) 2013-05-24 11:08:04 PDT
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] 2013-05-24 13:09:00 PDT
https://hg.mozilla.org/integration/fx-team/rev/c74840cfe392
Comment 28 Tim Taubert [:ttaubert] 2013-05-24 13:20:30 PDT
https://hg.mozilla.org/mozilla-central/rev/c74840cfe392

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