Last Comment Bug 695170 - History, visited link styles
: History, visited link styles
Status: VERIFIED FIXED
: feature
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: unspecified
: ARM Android
: P2 normal (vote)
: ---
Assigned To: Kartikaya Gupta (email:kats@mozilla.com)
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-10-17 14:25 PDT by Erin Lancaster [:elan]
Modified: 2016-07-29 14:20 PDT (History)
8 users (show)
andreea.pod: in‑litmus+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
11+


Attachments
Proposed patch (26.48 KB, patch)
2011-10-20 06:47 PDT, Kartikaya Gupta (email:kats@mozilla.com)
no flags Details | Diff | Splinter Review
Implement visited link styles (27.98 KB, patch)
2011-10-20 10:51 PDT, Kartikaya Gupta (email:kats@mozilla.com)
no flags Details | Diff | Splinter Review
Implement visited link styles (34.89 KB, patch)
2011-10-20 13:42 PDT, Kartikaya Gupta (email:kats@mozilla.com)
blassey.bugs: review+
Details | Diff | Splinter Review
Implement visited link styles (v3) (31.81 KB, patch)
2011-10-21 08:20 PDT, Kartikaya Gupta (email:kats@mozilla.com)
bugmail: review+
Details | Diff | Splinter Review
Implement visited link styles (v4) (31.75 KB, patch)
2011-10-24 12:51 PDT, Kartikaya Gupta (email:kats@mozilla.com)
bugmail: review+
Details | Diff | Splinter Review
Implement visited link styles (v5) (31.30 KB, patch)
2011-10-25 07:24 PDT, Kartikaya Gupta (email:kats@mozilla.com)
blassey.bugs: review+
Details | Diff | Splinter Review
Implement visited link styles (v6) (31.55 KB, patch)
2011-10-25 08:40 PDT, Kartikaya Gupta (email:kats@mozilla.com)
bugmail: review+
Details | Diff | Splinter Review
Fix comment (1.16 KB, patch)
2011-12-16 08:55 PST, Kartikaya Gupta (email:kats@mozilla.com)
gpascutto: review+
Details | Diff | Splinter Review

Description Erin Lancaster [:elan] 2011-10-17 14:25:57 PDT
Currently not supported due to removing places db
Comment 1 Kartikaya Gupta (email:kats@mozilla.com) 2011-10-20 06:47:54 PDT
Created attachment 568379 [details] [diff] [review]
Proposed patch

A preliminary version of the patch. It doesn't fully work yet (I'm still debugging) but let me know what you think of the general idea.
Comment 2 Kartikaya Gupta (email:kats@mozilla.com) 2011-10-20 10:51:52 PDT
Created attachment 568441 [details] [diff] [review]
Implement visited link styles

Works now, with no obvious problems/warnings showing up in logcat output.
Comment 3 Kartikaya Gupta (email:kats@mozilla.com) 2011-10-20 13:42:50 PDT
Created attachment 568498 [details] [diff] [review]
Implement visited link styles

Further improvements to the patch - fix some memory leaks on the native side caused by me not reading the interface description carefully, and optimize the java side so that it doesn't impact page load time nearly as much.
Comment 4 Brad Lassey [:blassey] (use needinfo?) 2011-10-20 15:56:44 PDT
Comment on attachment 568498 [details] [diff] [review]
Implement visited link styles

Review of attachment 568498 [details] [diff] [review]:
-----------------------------------------------------------------

2 nits, rename gHistory to sHistory and move nsAndroidHistory to toolkit/components/places

r=blassey with nits addressed

::: widget/src/android/nsAndroidHistory.h
@@ +62,5 @@
> +  nsAndroidHistory();
> +  static void NotifyURIVisited(const nsString& str);
> +
> +private:
> +  static nsAndroidHistory* gHistory;

this should be sHistory
Comment 5 Kartikaya Gupta (email:kats@mozilla.com) 2011-10-21 08:20:18 PDT
Created attachment 568653 [details] [diff] [review]
Implement visited link styles (v3)

Updated patch with requested changes and a couple other things:
- s/gHistory/sHistory/
- moved nsAndroidHistory.* to places folder, adjusted component registration and makefiles accordingly
- added JNIAutoLocalFrames to the AndroidBridge methods to avoid leaking jstrings
- call notifyURIVisited from GlobalHistory.add so that clicking on a link marks it as visited as it loads
Comment 6 Kartikaya Gupta (email:kats@mozilla.com) 2011-10-24 12:51:03 PDT
Created attachment 569144 [details] [diff] [review]
Implement visited link styles (v4)

Rebased patch against latest birch code, no other changes.

I also measured page load time on a static Twitter page (http://people.mozilla.com/~nhirata/Perf/favorites1.html saved on the SD card) to see the performance impact of this change. I did 10 page loads with both versions (before and after the patch) and got these results:

no patch: 3309, 3401, 3193, 3455, 3186, 3350, 3305, 3214, 3339, 3396
mean: 3314.8
stddev: 92.628

with patch: 3490, 3584, 3701, 3723, 3562, 3661, 3468, 3774, 3595, 3560
mean: 3611.8
stddev: 100.43

times are in milliseconds, and indicate a ~9% degradation in performance on static content off the SD card (presumably this degradation will be less significant on network content).
Comment 7 Kartikaya Gupta (email:kats@mozilla.com) 2011-10-25 07:24:29 PDT
Created attachment 569362 [details] [diff] [review]
Implement visited link styles (v5)

Updated patch with a change to GlobalHistory, and rebased to latest birch. This removes the synchronization requirement at the cost of creating many more Runnable objects. It does appear to result in faster page load time though.

On the same static twitter page (but with the patch rebased to new code) I now get these numbers:

no-patch: 3443, 3529, 3634, 3419, 3553, 3480, 3706, 3428, 3563, 3422
mean: 3517.7
sd: 97.955

with-runnable: 3719, 3525, 3532, 3494, 3723, 3706, 3449, 3476, 3741, 3695
mean: 3606
sd: 119.6

This indicates a 2.5% performance hit from implementing visited link styles.
Comment 8 Brad Lassey [:blassey] (use needinfo?) 2011-10-25 08:09:33 PDT
Comment on attachment 569362 [details] [diff] [review]
Implement visited link styles (v5)

Review of attachment 569362 [details] [diff] [review]:
-----------------------------------------------------------------

::: embedding/android/GlobalHistory.java
@@ +117,5 @@
> +
> +    public void checkUriVisited(final String uri) {
> +        mHandler.post(new Runnable() {
> +            public void run() {
> +                mPendingUris.add(uri);

please add comment that this doesn't need to be thread safe because it all runs on the same thread
Comment 9 Kartikaya Gupta (email:kats@mozilla.com) 2011-10-25 08:40:36 PDT
Created attachment 569386 [details] [diff] [review]
Implement visited link styles (v6)

Added comments as requested
Comment 10 Mark Finkle (:mfinkle) (use needinfo?) 2011-10-25 10:57:11 PDT
https://hg.mozilla.org/projects/birch/rev/0f3b96698f17
Comment 11 Aaron Train [:aaronmt] 2011-10-26 06:34:11 PDT
Mozilla/5.0 (Android; Linux armv7l; rv:10.0a1) Gecko/20111026 Firefox/10.0a1 Fennec/10.0a1
Comment 12 Gian-Carlo Pascutto [:gcp] 2011-12-16 02:19:50 PST
>please add comment that this doesn't need to be thread safe because it all runs on 
>the same thread

The actual comment says:
                // this runs on the same handler thread as the processing loop,
                // so synchronization needed

I think a crucial "no" is missing there?
Comment 13 Kartikaya Gupta (email:kats@mozilla.com) 2011-12-16 08:55:30 PST
Created attachment 582288 [details] [diff] [review]
Fix comment

Good catch, thanks!
Comment 14 Kartikaya Gupta (email:kats@mozilla.com) 2011-12-19 06:28:25 PST
Landed on inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/a55e32a938ea
Comment 15 Matt Brubeck (:mbrubeck) 2011-12-19 11:17:14 PST
Comment fix: https://hg.mozilla.org/mozilla-central/rev/a55e32a938ea
Comment 16 Andreea Pod 2012-01-17 07:18:56 PST
Testcase added: https://litmus.mozilla.org/show_test.cgi?id=44976

Note You need to log in before you can comment on or make changes to this bug.