Disable GlobalHistory when running a WebApp

RESOLVED WONTFIX

Status

()

Firefox for Android
Web Apps
P2
normal
RESOLVED WONTFIX
5 years ago
2 years ago

People

(Reporter: mfinkle, Unassigned)

Tracking

({perf})

Trunk
All
Android
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 obsolete attachment)

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.
What memory/perf wins are we expecting here?
Mark - Can you address my comment 1 question?
Flags: needinfo?(mark.finkle)
(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.
Flags: needinfo?(mark.finkle)
Hmm...okay. Maybe P2? Don't know if it's worth tracking though for A4A.
OS: Linux → Android
Priority: -- → P2
Hardware: x86_64 → ARM
Version: Firefox 15 → Trunk
(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
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).
Keywords: perf
Hardware: ARM → All
Created attachment 8448769 [details] [diff] [review]
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.
Assignee: nobody → rnewman
Status: NEW → ASSIGNED
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?
Attachment #8448769 - Flags: feedback?(myk)
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.
Attachment #8448769 - Flags: feedback?(myk) → feedback+

Updated

4 years ago
Blocks: 1065004
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.
Attachment #8448769 - Flags: review+
Depends on: 1093886
(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.
Attachment #8448769 - Attachment is obsolete: true
Assignee: rnewman → nobody
Status: ASSIGNED → NEW

Comment 13

3 years ago
Does anyone know why this bug was set to block bug 1065004? I don't think it needs to...
(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. :-)
No longer blocks: 1065004
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!)
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.