1313295 - [jsplugins] Merging mortar into mozilla-central

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Louis Chang [:louis]]

In this bug, we would submit a first drop-in version of flapper for merging into mozilla-central.

Comment 1

[Louis Chang [:louis]]

Attachment: 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.

Comment 2

[Louis Chang [:louis]]

Hi Vance,

Could you please ask the legal team to check the patch as well?

Comment 3

[Benjamin Smedberg]

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.

Comment 4

[Louis Chang [:louis]]

(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.

Comment 5

[Louis Chang [:louis]]

Attachment: Bug 1313295 - Merging mortar into mozilla-central

Sorry for that we should rename the folder name "flapper" to "mortar".

Comment 6

[Louis Chang [:louis]]

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

Reset the review flag.

Comment 7

[Peter Van der Beken [:peterv]]

(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?

Comment 8

[Benjamin Smedberg]

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.

Comment 10

[Louis Chang [:louis]]

Attachment: 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.

Comment 11

[Louis Chang [:louis]]

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?

Comment 12

[Louis Chang [:louis]]

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.

Comment 13

[Mike Hommey [:glandium]]

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?

Comment 14

[Louis Chang [:louis]]

(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.

Comment 15

[Mike Hommey [:glandium]]

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']

Comment 16

[Louis Chang [:louis]]

Attachment: 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?

Comment 17

[Louis Chang [:louis]]

Attachment: 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

Comment 18

[Louis Chang [:louis]]

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

Comment 19

[Mike Hommey [:glandium]]

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.

Comment 20

[Louis Chang [:louis]]

Attachment: [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.

Comment 21

[Louis Chang [:louis]]

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

per comment 19, carry over r+

Comment 22

[Evelyn Hung]

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?

Comment 23

[Louis Chang [:louis]]

> 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.

Comment 24

[Louis Chang [:louis]]

Attachment: [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.

Comment 25

[Louis Chang [:louis]]

Attachment: Diff of git repo for review

This is the diff of git repo.

Comment 26

[Louis Chang [:louis]]

The patches have all reviewed. But we will wait for the legal bug 1323120 being reviewed, then land them together.

Comment 27

[Pulsebot]

Pushed by [e-mail]:
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

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b343039330c5
Add browser/extensions/mortar/** to .eslintignore for now r=me

Comment 29

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

(In reply to Pulsebot from comment #28)
> Pushed by [e-mail]:
> 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

Comment 30

[Louis Chang [:louis]]

(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.

Comment 31

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5956d0c8b4f0
https://hg.mozilla.org/mozilla-central/rev/4903bfce17c6
https://hg.mozilla.org/mozilla-central/rev/b343039330c5