[jsplugins] Merging mortar into mozilla-central

RESOLVED FIXED in Firefox 53

Status

()

Core
Plug-ins
RESOLVED FIXED
6 months ago
a month ago

People

(Reporter: lochang, Assigned: lochang)

Tracking

unspecified
mozilla53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(2 attachments, 6 obsolete attachments)

(Assignee)

Description

6 months ago
In this bug, we would submit a first drop-in version of flapper for merging into mozilla-central.
(Assignee)

Updated

6 months ago
Blocks: 1309444
(Assignee)

Comment 1

6 months ago
Created attachment 8808453 [details] [diff] [review]
Bug 1313295 - Merging flapper into mozilla-central

Hi Peter, Benjamin,

The Mortar project is planned to move into M-C. And this patch is the drop-in version of flapper. Would you please review the patch? Thanks.
Assignee: nobody → lochang
Attachment #8808453 - Flags: review?(peterv)
Attachment #8808453 - Flags: review?(benjamin)
(Assignee)

Comment 2

6 months ago
Hi Vance,

Could you please ask the legal team to check the patch as well?
Flags: needinfo?(vchen)
Comment on attachment 8808453 [details] [diff] [review]
Bug 1313295 - Merging flapper into mozilla-central

What kind of review are you looking for? I assumed that Flapper development was happening with constant review, and so there would not be a giant ball of code review like this. If you are asking for review of particular things, please call that out.
Flags: needinfo?(lochang)
(Assignee)

Comment 4

6 months ago
(In reply to Benjamin Smedberg [:bsmedberg] from comment #3)
> What kind of review are you looking for? I assumed that Flapper development
> was happening with constant review, and so there would not be a giant ball
> of code review like this. If you are asking for review of particular things,
> please call that out.

Hi Benjamin,

The Mortar project was originally working on a private github repo. And the codes we expect to land were all reviewed by Peter on the github. So here we are seeking for your help to verify if it is available for us to check flapper into m-c?

The patch is only the drop-in version of flapper. Once it lands into m-c, it is not runnable and will not affect the build of firefox. However, we can develop flapper on m-c and leverage m-c testing framework to write some test case for flapper in short term and to release the Mortar project in the future. Eventually, we would wrap flapper into system add-on aonce the missing parts[1] are all landed.


[1] There are some missing parts such as Peter's gecko patch, integrate building stuff into moz.build, PDF plugin, and so on.

Thanks.
Flags: needinfo?(lochang)
(Assignee)

Updated

6 months ago
Flags: needinfo?(benjamin)
(Assignee)

Comment 5

6 months ago
Created attachment 8810293 [details] [diff] [review]
Bug 1313295 - Merging mortar into mozilla-central

Sorry for that we should rename the folder name "flapper" to "mortar".
Attachment #8808453 - Attachment is obsolete: true
Attachment #8808453 - Flags: review?(peterv)
Attachment #8808453 - Flags: review?(benjamin)
(Assignee)

Comment 6

6 months ago
Comment on attachment 8810293 [details] [diff] [review]
Bug 1313295 - Merging mortar into mozilla-central

Reset the review flag.
Attachment #8810293 - Attachment description: 0001-Bug-1313295-Merging-mortar-into-mozilla-central.patch → Bug 1313295 - Merging mortar into mozilla-central
Attachment #8810293 - Flags: review?(peterv)
Attachment #8810293 - Flags: review?(benjamin)
(Assignee)

Updated

6 months ago
Summary: [jsplugins] Merging flapper into mozilla-central → [jsplugins] Merging mortar into mozilla-central
(In reply to Benjamin Smedberg [:bsmedberg] from comment #3)
> What kind of review are you looking for?

I think we ended up asking for your review because you originally reviewed some of the patches in bug 740795 for pdf.js as a system extension. I think we need some signoff on landing this as a system extension. For example, I see pdf.js has a jar.mn but we don't have one in this patch. We probably need one, right?
jar.mn is a build file, so if this builds correctly then you don't need it. I'm not the right person to review the build changes though, so I'm going bow out of being a reviewer here unless there is very specific pieces of code which should have my review.
Flags: needinfo?(benjamin)
Attachment #8810293 - Flags: review?(benjamin)
(Assignee)

Updated

5 months ago
Duplicate of this bug: 1309444
(Assignee)

Comment 10

5 months ago
Created attachment 8812106 [details] [diff] [review]
part 2 - Integrate mortar into gecko build system

Hi Mike,

Would you please review the patch again?
This patch was the patch originally on bug 1309444, and we have moved it here since it depends on the merging patch on this bug.
Attachment #8812106 - Flags: review?(mh+mozilla)
(Assignee)

Comment 11

5 months ago
Hi Mike, 

So we are not trying to release the mortar right now. However, currently we would build out a repo 
obj-xxx/browser/extension/mortar including librpc.dylib with the build patch. And we think that it is better to land the mortar without affecting the size of binary that everyone built. Is there any build flag we can add to prevent us from building out the mortar repo in the release build?
Flags: needinfo?(mh+mozilla)
(Assignee)

Comment 12

5 months ago
Comment on attachment 8812106 [details] [diff] [review]
part 2 - Integrate mortar into gecko build system

We would ask for Mike's review after we add a build flag.
Attachment #8812106 - Flags: review?(mh+mozilla)
What do you want to do? Enable it on nightly only? On a project branch? On local builds for people who opt-in for it?
Flags: needinfo?(mh+mozilla)
(Assignee)

Comment 14

5 months ago
(In reply to Mike Hommey [:glandium] from comment #13)
> What do you want to do? Enable it on nightly only? On a project branch? On
> local builds for people who opt-in for it?

I think what we want now is that we can build out mortar on local builds only.

Do you have any suggestion? Thanks.
Flags: needinfo?(mh+mozilla)
What you can do is add something like the following to toolkit/moz.configure:

  option('MOZ_MORTAR', help='description of what it is')

  set_config('MOZ_MORTAR', True, when='MOZ_MORTAR')

Then you can enable it locally with this in your mozconfig:

  ac_add_options MOZ_MORTAR=1

Then, in moz.build, you can use CONFIG['MOZ_MORTAR']
Flags: needinfo?(mh+mozilla)
(Assignee)

Comment 16

5 months ago
Created attachment 8813588 [details] [diff] [review]
part 2 - Integrate mortar into gecko build system

Hi Mike,

First of all, thanks for your help. So i have added a build flag so that we can only enable mortar in local build. And would you please review the patch based on the other patch in the bug?
Attachment #8812106 - Attachment is obsolete: true
Attachment #8813588 - Flags: review?(mh+mozilla)
(Assignee)

Comment 17

5 months ago
Created attachment 8814010 [details] [diff] [review]
part 1 - Merging mortar into mozilla-central

Hi Peter, Evelyn,
Would you please take a look at the patch? Thanks.

I have updated the patch to include some license stuff. Below are the details what i have added to the patch:

1) mortar/Makefile: 
  a) Add MPL license header
2) mortar/ppapi:
  a) Add a LICENSE file (BSD license file copy from chromium)
  b) Add MPL license header in ppapi/out/rpc.cc
3) mortar/json:
  a) Remain the same (there is a license header in json.cpp already)
4) mortar/host:
  a) Remove webgl-capture.jsm and it's dependency in ppapi-runtime.jsm
  b) opengles2-utils.jsm remain the same
  c) Add a LICENSE file (Apache license file) in pdf/chrome/locale and pdf/chrome/style
  d) Add MPL license header to rest of the files under mortar/host except pdf/chrome.manifest and flash/chrome.manifest
Attachment #8810293 - Attachment is obsolete: true
Attachment #8810293 - Flags: review?(peterv)
Flags: needinfo?(vchen)
Attachment #8814010 - Flags: review?(peterv)
Attachment #8814010 - Flags: feedback?(ehung)
(Assignee)

Comment 18

5 months ago
Hi Vance,

Could you please double check if we need to deal with the license for the files mentioned in item 3-a and 4-b on comment 17?

Thanks
Flags: needinfo?(vchen)
Comment on attachment 8813588 [details] [diff] [review]
part 2 - Integrate mortar into gecko build system

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

::: toolkit/moz.configure
@@ +858,5 @@
> +def enable_mortar(value):
> +    if value:
> +        return True
> +
> +set_config('MOZ_MORTAR', enable_mortar)

set_config('MOZ_MORTAR', True, when='--enable-mortar')

would be simpler and wouldn't require the enable_mortar function.
Attachment #8813588 - Flags: review?(mh+mozilla) → review+
(Assignee)

Comment 20

5 months ago
Created attachment 8815548 [details] [diff] [review]
[Final] part 2 - Integrate mortar into gecko build system, r=glandiium

Thanks for Mike's review, the updated patch address the issue mentioned in comment 19.
Attachment #8813588 - Attachment is obsolete: true
(Assignee)

Comment 21

5 months ago
Comment on attachment 8815548 [details] [diff] [review]
[Final] part 2 - Integrate mortar into gecko build system, r=glandiium

per comment 19, carry over r+
Attachment #8815548 - Flags: review+
Comment on attachment 8814010 [details] [diff] [review]
part 1 - Merging mortar into mozilla-central

Questions to Peter:
1. do we need to keep test-json target in Makefile?
2. do we want to check-in interpose?
Attachment #8814010 - Flags: feedback?(ehung) → feedback+
(Assignee)

Comment 23

5 months ago
> Questions to Peter:
> 1. do we need to keep test-json target in Makefile?
> 2. do we want to check-in interpose?

These two questions we had a conclusion in prior offline discussion:
1. We will keep test.cpp (json) in repo, so it seems we can still keep the target in Makefile.
2. We will keep interpose.cc in repo for now.
(Assignee)

Comment 24

5 months ago
Created attachment 8818151 [details] [diff] [review]
[Final] part 1 - Merging mortar into mozilla central, r=peterv

Updated patch
1) remove Apache LICENSE files in mortar/host/pdf/chrome/locale and mortar/host/pdf/chrome/style
2) fix the license header in mortar/ppapi/out/rpc.cc
as the offline discussion with Vance and Elvin.
Attachment #8814010 - Attachment is obsolete: true
Attachment #8814010 - Flags: review?(peterv)
Flags: needinfo?(vchen)
Attachment #8818151 - Flags: review?(peterv)
(Assignee)

Updated

5 months ago
Blocks: 1323120
(Assignee)

Comment 25

5 months ago
Created attachment 8818201 [details] [diff] [review]
Diff of git repo for review

This is the diff of git repo.
Attachment #8818151 - Flags: review?(peterv) → review+
(Assignee)

Updated

4 months ago
Attachment #8818201 - Attachment is obsolete: true
(Assignee)

Updated

4 months ago
Attachment #8818151 - Attachment description: part 1 - Merging mortar into mozilla central → [Final] part 1 - Merging mortar into mozilla central, r=peterv
(Assignee)

Comment 26

4 months ago
The patches have all reviewed. But we will wait for the legal bug 1323120 being reviewed, then land them together.
(Assignee)

Updated

4 months ago
Keywords: checkin-needed

Comment 27

4 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/5956d0c8b4f0
Merge mortar into mozilla-central. r=peterv
https://hg.mozilla.org/integration/autoland/rev/4903bfce17c6
Integrate mortar into gecko build system. r=glandium
Keywords: checkin-needed

Comment 28

4 months ago
Pushed by kwierso@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/b343039330c5
Add browser/extensions/mortar/** to .eslintignore for now r=me
(In reply to Pulsebot from comment #28)
> Pushed by kwierso@gmail.com:
> https://hg.mozilla.org/integration/autoland/rev/b343039330c5
> Add browser/extensions/mortar/** to .eslintignore for now r=me

I'm not sure what followup needs to happen for this. Here's what eslint was failing without this: https://treeherder.mozilla.org/logviewer.html#?job_id=8131111&repo=autoland
Flags: needinfo?(lochang)
(Assignee)

Comment 30

4 months ago
(In reply to Wes Kocher (:KWierso) from comment #29)
> I'm not sure what followup needs to happen for this. Here's what eslint was
> failing without this:
> https://treeherder.mozilla.org/logviewer.html#?job_id=8131111&repo=autolan

Ok, thanks. We will deal with the eslint issues on bug 1324623.
Flags: needinfo?(lochang)

Comment 31

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5956d0c8b4f0
https://hg.mozilla.org/mozilla-central/rev/4903bfce17c6
https://hg.mozilla.org/mozilla-central/rev/b343039330c5
Status: NEW → RESOLVED
Last Resolved: 4 months ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
(Assignee)

Updated

2 months ago
Blocks: 1347440
(Assignee)

Updated

a month ago
No longer blocks: 1347440
You need to log in before you can comment on or make changes to this bug.