Closed Bug 1139379 Opened 9 years ago Closed 9 years ago

Rename "Form & search history" to "Form history" under clear private data

Categories

(Firefox for Android Graveyard :: Settings and Preferences, defect)

ARM
Android
defect
Not set
normal

Tracking

(firefox36 affected, firefox37 affected, firefox38 affected, firefox39 affected, firefox40 verified, fennec39+)

RESOLVED FIXED
Firefox 40
Tracking Status
firefox36 --- affected
firefox37 --- affected
firefox38 --- affected
firefox39 --- affected
firefox40 --- verified
fennec 39+ ---

People

(Reporter: cos_flaviu, Assigned: AndyP)

References

Details

Attachments

(1 file, 4 obsolete files)

Environment: 
Device: Asus Transformer Tab(Android 4.2.1);
Build: Nightly 39.0a1 (2015-03-04);

Steps to reproduce:
1. Go to Settings -> Privacy -> Clear now;

Expected result:
"Form history" is the 3rd option.

Actual result:
"Form & search history" is the 3rd option.

Notes:
The name is misleading because the meaning is similar to "Browsing history".
Thoughts?
Flags: needinfo?(alam)
My initial thought was: "How did we not see the 'search history' part and add the clearing of search history to that checkbox!?"

Personally, I'd rather see the search history cleared with form data.

Anyone else?
Flags: needinfo?(margaret.leibovic)
(In reply to Mark Finkle (:mfinkle) from comment #2)
> My initial thought was: "How did we not see the 'search history' part and
> add the clearing of search history to that checkbox!?"
> 
> Personally, I'd rather see the search history cleared with form data.
> 
> Anyone else?

Oh, we did see that, and I thought it would make more sense to go in "browsing data", since that form history entry just refers to stuff you're doing in content.

I think that we should lump together the search and browsing history, since it's "stuff you type in the urlbar" vs. "stuff you type in webpages".

We should update the strings to reflect that.
tracking-fennec: --- → ?
Flags: needinfo?(margaret.leibovic)
It looks like Desktop uses the same "Form data and search history" checkbox. I think it's because search history (previously typed items in the Search Box) is saved as form data on Desktop.
(In reply to Mark Finkle (:mfinkle) from comment #4)
> It looks like Desktop uses the same "Form data and search history" checkbox.
> I think it's because search history (previously typed items in the Search
> Box) is saved as form data on Desktop.

Oh, that makes lots of sense. So I think we should update the clear data list to include:

"Browsing & search history"
"Form history"

Or alternately, we could add a new item explicitly for search history, since we do store that separately.

Andy, would you be interested in working on a patch for this?

I also wonder if we should change what we landed in bug 1124884 to make clearing search history part of that "Form & search history" item until these new strings ride the trains.
Blocks: 1124884
Flags: needinfo?(drag)
(In reply to :Margaret Leibovic from comment #5)
> (In reply to Mark Finkle (:mfinkle) from comment #4)
> > It looks like Desktop uses the same "Form data and search history" checkbox.
> > I think it's because search history (previously typed items in the Search
> > Box) is saved as form data on Desktop.
> 
> Oh, that makes lots of sense. So I think we should update the clear data
> list to include:
> 
> "Browsing & search history"
> "Form history"
> 
> Or alternately, we could add a new item explicitly for search history, since
> we do store that separately.

+1

how about "Form data" though? it seems more consistent with Desktop
Flags: needinfo?(alam)
(In reply to Anthony Lam (:antlam) from comment #6)
> (In reply to :Margaret Leibovic from comment #5)
> > (In reply to Mark Finkle (:mfinkle) from comment #4)
> > > It looks like Desktop uses the same "Form data and search history" checkbox.
> > > I think it's because search history (previously typed items in the Search
> > > Box) is saved as form data on Desktop.
> > 
> > Oh, that makes lots of sense. So I think we should update the clear data
> > list to include:
> > 
> > "Browsing & search history"
> > "Form history"
> > 
> > Or alternately, we could add a new item explicitly for search history, since
> > we do store that separately.
> 
> +1
> 
> how about "Form data" though? it seems more consistent with Desktop

I think that sounds good. So we would have:

"Browsing history"
"Search history"
...
"Form data"
...
(In reply to :Margaret Leibovic from comment #5)
> Andy, would you be interested in working on a patch for this?

I'd be interested in working on this.
Flags: needinfo?(drag)
(In reply to Andy Pusch [:AndyP] from comment #8)
> (In reply to :Margaret Leibovic from comment #5)
> > Andy, would you be interested in working on a patch for this?
> 
> I'd be interested in working on this.

Awesome! Let me know if you need help figuring out how to do this :)
Assignee: nobody → drag
tracking-fennec: ? → 39+
(In reply to Anthony Lam (:antlam) from comment #6)
> (In reply to :Margaret Leibovic from comment #5)
> > (In reply to Mark Finkle (:mfinkle) from comment #4)
> > > It looks like Desktop uses the same "Form data and search history" checkbox.
> > > I think it's because search history (previously typed items in the Search
> > > Box) is saved as form data on Desktop.
> > 
> > Oh, that makes lots of sense. So I think we should update the clear data
> > list to include:
> > 
> > "Browsing & search history"
> > "Form history"
> > 
> > Or alternately, we could add a new item explicitly for search history, since
> > we do store that separately.
> 
> +1
> 
> how about "Form data" though? it seems more consistent with Desktop

Desktop doesn't use "Form data". I see "Form and search history", which implies "Form history" to me.
I've tried to rename the two options and add a dummy option for search history to the menu. This is as far as I got. Fennec crashes as soon as I tap on "Privacy" in the settings. Can you give me a few pointers on what I did wrong?
Attachment #8574132 - Flags: feedback?(margaret.leibovic)
Comment on attachment 8574132 [details] [diff] [review]
bug1139379_renameClearPrivateDataOptions.diff

Review of attachment 8574132 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/locales/en-US/android_strings.dtd
@@ +226,5 @@
>  <!ENTITY pref_import_android "Import from Android">
>  <!ENTITY pref_import_android_summary "Import bookmarks and history from the native browser">
>  <!ENTITY pref_private_data_history2 "Browsing history">
> +<!ENTITY pref_private_data_searchHistory "Browsing history">
> +<!ENTITY pref_private_data_formdata "Form data">

You should rev this entity (you can add a "2" to the end of it to do this), so that localizers know this has changed.

::: mobile/android/base/resources/values/arrays.xml
@@ +72,5 @@
>          <item>android_import.data.history</item>
>      </string-array>
>      <string-array name="pref_private_data_entries">
>          <item>@string/pref_private_data_history2</item>
> +        <item>@string/pref_private_data_searchHistory</item>

I suspect the reason you're seeing a crash is that you didn't also add a new item to the "pref_private_data_defaults" array below:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/values/arrays.xml#84
Attachment #8574132 - Flags: feedback?(margaret.leibovic) → feedback+
First of all, sorry for the late response. It does work now thanks. I have another question.
I'm about to add a new method to GeckoApp.java to clear the search history only. This of course changes things I've done in bug #1124884. I'd like to remove the changes from LocalBrowserDB and put the code for clearing the search history in GeckoApp.java. Would that be alright? 
Alternatively I could add a clearSearchHistory method to LocalBrowserDB but then I'd have to either add that method to BrowserDB as well or add a cast to LocalBrowserDB in GeckoApp.java.
Flags: needinfo?(margaret.leibovic)
(In reply to Andy Pusch [:AndyP] from comment #13)
> First of all, sorry for the late response. It does work now thanks. I have
> another question.
> I'm about to add a new method to GeckoApp.java to clear the search history
> only. This of course changes things I've done in bug #1124884. I'd like to
> remove the changes from LocalBrowserDB and put the code for clearing the
> search history in GeckoApp.java. Would that be alright? 
> Alternatively I could add a clearSearchHistory method to LocalBrowserDB but
> then I'd have to either add that method to BrowserDB as well or add a cast
> to LocalBrowserDB in GeckoApp.java.

I'm not a fan of adding more logic to GeckoApp. In fact, I wonder why our "Sanitize:ClearHistory" message handler lives there, instead of in BrowserApp (or ideally in some other helper class). Can you file a follow-up bug for moving that message handler out of GeckoApp?

But to minimize the scope of this bug, let's add a parameter to go along with the "Sanitize:ClearHistory" message to indicate whether we want to clear search history or browsing history. Then, we should update handleClearHistory to either clear browsing history or search history.

I think we should continue to abstract away the content provider logic behind the BrowserDB interface, so I think you should add a new clearSearchHistory method, and perhaps rename the current clearHistory method to clearBrowsingHistory.
Flags: needinfo?(margaret.leibovic)
> Can you file a follow-up
> bug for moving that message handler out of GeckoApp?

Sure I'll do that once we are finished here.

> But to minimize the scope of this bug, let's add a parameter to go along
> with the "Sanitize:ClearHistory" message to indicate whether we want to
> clear search history or browsing history.

I'm not quite sure on how to approach this. I thought that Sanitizer receives the item selected in the menu (private.data.searchHistory) and checks if there is a "searchHistory clear" function to call. However that does not seem to work. When I select only "Clear history" nothing happens. 
Can you help me out a little? :)
Attachment #8574132 - Attachment is obsolete: true
Attachment #8580759 - Flags: feedback?(margaret.leibovic)
(In reply to Andy Pusch [:AndyP] from comment #15)
> Created attachment 8580759 [details] [diff] [review]
> bug1139379_renameClearPrivateDataOptions.diff -v2
> 
> > Can you file a follow-up
> > bug for moving that message handler out of GeckoApp?
> 
> Sure I'll do that once we are finished here.
> 
> > But to minimize the scope of this bug, let's add a parameter to go along
> > with the "Sanitize:ClearHistory" message to indicate whether we want to
> > clear search history or browsing history.
> 
> I'm not quite sure on how to approach this. I thought that Sanitizer
> receives the item selected in the menu (private.data.searchHistory) and
> checks if there is a "searchHistory clear" function to call. However that
> does not seem to work. When I select only "Clear history" nothing happens. 
> Can you help me out a little? :)

Ah, sorry, you can just add more data to go along with the "Sanitize:ClearHistory" message by adding more properties in that message JSON object. So instead of:

Messaging.sendRequestForResult({ type: "Sanitize:ClearHistory" })...

You could do something like:

Messaging.sendRequestForResult({
  type: "Sanitize:ClearHistory",
  clearSearchHistory: true
})...


And then in the "Sanitize:ClearHistory" message handler in GeckoApp, you can check whether we're trying to clear search history. You can get that value with something like:

message.optBoolean("clearSearchHistory")

Then you would pass that value along to handleClearHistory and to db.clearHistory to handle the clearing in the content provider.

Hopefully that makes sense. Let me know if you have more questions!
Comment on attachment 8580759 [details] [diff] [review]
bug1139379_renameClearPrivateDataOptions.diff -v2

Review of attachment 8580759 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/GeckoApp.java
@@ +628,1 @@
>              handleClearHistory();

You'll need to change the handleClearHistory implementation to only clear either browsing history or search history, depending on what the user requested.

::: mobile/android/modules/Sanitizer.jsm
@@ +173,5 @@
> +            try {
> +              var predictor = Cc["@mozilla.org/network/predictor;1"].getService(Ci.nsINetworkPredictor);
> +              predictor.reset();
> +            } catch (e) { }
> +          });

It looks like you copied this `then` call from the history logic above. We don't need either of these for search history, so you can just remove this entire `then` call.
Attachment #8580759 - Flags: feedback?(margaret.leibovic) → feedback+
I've made some adjustments but I'm still having the problem that clearing only "Search history" doesn't do anything. It seems like the searchHistory clear function is never called in Sanitizer.jsm. Did I miss something? Thanks again for your help.
Attachment #8580759 - Attachment is obsolete: true
Attachment #8583115 - Flags: feedback?(margaret.leibovic)
(In reply to Andy Pusch [:AndyP] from comment #18)
> Created attachment 8583115 [details] [diff] [review]
> bug1139379_renameClearPrivateDataOptions.diff -v3
> 
> I've made some adjustments but I'm still having the problem that clearing
> only "Search history" doesn't do anything. It seems like the searchHistory
> clear function is never called in Sanitizer.jsm. Did I miss something?
> Thanks again for your help.

Can you log the JSON here to make sure we're sending the right data to JS?
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/preferences/PrivateDataPreference.java#58

You could also add a `debugger` statement in here and use the remote debugger to step through what's happening:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#1475
Comment on attachment 8583115 [details] [diff] [review]
bug1139379_renameClearPrivateDataOptions.diff -v3

Review of attachment 8583115 [details] [diff] [review]:
-----------------------------------------------------------------

Looking good, this is on the right track.

::: mobile/android/base/GeckoApp.java
@@ +625,5 @@
>              mPrivateBrowsingSession = message.optString("session", null);
>  
>          } else if ("Sanitize:ClearHistory".equals(event)) {
> +            final boolean clearSearchHistory = false;
> +            handleClearHistory(message.optBoolean("clearSearchHistory", clearSearchHistory));

You don't need to create a local variable here if you're just passing it in as the default value to optBoolean (you can just pass false directly).

::: mobile/android/modules/Sanitizer.jsm
@@ +161,5 @@
>  
> +    searchHistory: {
> +      clear: function ()
> +      {
> +        return Messaging.sendRequestForResult({ type: "Sanitize:clearHistory", clearSearchHistory: true })

Looks like you have a typo here, and this should be "Sanitize:ClearHistory". That might be why you weren't seeing anything happen.
Attachment #8583115 - Flags: feedback?(margaret.leibovic) → feedback+
(In reply to :Margaret Leibovic from comment #20)
> Looks like you have a typo here, and this should be "Sanitize:ClearHistory".
> That might be why you weren't seeing anything happen.

Thanks! That was the problem. :)

I've tested it on my smartphone and it seems to work.
I've addressed comment #20 and also changed "Form data" to "Form history".

try submission: https://treeherder.mozilla.org/#/jobs?repo=try&revision=699149305556
Attachment #8583115 - Attachment is obsolete: true
Attachment #8583409 - Flags: review?(margaret.leibovic)
(In reply to Andy Pusch [:AndyP] from comment #21)
> Created attachment 8583409 [details] [diff] [review]
> bug1139379_renameClearPrivateDataOptions.diff -v4
> 
> (In reply to :Margaret Leibovic from comment #20)
> > Looks like you have a typo here, and this should be "Sanitize:ClearHistory".
> > That might be why you weren't seeing anything happen.
> 
> Thanks! That was the problem. :)
> 
> I've tested it on my smartphone and it seems to work.
> I've addressed comment #20 and also changed "Form data" to "Form history".
> 
> try submission:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=699149305556

Thanks for the try push. It looks like you need to update testSettingsMenuItems:

https://treeherder.mozilla.org/logviewer.html#?job_id=5955937&repo=try
Comment on attachment 8583409 [details] [diff] [review]
bug1139379_renameClearPrivateDataOptions.diff -v4

Review of attachment 8583409 [details] [diff] [review]:
-----------------------------------------------------------------

Awesome, this looks great! Please just write a small follow-up patch to fix the test, and then we can land this.
Attachment #8583409 - Flags: review?(margaret.leibovic) → review+
I've modified the test "testSettingsMenuItems".

try submission: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d06c9435d4d4
Attachment #8583409 - Attachment is obsolete: true
Attachment #8585723 - Flags: review?(margaret.leibovic)
Comment on attachment 8585723 [details] [diff] [review]
bug1139379_renameClearPrivateDataOptions.diff -v5

Review of attachment 8585723 [details] [diff] [review]:
-----------------------------------------------------------------

Awesome, looks good!
Attachment #8585723 - Flags: review?(margaret.leibovic) → review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/01b90618d019
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/01b90618d019
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 40
Verified as fixed in build 40.0a1 2015-04-03;
Device: Motorola Razr (Android 4.1.2).
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: