Closed Bug 474464 Opened 15 years ago Closed 9 years ago

Fennec should be using the browser status filter

Categories

(Firefox for Android Graveyard :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 37

People

(Reporter: Gavin, Assigned: mfinkle)

References

(Depends on 1 open bug)

Details

(Keywords: perf)

Attachments

(1 file, 3 obsolete files)

http://mxr.mozilla.org/mozilla-central/source/xpfe/browser/src/nsBrowserStatusFilter.cpp

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#1148

The filter "smooths out" status events, and otherwise reduces the amount of JS called in response to network traffic. Tabbrowser hooks it up for Firefox, but we don't appear to be using it. I bet it would help pageload a lot.
Attached patch patch (obsolete) — Splinter Review
Something like this (haven't tested it yet)
Assignee: nobody → gavin.sharp
Status: NEW → ASSIGNED
Looks like it depends on your TabProgressController patch
Er, yeah, forgot I had that in there.
Attached patch (no longer) working patch (obsolete) — Splinter Review
This one actually works, except that it doesn't, because now we're not getting NETWORK_STOP onStateChange notifications. I think it's due to bug 463210.
Attachment #358018 - Attachment is obsolete: true
Depends on: 463210
Assignee: gavin.sharp → nobody
I don't see any use of the filter in that file.
Oh whoops. Didn't look at your WIP patch closely enough. Thanks!
Assignee: nobody → mbrubeck
Blocks: 679927
nsBrowserStatusFilter is not hooked up in e10s desktop Firefox yet:
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#1332

See bug 666801 for some related discussion.
Depends on: 666801
Assignee: mbrubeck → nobody
This still applies to current Fennec, AFAIK - the listener at https://hg.mozilla.org/mozilla-central/annotate/48e2bab81030/mobile/android/chrome/content/browser.js#l4305 is probably being called way more often than it should be (see also recent bug 1099490).
No longer blocks: 459117, 679927
No longer depends on: 666801
Product: Fennec Graveyard → Firefox for Android
Status: ASSIGNED → NEW
Attachment #359409 - Attachment description: working patch → (no longer) working patch
Keywords: perf
Summary: fennec should be using the browser status filter → Fennec should be using the browser status filter
Attached patch browser-filter v0.1 (obsolete) — Splinter Review
Gavin - I think you know this pattern well enough to take this review. I added comments to onProgressChange and onStatusChange to let people know that aWebProgress and aRequest will always be null, even though we don't listen for those notifications right now.

I think there are two levels of performance to worry about here:
1. XPCOM -> JS
2. JS -> Java

#2 is not really affected even with this patch, since we kinda filter already. #1 is really affected with the patch. Using the nytimes.com test, the patch reduced onStateChange calls from 430 to 10. From the old XUL Fennec days, we know this should be a noticeable difference.

Thanks for poking this bug!
Assignee: nobody → mark.finkle
Attachment #359409 - Attachment is obsolete: true
Attachment #8543572 - Flags: review?(gavin.sharp)
Comment on attachment 8543572 [details] [diff] [review]
browser-filter v0.1

Looks fine to me, assuming the issue from comment 4 no longer exists.

I think you should probably drop the reference to this.filter in destroy()?

I actually was not aware of the behavior impact to onProgressChange/onStatusChange, I guess desktop is just lucky to have not depended on that. I would maybe add a bit more detail in the comment and mention "filtering notifications using nsBrowserStatusFilter"
Attachment #8543572 - Flags: review?(gavin.sharp) → feedback+
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #13)
> Comment on attachment 8543572 [details] [diff] [review]
> browser-filter v0.1
> 
> Looks fine to me, assuming the issue from comment 4 no longer exists.

Logs show that we get both START and STOP messages in Java.
> 
> I think you should probably drop the reference to this.filter in destroy()?

Yeah, I can add that.

> I actually was not aware of the behavior impact to
> onProgressChange/onStatusChange, I guess desktop is just lucky to have not
> depended on that. I would maybe add a bit more detail in the comment and
> mention "filtering notifications using nsBrowserStatusFilter"

OK
Updated based on gavin's feedback. Margaret for the next review, so someone on the team besides me is aware of the change.
Attachment #8543572 - Attachment is obsolete: true
Attachment #8543691 - Flags: review?(margaret.leibovic)
Attachment #8543691 - Flags: review?(margaret.leibovic) → review+
https://hg.mozilla.org/mozilla-central/rev/edabb01e0db7
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: