581958 - Filter web progress STATUS and PROGRESS messages to improve performance

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch

We send a lot of webprogress messages during a page load. This patch adds a bit of Ben Stover's idea for child-side filtering.

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

See bug 581930 for message loading affect on performance

Comment 2

[Benjamin Stover (:stechz)]

Comment on attachment 460248 [details] [diff] [review]
patch

>+  _calculatedNotifyFlags: Ci.nsIWebProgress.NOTIFY_ALL,

Shouldn't this be 0?  Otherwise, unless a progress listener is removed they will all be sent over.

Otherwise, looked good.

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #2)
> Comment on attachment 460248 [details] [diff] [review]
> patch
> 
> >+  _calculatedNotifyFlags: Ci.nsIWebProgress.NOTIFY_ALL,
> 
> Shouldn't this be 0?  Otherwise, unless a progress listener is removed they
> will all be sent over.

My initial code used 0, just as you're patch did. However, we never setup the listener notification flags fast enough. When starting a load in a new tab (including the first) we would never get the initial STATE_NETWORK events, so networkStarting is never called.

Comment 4

[Benjamin Stover (:stechz)]

How is this any faster then, if we are sending all notifications over anyway (until we remove a listener, which I assume doesn't ever happen)?

At the 1000 foot view, this is just a band-aid solution.  We don't want to significantly regress if extensions decide to use web progress listeners on the browser element.  So much is breaking with progress listeners anyways, devs are going to have to change our code.  Now is a good time to rethink what devs use these listeners for.

I suggest we remove support for web progress listeners in the parent process.  To help out developers we can support common use cases through messages, like page is now loading, page is no longer loading, security changed, etc and leave out the flood of others.  If our messages don't give them enough, then they can add their own web progress listener in content.

Comment 5

[Benjamin Stover (:stechz)]

s/our code/their code/

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch 2

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 460327 [details] [diff] [review]
patch 2

This patch does not filter OnState messages, but does default _calcNotifyFlags to 0

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

This is not the best we can do:
* State messages are not correctly filtered yet, due to a timing issue

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

Filed bug 582132 for filtering the STATE messages
Ben filed bug 582060 for coalescing the STATUS and PROGRESS messages

pushed:
http://hg.mozilla.org/mobile-browser/rev/15e0c7594f64

Comment 10

[Dão Gottwald [:dao]]

bugspam