Closed Bug 880259 Opened 7 years ago Closed 6 years ago

Firefox should use GeckoView

Categories

(Firefox for Android :: General, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 26

People

(Reporter: blassey, Assigned: blassey)

References

Details

Attachments

(1 file, 3 obsolete files)

Attached patch patch (obsolete) — Splinter Review
We need the patch on bug 876689 before this lands, but other than that things work fine.
Attachment #759131 - Flags: review?(mark.finkle)
I'd like to see a Try server run. I also think we should hold off on landing this until we merge for Fx25, just to give us as much time as possible. We're not in a rush to get GeckoView into Fx24 and I'd rather not take the added risk.

Sound OK?
Attachment #759131 - Flags: review?(mark.finkle) → review+
(In reply to Mark Finkle (:mfinkle) from comment #1)
> I'd like to see a Try server run. I also think we should hold off on landing
> this until we merge for Fx25, just to give us as much time as possible.
> We're not in a rush to get GeckoView into Fx24 and I'd rather not take the
> added risk.
> 
> Sound OK?

yup
Attached patch patch (obsolete) — Splinter Review
adding sLock to GeckoThread is the thing that fixed the test failures from the original patch. It was on my to-do list to fix that since Kats pointed it out last week.

Along the way of diagnosing this I did some of the other things on my to-do list such as moving GeckoThread initialization into GeckoThread. That is a good thing, but I don't think it is strictly required for this to land.

Here's a try run:

https://tbpl.mozilla.org/?tree=Try&rev=23d09d8c6b2a
Assignee: nobody → blassey.bugs
Attachment #759131 - Attachment is obsolete: true
Attachment #773392 - Flags: review?(mark.finkle)
Comment on attachment 773392 [details] [diff] [review]
patch

The Try run had a few oranges. I retriggered. Code seems sane.
Attachment #773392 - Flags: review?(mark.finkle) → review+
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/855854e05453 - the try run had quite a few oranges, and the retriggers remained orange, but it was actually quite a bit more orange on inbound, mixing in reftest failures and unexpected passes and talos timeouts and failures to initialize and shutdown crashes at mozilla::layers::CompositorParent::ResumeComposition() with the same timeouts the try run showed.
Attached patch use_geckoview.patch (obsolete) — Splinter Review
now that bug 894313 has the gecko thread ownership model sorted out, this works locally (running test via adb). Pushed to try to confirm.

https://tbpl.mozilla.org/?tree=Try&rev=c16cf271aca6

The doinit is a bit of a crutch. There are two remaining issues to get sorted out before I'm really happy with this. The first is moving the ownership of GeckoApp.mProfile to GeckoProfile (something like get/setSelectedProfile()). The other is to sort out the start up timeline for starting the gecko thread. Right now it is a bit racy, so I prefer it to be just handled by GeckoApp for Firefox until it that gets sorted.
Attachment #785184 - Flags: review?(mark.finkle)
(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #6)
> Created attachment 785184 [details] [diff] [review]
> use_geckoview.patch
> 
> now that bug 894313 has the gecko thread ownership model sorted out, this
> works locally (running test via adb). Pushed to try to confirm.
> 
> https://tbpl.mozilla.org/?tree=Try&rev=c16cf271aca6
> 
> The doinit is a bit of a crutch.

I hope this means you plan to remove it someday soon?

> There are two remaining issues to get
> sorted out before I'm really happy with this. The first is moving the
> ownership of GeckoApp.mProfile to GeckoProfile (something like
> get/setSelectedProfile()).

Wes opened bug 902060 for this

> The other is to sort out the start up timeline
> for starting the gecko thread. Right now it is a bit racy, so I prefer it to
> be just handled by GeckoApp for Firefox until it that gets sorted.

Is this the doinit part?
Comment on attachment 785184 [details] [diff] [review]
use_geckoview.patch

The doinit part of this is not acceptable for GeckoView as a user library. It's OK for Fennec  to use internally while we figure out the proper fix.

The Try run has some orange. I re-triggered to see if the orange is persistent. Check that out before landing.
Attachment #785184 - Flags: review?(mark.finkle) → review+
(In reply to Mark Finkle (:mfinkle) from comment #7)
> (In reply to Brad Lassey [:blassey] (use needinfo?) from comment #6)
> > Created attachment 785184 [details] [diff] [review]
> > use_geckoview.patch

> > The other is to sort out the start up timeline
> > for starting the gecko thread. Right now it is a bit racy, so I prefer it to
> > be just handled by GeckoApp for Firefox until it that gets sorted.
> 
> Is this the doinit part?

The doinit part fixes works around both the profile stuff and the racy start

(In reply to Mark Finkle (:mfinkle) from comment #8)
> Comment on attachment 785184 [details] [diff] [review]
> use_geckoview.patch
> 
> The doinit part of this is not acceptable for GeckoView as a user library.
> It's OK for Fennec  to use internally while we figure out the proper fix.
> 
> The Try run has some orange. I re-triggered to see if the orange is
> persistent. Check that out before landing.

I already retriggered the oranges and got greens.
Were you expecting any talos regressions?

Regression: Mozilla-Inbound - Robocop Checkerboarding Real User Benchmark - Android 4.0.4 - 22.2% increase
----------------------------------------------------------------------------------------------------------
    Previous: avg 4.946 stddev 0.373 of 12 runs up to revision aad7f060dadb
    New     : avg 6.046 stddev 0.394 of 12 runs since revision 5de4d20f6cfd
    Change  : +1.100 (22.2% / z=2.950)
    Graph   : http://mzl.la/17BhVO1

Changeset range: http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=aad7f060dadb&tochange=5de4d20f6cfd

Changesets:
  * http://hg.mozilla.org/integration/mozilla-inbound/rev/5de4d20f6cfd
    : Brad Lassey <blassey@mozilla.com> - bug 880259 - Firefox should use GeckoView r=mfinkle
    : http://bugzilla.mozilla.org/show_bug.cgi?id=880259

Bugs:
  * http://bugzilla.mozilla.org/show_bug.cgi?id=880259 - Firefox should use GeckoView


------------------------------


Regression: Mozilla-Inbound - Robocop Checkerboarding Real User Benchmark - Android 2.2 (Native) - 18.7% increase
-----------------------------------------------------------------------------------------------------------------
    Previous: avg 6.106 stddev 0.437 of 12 runs up to revision aad7f060dadb
    New     : avg 7.249 stddev 0.227 of 12 runs since revision f538bf3eafc3
    Change  : +1.143 (18.7% / z=2.613)
    Graph   : http://mzl.la/17BhX8D

Changeset range: http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=aad7f060dadb&tochange=f538bf3eafc3

Changesets:
  * http://hg.mozilla.org/integration/mozilla-inbound/rev/5de4d20f6cfd
    : Brad Lassey <blassey@mozilla.com> - bug 880259 - Firefox should use GeckoView r=mfinkle
    : http://bugzilla.mozilla.org/show_bug.cgi?id=880259
(In reply to Geoff Brown [:gbrown] from comment #10)
> Were you expecting any talos regressions?
> 
no
Depends on: 903482
solid green this time

https://tbpl.mozilla.org/?tree=Try&rev=c95798c546fc
Attachment #773392 - Attachment is obsolete: true
Attachment #785184 - Attachment is obsolete: true
Attachment #789331 - Flags: review?(mark.finkle)
Comment on attachment 789331 [details] [diff] [review]
use_geckoview.patch

Try looked fine and the only difference I can see is that you moved the doInit check sooner in the process. We still want to remove that in the future.
Attachment #789331 - Flags: review?(mark.finkle) → review+
https://hg.mozilla.org/mozilla-central/rev/25a567f1e260
https://hg.mozilla.org/mozilla-central/rev/000bb98adba6
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
You need to log in before you can comment on or make changes to this bug.