'Tabs From Last Time' not wiped on Clear History

VERIFIED FIXED in Firefox 13

Status

()

Firefox for Android
General
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: aaronmt, Assigned: lucasr)

Tracking

Trunk
Firefox 13
ARM
Android
Points:
---

Firefox Tracking Flags

(firefox11 affected, firefox12 affected, firefox13 verified, fennec+)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

6 years ago
Currently when one clears history, tabs listed under 'Tabs From Last Time' are not wiped out. I would have expected this area to be cleared.

--
Samsung Galaxy SII (Android 2.3.4)
Mozilla/5.0 (Android; Linux armv7l; rv:12.0a1) Gecko/20120119 Firefox/12.0a1 Fennec/12.0a1
(Assignee)

Comment 1

6 years ago
Adding Brian to CC since he implemented this feature in about:home.
SessionStore.js listens for a notification to clear it's state:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/SessionStore.js#260

If Java could fire this notification using GeckoEvent, it should just work.

GeckoAppShell.sendEventToGecko(new GeckoEvent("browser:purge-session-history", null));

Should be enough
(In reply to Mark Finkle (:mfinkle) from comment #2)
> SessionStore.js listens for a notification to clear it's state:
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/
> SessionStore.js#260
> 
> If Java could fire this notification using GeckoEvent, it should just work.
> 
> GeckoAppShell.sendEventToGecko(new
> GeckoEvent("browser:purge-session-history", null));
> 
> Should be enough

sessionstore.js is only read once, when about:home is created. Sending browser:purge-session-history will purge sessionstore.js, but the tabs from last time in the UI will persist until the browser is restarted...so we should remove them from the layout.
tracking-fennec: --- → +
(Assignee)

Updated

6 years ago
Blocks: 723103
(Assignee)

Comment 4

6 years ago
Created attachment 593437 [details] [diff] [review]
(1/2) Purge session history files when history is cleared
Attachment #593437 - Flags: review?(mark.finkle)
(Assignee)

Comment 5

6 years ago
Created attachment 593439 [details] [diff] [review]
(1/2) Purge session history files when history is cleared
Attachment #593439 - Flags: review?(mark.finkle)
(Assignee)

Updated

6 years ago
Attachment #593437 - Attachment is obsolete: true
Attachment #593437 - Flags: review?(mark.finkle)
(Assignee)

Comment 6

6 years ago
Created attachment 593440 [details] [diff] [review]
(2/2) Hide last tabs from about:home when history is cleared
Attachment #593440 - Flags: review?(mark.finkle)
Comment on attachment 593439 [details] [diff] [review]
(1/2) Purge session history files when history is cleared


>         if ("clear_history".equalsIgnoreCase(mAction)) {
>             GeckoAppShell.getHandler().post(new Runnable(){
>                 public void run() {
>                     BrowserDB.clearHistory(mContext.getContentResolver());
>                     GeckoApp.mAppContext.mFavicons.clearFavicons();
>                 }
>             });
>+
>+            GeckoAppShell.sendEventToGecko(new GeckoEvent("browser:purge-session-history", null));

Should the sendEventToGecko be inside the runnable?
Attachment #593439 - Flags: review?(mark.finkle) → review+
Comment on attachment 593440 [details] [diff] [review]
(2/2) Hide last tabs from about:home when history is cleared

The coupling is a slippery slope. It would be nice if we had events/messages, or even interfaces, that outside code could use to be notified of history (and other) changes.

Also, should that code be in the runnable too?
Attachment #593440 - Flags: review?(mark.finkle) → review+
(Reporter)

Updated

6 years ago
status-firefox13: --- → affected
Keywords: checkin-needed
(Reporter)

Updated

6 years ago
Assignee: nobody → lucasr.at.mozilla
Lucasr said on irc he's redoing the patches in this bug.
(Assignee)

Comment 10

6 years ago
I'm redoing those patches. No check-in needed right now.
Keywords: checkin-needed
(Assignee)

Comment 11

6 years ago
Created attachment 595013 [details] [diff] [review]
(1/3) Rename SessionStore event for clarity and consistency
Attachment #595013 - Flags: review?(mark.finkle)
(Assignee)

Comment 12

6 years ago
Created attachment 595014 [details] [diff] [review]
(2/3) Add notifification/message for when SessionStore purges its state
Attachment #595014 - Flags: review?(mark.finkle)
(Assignee)

Comment 13

6 years ago
Created attachment 595015 [details] [diff] [review]
(3/3) Purge session files when clearing history and update about:home accordingly
Attachment #595015 - Flags: review?(mark.finkle)
(Assignee)

Updated

6 years ago
Attachment #593439 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #593440 - Attachment is obsolete: true
Comment on attachment 595013 [details] [diff] [review]
(1/3) Rename SessionStore event for clarity and consistency

I don't think we should do this. The notification name is the same as used on desktop Firefox. We should be consistent. This would also affect add-ons listening for the notification.

If we want to change it, we should see what desktop is doing and look to change it there too. More work than I think we want.
Attachment #595013 - Flags: review?(mark.finkle) → review-
Comment on attachment 595014 [details] [diff] [review]
(2/3) Add notifification/message for when SessionStore purges its state

Since this message is new, and fits the "sessionstore-*" pattern, I am fine with it.
Attachment #595014 - Flags: review?(mark.finkle) → review+
Comment on attachment 595015 [details] [diff] [review]
(3/3) Purge session files when clearing history and update about:home accordingly

Missed this from patch 2. Change "SessionStore:StatePurged" -> "Session:StatePurged"

We have some other "Session:RestoreXxx" messages too.
Attachment #595015 - Flags: review?(mark.finkle) → review+
(Assignee)

Comment 17

6 years ago
(In reply to Mark Finkle (:mfinkle) from comment #16)
> Comment on attachment 595015 [details] [diff] [review]
> (3/3) Purge session files when clearing history and update about:home
> accordingly
> 
> Missed this from patch 2. Change "SessionStore:StatePurged" ->
> "Session:StatePurged"

Done.
(Assignee)

Updated

6 years ago
Attachment #595013 - Attachment is obsolete: true
(Assignee)

Comment 19

6 years ago
Comment on attachment 595014 [details] [diff] [review]
(2/3) Add notifification/message for when SessionStore purges its state

[Beta/Approval Request Comment]
Regression caused by (bug #): none
User impact if declined: Inconsistent state in about:home
Testing completed (on m-c, etc.): locally
Risk to taking this patch (and alternatives if risky): low
String changes made by this patch: none
Attachment #595014 - Flags: approval-mozilla-beta?
Attachment #595014 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 20

6 years ago
Comment on attachment 595015 [details] [diff] [review]
(3/3) Purge session files when clearing history and update about:home accordingly

[Beta/Approval Request Comment]
Regression caused by (bug #): none
User impact if declined: Inconsistent state in about:home
Testing completed (on m-c, etc.): locally
Risk to taking this patch (and alternatives if risky): low
String changes made by this patch: none
Attachment #595015 - Flags: approval-mozilla-beta?
Attachment #595015 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/fd88e8b59387
https://hg.mozilla.org/mozilla-central/rev/9cedf56c0200
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 13

Updated

6 years ago
status-firefox13: affected → ---
Comment on attachment 595014 [details] [diff] [review]
(2/3) Add notifification/message for when SessionStore purges its state

[Triage Comment]
Mobile only - approved for Aurora 12 and Beta 11.
Attachment #595014 - Flags: approval-mozilla-beta?
Attachment #595014 - Flags: approval-mozilla-beta+
Attachment #595014 - Flags: approval-mozilla-aurora?
Attachment #595014 - Flags: approval-mozilla-aurora+

Updated

6 years ago
Attachment #595015 - Flags: approval-mozilla-beta?
Attachment #595015 - Flags: approval-mozilla-beta+
Attachment #595015 - Flags: approval-mozilla-aurora?
Attachment #595015 - Flags: approval-mozilla-aurora+

Comment 23

6 years ago
Verified fixed on nighty:

Nightly 13.0a1 (2012-02-21)
Device: Samsung Nexus S - Android 2.3.6

Updated

6 years ago
status-firefox13: --- → verified
Comment on attachment 595014 [details] [diff] [review]
(2/3) Add notifification/message for when SessionStore purges its state

Clearing approval for Aurora 12 and Beta 11 because we are not currently planning a Native Fennec release of these versions.  If this changes in the future, we will likely do a mass uplift of all native fennec changes.  For now, let's get these bugs off the channel triage radar.

[Filter on the string "mbrubeck-bugspam" if you want to delete all of these emails at once.]
Attachment #595014 - Flags: approval-mozilla-beta+
Attachment #595014 - Flags: approval-mozilla-aurora+
Attachment #595015 - Flags: approval-mozilla-beta+
Attachment #595015 - Flags: approval-mozilla-aurora+

Comment 25

6 years ago
Verified fixed on nighty:

Nightly 13.0a1 (2012-02-28)
Device: Samsung Nexus S - Android 2.3.6
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.