Closed Bug 949639 Opened 6 years ago Closed 6 years ago

Move CanAddURI to nsAndroidHistory


(Firefox for Android :: Data Providers, defect)

Not set



Firefox 29
Tracking Status
firefox27 --- fixed
firefox28 --- fixed
firefox29 --- fixed


(Reporter: mfinkle, Assigned: mfinkle)


(Blocks 1 open bug)



(1 file)

We implement CanAddURI in which is not optimal. nsAndroidHistory.cpp will call VisitURI and SetURITitle via JNI into GeckoAppShell, which starts a background thread and calls the GlobalHistory methods. Those methods then call the local CanAddURI, which can return false. Why bother to make a JNI call and post a runnable to the background thread if we are only going to ignore the visit?

Let's move CanAddURI back into nsAndroidHistory. GlobalHistory.add and GlobalHistory.update are only called via the JNI stub in GeckoAppShell. We should be fine with moving CanAddVisit into nsAndroidHistory.

Patch seems to work fine. I will also try to add some simple tests in a different patch.
Attachment #8346748 - Flags: review?(blassey.bugs)
Attachment #8346748 - Flags: review?(blassey.bugs) → review+
(In reply to Mark Finkle (:mfinkle) from comment #0)

> Patch seems to work fine. I will also try to add some simple tests in a
> different patch.

Seems like the code is not very testable on it's own. Existing tests for history UI do well enough, but we should add a public API around nsAndroidHistory so JS can use it for some history notifications like nsINavHistoryService on desktop. This will be needed for bug 894628 anyway.
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
Blocks: 947390
Comment on attachment 8346748 [details] [diff] [review]
Move CanAddURI v0.1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): I want to see if this patch affects the "SQL DB Locked" crashes we get on all channels but Nightly. It could be that Nightly is too small, but this patch is safe and I'd like to see if it has an affect on the crashes on Aurora. If it has an affect, we can request moving to Beta.

User impact if declined: Just testing a theory. The "SQL DB Locked" crashes are in the top 5 for Beta and Release.

Testing completed (on m-c, etc.): It's been on m-c for a while
Risk to taking this patch (and alternatives if risky): Low risk
String or IDL/UUID changes made by this patch: None
Attachment #8346748 - Flags: approval-mozilla-aurora?
Comment on attachment 8346748 [details] [diff] [review]
Move CanAddURI v0.1

wfm, nominate for tracking on beta if this does what you hope.
Attachment #8346748 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8346748 [details] [diff] [review]
Move CanAddURI v0.1

[Approval Request Comment]
No crashes on Aurora since 12/31/2013. Let's uplift to Beta and see if it's just coincidence.
Attachment #8346748 - Flags: approval-mozilla-beta?
Attachment #8346748 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.