851854 - Disable GlobalHistory when running a WebApp

Status: RESOLVED WONTFIX

(Firefox for Android Graveyard :: Web Apps (PWAs), defect, P2)

Description

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

GlobalHistory is used to implemented "visited links" in Gecko. I don't think we care about showing links as visited in webapps. This will save some memory and loading of the history DB.

GlobalHistory also save the title changes for a URL. Again, I don't think we care about that. In fact, we probably don't care about saving any history when running webapps. I'll file a separate bug for disabling BrowserProvider.

Comment 1

[Jason Smith [:jsmith]]

What memory/perf wins are we expecting here?

Comment 2

[Jason Smith [:jsmith]]

Mark - Can you address my comment 1 question?

Comment 3

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

(In reply to Jason Smith [:jsmith] from comment #1)
> What memory/perf wins are we expecting here?

Writing to a database is not cheap, especially SQLite on a mobile device. How much faster will we be? I don't know. How much memory will we save? I don't know. Will pages load faster? I don't know.

Removing the extra overhead just makes sense though.

Comment 4

[Jason Smith [:jsmith]]

Hmm...okay. Maybe P2? Don't know if it's worth tracking though for A4A.

Comment 5

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

(In reply to Jason Smith [:jsmith] from comment #4)
> Hmm...okay. Maybe P2? Don't know if it's worth tracking though for A4A.

Agreed

Comment 6

[Richard Newman [:rnewman]]

We have telemetry for FENNEC_GLOBALHISTORY_* that shows how much this costs. It should be pretty easy to turn this off, if we want to do it -- and it'll also allow us to provide a hook for users to turn off history recording if they so choose (and some of them want to).

Comment 7

[Richard Newman [:rnewman]]

Potential solution: add a pref check (cached) to CanAddURI:

http://dxr.mozilla.org/mozilla-central/source/mobile/android/components/build/nsAndroidHistory.cpp#263

Comment 8

[Richard Newman [:rnewman]]

Attachment: Part 1: allow nsAndroidHistory to be disabled. v1

Untested, but it builds. Dumping it here in case I don't get around to finishing it.

Comment 9

[Richard Newman [:rnewman]]

Comment on attachment 8448769 [details] [diff] [review]
Part 1: allow nsAndroidHistory to be disabled. v1

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

Let's get some preliminary feedback. Myk?

Comment 10

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 8448769 [details] [diff] [review]
Part 1: allow nsAndroidHistory to be disabled. v1

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

This is ok, and similar to what we've done elsewhere, but I'm growing a bit weary of using prefs to make these webapp-specific runtime configuration changes, and not just because I've been debugging issues with other such prefs.  This isn't really a "pref," as it should never change.  And using a pref introduces the possibility of it changing, even if it's hard to do so in a webapp profile.

I'd rather we have some way to definitively identify whether or not a given process represents a webapp and then disable features like this one on that basis.  I've been working on that over in bug 1035999, albeit with nothing to show for my efforts yet.

My current thinking is to use the new implementation of JNI.jsm in bug 918309 to query some Java class for the name of the activity class that launched the process, which I suppose might vary for browser processes but should always be org.mozilla.gecko.Webapp (or some slot-specific variant thereof) for webapp ones.

Maybe that's the wrong approach too, but there should be *something* better than setting and querying a pref for configuration that should never vary.

Comment 11

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

Comment on attachment 8448769 [details] [diff] [review]
Part 1: allow nsAndroidHistory to be disabled. v1


>+// Note that we don't yet observe this pref.
>+#define PREF_HISTORY_ENABLED "places.history.enabled"

I might change the comment to:

  // Note that we don't yet observe this pref at runtime.


>     sHistory = new nsAndroidHistory();
>     NS_ENSURE_TRUE(sHistory, nullptr);
>+    sHistory->LoadPrefs();

Any reason to not put this in the constructor itself?

This patch alone might not be the right answer to disabling History for Webapps, but it is Desktop parity and we have had more than a few requests for this explicit functionality.

Comment 12

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Myk Melez [:myk] [@mykmelez] from comment #10)
> I'd rather we have some way to definitively identify whether or not a given
> process represents a webapp and then disable features like this one on that
> basis.  I've been working on that over in bug 1035999, albeit with nothing
> to show for my efforts yet.

How about a command line flag? Don't we already pass something for webapps?

> 
> My current thinking is to use the new implementation of JNI.jsm in bug
> 918309 to query some Java class for the name of the activity class that
> launched the process, which I suppose might vary for browser processes but
> should always be org.mozilla.gecko.Webapp (or some slot-specific variant
> thereof) for webapp ones.
> 
> Maybe that's the wrong approach too, but there should be *something* better
> than setting and querying a pref for configuration that should never vary.

I don't like using the class name, because that should be able to change at any time without breaking stuff like this.

Comment 13

[:Margaret Leibovic]

Does anyone know why this bug was set to block bug 1065004? I don't think it needs to...

Comment 14

[Myk Melez [:myk] [@mykmelez]]

(In reply to :Margaret Leibovic from comment #13)
> Does anyone know why this bug was set to block bug 1065004? I don't think it
> needs to...

Unsure, although perhaps someone thought this bug would help fix that one.  In any case, that bug is fixed, and this one isn't, so that one definitely doesn't depend on this one. :-)

Comment 15

[Myk Melez [:myk] [@mykmelez]]

Per bug 1235869, we're going to disable the Android web runtime, so we won't fix this bug in it.

(This is part of a bulk resolution of bugs in the Firefox for Android::Web Apps component, from which I attempted to exclude bugs that are not specific to the runtime, but it's possible that I included one accidentally.  If so, I'm sorry, and please reopen the bug!)