Closed Bug 702037 Opened 8 years ago Closed 8 years ago

Merge Android History into mozilla-central

Categories

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

x86
macOS
defect

Tracking

()

RESOLVED FIXED

People

(Reporter: dougt, Assigned: dougt)

References

Details

(Whiteboard: [QA?])

Attachments

(1 file, 2 obsolete files)

Attached patch patch v.1 (obsolete) — Splinter Review
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.
Attachment #574079 - Flags: review?(dolske)
Assignee: nobody → doug.turner
Attached patch patch v.2 (obsolete) — Splinter Review
Attachment #574079 - Attachment is obsolete: true
Attachment #574079 - Flags: review?(dolske)
Attachment #574083 - Flags: review?(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.
Attachment #574083 - Flags: review?(dolske) → review?(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.
Attachment #574083 - Flags: review?(mak77)
Attachment #574083 - Flags: review?(dietrich)
Attachment #574083 - Flags: review+
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.
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.
(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.
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?
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
(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.
> 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.
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.
Priority: -- → P5
Attached patch patch v.3Splinter Review
Attachment #574083 - Attachment is obsolete: true
Attachment #574083 - Flags: review?(mak77)
Attachment #575510 - Flags: review?(mak77)
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.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Attachment #575510 - Flags: review?(mak77) → review+
Whiteboard: [QA?]
You need to log in before you can comment on or make changes to this bug.