Closed Bug 695170 Opened 8 years ago Closed 8 years ago

History, visited link styles

Categories

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

ARM
Android
defect

Tracking

()

VERIFIED FIXED
Tracking Status
firefox11 --- fixed
fennec 11+ ---

People

(Reporter: elan, Assigned: kats)

Details

(Keywords: feature)

Attachments

(2 files, 6 obsolete files)

Currently not supported due to removing places db
Priority: -- → P2
Assignee: nobody → kgupta
Attached patch Proposed patch (obsolete) — Splinter Review
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)
Attached patch Implement visited link styles (obsolete) — Splinter Review
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)
Attached patch Implement visited link styles (obsolete) — Splinter Review
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+
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+
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+
Whiteboard: [QA+]
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+
Added comments as requested
Attachment #569362 - Attachment is obsolete: true
Attachment #569386 - Flags: review+
https://hg.mozilla.org/projects/birch/rev/0f3b96698f17
Status: NEW → RESOLVED
Closed: 8 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?
Attached patch Fix commentSplinter Review
Good catch, thanks!
Attachment #582288 - Flags: review?(gpascutto)
Attachment #582288 - Flags: review?(gpascutto) → review+
Flags: in-litmus?(andreea.pod)
tracking-fennec: --- → 11+
Keywords: feature
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.