The default bug view has changed. See this FAQ.

History, visited link styles

VERIFIED FIXED

Status

()

Firefox for Android
General
P2
normal
VERIFIED FIXED
6 years ago
8 months ago

People

(Reporter: elan, Assigned: kats)

Tracking

({feature})

unspecified
ARM
Android
feature
Points:
---
Bug Flags:
in-litmus +

Firefox Tracking Flags

(firefox11 fixed, fennec11+)

Details

Attachments

(2 attachments, 6 obsolete attachments)

(Reporter)

Description

6 years ago
Currently not supported due to removing places db
Priority: -- → P2
Assignee: nobody → kgupta
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.
Attachment #568379 - Flags: review?(blassey.bugs)
Created attachment 568441 [details] [diff] [review]
Implement visited link styles

Works now, with no obvious problems/warnings showing up in logcat output.
Attachment #568441 - Flags: review?(blassey.bugs)
Attachment #568379 - Attachment is obsolete: true
Attachment #568379 - Flags: review?(blassey.bugs)
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.
Attachment #568441 - Attachment is obsolete: true
Attachment #568441 - Flags: review?(blassey.bugs)
Attachment #568498 - Flags: review?(blassey.bugs)
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
Attachment #568498 - Flags: review?(blassey.bugs) → review+
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
Attachment #568498 - Attachment is obsolete: true
Attachment #568653 - Flags: review+
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).
Attachment #568653 - Attachment is obsolete: true
Attachment #569144 - Flags: review+

Updated

6 years ago
Whiteboard: [QA+]
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.
Attachment #569144 - Attachment is obsolete: true
Attachment #569362 - Flags: review?(blassey.bugs)
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
Attachment #569362 - Flags: review?(blassey.bugs) → review+
Created attachment 569386 [details] [diff] [review]
Implement visited link styles (v6)

Added comments as requested
Attachment #569362 - Attachment is obsolete: true
Attachment #569386 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/projects/birch/rev/0f3b96698f17
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Mozilla/5.0 (Android; Linux armv7l; rv:10.0a1) Gecko/20111026 Firefox/10.0a1 Fennec/10.0a1
Status: RESOLVED → VERIFIED
Keywords: checkin-needed
OS: Mac OS X → Android
Hardware: x86 → ARM
Whiteboard: [QA+]
>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?
Created attachment 582288 [details] [diff] [review]
Fix comment

Good catch, thanks!
Attachment #582288 - Flags: review?(gpascutto)
Attachment #582288 - Flags: review?(gpascutto) → review+
Landed on inbound:

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

Updated

5 years ago
Flags: in-litmus?(andreea.pod)
tracking-fennec: --- → 11+
status-firefox11: --- → fixed
(Reporter)

Updated

5 years ago
Keywords: feature

Comment 16

5 years ago
Testcase added: https://litmus.mozilla.org/show_test.cgi?id=44976
Flags: in-litmus?(andreea.pod) → in-litmus+
You need to log in before you can comment on or make changes to this bug.