If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Notify Gecko when browser history is cleared from HistoryPanel

VERIFIED FIXED in Firefox 33

Status

()

Firefox for Android
General
VERIFIED FIXED
3 years ago
a year ago

People

(Reporter: liuche, Assigned: liuche)

Tracking

Trunk
Firefox 35
ARM
Android
Points:
---

Firefox Tracking Flags

(firefox33+ verified, firefox34+ verified, firefox35+ verified, fennec33+)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

3 years ago
This is a follow-up from bug 1031273 - my inclination is to also clear everything from Recent Tabs when we clear browsing history from the History tab.
(Assignee)

Comment 1

3 years ago
Yuan, what do you think? Is it confusing to clear (or not clear) Recent Tabs?
Flags: needinfo?(ywang)
I agree. Recent tabs is part of browsing history. It makes sense to clear recent tabs when clear history.
Flags: needinfo?(ywang)

Updated

3 years ago
tracking-fennec: --- → ?

Comment 3

3 years ago
I thought that the patch in bug 1031273 would fix this. Is this still a problem?
(Assignee)

Comment 4

3 years ago
I took a quick look and it looks like our code for clearing history from the History panel doesn't actually ever hit Gecko, and only makes a call to BrowserDB. This seems like a bug, and we should clear history the same way we clear it from Preferences.
(Assignee)

Updated

3 years ago
Assignee: nobody → liuche

Comment 5

3 years ago
(In reply to Chenxia Liu [:liuche] from comment #4)
> I took a quick look and it looks like our code for clearing history from the
> History panel doesn't actually ever hit Gecko, and only makes a call to
> BrowserDB. This seems like a bug, and we should clear history the same way
> we clear it from Preferences.

Ah, yeah, it just removes history items from the db, instead of performing some more thorough "sanitize" operation. You're right, it does seem like a bug that it doesn't clear cookies or anything :/

The current behavior is in line with what the "Remove" context menu item does, but "Clear browsing history" sets a different expectation.

It's unfortunate we didn't spot this until now, but maybe we can uplift a fix to beta for 33.
(Assignee)

Updated

3 years ago
Summary: Clear Recent Tabs data if "Clear browsing history" has been clicked from History tab → Notify Gecko when browser history is cleared from HistoryPanel
(Assignee)

Comment 6

3 years ago
Created attachment 8483009 [details] [diff] [review]
Patch: Notify Gecko

Instead of just clearing history in BrowserDB, send a message to Gecko to properly clear history.

Margaret, I just included history because the implication of a button in the HistoryPanel is that we're clearing history items. I'm not sure if we should include cookies, form & search history, site settings, etc.

Yuan, Anthony - do you have any thoughts on what should be grouped under "Clear browsing history" besides History (if anything at all)?
Attachment #8483009 - Flags: review?(margaret.leibovic)
Flags: needinfo?(ywang)
Flags: needinfo?(alam)

Comment 7

3 years ago
Comment on attachment 8483009 [details] [diff] [review]
Patch: Notify Gecko

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

This is a definite improvement because it will now clear session history.

I mentioned cookies in my last comment because my brain associated that with session history, but I agree it feels more like "private data" than "browsing history". So I think this patch is correct.

::: mobile/android/base/home/HistoryPanel.java
@@ +127,5 @@
>                  dialogBuilder.setPositiveButton(R.string.button_ok, new AlertDialog.OnClickListener() {
>                      @Override
>                      public void onClick(final DialogInterface dialog, final int which) {
>                          dialog.dismiss();
>                          ThreadUtils.postToBackgroundThread(new Runnable() {

This doesn't need to be run on the background thread anymore, so we can get rid of this surrounding runnable.
Attachment #8483009 - Flags: review?(margaret.leibovic) → review+
I think there is benefit clearing all related items at once, especially for non-technical users.
This is another terminology issue. I think whatever decision we make will need to be consistent across desktop and mobile. 

On desktop, "clear recent history" pre-selects a few things:
- browsing & download history
- form & search history
- cookies
- cache
- active logins

"Browsing history" is one section of "History", for desktop Firefox.

If we are adding other types of data, we should change the term to "Clear history" from "Clear browsing history", just to be accurate and consistent.
Flags: needinfo?(ywang)
(Assignee)

Comment 9

3 years ago
https://hg.mozilla.org/integration/fx-team/rev/013d88f8949f

It seems like we should keep things the way they are now, and file a bug later per Yuan's comment 8 with string changes if we decide to go that direction.
(Assignee)

Comment 10

3 years ago
Created attachment 8483074 [details] [diff] [review]
Patch: Notify Gecko

Approval Request Comment
[Feature/regressing bug #]: Clear history from History panel
[User impact if declined]: Recent tabs won't be cleared when clearing history from the History panel
[Describe test coverage new/current, TBPL]: local testing
[Risks and why]: very low, changes path to use an existing Gecko notification

Will request beta after baking.
[String/UUID change made/needed]:
Attachment #8483009 - Attachment is obsolete: true
Attachment #8483074 - Flags: review+
Attachment #8483074 - Flags: approval-mozilla-aurora?
Flags: needinfo?(alam)
(Assignee)

Comment 11

3 years ago
[String/UUID change made/needed]: No string changes.
(Assignee)

Updated

3 years ago
status-firefox33: --- → affected
status-firefox34: --- → affected
status-firefox35: --- → affected
https://hg.mozilla.org/mozilla-central/rev/013d88f8949f
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
(Assignee)

Updated

3 years ago
Depends on: 998009
tracking-fennec: ? → 33+
[Tracking Requested - why for this release]:
Reression

Chenxia - Can you request an uplift to Beta too
tracking-firefox33: --- → ?
tracking-firefox34: --- → ?
tracking-firefox35: --- → ?
Flags: needinfo?(liuche)
(Assignee)

Comment 14

3 years ago
Comment on attachment 8483074 [details] [diff] [review]
Patch: Notify Gecko

Approval Request Comment
[Feature/regressing bug #]: Clear history from History panel
[User impact if declined]: Recent tabs won't be cleared when clearing history from the History panel
[Describe test coverage new/current, TBPL]: local testing, baking in Nightly
[Risks and why]: very low, changes path to use an existing Gecko notification
[String/UUID change made/needed]: none
Attachment #8483074 - Flags: approval-mozilla-beta?
Flags: needinfo?(liuche)
status-firefox35: affected → fixed
tracking-firefox33: ? → +
tracking-firefox34: ? → +
tracking-firefox35: ? → +
Comment on attachment 8483074 [details] [diff] [review]
Patch: Notify Gecko

Approved for beta and aurora. Let's get this into 33 beta2.
Attachment #8483074 - Flags: approval-mozilla-beta?
Attachment #8483074 - Flags: approval-mozilla-beta+
Attachment #8483074 - Flags: approval-mozilla-aurora?
Attachment #8483074 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/077f8d985433
https://hg.mozilla.org/releases/mozilla-beta/rev/957e1ef7f769
status-firefox33: affected → fixed
status-firefox34: affected → fixed
Verified as fixed on
Builds:
Firefox for Android 33
Firefox for Android 34 Beta 1
Firefox for Android 35.0a2 (2014-10-15)

Device: Asus Transformer Pad TF300T (Android 4.2.1)
Status: RESOLVED → VERIFIED
status-firefox33: fixed → verified
status-firefox34: fixed → verified
status-firefox35: fixed → verified
You need to log in before you can comment on or make changes to this bug.