Closed Bug 799609 Opened 12 years ago Closed 12 years ago

Disable places, global history in b2g

Categories

(Firefox OS Graveyard :: General, defect)

defect
Not set
normal

Tracking

(blocking-basecamp:+, firefox18 fixed, firefox19 fixed)

RESOLVED FIXED
blocking-basecamp +
Tracking Status
firefox18 --- fixed
firefox19 --- fixed

People

(Reporter: justin.lebar+bug, Assigned: mak)

References

Details

(Whiteboard: [MemShrink:P2][slim:>0.3MB])

Attachments

(5 files, 3 obsolete files)

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
Whiteboard: [MemShrink]
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.)
blocking-basecamp: --- → ?
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.
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.
Summary: Disable global history in b2g → Disable place,s global history in b2g
Summary: Disable place,s global history in b2g → Disable places, global history in b2g
blocking-basecamp: ? → +
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.
blocking-basecamp: + → ?
Erm, I did not mean to unset that flag, but now I can't set it back.  Sorry.
blocking-basecamp: ? → +
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?
Action appears to be blocked on technical knowledge from Marco.
Flags: needinfo?(mak77)
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.
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.
Flags: needinfo?(mak77)
Marco, would you be able to fix this the Right Way for B2G v1 (before the next merge)?
I'm not sure why ContentParent.cpp includes History.h, at first glance it should just require IHistory.h...
(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.
Assignee: nobody → mak77
> 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.
Whiteboard: [MemShrink] → [MemShrink][slim:>0.3MB]
Whiteboard: [MemShrink][slim:>0.3MB] → [MemShrink:P2][slim:>0.3MB]
How are things going with this bug, Marco?
Flags: needinfo?(mak77)
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.
Flags: needinfo?(mak77)
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).
Attached patch Places Changes V1 (obsolete) — Splinter Review
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)
Attached patch Docshell/Content changes V1 (obsolete) — Splinter Review
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.
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).
Attached patch Mobile Changes v1 (obsolete) — Splinter Review
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.
Attached patch B2G changesSplinter Review
Well, this trivially stops building Places into B2G.
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?
Attachment #676736 - Flags: review?(mbrubeck)
Comment on attachment 676732 [details] [diff] [review]
Jumplist Changes V1

I think jimm is the best person to look at this part
Attachment #676732 - Flags: review?(jmathies)
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.
Attachment #676730 - Flags: review?(justin.lebar+bug)
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 on attachment 676724 [details] [diff] [review]
Places Changes V1

oh well, let's continue the gifts distribution, a check here would be welcome paolo!
Attachment #676724 - Flags: review?(paolo.mozmail)
Attachment #676732 - Flags: review?(jmathies) → review+
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!
Attachment #676730 - Flags: review?(justin.lebar+bug) → review+
(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 on attachment 676736 [details] [diff] [review]
Mobile Changes v1

Handing off review to Brad for the Android patch.
Attachment #676736 - Flags: review?(mbrubeck) → review?(blassey.bugs)
Fixed the iid.
Attachment #676730 - Attachment is obsolete: true
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 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 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.
Attachment #676736 - Flags: review?(blassey.bugs) → review-
(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 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?
Attachment #676724 - Flags: review?(paolo.mozmail) → review+
(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.
- use hg mv
- added moz_places check to configure.in
- check argument in notifyVisited
Attachment #676736 - Attachment is obsolete: true
Attachment #678328 - Flags: review?(blassey.bugs)
- 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
Attachment #676724 - Attachment is obsolete: true
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?
Attachment #676737 - Flags: review?(justin.lebar+bug)
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.
Attachment #676737 - Flags: review?(justin.lebar+bug) → review+
Attachment #678328 - Flags: review?(blassey.bugs) → review+
(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
OOps I somehow missed blassey's review... will do a last try run and then push. Thanks all for the help.
I know there's some requirements for B2G and version 18, thus, should this be backported to Aurora?
(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.
Thank you Ryan, I was going to look at it today but you've been faster :)
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).
You need to log in before you can comment on or make changes to this bug.