Closed Bug 501566 Opened 10 years ago Closed 10 years ago

It takes way too long to highlight a selected link

Categories

(Firefox for Android Graveyard :: General, defect)

x86
Windows Vista
defect
Not set

Tracking

(fennec1.0+)

VERIFIED FIXED
fennec1.0b4
Tracking Status
fennec 1.0+ ---

People

(Reporter: blassey, Assigned: stechz)

References

(Depends on 1 open bug)

Details

(Keywords: mobile)

Attachments

(1 file, 2 obsolete files)

when I click on a link in Fennec, it can take as long as 40 seconds for the link to be highlighted.  I've been told that its because the highlight is causing a reflow, regardless we should fix this.
Flags: wanted-fennec1.0?
tracking-fennec: --- → ?
tracking-fennec: ? → 1.0+
Flags: wanted-fennec1.0? → wanted-fennec1.0+
Thankfully, this happens quick enough on maemo
Assignee: nobody → webapps
Attached patch Send mouseDown immediately (obsolete) — Splinter Review
Not sure about the reflow or 40 second claims, but there were two big reasons a delay was occuring:
1. Content mousedown wasn't sent until we knew if event was a doubleclick. Hence, the link was not highlighted until 400msec after mousedown.
2. When mousedown occurs, the browser stops rendering until mouseup for dragging. This patch causes browser to render once after link is highlighted.
Attachment #397906 - Flags: review?
Comment on attachment 397906 [details] [diff] [review]
Send mouseDown immediately

>+    if (this._clicker) {
>+      this._clicker.mouseDown(evInfo.event.clientX, evInfo.event.clientY);
>+    }

No braces needed here

>+    if (this._clicker) {
>+      this._clicker.mouseUp(evInfo.event.clientX, evInfo.event.clientY);
>+    }

And here

>diff -r ae0bd24cb606 -r 346cc81f2152 chrome/content/Util.js

>+  /** Executes aFunc after other events have been processed. */
>+  executeSoon: function executeSoon(aFunc) {
>+    var tm = Components.classes["@mozilla.org/thread-manager;1"]
>+                       .getService(Components.interfaces.nsIThreadManager);

Use let. Use Ci and Cc


Adding froystig for a review too
Attachment #397906 - Flags: review?(froystig)
Attachment #397906 - Flags: review?
Attachment #397906 - Flags: review+
Comment on attachment 397906 [details] [diff] [review]
Send mouseDown immediately


>+      // Re-render content after mousedown event has possibly selected something.
>+      Util.executeSoon(Util.bind(this._browserView.renderNow, this._browserView));

Everything in a BrowserView instance is already self-bound.  No need for Util.bind here, though it may be worth an inline comment if you think that's appropriate.

r+ with that change.
Attachment #397906 - Flags: review?(froystig) → review+
Attached patch Code review changes (obsolete) — Splinter Review
Opted to not add inline comment about bind as I did not feel it was necessary.
Attachment #397906 - Attachment is obsolete: true
pushed: https://hg.mozilla.org/mobile-browser/rev/4b520153dd86
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → B4
Duplicate of this bug: 492827
Verified on n810 with 1.9.2 nightly build from 20090921
Status: RESOLVED → VERIFIED
This was backed out by bug 519231
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
When this patch is relanded, incorporate focus fix from bug 518467.
Depends on: 517153, 504524
Some background: this patch has been removed due to large panning perf regressions.  The regressions are caused by large invalidates when links are focused, and can take up the entire page.

@roc: Has a bug been filed on large invalidates for tables?  It should be a blocker for this bug.
I don't know. It depends on what exactly the issue is. Do you have a small testcase?
Depends on: 519339
Table bug filed as bug 519339
Attached patch New approachSplinter Review
This adds a new canvas to the fix used for highlighting content on mousedown.  I picked a new canvas for two reasons:
1) It doesn't need to be as big as the entire screen
2) It must not be opaque

I profiled this JS and made it as fast as I could.  Playing around on the N900, the average time was somewhere around 16msecs, with a max of 32msecs.  Panning speed is still great and highlight pops up in less than a quarter of a second.
Attachment #397921 - Attachment is obsolete: true
Attachment #417132 - Flags: review?(mark.finkle)
Comment on attachment 417132 [details] [diff] [review]
New approach

> 
>+    let clicker = this._clicker;
>+//    Util.dumpLn('got here', oldIsPan, dragData.isPan(), clicker);

remove

>+        // Let clicker know when mousemove begins a pan
>+        let clicker = this._clicker;
>+        if (!oldIsPan && clicker)
>+          clicker.mousePanBegin();

mousePaneBegin -> panBegin

> function ContentCustomClicker(browserView) {
>   this._browserView = browserView;
>+  this._canvas = document.getElementById("content-overlay");

this._canvas  ->  this._overlay
Attachment #417132 - Flags: review?(mark.finkle) → review+
Pushed http://hg.mozilla.org/mobile-browser/rev/1b45624dd688
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
verified FIXED on builds:

Mozilla/5.0 (X11; U; Linux armv7l; Nokia N900; en-US; rv:1.9.2b5pre) Gecko/20091214 Firefox/3.6b5pre Fennec/1.0b6pre

and

Mozilla/5.0 (X11; U; Linux armv6l; Nokia N8xx; en-US; rv:1.9.3a1pre) Gecko/20091214 Firefox/3.7a1pre Fennec/1.0b5
Status: RESOLVED → VERIFIED
Flags: in-litmus?
litmus testcase https://litmus.mozilla.org/show_test.cgi?id=11536 has been created to regression test this bug.
Flags: in-litmus? → in-litmus+
You need to log in before you can comment on or make changes to this bug.