Implement filtering out certain types of requests

RESOLVED FIXED in Firefox 24

Status

()

Firefox
Developer Tools: Netmonitor
P2
normal
RESOLVED FIXED
4 years ago
2 months ago

People

(Reporter: vporof, Assigned: vporof)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 24
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 6 obsolete attachments)

(Assignee)

Description

4 years ago
A classic "show only HTML, CSS, JS, XHR, Images, Media, Flash" type of filtering.
(Assignee)

Comment 1

4 years ago
Moving into Developer Tools: Netmonitor component. Filter on NETMONITORAMA.
Component: Developer Tools → Developer Tools: Netmonitor
Summary: [netmonitor] Implement filtering out certain types of requests → Implement filtering out certain types of requests
(Assignee)

Updated

4 years ago
Blocks: 859047
(Assignee)

Updated

4 years ago
Duplicate of this bug: 862907
(Assignee)

Updated

4 years ago
Priority: -- → P2
(Assignee)

Comment 3

4 years ago
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.
(Assignee)

Updated

4 years ago
See Also: → bug 859039
(Assignee)

Comment 4

4 years ago
Created attachment 748765 [details] [diff] [review]
v1

WIP.
(Assignee)

Comment 5

4 years ago
Created attachment 748767 [details] [diff] [review]
v1.1

Everything works except filtering on XHR. Need to be smart about that.
Attachment #748765 - Attachment is obsolete: true
(Assignee)

Comment 6

4 years ago
Bug: sorting then filtering makes everything visible again.
(Assignee)

Comment 7

4 years ago
Honza, how does Firebug filter on XHR? It sounds like it should be a bit tricky.
Flags: needinfo?(odvarko)
(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
Flags: needinfo?(odvarko)
(Assignee)

Comment 9

4 years ago
Thanks!
(Assignee)

Comment 10

4 years ago
Created attachment 749386 [details] [diff] [review]
v1.2

Moar WIP. Fixes most issues, adds font sorting.
Assignee: nobody → vporof
Attachment #748767 - Attachment is obsolete: true
Status: NEW → ASSIGNED
(Assignee)

Comment 11

4 years ago
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.
Flags: needinfo?(mihai.sucan)
(Assignee)

Comment 12

4 years ago
In the meantime, here's a try run: https://tbpl.mozilla.org/?tree=Try&rev=f7f130c0c96f
Agreed. Add isXHR to the networkEventActor grip.
Flags: needinfo?(mihai.sucan)
(Assignee)

Comment 14

4 years ago
Created attachment 749802 [details] [diff] [review]
v2

Mihai, let me know if the webconsole backend changes are ok.
Attachment #749386 - Attachment is obsolete: true
Attachment #749802 - Flags: feedback?(mihai.sucan)
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/
Attachment #749802 - Flags: feedback?(mihai.sucan) → feedback+
(Assignee)

Comment 16

4 years ago
Created attachment 750072 [details] [diff] [review]
v2.1

Addressed comments. Need to write some tests and 'tis done.
Attachment #749802 - Attachment is obsolete: true
(Assignee)

Comment 17

4 years ago
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*.
Flags: needinfo?(akeybl)
(Assignee)

Comment 18

4 years ago
s/string/strict in comment #17 :)
(Assignee)

Comment 19

4 years ago
(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.
Flags: needinfo?(akeybl)
(Assignee)

Comment 20

4 years ago
Created attachment 751037 [details] [diff] [review]
v3

Added tests. Going through try. Reviewable.
Attachment #750072 - Attachment is obsolete: true
Attachment #751037 - Flags: review?(rcampbell)
(Assignee)

Comment 21

4 years ago
Green! https://tbpl.mozilla.org/?tree=Try&rev=d6a799669713
(Assignee)

Updated

4 years ago
Duplicate of this bug: 873761
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!
Attachment #751037 - Flags: review?(rcampbell)
(Assignee)

Comment 24

4 years ago
(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.
(Assignee)

Comment 25

4 years ago
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.
Attachment #751037 - Attachment is obsolete: true
Attachment #753882 - Flags: review?(rcampbell)
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.
Attachment #753882 - Flags: review?(rcampbell) → review+
(Assignee)

Comment 27

4 years ago
https://hg.mozilla.org/integration/fx-team/rev/c74840cfe392
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/c74840cfe392
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 24

Updated

4 years ago
Depends on: 879185
Blocks: 1093269
No longer blocks: 859047
You need to log in before you can comment on or make changes to this bug.