719434 - 'Tabs From Last Time' not wiped on Clear History

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Aaron Train [:aaronmt]]

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

Comment 1

[Lucas Rocha (:lucasr)]

Adding Brian to CC since he implemented this feature in about:home.

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

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

Comment 3

[Brian Nicholson (:bnicholson)]

(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.

Comment 4

[Lucas Rocha (:lucasr)]

Attachment: (1/2) Purge session history files when history is cleared

Comment 5

[Lucas Rocha (:lucasr)]

Attachment: (1/2) Purge session history files when history is cleared

Comment 6

[Lucas Rocha (:lucasr)]

Attachment: (2/2) Hide last tabs from about:home when history is cleared

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

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?

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

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?

Comment 9

[Martijn Wargers (dead)]

Lucasr said on irc he's redoing the patches in this bug.

Comment 10

[Lucas Rocha (:lucasr)]

I'm redoing those patches. No check-in needed right now.

Comment 11

[Lucas Rocha (:lucasr)]

Attachment: (1/3) Rename SessionStore event for clarity and consistency

Comment 12

[Lucas Rocha (:lucasr)]

Attachment: (2/3) Add notifification/message for when SessionStore purges its state

Comment 13

[Lucas Rocha (:lucasr)]

Attachment: (3/3) Purge session files when clearing history and update about:home accordingly

Comment 14

[Mark Finkle (:mfinkle) (use needinfo?)]

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.

Comment 15

[Mark Finkle (:mfinkle) (use needinfo?)]

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.

Comment 16

[Mark Finkle (:mfinkle) (use needinfo?)]

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.

Comment 17

[Lucas Rocha (:lucasr)]

(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.

Comment 18

[Lucas Rocha (:lucasr)]

Pushed:
http://hg.mozilla.org/integration/mozilla-inbound/rev/fd88e8b59387
http://hg.mozilla.org/integration/mozilla-inbound/rev/9cedf56c0200

Comment 19

[Lucas Rocha (:lucasr)]

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

Comment 20

[Lucas Rocha (:lucasr)]

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

Comment 21

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/fd88e8b59387
https://hg.mozilla.org/mozilla-central/rev/9cedf56c0200

Comment 22

[Alex Keybl [:akeybl]]

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.

Comment 23

[Camelia Urian]

Verified fixed on nighty:

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

Comment 24

[Matt Brubeck (:mbrubeck)]

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.]

Comment 25

[Camelia Urian]

Verified fixed on nighty:

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