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)
Firefox for Android Graveyard
General
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)
3.71 KB,
patch
|
Margaret
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•15 years ago
|
||
Something like this (haven't tested it yet)
Assignee: nobody → gavin.sharp
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•15 years ago
|
||
Looks like it depends on your TabProgressController patch
Reporter | ||
Comment 3•15 years ago
|
||
Er, yeah, forgot I had that in there.
Reporter | ||
Comment 4•15 years ago
|
||
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
Reporter | ||
Updated•13 years ago
|
Assignee: gavin.sharp → nobody
Comment 5•13 years ago
|
||
Fixed?: http://mxr.mozilla.org/mobile-browser/source/chrome/content/bindings/browser.js#15
Reporter | ||
Comment 6•13 years ago
|
||
I don't see any use of the filter in that file.
Comment 7•13 years ago
|
||
Oh whoops. Didn't look at your WIP patch closely enough. Thanks!
Comment 9•13 years ago
|
||
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
Updated•12 years ago
|
Assignee: mbrubeck → nobody
Reporter | ||
Comment 10•9 years ago
|
||
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).
Reporter | ||
Updated•9 years ago
|
Status: ASSIGNED → NEW
Reporter | ||
Updated•9 years ago
|
Attachment #359409 -
Attachment description: working patch → (no longer) working patch
Updated•9 years ago
|
Keywords: perf
Summary: fennec should be using the browser status filter → Fennec should be using the browser status filter
Assignee | ||
Comment 11•9 years ago
|
||
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)
Assignee | ||
Comment 12•9 years ago
|
||
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=ffad622df42b
Reporter | ||
Comment 13•9 years ago
|
||
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+
Assignee | ||
Comment 14•9 years ago
|
||
(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
Assignee | ||
Comment 15•9 years ago
|
||
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)
Updated•9 years ago
|
Attachment #8543691 -
Flags: review?(margaret.leibovic) → review+
Assignee | ||
Comment 16•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/edabb01e0db7
Comment 17•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/edabb01e0db7
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•