Closed
Bug 949639
Opened 11 years ago
Closed 11 years ago
Move CanAddURI to nsAndroidHistory
Categories
(Firefox for Android Graveyard :: Data Providers, defect)
Tracking
(firefox27 fixed, firefox28 fixed, firefox29 fixed)
RESOLVED
FIXED
Firefox 29
People
(Reporter: mfinkle, Assigned: mfinkle)
References
Details
Attachments
(1 file)
(deleted),
patch
|
blassey
:
review+
lsblakk
:
approval-mozilla-aurora+
bajaj
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
We implement CanAddURI in GlobalHistory.java 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)
Updated•11 years ago
|
Attachment #8346748 -
Flags: review?(blassey.bugs) → review+
Assignee | ||
Comment 1•11 years ago
|
||
(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.
Assignee | ||
Comment 2•11 years ago
|
||
Comment 3•11 years ago
|
||
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
Assignee | ||
Comment 4•11 years ago
|
||
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 5•11 years ago
|
||
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 6•11 years ago
|
||
Assignee | ||
Comment 7•11 years ago
|
||
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?
Updated•11 years ago
|
Attachment #8346748 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 8•11 years ago
|
||
status-firefox27:
--- → fixed
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•