Clear/update GlobalHistory when clearing history from Clear Private Data or after a sync

ASSIGNED
Assigned to

Status

()

ASSIGNED
3 years ago
3 years ago

People

(Reporter: rnewman, Assigned: astanisz1, Mentored)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good next bug][lang=java])

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

3 years ago
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
(Assignee)

Comment 1

3 years ago
Hi, I would like to resolve this issue as my first bug.
(Reporter)

Comment 2

3 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!
(Assignee)

Comment 3

3 years ago
Hi, I would like to add a test - where should I locate it?

Comment 4

3 years ago
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 ?

Comment 5

3 years ago
Created attachment 8719343 [details] [diff] [review]
Clearing GlobalHistory VisitedCache in PrivateDataPreference

Comment 6

3 years ago
Created attachment 8719344 [details] [diff] [review]
Clearing GlobalHistory VisitedCache in PrivateDataPreference
Attachment #8719343 - Attachment is obsolete: true
(Assignee)

Comment 7

3 years ago
Created attachment 8719487 [details]
MozReview Request: Bug 1197869 - Clearing GlobalHistory cache after clearing history from Clear Private Data; r?rnewman

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)
(Assignee)

Comment 8

3 years ago
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

3 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

3 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

3 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

3 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

3 years ago
Attachment #8719487 - Flags: feedback+
(Assignee)

Comment 13

3 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

3 years ago
Created attachment 8721246 [details]
MozReview Request: Bug 1197869 - Clearing GlobalHistory cache after Sanitize:ClearHistory message; r?rnewman

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

3 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

3 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

3 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.
You need to log in before you can comment on or make changes to this bug.