[Browser] Clear History doesn't clear back/forward history in open tabs

VERIFIED FIXED

Status

Firefox OS
Gaia::Browser
P3
normal
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: dholbert, Assigned: baku)

Tracking

(Blocks: 1 bug, {b2g-testdriver, privacy, unagi})

unspecified
x86_64
Linux
b2g-testdriver, privacy, unagi
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking-basecamp:+, firefox18 fixed, firefox19 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

10.13 KB, patch
Justin Lebar (not reading bugmail)
: review+
Details | Diff | Splinter Review
1.46 KB, patch
benfrancis
: review+
Details | Diff | Splinter Review
STR:
 1. Open a tab in browser. Navigate through a few sites; hit "back", so you've got both a "Back" and a "Forward" button available.
 2. Open Browser "Settings" pane (gear icon in tab listing), and press Clear History button. Confirm when prompted.
 3. Click upper-left back button to get out of Browser Settings. See if your Back/Forward buttons are still available/functional.

EXPECTED RESULTS: Back/Forward buttons should be grayed out, since your browsing history should've been cleared.

ACTUAL RESULTS: Back/Forward buttons are still there, and they work (they take you to pages in your history that you'd expect would have been purged).


For comparison: In desktop Firefox, I get EXPECTED RESULTS after performing "Tools | Clear Recent History". So this is a place where we're breaking users' privacy expectations by diverging from desktop Firefox behavior.
(Assignee)

Comment 1

5 years ago
It seems that purgeHistory is not called.
Assignee: nobody → amarchesini
Flags: needinfo?
The Clear History button clears the global history, which is stored in IndexedDB inside the app.

Session history is stored separately by the platform and as far as I know we have no API to clear session history from content.
Flags: needinfo?
Blocks: 693515
Blocking + ; looks like it's broken.
blocking-basecamp: ? → +
Priority: -- → P3
(Assignee)

Comment 4

5 years ago
> Session history is stored separately by the platform and as far as I know we
> have no API to clear session history from content.

This is what I'm going to implement. The patch is almost ready. I'm going to upload it tonight or tomorrow morning.
(Assignee)

Comment 5

5 years ago
Created attachment 677778 [details] [diff] [review]
patch 1

With this patch, purgeHistory() is added in the browserElement*.js.
Attachment #677778 - Flags: review?(fabrice)
(Assignee)

Comment 6

5 years ago
Created attachment 677781 [details] [diff] [review]
Gaia patch browser 1

Here how to use the new API in browser.js
Attachment #677781 - Flags: review?(fabrice)
Attachment #677781 - Attachment description: patch browser 1 → Gaia patch browser 1
Comment on attachment 677778 [details] [diff] [review]
patch 1

I'd prefer to review this, if you don't mind, Fabrice.
Attachment #677778 - Flags: review?(fabrice) → review?(justin.lebar+bug)
Comment on attachment 677778 [details] [diff] [review]
patch 1

>+    try {
>+      if (history && history.count) {
>+        history.PurgeHistory(history.count);
>+      }
>+    } catch(e) {}

Why the heck does this start with a capital letter in the interface?  That's bizarre.

Anyway, looks great.  :)
Attachment #677778 - Flags: review?(justin.lebar+bug) → review+
(Assignee)

Comment 9

5 years ago
Justin, can you review and if you like, push the patch for gaia?
(Assignee)

Updated

5 years ago
Attachment #677781 - Flags: review?(fabrice) → review?(justin.lebar+bug)
Comment on attachment 677781 [details] [diff] [review]
Gaia patch browser 1

Although this looks fine to me, I'm not familiar with this code, so I don't feel comfortable reviewing it.  The way to get a Gaia review in general is to make a pull request and ping your reviewers (in this case @benfrancis and @daleharvey).

You can also flag them on bugzilla if you want to make sure there's no getting out of the review.  :)  People usually post an attachment which points to the github pull request.
Attachment #677781 - Flags: review?(justin.lebar+bug) → feedback+
(Assignee)

Comment 11

5 years ago
Ok. In the meantime, we can check-in the first patch.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/20aa8606eca4
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Because we don't have a separate bug for the Gaia work here, can we leave this open until Gaia is updated?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Sure, you can use [leave open] in the whiteboard to signify that in the future.

That being said, my Bugzilla queries won't find this when I'm looking for bb+ bugs that need Aurora landing if it's still open, so adding checkin-needed so I don't forget about it.
Keywords: checkin-needed
(Assignee)

Comment 15

5 years ago
Comment on attachment 677781 [details] [diff] [review]
Gaia patch browser 1

Ben, can you review these few lines about purgeHistory() in browser.js ?
Attachment #677781 - Flags: review?(ben)
https://hg.mozilla.org/releases/mozilla-aurora/rev/21d6e7368335
status-firefox18: --- → fixed
status-firefox19: --- → fixed
Keywords: checkin-needed
Comment on attachment 677781 [details] [diff] [review]
Gaia patch browser 1

A couple of nits:
1) In the browser app we would usually use "this.tabs.foreach" instead of "for each" to which you can pass "this" as the third argument. That would mean you don't need need to do self = this.
2) We only need to to refreshButtons() for the current tab, not every tab, so it might be better to add a conditional for that purpose to prevent calling it multiple times unnecessarily.
Attachment #677781 - Flags: review?(ben) → review-
(Assignee)

Comment 18

5 years ago
> 1) In the browser app we would usually use "this.tabs.foreach" instead of
> "for each" to which you can pass "this" as the third argument. That would
> mean you don't need need to do self = this.

I think this.tabs is an object. foreach is available only for arrays. is it?

> 2) We only need to to refreshButtons() for the current tab, not every tab,
> so it might be better to add a conditional for that purpose to prevent
> calling it multiple times unnecessarily.

good point.
(Assignee)

Comment 19

5 years ago
Actually I see other places in the code where we use for each(var tab in this.tabs) ...
Ah OK, well you could do foreach on Object.keys, but if you want to leave it using for each then OK.

Thanks
(Assignee)

Comment 21

5 years ago
Created attachment 678797 [details] [diff] [review]
Gaia patch browser 2

This patch refreshes the buttons only for the current tab.

Then I saw another bug related to the iconUrl that can be null.
Probably it's better to have a separated bug for this..
Attachment #677781 - Attachment is obsolete: true
Attachment #678797 - Flags: review?(ben)
Andrea, this looks good thank you.

Would you mind running gjslint on each of the js files:

  gjslint --nojsdoc filename.js

and then sending a pull request on GitHub?
(Assignee)

Comment 23

5 years ago
Done. Can we mark this bug as 'fixed' ?
Not until the PR lands, please.
Attachment #678797 - Flags: review?(ben) → review+
(Assignee)

Updated

5 years ago
Blocks: 807030
(Assignee)

Comment 25

5 years ago
https://github.com/mozilla-b2g/gaia/commit/623018214167191e6caac9672db8af5e15af74b1 patch merged.
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
No longer blocks: 807030

Comment 26

5 years ago
Verified as fixed on Unagi build 20121231070201.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.