695170 - History, visited link styles

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P2)

Description

[Erin Lancaster [:elan]]

Currently not supported due to removing places db

Comment 1

[Kartikaya Gupta (email:[e-mail])]

Attachment: 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:[e-mail])]

Attachment: Implement visited link styles

Works now, with no obvious problems/warnings showing up in logcat output.

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Attachment: 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?)]

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:[e-mail])]

Attachment: 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:[e-mail])]

Attachment: 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:[e-mail])]

Attachment: 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?)]

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:[e-mail])]

Attachment: Implement visited link styles (v6)

Added comments as requested

Comment 10

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

https://hg.mozilla.org/projects/birch/rev/0f3b96698f17

Comment 11

[Aaron Train [:aaronmt]]

Mozilla/5.0 (Android; Linux armv7l; rv:10.0a1) Gecko/20111026 Firefox/10.0a1 Fennec/10.0a1

Comment 12

[Gian-Carlo Pascutto [:gcp]]

>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:[e-mail])]

Attachment: Fix comment

Good catch, thanks!

Comment 14

[Kartikaya Gupta (email:[e-mail])]

Landed on inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/a55e32a938ea

Comment 15

[Matt Brubeck (:mbrubeck)]

Comment fix: https://hg.mozilla.org/mozilla-central/rev/a55e32a938ea

Comment 16

[Andreea Pod]

Testcase added: https://litmus.mozilla.org/show_test.cgi?id=44976