Closed
Bug 1197869
Opened 9 years ago
Closed 3 years ago
Clear/update GlobalHistory when clearing history from Clear Private Data or after a sync
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(Not tracked)
RESOLVED
INCOMPLETE
People
(Reporter: rnewman, Assigned: astanisz1, Mentored)
Details
(Whiteboard: [good next bug][lang=java])
Attachments
(3 files, 1 obsolete file)
As far as I can tell, we never blank GlobalHistory's visited link set. That means link highlighting will be stale until the browser is restarted. We should attempt to throw away this cache whenever appropriate, probably via a LocalBroadcast. https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/GlobalHistory.java
Reporter | ||
Comment 2•8 years ago
|
||
Hi Anita! To start, set up a build environment - you can see the instructions here: https://wiki.mozilla.org/Mobile/Fennec/Android Then, you'll need to create a patch to upload - see https://wiki.mozilla.org/Mobile/Fennec/Android#Creating_commits_and_submitting_patches You'll need to familiarize yourself with how Clear Private Data works in Fennec: we send an event from the Java UI to Gecko here: https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/preferences/PrivateDataPreference.java#58 then we have a static instance of GlobalHistory: https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/GlobalHistory.java We want to clear the visited set: https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/GlobalHistory.java#46 whenever the user requests to clear their history. Come find us on irc.mozilla.org, #mobile, or let us know here, if you have questions!
I figured I would take a look at this one. Could it be as simple as calling GlobalHistory.getInstance().mVisitedCache.clear() just below GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("Sanitize:ClearData", json.toString())); in PrivateDataPreference ?
Attachment #8719343 -
Attachment is obsolete: true
Review commit: https://reviewboard.mozilla.org/r/34977/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/34977/
Attachment #8719487 -
Flags: review?(rnewman)
I also would like to commit an unit test but I don't know where is the best place to locate it.
Reporter | ||
Comment 9•8 years ago
|
||
Sorry for the delayed response, Anita; both of the mentors on this bug are enjoying a holiday weekend in the US. I'll give you some feedback and a pointer for the test in a moment.
Reporter | ||
Comment 10•8 years ago
|
||
(In reply to Anita from comment #8) > I also would like to commit an unit test but I don't know where is the best > place to locate it. In this case you probably want to add a Robocop test, which you'll do here: /mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests You can look at testOSLocale.java and testEventDispatcher.java as sources of inspiration.
Reporter | ||
Comment 11•8 years ago
|
||
Comment on attachment 8719487 [details] MozReview Request: Bug 1197869 - Clearing GlobalHistory cache after clearing history from Clear Private Data; r?rnewman https://reviewboard.mozilla.org/r/34977/#review31551 ::: mobile/android/base/java/org/mozilla/gecko/GeckoAppShell.java:290 (Diff revision 1) > + public static void clearPrivateData(String data){ We prefer not to add things to `GeckoAppShell` if we can avoid it -- it's the entry point for calls via JNI, which we don't need here. You're not wrong in adding a method here -- it encapsulates to make sure you can't clear private data without also having the opportunity to clear GlobalHistory. But there's a little more elegant way to do this, and there's a nuance that this linkage doesn't address. The nuance is that not all `ClearData` operations intend to clear history. There's no point in blowing away the cache if we're not actually deleting history. You can see the way we construct a JSON object in `PrivateDataPreference.java` to specify which boxes are checked. To do this right we need to look in that object. But! There is a more elegant way to do this that happens to take care of that nuance. Messages are passed over the bridge, including `Sanitize:ClearData` (Java to Gecko), `Sanitize:ClearHistory` (Gecko to Java), and `Sanitize:Finished` (Gecko to Java). (These messages tie in to the Sanitizer on the Gecko side. See modules/Sanitizer.jsm.) We can listen for those messages in `GlobalHistory`, just as `BrowserApp` does. So it _should_ work to have `GlobalHistory` listen for `Sanitize:ClearHistory`, just like `BrowserApp`, and drop the cache in response to that message. That's elegant because no matter how `ClearHistory` is triggered -- Clear Private Data, or some action in Gecko -- we'll end up dropping the cache. To test this, you'd send `Sanitize:ClearHistory`, and make sure that the right thing happens. Easier than clicking around in the UI via test code. ::: mobile/android/base/java/org/mozilla/gecko/GlobalHistory.java:161 (Diff revision 1) > + mVisitedCache.clear(); `mVisitedCache` is a `SoftReference`, so you need to dereference it and also check whether it points to `null`. Here you'd need to do this: ``` Set<String> cache = this.mVisitedCache.get(); if (cache != null) { cache.clear(); } ```
Attachment #8719487 -
Flags: review?(rnewman)
Reporter | ||
Comment 12•8 years ago
|
||
(In reply to zmiller12 from comment #4) > I figured I would take a look at this one. Hi! Anita is already working on this bug; maybe go take a look at http://www.joshmatthews.net/bugsahoy/?mobileandroid=1 to find another one for yourself?
Assignee: nobody → astanisz1
Status: NEW → ASSIGNED
Reporter | ||
Updated•8 years ago
|
Attachment #8719487 -
Flags: feedback+
Assignee | ||
Comment 13•8 years ago
|
||
Hi, I faced a problem - GlobalHistory is package-private and it is in package org.mozilla.gecko, so I can't access it from org.mozilla.gecko.tests. What should I do in this case?
Assignee | ||
Comment 14•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/35615/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/35615/
Attachment #8721246 -
Flags: review?(rnewman)
Assignee | ||
Comment 15•8 years ago
|
||
I've changed GlobalHistory to public temporarily for tests reasons - on IRC I've got an answer that a reviewer will suggest a proper solution
Reporter | ||
Comment 16•8 years ago
|
||
Comment on attachment 8721246 [details] MozReview Request: Bug 1197869 - Clearing GlobalHistory cache after Sanitize:ClearHistory message; r?rnewman https://reviewboard.mozilla.org/r/35615/#review32555 This is looking good! Note that you'll need to annotate any methods that you call from Robocop with @RobocopTarget, otherwise they might be removed by the optimizer and your tests will fail. Please also squash down these commits. More comments inline. ::: mobile/android/base/java/org/mozilla/gecko/GlobalHistory.java:27 (Diff revision 1) > -class GlobalHistory { > +public class GlobalHistory implements NativeEventListener{ Nit: space between interface name and '{'. ::: mobile/android/base/java/org/mozilla/gecko/GlobalHistory.java:49 (Diff revision 1) > - SoftReference<Set<String>> mVisitedCache; // cache of the visited URI list > + public SoftReference<Set<String>> mVisitedCache; // cache of the visited URI list Rather than making this public, I'd rather have a public method: `cacheIsEmpty`. ::: mobile/android/base/java/org/mozilla/gecko/GlobalHistory.java:165 (Diff revision 1) > - public void clear(){ > + @Override Nit: make indenting match. ::: mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests/testClearGlobalHistory.java:45 (Diff revision 1) > + GlobalHistory.getInstance().mVisitedCache = new SoftReference<>(visitedSet); Rather than doing this -- which potentially invalidates the thing you're testing -- try to simulate a real recording of history. You can do that by navigating to a page within your test page -- indeed, you might find that loading the test page itself adds something to history.
Attachment #8721246 -
Flags: review?(rnewman)
Reporter | ||
Comment 17•8 years ago
|
||
(In reply to Anita from comment #15) > I've changed GlobalHistory to public temporarily for tests reasons - on IRC > I've got an answer that a reviewer will suggest a proper solution I don't see too much of a problem with it being public.
Comment 18•3 years ago
|
||
We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → INCOMPLETE
Updated•3 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
•