Closed Bug 574117 Opened 10 years ago Closed 10 years ago

Page content never updates

Categories

(Firefox for Android Graveyard :: General, defect)

x86
Linux
defect
Not set

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: stechz, Assigned: stechz)

Details

Attachments

(1 file, 1 obsolete file)

Attached patch v1 (obsolete) — Splinter Review
Breakage from bug 573443, where we tried to use a timer tied to the window.  I think this is because the timer is destroyed as soon as the web page changes (which happens after network start, when the timer is created).

This patch switches back to nsITimer and removes the code in browser.js that was previously used for coalescing.  Now coalescing happens per-tab!  Loading other pages will not affect your current tab rendering.
Attachment #453476 - Flags: review?(mark.finkle)
Attachment #453476 - Flags: feedback?(21)
Attachment #453476 - Flags: review?(mark.finkle) → review-
Comment on attachment 453476 [details] [diff] [review]
v1

>diff --git a/chrome/content/browser.js b/chrome/content/browser.js

>   startLoading: function startLoading() {

>     if (!this._loadingTimeout) {
>       let bv = Browser._browserView;

Shouldn't we be removing all occurrences of _loadingTimeout ?


>diff --git a/chrome/content/content.js b/chrome/content/content.js

> Coalescer.prototype = {
>-  start: function startCoalescing() {
>+  start: function start() {
>+    let self = this;

Do we need this "self"?

>+  stop: function stop() {
>     content.document.defaultView.clearInterval(this._timer);
>-    this._timer = null;
>-    this.flush()
>+    this._timer.flush();
>+  },

Shouldn't we remove the "content.document.defaultView.clearInterval(this._timer);" ?
Attached patch v2Splinter Review
Attachment #453476 - Attachment is obsolete: true
Attachment #453523 - Flags: review?(mark.finkle)
Attachment #453476 - Flags: feedback?(21)
Attachment #453523 - Flags: review?(mark.finkle) → review+
pushed:
http://hg.mozilla.org/mobile-browser/rev/bdcc080c841a
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Comment on attachment 453523 [details] [diff] [review]
v2

>+  /** Timer callback. Don't call this manually. */
>+  notify: function notify() {
>+    this._active = false;
>+    if (this._callback.notify)
>+      this._callback.notify();
>+    else
>+      this._callback.apply(null);
>+  },

Is it correct that we put this._active = false every time that notify is called. Shouldn't this happen only when single shot timer was used? Now if interval timer is used, flush() won't do anything when after first notify call.
Verified on:
Mozilla/5.0(Android; Linux armv7l; rv:2.0b5pre) Gecko/20100820
Namoroka/4.0b5pre Fennec/2.0a1pre

and


Mozilla/5.0(X11; Linux armv7l; rv:2.0b5pre) Gecko/20100820
Namoroka/4.0b5pre Fennec/2.0a1pre
Status: RESOLVED → VERIFIED
bugspam
Assignee: nobody → ben
You need to log in before you can comment on or make changes to this bug.