1204965 - Merge larch back to mozilla-central

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Kevin Grandon :kgrandon]

Filing a tracking bug to get larch merged back to m-c. I'm not yet sure if we want to track this in individual bugs, but opening this for discussion/tracking.

Comment 1

[Kevin Grandon :kgrandon]

Attachment: Current full diff of larch to mozilla-central

I have a current diff of all changes here: https://github.com/KevinGrandon/gecko-projects/compare/gecko_dev_master...KevinGrandon:larch

I'm also uploading a patch here with the full changes.

Comment 2

[Kevin Grandon :kgrandon]

Fabrice - any opinion here? Are we ready to do this, and should we do so by landing individual bugs to m-c?

Comment 3

[[:fabrice] Fabrice Desré]

I'm not too worried about the code merge itself, but we need to figure out:
- releng builds.
- can we run some/all of the browser.html tests? are there any?

Comment 4

[Kevin Grandon :kgrandon]

(In reply to [:fabrice] Fabrice Desré from comment #3)
> I'm not too worried about the code merge itself, but we need to figure out:
> - releng builds.

Seems like we should be able to do this since we're building multiple products on larch now. Just need to hookup the new mozconfigs, right?


> - can we run some/all of the browser.html tests? are there any?

There is one test in browser.html, and horizon will have tests as well. I would like to run the horizon tests on treeherder, and potentially browser.html tests as well.

Comment 5

[[:fabrice] Fabrice Desré]

(In reply to Kevin Grandon :kgrandon from comment #4)
> (In reply to [:fabrice] Fabrice Desré from comment #3)
> > I'm not too worried about the code merge itself, but we need to figure out:
> > - releng builds.
> 
> Seems like we should be able to do this since we're building multiple
> products on larch now. Just need to hookup the new mozconfigs, right?

Sure, but the load generated by 2 new products on all integration branches + m-c will be higher than just being on larch.

> > - can we run some/all of the browser.html tests? are there any?
> 
> There is one test in browser.html, and horizon will have tests as well. I
> would like to run the horizon tests on treeherder, and potentially
> browser.html tests as well.

Sounds good.

Comment 6

[Kevin Grandon :kgrandon]

Comment on attachment 8661347 [details] [diff] [review]
Current full diff of larch to mozilla-central

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

First pass notes.

::: b2g/app/b2g.js
@@ -440,1 @@
>  pref("dom.meta-viewport.enabled", true);

Needs to be wrapped by MOZ_MULET || MOZ_GRAPHENE.

@@ -1072,5 @@
>  // Enable webapps add-ons
>  pref("dom.apps.customization.enabled", true);
>  
> -// Original caret implementation on collapsed selection.
> -pref("touchcaret.enabled", false);

Needs to be wrapped in MOZ_GRAPHENE.

@@ +1090,5 @@
>  pref("dom.mapped_arraybuffer.enabled", true);
>  #endif
>  
> +// BroadcastChannel API
> +pref("dom.broadcastChannel.enabled", true);

Remove this.

::: b2g/branding/unofficial/configure.sh
@@ -3,4 @@
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
>  MOZ_APP_DISPLAYNAME=B2G
> -MOZ_UPDATER=

Revert this if it doesn't break.

::: b2g/installer/package-manifest.in
@@ -477,4 @@
>  @RESPATH@/components/NetworkStatsManager.manifest
>  @RESPATH@/components/NetworkStatsServiceProxy.js
>  @RESPATH@/components/NetworkStatsServiceProxy.manifest
> -@RESPATH@/components/TetheringService.js

We should not change this.

::: dom/ipc/Blob.cpp
@@ +4447,5 @@
>    MOZ_ASSERT(mOwnsBlobImpl);
>  
>    // In desktop e10s the file picker code sends this message.
> +
> +#if defined(MOZ_CHILD_PERMISSIONS) && !defined(MOZ_GRAPHENE)

Exclude this, and land in bug 1180288.

::: dom/workers/test/promise_worker.js
@@ +170,4 @@
>    xhr.open("GET", "testXHR.txt", false);
>    xhr.send(null);
>  
> +  todo(!handlerExecuted, "Sync XHR should not trigger microtask execution.");

This is from bug 1185187.

https://github.com/KevinGrandon/gecko-projects/commit/91b752fdf8331a2aa2999e407610f77fa25af60e

Let's back this out.

::: gfx/vr/OVR_CAPIShim.c
@@ +1,1 @@
> +/************************************************************************************

This file should not be here.

::: mobile/android/app/mobile.js
@@ +804,5 @@
>  pref("dom.payment.provider.0.type", "mozilla/payments/pay/v1");
>  pref("dom.payment.provider.0.requestMethod", "GET");
>  
> +// Enable Cardboard VR on mobile, assuming VR at all is enabled
> +pref("dom.vr.cardboard.enabled", true);

Seems like VR. Don't make this change.

Comment 7

[Kevin Grandon :kgrandon]

Attachment: Larch -> mozilla-central merge

Here is a patch which fixes all of the found issues in the initial pass.

Comment 8

[Kevin Grandon :kgrandon]

Attachment: Larch -> mozilla-central merge

Comment 9

[Kevin Grandon :kgrandon]

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

Comment 10

[Kevin Grandon :kgrandon]

Attachment: Larch -> mozilla-central merge

New patch, fixing ifdef statements to address emulator mochitest failures.

Comment 11

[Kevin Grandon :kgrandon]

Attachment: Larch -> mozilla-central merge

Rebasing.

Comment 12

[Kevin Grandon :kgrandon]

New try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4ad0583f26ad

Comment 13

[Kevin Grandon :kgrandon]

Comment on attachment 8665448 [details] [diff] [review]
Larch -> mozilla-central merge

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

Fabrice - please take a look if you can. I would like to review this and get it landed to start working on treeherder tests for browser.html/horizon.

I've tested a device build and a mulet build and everything seems fine to me. I think the try run looks ok as well, but please verify: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4ad0583f26ad

Thanks!

Comment 14

[[:fabrice] Fabrice Desré]

Comment on attachment 8665448 [details] [diff] [review]
Larch -> mozilla-central merge

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

Looks fine to me, thanks!

Mike, we are moving to m-c the changes we did on larch for the browser.html & horizon runtime. You did look at them at the time, there should not be anything new in configure.in

Comment 15

[Mike Hommey [:glandium]]

Comment on attachment 8665448 [details] [diff] [review]
Larch -> mozilla-central merge

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

Only skimmed over the build system changes. I really hate our mozconfigs.

::: b2g/app/macbuild/Contents/Info.plist.in
@@ +32,4 @@
>  	<true/>
>  	<key>NSPrincipalClass</key>
>  	<string>GeckoNSApplication</string>
> +  <key>CFBundleURLTypes</key>

Indentation doesn't seem to match

::: b2g/graphene/config/horizon-mozconfigs/common
@@ +1,5 @@
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +# This file is included at the top of all b2g mozconfigs

That's obviously not true :)

::: configure.in
@@ +7521,5 @@
>  dnl ========================================================
> +dnl = Horizon build options - set default preferences for
> +dnl   the horizon project.
> +dnl ========================================================
> +if test -n "$MOZ_HORIZON"; then

What the hell is horizon? Same applies to graphene, btw. Code names are nice, but somewhere, there needs to be an explanation what it is for.

Comment 16

[Kevin Grandon :kgrandon]

Thanks for the review, will work on addressing your comments.

(In reply to Mike Hommey [:glandium] from comment #15)
> ::: configure.in
> @@ +7521,5 @@
> >  dnl ========================================================
> > +dnl = Horizon build options - set default preferences for
> > +dnl   the horizon project.
> > +dnl ========================================================
> > +if test -n "$MOZ_HORIZON"; then
> 
> What the hell is horizon? Same applies to graphene, btw. Code names are
> nice, but somewhere, there needs to be an explanation what it is for.

It's a VR product. I'll add a comment which will provide some more context.

Comment 17

[Kevin Grandon :kgrandon]

Attachment: [Patch] Larch -> mozilla-central merge

Updated comments, fixed whitespace, and rebased against m-c. Carrying reviews.

Running on try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8fed7128cfaf

Comment 18

[Kevin Grandon :kgrandon]

Attachment: [Patch] Larch -> mozilla-central merge

Oops, the graphene specific binary files with that last patch were dropped. This is the same patch with the review fixes, but just with the additional blobs. Carrying the review.

Comment 19

[Kevin Grandon :kgrandon]

Adding checkin-needed for the patch here: https://bug1204965.bmoattachments.org/attachment.cgi?id=8668103
Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8fed7128cfaf

Thanks!

Comment 20

[Carsten Book [:Tomcat]]

Hi,

this failed to apply:

renamed 1204965 -> 0001-Bug-1204965-Graphene-support.-Merge-larch-into-mozil.patch
applying 0001-Bug-1204965-Graphene-support.-Merge-larch-into-mozil.patch
patching file b2g/chrome/jar.mn
Hunk #1 FAILED at 11
1 out of 2 hunks FAILED -- saving rejects to file b2g/chrome/jar.mn.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and refresh 0001-Bug-1204965-Graphene-support.-Merge-larch-into-mozil.patch

Kevin, could you take a look, thanks!

Comment 21

[Kevin Grandon :kgrandon]

Attachment: [Patch] Larch -> mozilla-central merge

Rebased patch, carrying review.

Comment 22

[Kevin Grandon :kgrandon]

This was rebased since the try run, but the rebase was quite simple and I don't think it should impact the try run. 

Adding checkin-needed for the patch here: https://bug1204965.bmoattachments.org/attachment.cgi?id=8668103
Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8fed7128cfaf

Comment 23

[Kevin Grandon :kgrandon]

Hold on, noticed that webapps.js in devtools looks wrong, I need to fix that first.

Comment 24

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/de3b5f164ac5

Comment 25

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Hi Kevin, 

Why are touchcaret and selectioncarets enabled on graphine in [1]? Do you bump into some try fail before? Enable both of them should have no effect since accessiblecaret are enabled on b2g now.

[1] https://hg.mozilla.org/integration/mozilla-inbound/diff/de3b5f164ac5/b2g/app/b2g.js#l1.49

Comment 26

[Paul Rouget [:paul]]

Carets should be disabled.

They are disabled here: https://hg.mozilla.org/integration/mozilla-inbound/file/bc20a0abec61/b2g/graphene/graphene.js#l30

Comment 27

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Re comment 26:

I notice that graphene.js are included at the bottom b2g.js. If carets are not needed by graphine, I would like to remove the prefs. They are disabled by default in all.js. Filed bug 1210337.

Comment 28

[Carsten Book [:Tomcat]]

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

Comment 29

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Is FxOS :: Runtime a better location for this bug?  Core :: DOM seems random to me, but I wasn't sure.

Comment 30

[Kevin Grandon :kgrandon]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #29)
> Is FxOS :: Runtime a better location for this bug?  Core :: DOM seems random
> to me, but I wasn't sure.

Yeah, Firefox OS : General seems better for now - until graphene has a product.