Closed
Bug 799609
Opened 12 years ago
Closed 12 years ago
Disable places, global history in b2g
Categories
(Firefox OS Graveyard :: General, defect)
Firefox OS Graveyard
General
Tracking
(blocking-basecamp:+, firefox18 fixed, firefox19 fixed)
RESOLVED
FIXED
blocking-basecamp | + |
People
(Reporter: justin.lebar+bug, Assigned: mak)
References
Details
(Whiteboard: [MemShrink:P2][slim:>0.3MB])
Attachments
(5 files, 3 obsolete files)
(deleted),
patch
|
jimm
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
justin.lebar+bug
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
blassey
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review |
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
Reporter | ||
Updated•12 years ago
|
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.
Correct.
Comment 4•12 years ago
|
||
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.
Reporter | ||
Updated•12 years ago
|
Summary: Disable global history in b2g → Disable place,s global history in b2g
Reporter | ||
Updated•12 years ago
|
Summary: Disable place,s global history in b2g → Disable places, global history in b2g
Updated•12 years ago
|
blocking-basecamp: ? → +
Reporter | ||
Comment 5•12 years ago
|
||
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: + → ?
Reporter | ||
Comment 6•12 years ago
|
||
Erm, I did not mean to unset that flag, but now I can't set it back. Sorry.
Updated•12 years ago
|
blocking-basecamp: ? → +
Reporter | ||
Comment 7•12 years ago
|
||
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•12 years ago
|
||
Action appears to be blocked on technical knowledge from Marco.
Flags: needinfo?(mak77)
Comment 9•12 years ago
|
||
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.
Assignee | ||
Comment 10•12 years ago
|
||
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)
Reporter | ||
Comment 11•12 years ago
|
||
Marco, would you be able to fix this the Right Way for B2G v1 (before the next merge)?
Assignee | ||
Comment 12•12 years ago
|
||
I'm not sure why ContentParent.cpp includes History.h, at first glance it should just require IHistory.h...
Assignee | ||
Comment 13•12 years ago
|
||
(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
Reporter | ||
Comment 14•12 years ago
|
||
> 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.
Updated•12 years ago
|
Whiteboard: [MemShrink] → [MemShrink][slim:>0.3MB]
Updated•12 years ago
|
Whiteboard: [MemShrink][slim:>0.3MB] → [MemShrink:P2][slim:>0.3MB]
Assignee | ||
Comment 16•12 years ago
|
||
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)
Assignee | ||
Comment 17•12 years ago
|
||
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).
Assignee | ||
Comment 18•12 years ago
|
||
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)
Assignee | ||
Comment 19•12 years ago
|
||
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.
Assignee | ||
Comment 20•12 years ago
|
||
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).
Assignee | ||
Comment 21•12 years ago
|
||
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.
Assignee | ||
Comment 22•12 years ago
|
||
Well, this trivially stops building Places into B2G.
Assignee | ||
Comment 23•12 years ago
|
||
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)
Assignee | ||
Comment 24•12 years ago
|
||
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)
Assignee | ||
Comment 25•12 years ago
|
||
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)
Assignee | ||
Comment 26•12 years ago
|
||
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?
Assignee | ||
Comment 27•12 years ago
|
||
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)
Updated•12 years ago
|
Attachment #676732 -
Flags: review?(jmathies) → review+
Reporter | ||
Comment 28•12 years ago
|
||
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+
Assignee | ||
Comment 29•12 years ago
|
||
(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•12 years ago
|
||
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)
Assignee | ||
Comment 32•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
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-
Assignee | ||
Comment 35•12 years ago
|
||
(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•12 years ago
|
||
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+
Assignee | ||
Comment 37•12 years ago
|
||
(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.
Assignee | ||
Comment 38•12 years ago
|
||
- 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)
Assignee | ||
Comment 39•12 years ago
|
||
- 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
Assignee | ||
Comment 40•12 years ago
|
||
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)
Reporter | ||
Comment 41•12 years ago
|
||
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+
Updated•12 years ago
|
Attachment #678328 -
Flags: review?(blassey.bugs) → review+
Assignee | ||
Comment 42•12 years ago
|
||
(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
Assignee | ||
Comment 43•12 years ago
|
||
OOps I somehow missed blassey's review... will do a last try run and then push. Thanks all for the help.
Assignee | ||
Comment 44•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/31844532f0e8
https://hg.mozilla.org/integration/mozilla-inbound/rev/3755cf189e57
https://hg.mozilla.org/integration/mozilla-inbound/rev/a7f1ae3b4225
https://hg.mozilla.org/integration/mozilla-inbound/rev/695612ed3809
https://hg.mozilla.org/integration/mozilla-inbound/rev/83157913f81f
Comment 45•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/31844532f0e8
https://hg.mozilla.org/mozilla-central/rev/3755cf189e57
https://hg.mozilla.org/mozilla-central/rev/a7f1ae3b4225
https://hg.mozilla.org/mozilla-central/rev/695612ed3809
https://hg.mozilla.org/mozilla-central/rev/83157913f81f
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 46•12 years ago
|
||
I know there's some requirements for B2G and version 18, thus, should this be backported to Aurora?
Comment 47•12 years ago
|
||
(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 48•12 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/1e1c94786c33
https://hg.mozilla.org/releases/mozilla-aurora/rev/087a851e26d8
https://hg.mozilla.org/releases/mozilla-aurora/rev/b15f46e94bd2
https://hg.mozilla.org/releases/mozilla-aurora/rev/c716b7a78bdb
https://hg.mozilla.org/releases/mozilla-aurora/rev/d3da40fc2e3f
status-firefox18:
--- → fixed
status-firefox19:
--- → fixed
Assignee | ||
Comment 49•12 years ago
|
||
Thank you Ryan, I was going to look at it today but you've been faster :)
Comment 50•12 years ago
|
||
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.
Description
•