Move "Sanitize:ClearHistory" message handler out of GeckoApp

RESOLVED FIXED in Firefox 40

Status

()

Firefox for Android
Settings and Preferences
--
minor
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: AndyP, Assigned: AndyP)

Tracking

Trunk
Firefox 40
ARM
Android
Points:
---

Firefox Tracking Flags

(firefox40 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
As requested[1] this is a follow-up bug to move the "Sanitize:ClearHistory" message handler out of GeckoApp.

[1]https://bugzilla.mozilla.org/show_bug.cgi?id=1139379#c14
(Assignee)

Updated

3 years ago
Assignee: nobody → drag
(Assignee)

Comment 1

3 years ago
Did you have any specific helper class in mind or would you like it to be moved to BrowserApp?
Flags: needinfo?(margaret.leibovic)

Comment 2

3 years ago
(In reply to Andy Pusch [:AndyP] from comment #1)
> Did you have any specific helper class in mind or would you like it to be
> moved to BrowserApp?

Sorry for the delay here! I think moving this into BrowserApp would be a good start. But I want to double check to make sure we don't need this for shared GeckoApp implementations...

mfinkle, do we include browser db logic with GeckoView? Would this be necessary for things like visited link coloring?

I also see bookmarks logic in GeckoApp that should probably move into BrowserApp if we don't use it for anything more general.
Flags: needinfo?(margaret.leibovic) → needinfo?(mark.finkle)
(In reply to :Margaret Leibovic from comment #2)

> mfinkle, do we include browser db logic with GeckoView? Would this be
> necessary for things like visited link coloring?
> 
> I also see bookmarks logic in GeckoApp that should probably move into
> BrowserApp if we don't use it for anything more general.

It's a hodge-podge right now. Link coloring should work in GeckoView, but GeckoView has no history DB!

Moving "Sanitize:ClearHistory" to BrowserApp seems like a safe move though. GeckoView-based apps should handle clearing data in an app-specific way, not a GeckoView way.
Flags: needinfo?(mark.finkle)
(Assignee)

Comment 4

3 years ago
Created attachment 8591927 [details] [diff] [review]
bug1149799_moveClearHistoryMessageHandler.diff

I've moved "Sanitize:ClearHistory" to BrowserApp and declared the handleClearHistory method private.

try submission: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c59df87aeff9
Attachment #8591927 - Flags: review?(margaret.leibovic)

Comment 5

3 years ago
Comment on attachment 8591927 [details] [diff] [review]
bug1149799_moveClearHistoryMessageHandler.diff

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

Looks good to me! I trust that you also manually tested that clearing history still works :)
Attachment #8591927 - Flags: review?(margaret.leibovic) → review+
(Assignee)

Comment 6

3 years ago
(In reply to :Margaret Leibovic from comment #5)
> Comment on attachment 8591927 [details] [diff] [review]
> bug1149799_moveClearHistoryMessageHandler.diff
> 
> Review of attachment 8591927 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good to me! I trust that you also manually tested that clearing
> history still works :)

Yes I have tested clearing search history and browsing history both separated and together. :)
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/0ec8fdc017f8
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/0ec8fdc017f8
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox40: affected → fixed
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 40
You need to log in before you can comment on or make changes to this bug.