Last Comment Bug 799609 - Disable places, global history in b2g
: Disable places, global history in b2g
Status: RESOLVED FIXED
[MemShrink:P2][slim:>0.3MB]
:
Product: Firefox OS
Classification: Client Software
Component: General (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: Marco Bonardo [::mak]
:
Mentors:
Depends on:
Blocks: slim-fast
  Show dependency treegraph
 
Reported: 2012-10-09 11:43 PDT by Justin Lebar (not reading bugmail)
Modified: 2012-11-10 04:39 PST (History)
20 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
+
fixed
fixed


Attachments
Places Changes V1 (12.83 KB, patch)
2012-10-30 12:59 PDT, Marco Bonardo [::mak]
paolo.mozmail: review+
Details | Diff | Review
Docshell/Content changes V1 (11.06 KB, patch)
2012-10-30 13:05 PDT, Marco Bonardo [::mak]
justin.lebar+bug: review+
Details | Diff | Review
Jumplist Changes V1 (6.91 KB, patch)
2012-10-30 13:07 PDT, Marco Bonardo [::mak]
jmathies: review+
Details | Diff | Review
Mobile Changes v1 (9.72 KB, patch)
2012-10-30 13:09 PDT, Marco Bonardo [::mak]
blassey.bugs: review-
Details | Diff | Review
B2G changes (415 bytes, patch)
2012-10-30 13:10 PDT, Marco Bonardo [::mak]
justin.lebar+bug: review+
Details | Diff | Review
Docshell/Content changes V2 (11.71 KB, patch)
2012-10-31 06:24 PDT, Marco Bonardo [::mak]
no flags Details | Diff | Review
Mobile Changes v2 (7.99 KB, patch)
2012-11-05 08:18 PST, Marco Bonardo [::mak]
blassey.bugs: review+
Details | Diff | Review
Places Changes V2 (8.32 KB, patch)
2012-11-05 08:20 PST, Marco Bonardo [::mak]
no flags Details | Diff | Review

Description Justin Lebar (not reading bugmail) 2012-10-09 11:43:59 PDT
See bug 799549 comment 6: cjones would like us to disable b2g global history.

This will mean that all links are blue (unvisited), but that should be the only change.  AIUI this should save us ~300kb in the main process, because we then should be able to get away with not having a places.sqlite file:

│   ├────337,816 B (00.45%) -- places.sqlite
Comment 1 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-10-09 11:55:28 PDT
We discussed this previously and had agreed to disable it, as I recall.  It would not be expected that link coloring persists across facebook.com in the browser and a facebook app, e.g.  While I believe our visited-link code is sufficiently locked down that this isn't a security issue, I think it's a major user-psychology issue.

(Also, global history is rather expensive.)
Comment 2 Jonas Sicking (:sicking) 2012-10-09 15:00:13 PDT
Note that this would disable link coloring even within an app. I.e. links would never turn purple in the firefox app even if you click the link in the firefox app.

Or am I misunderstanding?

Either way, I'm ok with fixing this bug.
Comment 3 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-10-09 15:11:20 PDT
Correct.
Comment 4 Andreas Gal :gal 2012-10-10 03:08:48 PDT
Places is such a POS I would rather go without link coloring. We can implement that in a lightweight way for v2 specifically for the browser app. Not sure we want link coloring in apps anyway.
Comment 5 Justin Lebar (not reading bugmail) 2012-10-10 10:45:03 PDT
This isn't as simple as setting "MOZ_PLACES=" in the configuration, unfortunately; that causes compile errors.

> In file included from B2G/gecko/dom/ipc/ContentParent.cpp:20:
> B2G/gecko/dom/ipc/../../toolkit/components/places/History.h:12:30: error:
> mozIAsyncHistory.h: No such file or directory

and so on.
Comment 6 Justin Lebar (not reading bugmail) 2012-10-10 10:45:44 PDT
Erm, I did not mean to unset that flag, but now I can't set it back.  Sorry.
Comment 7 Justin Lebar (not reading bugmail) 2012-10-10 10:49:05 PDT
Marco, what's the correct way to disable Places?  Surely there's a way to keep the code around (so things compile) but turn it off?
Comment 8 Gregory Szorc [:gps] 2012-10-10 12:42:18 PDT
Action appears to be blocked on technical knowledge from Marco.
Comment 9 [:fabrice] Fabrice Desré 2012-10-10 12:43:33 PDT
I guess one way to do it would be to replicate the MOZ_ANDROID_HISTORY approach, with a MOZ_GONK_HISTORY which just does nothing.
Comment 10 Marco Bonardo [::mak] 2012-10-10 12:54:29 PDT
MOZ_PLACES would be the right way, but it broke some time ago and nobody fixed it yet, indeed MOZ_ANDROID_HISTORY approach is sort of wrong (HACK!) as I explained in the bug that implemented it (it should have built without MOZ_PLACES and put its own files to a new AndroidHistory component).
Surely you can replicate the MOZ_ANDROID_HISTORY method, but that wouldn's save the library size taken by Places source.
So my suggestion would be to properly fix MOZ_PLACES (so basically make the build work) and then provide a separate GonkHistory component that implements global history. The docshell just initializes whatever component exposes itself as a global history implementation.
Comment 11 Justin Lebar (not reading bugmail) 2012-10-10 13:04:09 PDT
Marco, would you be able to fix this the Right Way for B2G v1 (before the next merge)?
Comment 12 Marco Bonardo [::mak] 2012-10-10 13:04:49 PDT
I'm not sure why ContentParent.cpp includes History.h, at first glance it should just require IHistory.h...
Comment 13 Marco Bonardo [::mak] 2012-10-10 13:07:29 PDT
(In reply to Justin Lebar [:jlebar] from comment #11)
> Marco, would you be able to fix this the Right Way for B2G v1 (before the
> next merge)?

You mean 19 Nov? yeah I think so, I have to look back a couple details but shouldn't be too hard, eventually will ask for help :) Taking.
Comment 14 Justin Lebar (not reading bugmail) 2012-10-10 15:40:10 PDT
> but that wouldn's save the library size taken by Places source.

Saving space in libxul would actually be quite nice, if we can swing that.
Comment 15 Andrew Overholt [:overholt] 2012-10-29 14:34:56 PDT
How are things going with this bug, Marco?
Comment 16 Marco Bonardo [::mak] 2012-10-30 03:45:22 PDT
I have a wip patch that is compiling right now, I have to finish fixing a desktop build without Places (still a couple includes to fix), then build Fennec. I suppose making the build end for Fennec may take a bit of time but likely will be done soon.
Comment 17 Marco Bonardo [::mak] 2012-10-30 03:49:29 PDT
To clarify the plan, once Fennec is fixed to build with MOZ_PLACES= you should be able to just build the same way, and if you don't provide an history implementations (Fennec does) there will just no history calls at all.
In future you should be able to provide your own IHistory implementation, or restart using Places if wanted (the history implementation is fast and asynchronous, bookmarks are fast until you leave out fancy extensions like tags and annotations that we still have to work on).
Comment 18 Marco Bonardo [::mak] 2012-10-30 12:59:28 PDT
Created attachment 676724 [details] [diff] [review]
Places Changes V1

This removes the MOZ_ANDROID_HISTORY changes from Places (next part will move them to a mobile component) and converts NotifyVisited to an API implementation (cause it's used as such and this allows to plug any other implementation than Places the same way)
Comment 19 Marco Bonardo [::mak] 2012-10-30 13:05:05 PDT
Created attachment 676730 [details] [diff] [review]
Docshell/Content changes V1

This removes Places dependancies from docshell and content.

Unfortunately this means CopyFavicon method won't work without Places (I had to ifdef it) that basically means bug 655270 is not fixed on anything not using Places. This should be a minor problem though for B2G and mobile, maybe even an advantage to remove some icons cruft. Eventually we can file a separate bug to add some pluggable interface here.
Jlebar implemented this favicon copy code, so he may have insight about this choice. I don't think it's a priority or a blocker for now.

Other changes are just to avoid null deref and useless aborts if there is no history implmentation.
Comment 20 Marco Bonardo [::mak] 2012-10-30 13:07:22 PDT
Created attachment 676732 [details] [diff] [review]
Jumplist Changes V1

This ifdefs JumpList dependancies on Places. It's not mandatory, jumplists are on Win7 and Win8 and in both cases we have Places enabled. This should be done regardless for completion. Again this means these methods won't work without Places so far, a pluggable interface may be evaluated if needed (follow-up).
Comment 21 Marco Bonardo [::mak] 2012-10-30 13:09:29 PDT
Created attachment 676736 [details] [diff] [review]
Mobile Changes v1

This moves the Android history implementation to mobile/android components, and uses the pluggable IHistory::NotifyVisited interface.
Finally totally removes Places from Firefox for Android libxul.
Comment 22 Marco Bonardo [::mak] 2012-10-30 13:10:05 PDT
Created attachment 676737 [details] [diff] [review]
B2G changes

Well, this trivially stops building Places into B2G.
Comment 23 Marco Bonardo [::mak] 2012-10-30 13:12:12 PDT
Comment on attachment 676736 [details] [diff] [review]
Mobile Changes v1

Matt, I made a Fennec build successfully, but couldn't test it, do you have a chance to?
Basically what may break here and should be testes is link coloring.
That said, may you start looking at the patch, or suggest a reviewer if you're not comfortable with it?
Comment 24 Marco Bonardo [::mak] 2012-10-30 13:13:27 PDT
Comment on attachment 676732 [details] [diff] [review]
Jumplist Changes V1

I think jimm is the best person to look at this part
Comment 25 Marco Bonardo [::mak] 2012-10-30 13:14:16 PDT
Comment on attachment 676730 [details] [diff] [review]
Docshell/Content changes V1

Justin I'd like to have some insight, I know you touched some of this code already, especially the favicons one.
Comment 26 Marco Bonardo [::mak] 2012-10-30 13:15:24 PDT
I need someone from B2G to apply these patches, make a build and confirm Places is not there, link coloring doesn't work (as expected) and no strange crashes on navigation... Any volunteer?
Comment 27 Marco Bonardo [::mak] 2012-10-30 13:20:21 PDT
Comment on attachment 676724 [details] [diff] [review]
Places Changes V1

oh well, let's continue the gifts distribution, a check here would be welcome paolo!
Comment 28 Justin Lebar (not reading bugmail) 2012-10-30 13:36:58 PDT
Comment on attachment 676730 [details] [diff] [review]
Docshell/Content changes V1

>diff --git a/docshell/base/IHistory.h b/docshell/base/IHistory.h
>--- a/docshell/base/IHistory.h
>+++ b/docshell/base/IHistory.h

Please rev the IID.

>@@ -112,25 +112,34 @@ public:
>      * @pre aURI must not be null.
>      *
>      * @param aURI
>      *        The URI to set the title for.
>      * @param aTitle
>      *        The title string.
>      */
>     NS_IMETHOD SetURITitle(nsIURI* aURI, const nsAString& aTitle) = 0;
>+
>+    /**
>+     * Notifies about the visited status of a given URI.
>+     *
>+     * @param aURI
>+     *        The URI to notify about.
>+     */
>+    NS_IMETHOD NotifyVisited(nsIURI* aURI) = 0;
> };

It's not in this patch, but I presume you modified History.cpp so that
NotifyVisited is an IMETHODIMP, otherwise this won't even compile.

Looks good to me!
Comment 29 Marco Bonardo [::mak] 2012-10-30 13:44:22 PDT
(In reply to Justin Lebar [:jlebar] from comment #28)
> It's not in this patch, but I presume you modified History.cpp so that
> NotifyVisited is an IMETHODIMP, otherwise this won't even compile.

Yes it's in the Places patch (feel free to look at the other patches as well, I picked some reviewers based on knowledge but more eyes are always welcome)
Comment 30 Matt Brubeck (:mbrubeck) 2012-10-30 14:07:31 PDT
Comment on attachment 676736 [details] [diff] [review]
Mobile Changes v1

Handing off review to Brad for the Android patch.
Comment 31 Marco Bonardo [::mak] 2012-10-31 06:24:30 PDT
Created attachment 676992 [details] [diff] [review]
Docshell/Content changes V2

Fixed the iid.
Comment 32 Marco Bonardo [::mak] 2012-10-31 07:58:03 PDT
Tryrun:
https://tbpl.mozilla.org/?tree=Try&rev=5f39d37b0305
Afaict it looks fine (though I'm not sure if Android has any history link coloring test)
Comment 33 Brad Lassey [:blassey] (use needinfo?) 2012-10-31 15:25:15 PDT
Comment on attachment 676736 [details] [diff] [review]
Mobile Changes v1

Marco, rather than delete the nsAndroidHistory.{cpp,h} and then create them again I'd rather hg mv them so we can preserve the history. Can you post a patch that does that?
Comment 34 Brad Lassey [:blassey] (use needinfo?) 2012-10-31 15:33:31 PDT
Comment on attachment 676736 [details] [diff] [review]
Mobile Changes v1

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

This is basically fine, but I didn't look at nsAndroidHistory.{cpp,h} because I assume there are no changes there. r- for the loss of version history.
Comment 35 Marco Bonardo [::mak] 2012-11-01 04:25:55 PDT
(In reply to Brad Lassey [:blassey] from comment #33)
> Marco, rather than delete the nsAndroidHistory.{cpp,h} and then create them
> again I'd rather hg mv them so we can preserve the history.

oops sorry, yes, I dunno why I didn't do that :(
Comment 36 :Paolo Amadini 2012-11-02 06:30:24 PDT
Comment on attachment 676724 [details] [diff] [review]
Places Changes V1

This was an easy review, thanks!

>--- a/toolkit/components/places/History.cpp
>+++ b/toolkit/components/places/History.cpp
>-void
>+NS_IMETHODIMP
> History::NotifyVisited(nsIURI* aURI)
> {
>   NS_ASSERTION(aURI, "Ruh-roh!  A NULL URI was passed to us!");

This should likely become NS_ENSURE_ARG now that this is an interface method.

Now that MOZ_ANDROID_HISTORY and MOZ_PLACES are mutually exclusive, do we have a build-time check that they can't be enabled at the same time?
Comment 37 Marco Bonardo [::mak] 2012-11-05 01:55:51 PST
(In reply to Paolo Amadini [:paolo] from comment #36)
> Now that MOZ_ANDROID_HISTORY and MOZ_PLACES are mutually exclusive, do we
> have a build-time check that they can't be enabled at the same time?

Nope, I think MOZ_ANDROID_HISTORY may even just die since all the code is contained into the android folder. And there's nothing else than the android build you can make with that history implementation (so if you define both and build desktop firefox or any other app than firefox for android nothing bad happens). I left it in just cause I don't know if they want to be able to disable their implementation for testing purposes.
Comment 38 Marco Bonardo [::mak] 2012-11-05 08:18:44 PST
Created attachment 678328 [details] [diff] [review]
Mobile Changes v2

- use hg mv
- added moz_places check to configure.in
- check argument in notifyVisited
Comment 39 Marco Bonardo [::mak] 2012-11-05 08:20:43 PST
Created attachment 678330 [details] [diff] [review]
Places Changes V2

- check argument in NotifyVisited
- android history implementation wiil be hg mv in the mobile patch
- the configure.in check is moved to the mobile patch
Comment 40 Marco Bonardo [::mak] 2012-11-05 08:22:26 PST
Comment on attachment 676737 [details] [diff] [review]
B2G changes

Justin, can you review this b2g confvars change, or do I need to find a b2g peer?
Comment 41 Justin Lebar (not reading bugmail) 2012-11-05 08:25:57 PST
Comment on attachment 676737 [details] [diff] [review]
B2G changes

When I did something like this (now I forget which bug), people told me to remove the MOZ_PLACES= altogether.  FWIW I like it like you have it, so it's explicit what we're doing.

r=me either way.
Comment 42 Marco Bonardo [::mak] 2012-11-05 10:54:08 PST
(In reply to Justin Lebar [:jlebar] from comment #41)
> When I did something like this (now I forget which bug), people told me to
> remove the MOZ_PLACES= altogether.

hm, though in this case I think it's not an option cause we have to override the default http://mxr.mozilla.org/mozilla-central/source/configure.in#4264
Comment 43 Marco Bonardo [::mak] 2012-11-08 07:58:56 PST
OOps I somehow missed blassey's review... will do a last try run and then push. Thanks all for the help.
Comment 46 Marco Bonardo [::mak] 2012-11-09 07:29:45 PST
I know there's some requirements for B2G and version 18, thus, should this be backported to Aurora?
Comment 47 [:fabrice] Fabrice Desré 2012-11-09 08:24:28 PST
(In reply to Marco Bonardo [:mak] from comment #46)
> I know there's some requirements for B2G and version 18, thus, should this
> be backported to Aurora?

It is a basecamp blocker, so the sheriffs will uplift to aurora. If these patches need to be rebased, that would be helpful if you could do the aurora landing yourself.
Comment 49 Marco Bonardo [::mak] 2012-11-10 04:38:37 PST
Thank you Ryan, I was going to look at it today but you've been faster :)
Comment 50 Ryan VanderMeulen [:RyanVM] 2012-11-10 04:39:44 PST
Not a problem. It applied with very minimal cleanup. Just a couple cosmetic things (I leave it for the patch creator if it's more than trivially bitrotted).

Note You need to log in before you can comment on or make changes to this bug.