Closed Bug 1204965 Opened 4 years ago Closed 4 years ago

Merge larch back to mozilla-central

Categories

(Firefox OS Graveyard :: General, defect)

defect
Not set

Tracking

(firefox44 fixed)

RESOLVED FIXED
Tracking Status
firefox44 --- fixed

People

(Reporter: kgrandon, Assigned: kgrandon)

References

Details

Attachments

(1 file, 7 obsolete files)

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.
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.
Fabrice - any opinion here? Are we ready to do this, and should we do so by landing individual bugs to m-c?
Flags: needinfo?(fabrice)
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?
Flags: needinfo?(fabrice)
(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.
(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 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.
Attached patch Larch -> mozilla-central merge (obsolete) — Splinter Review
Here is a patch which fixes all of the found issues in the initial pass.
Attachment #8661347 - Attachment is obsolete: true
Attached patch Larch -> mozilla-central merge (obsolete) — Splinter Review
Attachment #8665142 - Attachment is obsolete: true
Attached patch Larch -> mozilla-central merge (obsolete) — Splinter Review
New patch, fixing ifdef statements to address emulator mochitest failures.
Attachment #8665182 - Attachment is obsolete: true
Attached patch Larch -> mozilla-central merge (obsolete) — Splinter Review
Rebasing.
Attachment #8665443 - Attachment is obsolete: true
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!
Attachment #8665448 - Flags: review?(fabrice)
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
Attachment #8665448 - Flags: review?(mh+mozilla)
Attachment #8665448 - Flags: review?(fabrice)
Attachment #8665448 - Flags: review+
Blocks: 1209389
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.
Attachment #8665448 - Flags: review?(mh+mozilla) → review+
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.
Updated comments, fixed whitespace, and rebased against m-c. Carrying reviews.

Running on try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8fed7128cfaf
Attachment #8665448 - Attachment is obsolete: true
Attachment #8667976 - Flags: review+
Assignee: nobody → kevingrandon
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.
Attachment #8668103 - Flags: review+
Attachment #8667976 - Attachment is obsolete: true
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!
Flags: needinfo?(kevingrandon)
Keywords: checkin-needed
Rebased patch, carrying review.
Attachment #8668103 - Attachment is obsolete: true
Flags: needinfo?(kevingrandon)
Attachment #8668288 - Flags: review+
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
Keywords: checkin-needed
Hold on, noticed that webapps.js in devtools looks wrong, I need to fix that first.
Keywords: checkin-needed
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
Flags: needinfo?(kevingrandon)
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.
Flags: needinfo?(kevingrandon)
Depends on: 1210414
https://hg.mozilla.org/mozilla-central/rev/de3b5f164ac5
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Blocks: 1210822
Blocks: graphene
Is FxOS :: Runtime a better location for this bug?  Core :: DOM seems random to me, but I wasn't sure.
Flags: needinfo?(kevingrandon)
(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.
Component: DOM → General
Flags: needinfo?(kevingrandon)
Product: Core → Firefox OS
Target Milestone: mozilla44 → ---
You need to log in before you can comment on or make changes to this bug.