1149799 - Move "Sanitize:ClearHistory" message handler out of GeckoApp

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Settings and Preferences, defect)

Description

[Andy Pusch [:AndyP]]

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

Comment 1

[Andy Pusch [:AndyP]]

Did you have any specific helper class in mind or would you like it to be moved to BrowserApp?

Comment 2

[:Margaret Leibovic]

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

Comment 3

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

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

Comment 4

[Andy Pusch [:AndyP]]

Attachment: 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

Comment 5

[:Margaret Leibovic]

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 :)

Comment 6

[Andy Pusch [:AndyP]]

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

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/0ec8fdc017f8

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0ec8fdc017f8