Closed Bug 1178513 Opened 4 years ago Closed 4 years ago

Gecko changes for OpenMobile Application Compatibility Layer

Categories

(Firefox OS Graveyard :: General, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(firefox42 fixed, b2g-v2.0 fixed, b2g-v2.0M fixed, b2g-v2.1 fixed, b2g-v2.1S fixed, b2g-v2.2 fixed, b2g-master fixed)

RESOLVED FIXED
FxOS-S2 (10Jul)
Tracking Status
firefox42 --- fixed
b2g-v2.0 --- fixed
b2g-v2.0M --- fixed
b2g-v2.1 --- fixed
b2g-v2.1S --- fixed
b2g-v2.2 --- fixed
b2g-master --- fixed

People

(Reporter: bwalker, Assigned: wchen)

References

Details

Attachments

(5 files, 2 obsolete files)

No description provided.
These changes support the integration of OpenMobile's Application Compatibility Layer into Firefox OS.
Attached patch wip-DomElementExtAPP.patch (obsolete) — Splinter Review
WIP patch for new "extapp" (external application) DOM element
WIP patch to add support for RGBA8888 to RGB565 conversion
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 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
Attachment #8627411 - Flags: review?(bugs)
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
Attachment #8627415 - Flags: review?(bugs)
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
Attachment #8627410 - Flags: review?(khuey)
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
Flags: needinfo?(ptheriault)
Attachment #8627415 - Flags: review?(bugs) → review?(matt.woodrow)
Attachment #8627411 - Flags: review?(bugs) → review?(matt.woodrow)
revised version of this patch using B2G_EXPORT macro
Attachment #8627415 - Attachment is obsolete: true
Attachment #8627415 - Flags: review?(matt.woodrow)
Attachment #8627711 - Flags: review?(matt.woodrow)
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.
Attachment #8627411 - Flags: review?(matt.woodrow) → review+
Patch updated
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.
(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 on attachment 8627410 [details] [diff] [review]
wip-DomElementExtAPP.patch

wchen says another version of this is forthcoming.
Attachment #8627410 - Flags: review?(khuey)
(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.
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.
Attachment #8627410 - Attachment is obsolete: true
Attachment #8627813 - Flags: review?(khuey)
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?
Attachment #8627813 - Flags: review?(khuey) → review+
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
Flags: needinfo?(hdiallo)
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
Flags: needinfo?(matt.woodrow)
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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
Keywords: leave-open
Well HTMLUnknownElement is a leaf ... and this should match it in parsing behavior when it's not enabled.
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.
(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.
Flags: needinfo?(hdiallo)
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.
Attachment #8627711 - Flags: review?(matt.woodrow) → review+
Flags: needinfo?(matt.woodrow)
(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 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
Attachment #8627813 - Flags: approval-gaia-v2.1?
Attachment #8627813 - Flags: approval-gaia-v2.0?
Comment on attachment 8627813 [details] [diff] [review]
<extapp> element and interfaces

Oops, wrong flags. Need b2g32 for v2.0m and b2g34 for v2.1s.
Attachment #8627813 - Flags: approval-mozilla-b2g34?
Attachment #8627813 - Flags: approval-mozilla-b2g32?
Attachment #8627813 - Flags: approval-gaia-v2.1?
Attachment #8627813 - Flags: approval-gaia-v2.0?
blocking-b2g: --- → 2.1S?
blocking-b2g: 2.1S? → ---
Comment on attachment 8627813 [details] [diff] [review]
<extapp> element and interfaces

[Triage Comment]
Attachment #8627813 - Flags: approval-mozilla-b2g37+
Attachment #8627813 - Flags: approval-mozilla-b2g34?
Attachment #8627813 - Flags: approval-mozilla-b2g34+
Attachment #8627813 - Flags: approval-mozilla-b2g32?
Attachment #8627813 - Flags: approval-mozilla-b2g32+
(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 ?
(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.
Flags: needinfo?(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.
Flags: needinfo?(wchen)
Commit for the RGBA8888 to RGB565 converter

https://hg.mozilla.org/integration/mozilla-inbound/rev/63da57eefeeb
^ 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.
Keywords: leave-open
This patch should be used when backporting the patch to export gecko symbols.
(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.
I asked about that, but I was told to land these patches onto m-c then uplift.
(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.
Duplicate of this bug: 1169383
Flags: needinfo?(ptheriault)
You need to log in before you can comment on or make changes to this bug.