1178513 - Gecko changes for OpenMobile Application Compatibility Layer

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Comment 1

[Bill Walker [:bwalker] [@wfwalker]]

These changes support the integration of OpenMobile's Application Compatibility Layer into Firefox OS.

Comment 2

[Bill Walker [:bwalker] [@wfwalker]]

Attachment: wip-DomElementExtAPP.patch

WIP patch for new "extapp" (external application) DOM element

Comment 3

[Bill Walker [:bwalker] [@wfwalker]]

Attachment: wip-GrallocImagesRGBA8888ToRGB565.patch

WIP patch to add support for RGBA8888 to RGB565 conversion

Comment 4

[Bill Walker [:bwalker] [@wfwalker]]

Attachment: wip-ExportGeckoSymbolsForACL.patch

WIP patch for making external a few symbols needed by OpenMobile ACL. Note: this patch will be resubmitted using B2G_EXPORT instead of NS_EXPORT

Comment 5

[Didem Ersoz [:didem]]

Comment on attachment 8627411 [details] [diff] [review]
wip-GrallocImagesRGBA8888ToRGB565.patch

Hi Jet,

This is WIP patch to add support for RGBA8888 to RGB565 conversion, provided by Open Mobile. Could you review it?

Thanks in advance,
Didem

Comment 6

[Didem Ersoz [:didem]]

Comment on attachment 8627415 [details] [diff] [review]
wip-ExportGeckoSymbolsForACL.patch

Hi Jet,

This is the WIP patch for making external a few symbols needed by OpenMobile ACL. Note: this patch will be resubmitted using B2G_EXPORT instead of NS_EXPORT

Could you review it?

Thanks in advance,
Didem

Comment 7

[Didem Ersoz [:didem]]

Comment on attachment 8627410 [details] [diff] [review]
wip-DomElementExtAPP.patch

Hi Kyle,

Could you review this patch (created by William Chen and updated by Open Mobile)?

Thanks in advance,
Didem

Comment 8

[Didem Ersoz [:didem]]

Hi Paul,

In this bug, you'll find the custom gecko integration components for the Open Mobile project. 

wip-DomElementExtAPP.patch: created by William Chen and updated by Open Mobile
wip-GrallocImagesRGBA8888ToRGB565.patch: created by Open Mobile to add support for RGBA8888 to RGB565 conversion
wip-ExportGeckoSymbolsForACL.patch: created by Open Mobile to export the symbols needed by ACL.

Thanks,
Didem

Comment 9

[Bill Walker [:bwalker] [@wfwalker]]

Attachment: wip-2-ExportGeckoSymbolsForACL.patch

revised version of this patch using B2G_EXPORT macro

Comment 10

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8627411 [details] [diff] [review]
wip-GrallocImagesRGBA8888ToRGB565.patch

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

::: gfx/layers/GrallocImages.cpp
@@ +362,5 @@
>    }
>  
> +  if (format == HAL_PIXEL_FORMAT_RGBA_8888) {
> +      uint32_t* buf32 = (uint32_t*)(buffer);
> +      uint16_t* buf16 = (uint16_t*)(aMappedSurface->mData);

Name these 'src' and 'dest' or similar, I think that's clearer.

@@ +364,5 @@
> +  if (format == HAL_PIXEL_FORMAT_RGBA_8888) {
> +      uint32_t* buf32 = (uint32_t*)(buffer);
> +      uint16_t* buf16 = (uint16_t*)(aMappedSurface->mData);
> +
> +      // Convert RGBA8888 to RGB565

This file is using two-space indenting

@@ +365,5 @@
> +      uint32_t* buf32 = (uint32_t*)(buffer);
> +      uint16_t* buf16 = (uint16_t*)(aMappedSurface->mData);
> +
> +      // Convert RGBA8888 to RGB565
> +      for (int i = 0; i < width * height; i++) {

uint32_t or size_t for i.

Comment 11

[Hamzata Diallo]

Attachment: GrallocImagesRGBA8888ToRGB565.patch

Patch updated

Comment 12

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8627711 [details] [diff] [review]
wip-2-ExportGeckoSymbolsForACL.patch

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

What mozilla branches/repositories are you planning on landing this on?

::: xpcom/base/nscore.h
@@ +135,5 @@
>  
>  #endif
>  
> +#ifdef MOZ_WIDGET_GONK
> +#define B2G_EXPORT NS_EXPORT

Please call this OPEN_MOBILE_EXPORT or something similar to make it very clear this is for the OpenMobile ACL.

Comment 13

[Bill Walker [:bwalker] [@wfwalker]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #12)
> Comment on attachment 8627711 [details] [diff] [review]
> wip-2-ExportGeckoSymbolsForACL.patch
> 
> Review of attachment 8627711 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> What mozilla branches/repositories are you planning on landing this on?

Hi Matt, this will be needed on all the b2g 2.X branches, but for now we need it on 2.1S and 2.0M.

Comment 14

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8627410 [details] [diff] [review]
wip-DomElementExtAPP.patch

wchen says another version of this is forthcoming.

Comment 15

[Bill Walker [:bwalker] [@wfwalker]]

(In reply to Hamzata Diallo from comment #11)
> Created attachment 8627755 [details] [diff] [review]
> GrallocImagesRGBA8888ToRGB565.patch
> 
> Patch updated


I believe this update satisfies mattwoodrow's concerns, and that GrallocImagesRGBA8888ToRGB565.patch is now ready to land.

Comment 16

[William Chen [:wchen]]

Attachment: <extapp> element and interfaces

This patch adds:
<extapp> element that calls into XPCOM component.
Interface methods on element to communicate from script to XPCOM component.
An event dispatching object to allow XPCOM component to generate "externalappevent" events on element.
All functionality should be hidden behind "external-app" permission.

Comment 17

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8627813 [details] [diff] [review]
<extapp> element and interfaces

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

This needs tests.  There should be additions to test_interfaces.html, there should be a test that if you have a page without the external-app permission <extapp> becomes HTMLUnknownElement (or whatever that is called), etc.

::: content/html/content/src/HTMLExtAppElement.cpp
@@ +75,5 @@
> +HTMLExtAppElement::PostMessage(const nsAString& aMessage, ErrorResult& aRv)
> +{
> +  if (mApp) {
> +    aRv = mApp->PostMessage(aMessage);
> +  }

You should throw if mApp is null.

::: dom/webidl/HTMLExtAppElement.webidl
@@ +1,1 @@
> +[CheckPermissions="external-app"]

Needs a license header

::: parser/htmlparser/nsElementTable.cpp
@@ +172,5 @@
>      /*parent,leaf*/ kSpecial, true
>    },
>    {
> +    /*tag*/         eHTMLTag_extapp,
> +    /*parent,leaf*/ kSpecial, false

How come this isn't a leaf element?

Comment 18

[Didem Ersoz [:didem]]

Hamzata,

Could you kindly address Matt's feedback in comment 12? He is simply asking to "call this OPEN_MOBILE_EXPORT or something similar to make it very clear this is for the OpenMobile ACL."

Thanks,
Didem

Comment 19

[Didem Ersoz [:didem]]

Hi Matt,

As Bill responded, we need the "wip-2-ExportGeckoSymbolsForACL.patch" primarily in 2.0m and 2.1s, and eventually in all 2.x branches. 

I pinged Hamzata to address your feedback in comment 12. Do you have any other comments? It looks like the patch is still under review. We need to land this patch by Thursday July 2nd. 

Thanks in advance,
Didem

Comment 20

[William Chen [:wchen]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #17)
> Comment on attachment 8627813 [details] [diff] [review]
> <extapp> element and interfaces
> 
> Review of attachment 8627813 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: parser/htmlparser/nsElementTable.cpp
> @@ +172,5 @@
> >      /*parent,leaf*/ kSpecial, true
> >    },
> >    {
> > +    /*tag*/         eHTMLTag_extapp,
> > +    /*parent,leaf*/ kSpecial, false
> 
> How come this isn't a leaf element?

No particular reason, it probably doesn't make a difference either way for this one-off specialized element.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=41e8aec78232

https://hg.mozilla.org/integration/mozilla-inbound/rev/26b00bd0c546

Comment 21

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Well HTMLUnknownElement is a leaf ... and this should match it in parsing behavior when it's not enabled.

Comment 22

[William Chen [:wchen]]

We don't use the element table in the HTML parser anymore, it only exists for some legacy code that we have a bug filed to remove. Also, tag names that become HTMLUnknownElement will be eHTMLTag_userdefined in this table rather than eHTMLTag_unknown, which are not marked as a leaf, although it probably wouldn't make much of a difference either way.

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/26b00bd0c546

Comment 24

[Hamzata Diallo]

(In reply to Didem Ersoz [:didem] from comment #18)
> Hamzata,
> 
> Could you kindly address Matt's feedback in comment 12? He is simply asking
> to "call this OPEN_MOBILE_EXPORT or something similar to make it very clear
> this is for the OpenMobile ACL."
> 
> Thanks,
> Didem

We could rename it to something else if you prefer, but I would rather not mention OpenMobile or ACL directly in these patches.

Comment 25

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8627711 [details] [diff] [review]
wip-2-ExportGeckoSymbolsForACL.patch

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

What mozilla branches/repositories are you planning on landing this on?

::: xpcom/base/nscore.h
@@ +135,5 @@
>  
>  #endif
>  
> +#ifdef MOZ_WIDGET_GONK
> +#define B2G_EXPORT NS_EXPORT

Please call this OPEN_MOBILE_EXPORT or something similar to make it very clear this is for the OpenMobile ACL.

Comment 26

[Matt Woodrow (:mattwoodrow)]

(In reply to Hamzata Diallo from comment #24)

> 
> We could rename it to something else if you prefer, but I would rather not
> mention OpenMobile or ACL directly in these patches.

Why not? These exports are specifically for the OpenMobile extension, and we want that to be very clear so that it's less confusing for other gecko developers.

Comment 27

[William Chen [:wchen]]

Comment on attachment 8627813 [details] [diff] [review]
<extapp> element and interfaces

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): This bug (Bug 1178513)
[User impact] if declined: We will not be able to integrate OpenMobile Application Compatibility Layer with FFOS.
[Testing completed]: No regressions on treeherder and I have manually tested features.
[Risk to taking this patch] (and alternatives if risky): Low risk, changes in the patch are mostly adding new features that are only visible to apps with "external-app" permission.
[String changes made]: None

Comment 28

[William Chen [:wchen]]

Comment on attachment 8627813 [details] [diff] [review]
<extapp> element and interfaces

Oops, wrong flags. Need b2g32 for v2.0m and b2g34 for v2.1s.

Comment 29

[[:fabrice] Fabrice Desré]

Comment on attachment 8627813 [details] [diff] [review]
<extapp> element and interfaces

[Triage Comment]

Comment 30

[Bill Walker [:bwalker] [@wfwalker]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #25)
> Comment on attachment 8627711 [details] [diff] [review]
> wip-2-ExportGeckoSymbolsForACL.patch
> 
> Review of attachment 8627711 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> What mozilla branches/repositories are you planning on landing this on?
> 
> ::: xpcom/base/nscore.h
> @@ +135,5 @@
> >  
> >  #endif
> >  
> > +#ifdef MOZ_WIDGET_GONK
> > +#define B2G_EXPORT NS_EXPORT
> 
> Please call this OPEN_MOBILE_EXPORT or something similar to make it very
> clear this is for the OpenMobile ACL.

How about B2G_ACL_EXPORT ?

Comment 31

[Hamzata Diallo]

(In reply to William Chen [:wchen] from comment #28)
> Comment on attachment 8627813 [details] [diff] [review]
> <extapp> element and interfaces
> 
> Oops, wrong flags. Need b2g32 for v2.0m and b2g34 for v2.1s.

William, in my original patch, we were using GetInnerWindow() instead of GetWindow(), that's the one ACL needs. I see that your patch reverted to use GetWindow(). Could you please update to use Innerwindow or tell us how to retrieve the "inner window" from the "window" ?  Thanks.

Comment 32

[William Chen [:wchen]]

The patch that went in the tree (https://hg.mozilla.org/mozilla-central/rev/26b00bd0c546) uses GetInnerWindow(), not sure why the patch in this bug doesn't. I might have uploaded an old one.

Comment 33

[William Chen [:wchen]]

Commit for the RGBA8888 to RGB565 converter

https://hg.mozilla.org/integration/mozilla-inbound/rev/63da57eefeeb

Comment 34

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8c09dc6015a3

Comment 35

[William Chen [:wchen]]

^ Commit for exporting gecko symbols

We need a special patch for uplifting this because some functions were moved into mozglue and no longer require exporting. I'll attach a backport friendly patch soon.

Comment 36

[William Chen [:wchen]]

Attachment: v2.2 friendly export for gecko symbols

This patch should be used when backporting the patch to export gecko symbols.

Comment 37

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/fa9ac75516dd

Comment 38

[Ryan VanderMeulen [:RyanVM]]

(In reply to Fabrice Desré [:fabrice] from comment #29)

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/b465f8d7254f
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/d13e7c16c4b7
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/60b91131dada
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/rev/60b91131dada

Still working on the b2g32 rebase.

Comment 39

[Matt Woodrow (:mattwoodrow)]

(In reply to William Chen [:wchen] from comment #35)
> ^ Commit for exporting gecko symbols
> 
> We need a special patch for uplifting this because some functions were moved
> into mozglue and no longer require exporting. I'll attach a backport
> friendly patch soon.

Why was that patch landed on mozilla-inbound?

I thought we were only landing this on the 2.x branches?

We don't really want this stuff littering mozilla-central.

Comment 40

[William Chen [:wchen]]

I asked about that, but I was told to land these patches onto m-c then uplift.

Comment 41

[[:fabrice] Fabrice Desré]

(In reply to William Chen [:wchen] from comment #40)
> I asked about that, but I was told to land these patches onto m-c then
> uplift.

For sure this is not ideal to take that in our tree, but branch specific patches are not great either. We would have to rebase again on new branches, and that has not worked well in the past.
Once again, I would prefer 10x that we offer a clean api, but until I prefer to not bitrot these changes.

Comment 42

[Ryan VanderMeulen [:RyanVM]]

(In reply to William Chen [:wchen] from comment #33)
(In reply to Pulsebot from comment #34)

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/a11ad34e629b
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/013939903669

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/06e365b6dc31
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/37de99064c33

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/rev/06e365b6dc31
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/rev/37de99064c33

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/fd55e33b1add
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/c1337ef29c96
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/2a92b52cc30e

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/fd55e33b1add
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/c1337ef29c96
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/2a92b52cc30e

Comment 43

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/63da57eefeeb
https://hg.mozilla.org/mozilla-central/rev/8c09dc6015a3

Comment 44

[Carsten Book [:Tomcat]]

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