702037 - Merge Android History into mozilla-central

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect, P5)

Description

[Doug Turner (:dougt)]

Attachment: patch v.1

We have an alternative and simpler implementation of bookmarks and history on Android for the Native UI. This bug is to move that implementation from our birch branch to mozilla-central.

This feature will be wrapped with MOZ_ANDROID_HISTORY.  When defined, we will disable parts of the 'places' functionality.

Comment 1

[Doug Turner (:dougt)]

Attachment: patch v.2

Comment 2

[Justin Dolske [:Dolske]]

Comment on attachment 574083 [details] [diff] [review]
patch v.2

I'm bouncing this over to Dietrich, as I really don't know places/history related code very well.

Comment 3

[Dietrich Ayala (:dietrich)]

Comment on attachment 574083 [details] [diff] [review]
patch v.2

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

module and makefile changes look ok to me. however, should have marco do a pass as well to ensure there aren't other spots to disable Places that need to be covered.

Comment 4

[Marco Bonardo [:mak]]

The points where Places hooks into Gecko (actually, into content) are docshell/base/nsDocShell.cpp and content/base/src/Link.cpp, as you saw.
You are implementing the IHistory interface, and that's OK, this is pluggable and will work fine.

I have a couple questions though:

- do you plan to drop only history are Places completely with this patch? Since this is bailing out in history, while to be on the safe side you should probably bailout in Database.cpp (start point of practically all Places services) and likely you may put your history in a toolkit/components/androidHistory and build without MOZ_PLACES defined, saving a bunch of libxul content you don't use.

- Why are you bailing out in History.cpp, you don't seem to build that service at all.

Comment 5

[Doug Turner (:dougt)]

1) Sounds like a good idea.  I can file a follow up and get data on the savings!

2) I think History::RegisterVisitedCallback is still being called.  the reason i added that was that there was a ton of calls during page load which all resulted in the gs failure.  I thought it would be better to cut those calls off here.

Comment 6

[Marco Bonardo [:mak]]

(In reply to Doug Turner (:dougt) from comment #5)
> 1) Sounds like a good idea.  I can file a follow up and get data on the
> savings!

Yes, in the meanwhile, you should likely still bail out in Database.cpp, since you are allowing all the other services to work and eventually init places.sqlite. I asked exactly for that.

> 2) I think History::RegisterVisitedCallback is still being called.

You should identify where those calls come from then, since your patch is not compiling History.cpp as an IHistory interface implementer, thus I don't understand where the calls come from.

Comment 7

[Marco Bonardo [:mak]]

As a side question, are the AndroidBridge methods doing IO off main-thread? Otherwise with this patch Mobile is shooting itself in the foot. I don't see the implementation of checkURIVisited and markURIVisited, is there another bug to follow?

Comment 8

[Doug Turner (:dougt)]

1) This patch works and has been tested alot, we can make improvements in followups.

2) Why do you think we aren't using History?  Did we disable it?  I don't think so.


3) AndroidBridge methods doing IO off main-thread.  How is that related to this patch?

4) Yes, 701996

Comment 9

[Marco Bonardo [:mak]]

(In reply to Doug Turner (:dougt) from comment #8)
> 1) This patch works and has been tested alot, we can make improvements in
> followups.

Well, unless this patch is not doing what you expect it do to, if you expect this patch to disable places.sqlite creation, it is not doing that. It is not even stopping History.cpp from using places.sqlite. Thus my suggestion to bail out in Database.cpp, rather than in nsNavHistory.cpp, that is the wrong place to hook. It's an easy change to do.

The followup can surely investigate the better approach of building without MOZ_PLACES, though (will likely break these changes, but that's probably not important till the new native product is released).

> 2) Why do you think we aren't using History?  Did we disable it?  I don't
> think so.

Because all entry points just instantiate the IHistory interface implementer afaict (even by just checking ServiceList.h), and you are replacing Places implementation with yours.
So, if some code is still entering the old History.cpp implementation, there's something really wrong and we should not ignore it. May even jsut be a build issue, like a missed clobber build, but it's scary.

> 3) AndroidBridge methods doing IO off main-thread.  How is that related to
> this patch?

It was a side note, since you are replacing an off-maint-hread implementer with a new one. I am mostly curious about the Android system implementation, nothing more.

Comment 10

[Doug Turner (:dougt)]

> The followup can surely investigate the better approach of building without MOZ_PLACES

Perfect.  I will file the bug when I close this bug out.

> Because all entry points just instantiate the IHistory interface implementer afaict (even by just checking ServiceList.h), and you are replacing Places implementation with yours.

I will remove this #ifdef.

Comment 11

[Marco Bonardo [:mak]]

So, I was looking at Bug 403651 that proposes to add a favicon service call to content, to store a resampled imageDocument as favicon.
Changes like that may be problematic for how this is implemented. Any external call like that one should be wrapped in an ifdef MOZ_PLACES, so that it can be excluded, but with this approach it's not excluded, and each call has to be wrapped by both #ifdef MOZ_PLACES and #ifundef MOZ_ANDROID_HISTORY that is useless complication and error-prone.
Again would be much better to have a separate android-only component implementing iHistory interface and build the product without MOZ_PLACES.
Not doing so means you save adding a visit to Places, but then something else (favicons for example) adds the page to Places from a "backdoor".

If you don't care that may happen, I'll r+ a patch that removes the #ifdef from History.cpp and moves the bailout return from nsNavHistory::Init() to Database::Init() (so that any try to use any Places service will just fail), even if this is more of an hack than a proper replacement.
Waiting for the updated patch.

Comment 12

[Doug Turner (:dougt)]

Attachment: patch v.3

Comment 13

[Doug Turner (:dougt)]

hey marco, didn't see you around.  i landed the patch with your suggestions:

https://hg.mozilla.org/mozilla-central/rev/ac667309bea6

We can fix it up if you see any other problems!  Thanks.