If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Send plugin layer from plugin-Child -> Chrome process

RESOLVED INCOMPLETE

Status

()

Core
Plug-ins
RESOLVED INCOMPLETE
7 years ago
6 months ago

People

(Reporter: romaxa, Assigned: romaxa)

Tracking

(Depends on: 1 bug)

Trunk
x86
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(fennec-)

Details

(Whiteboard: maemo6)

Attachments

(4 attachments, 10 obsolete attachments)

33.46 KB, application/x-shockwave-flash
Details
147 bytes, text/html
Details
44.72 KB, patch
cjones
: feedback-
Details | Diff | Splinter Review
11.51 KB, patch
roc
: review+
Details | Diff | Splinter Review
(Assignee)

Description

7 years ago
Currently we are sending plugin surface to content process, and content process does SHMem/X copy of layer surface to Sharable surface.... and send sharable surface to chrome process...

I think we should make it so that ShadowImage(plugin)Layer send surface directly to Chrome process without copy.

Updated

7 years ago
tracking-fennec: --- → 2.0+

Comment 1

7 years ago
This seems to be the cause of flash player plugin performance degradation on maemo6
Degradation compared to what?

Comment 3

7 years ago
A direct comparison between fennec versus microb indicates that flash performance on fennec is much slower, almost twice as slow.
How does perf compare to microb with flash running in-process in fennec?(dom.ipc.plugins.enabled=false)

Comment 5

7 years ago
remote.tabs=false yields some like 30% improvement.

Chris does this mean dom.ipc.plugins are disabled by that as well?

We will check with dom.ipc.plugins separately
No, dom.ipc.plugins is orthogonal to tabs.remote.

Comment 7

7 years ago
ok. So we have slowdown coming from 2 sources: remote tabs and abovementioned problem. We shall file a new bug. Meanwhile we will get numbers with dom.ipc.plugins.enabled set to off

Comment 8

7 years ago
Created attachment 486371 [details]
FpsMeter

Comment 9

7 years ago
Created attachment 486372 [details]
Html file to load FpsMeter

Comment 10

7 years ago
I obtained the following flash timings this morning for the N9 an N900
devices.  N9 has the most recent flash image.  As far as I know, The
N900 is update to date with service packs.

N900/Mode               FPS  
FpsMeter.swf            0 (fail)
A.html + FpsMeter.swf   48 (runs smooth)

N9 (dom.ipc.plugins.enabled = true)
FpsMeter.swf            18 (runs smooth)
A.html + FpsMeter.swf   28 (runs poorly)

N9 (dom.ipc.plugins.enabled = false)
FpsMeter.swf            20 (runs poorly)
A.html + FpsMeter.swf   28 (runs poorly)

I've attached the swf and html files that I used to obtain these results.

Comment 11

7 years ago
To implement that it would be required to change mFrontSurface to be a SurfaceDescriptor instead of a gfxASurface.

Than we would need something like a PluginSurfaceImage and would put the SurfaceDescriptor into a PluginSurfaceImage instead of a CairoImage in nsPluginInstanceOwner::SetCurrentImage (nsObjectFrame.cpp).

Comment 12

7 years ago
(In reply to comment #11)
> To implement that it would be required to change mFrontSurface to be a
> SurfaceDescriptor instead of a gfxASurface.

I guess it is better to use a gfxPluginSurface which is a gfxASurface for that.

Comment 13

7 years ago
Created attachment 486914 [details] [diff] [review]
Create a gfxPluginSurface (WIP)

Add a gfxPluginSurface class.

TODO:
* Add a PluginImage class
* Use the PluginImage instead of CairoImage in nsObjectFrame.cpp (nsPluginInstanceOwner::SetCurrentImage)
* Remove the temporary mSurface from gfxPluginSurface

Updated

7 years ago
Assignee: nobody → jap
Status: NEW → ASSIGNED

Comment 14

7 years ago
Created attachment 487340 [details] [diff] [review]
Add gfxPluginSurface and corresponding PluginImage (Image::PLUGIN_SURFACE) for plugins (WIP)
Attachment #486914 - Attachment is obsolete: true
Attachment #487340 - Flags: feedback?(romaxa)

Updated

7 years ago
Attachment #487340 - Attachment description: Add gfxPluginSurface and corresponding PluginImage (Image::PLUGIN_SURFACE) for plugins → Add gfxPluginSurface and corresponding PluginImage (Image::PLUGIN_SURFACE) for plugins (WIP)
Attachment #487340 - Flags: feedback?(romaxa)
(Assignee)

Updated

7 years ago
Attachment #487340 - Flags: feedback?(jones.chris.g)
(Assignee)

Comment 15

7 years ago
Comment on attachment 487340 [details] [diff] [review]
Add gfxPluginSurface and corresponding PluginImage (Image::PLUGIN_SURFACE) for plugins (WIP)

I'll check it tomorrow
Attachment #487340 - Flags: feedback?(romaxa)

Comment 16

7 years ago
Using an ImageLayer like in the current patch is not the right approach because it is rendered to a surface in the content process, so just what we do not want to do.

Comment 17

7 years ago
Maybe we should add a PluginLayer type instead, where the surface is just swapped into a ShadowPluginLayer instead of painting/rendering anything on the content side.

Another question, does it make sense to use the mozilla::layers::SurfaceDescriptor also for the plugins?

Comment 18

7 years ago
Yes, we do want a pluginlayer type eventually.
Comment on attachment 487340 [details] [diff] [review]
Add gfxPluginSurface and corresponding PluginImage (Image::PLUGIN_SURFACE) for plugins (WIP)

It's not clear to me how these patches get us closer to plugin gfx bypassing the content-process main thread.  There's already a plan for this in bug 598867, which should have been set to block this bug.

As for the patch itself, a gfxPluginSurface or imagelayer of type plugin doesn't make sense to me.  And yes, we should share code with gfx/layers/ipc as much as possible.
Attachment #487340 - Flags: feedback?(jones.chris.g) → feedback-
Depends on: 598867
We may or may not want a new PluginLayer type eventually, but I don't understand why we need one here. An X pixmap can be passed from plugin to content and from content to compositor without copying, or in bug 598867 directly from plugin to compositor.
(Assignee)

Comment 21

7 years ago
> We may or may not want a new PluginLayer type eventually, but I don't
I think so too...

Practically plugin-child calling SendShow, and in PluginInstanceParent::RecvShow we initialize mFrontSurface (as gfxXlibSurface with our XID) and call Invalidate

and in nsPluginInstanceOwner::InvalidateRect we call SetCurrentImage/SetData or something like that, and that should in Swap or similar ShadowImageLayer function send XID directly to Chrome process...

The only thing we need to do is to connect PluginInstanceParent::RecvShow and ShadowImageLayer::Swap and get back previous layer XID for recycling..

Comment 22

7 years ago
Per Oleg Comment #21, I have attacted a diagram which illustrates how this will work within the layers class hierarchy.  

Specifically, I'm going to look more closely at how ShadowImageLayerOGL works.  

What is the difference between:

BasicImageLayer, ShadowImageLayer, ImageLayerOGL, BasicShadowableImageLayer and BasicShadowImageLayer?

Comment 23

7 years ago
Created attachment 488053 [details]
Image of Oleg Comment #21
(In reply to comment #10)
> I obtained the following flash timings this morning for the N9 an N900
> devices.  N9 has the most recent flash image.  As far as I know, The
> N900 is update to date with service packs.
> 
> N900/Mode               FPS  
> FpsMeter.swf            0 (fail)
> A.html + FpsMeter.swf   48 (runs smooth)
> 

Are these numbers measuring fennec or microb?  What's the "0 (fail)"?

> N9 (dom.ipc.plugins.enabled = true)
> FpsMeter.swf            18 (runs smooth)
> A.html + FpsMeter.swf   28 (runs poorly)
> 
> N9 (dom.ipc.plugins.enabled = false)
> FpsMeter.swf            20 (runs poorly)
> A.html + FpsMeter.swf   28 (runs poorly)

Is for the tabs.remote=true or false?  It would be nice to get data for both configurations, to know how much the overhead of tabs.remote=true is reducing fps.

Also, kudos to romaxa for async plugin painting, which is most likely why dom.ipc.plugins.enabled=true has no overhead compared to false.
(In reply to comment #22)
> What is the difference between:
> 
> BasicImageLayer, ShadowImageLayer, ImageLayerOGL, BasicShadowableImageLayer and
> BasicShadowImageLayer?

Basic*ImageLayer are software-rendered images, currently used for plugin surfaces and <video> frames.  *ImageLayerOGL are the same but in a format that allows faster compositing with GL.  The Shadow* variants are the cross-process partners of "real" layers; for example, the content process might have a BasicImageLayer, and the chrome process would have a ShadowImageLayer corresponding to that.  The design is at [1] but that's a bit outdated and doesn't go into specifics.

[1] https://wiki.mozilla.org/Gecko:CrossProcessLayers

Comment 26

7 years ago
(In reply to comment #24)
> Are these numbers measuring fennec or microb?  What's the "0 (fail)"?

microb

> > N9 (dom.ipc.plugins.enabled = true)
> > FpsMeter.swf            18 (runs smooth)
> > A.html + FpsMeter.swf   28 (runs poorly)
> > 
> > N9 (dom.ipc.plugins.enabled = false)
> > FpsMeter.swf            20 (runs poorly)
> > A.html + FpsMeter.swf   28 (runs poorly)
> 
> Is for the tabs.remote=true or false?  It would be nice to get data for both
> configurations, to know how much the overhead of tabs.remote=true is reducing
> fps.


Ok.  I'll add this to the next set of timings.


> Also, kudos to romaxa for async plugin painting, which is most likely why
> dom.ipc.plugins.enabled=true has no overhead compared to false.

Nice!

Comment 27

7 years ago
> What's the "0 (fail)"?

Did not work at all -- no output.
Quick update --- I'm currently working on bug 564086 which will allow the plugin subprocess to communicate directly with the chrome process.  In parallel, there are some other problems that need to be solved

 - The content process's layer tree needs to have a Layer that represents plugin frames, even though that layer won't be backed with a real surface or be renderable.  One approach to this is bug 598872, but I'm not sure it's the best way.  Another possibility might be to make a special ImageLayer type that serves the same purpose as a Ref/PlaceholderLayer.

 - Need some way to look up a "phantom" plugin layer, like what bug 598872 would refer to, in the chrome process when the layer tree is painted.

 - Need a way to invalidate and repaint in the chrome process when a plugin has an updated surface.  Should be fairly easy.

 - The painting model in the plugin subprocess needs to be planned out.  It should be simpler on the whole than the current async-painting model, I think (no need to deal with nsObjectFrame), but the details of when surfaces are created, how they're shared, etc. need to worked out.

 - It would be nice to use PLayers for plugin subprocesses, for several reasons.  Does the current async plugin-painting model require features not present in PLayers.ipdl?  If so, we should add them I think; in the long run it would be nice to share all this code.  If adding those features looks hard, we'll need an intermediate PPluginCompositor protocol or something like that.

Comment 29

7 years ago
(In reply to comment #24)
> Is for the tabs.remote=true or false?  It would be nice to get data for both
> configurations, to know how much the overhead of tabs.remote=true is reducing
> fps.

According to my tests it is tabs.remote=true 8fps, tabs.remote=false 16fps.

Comment 30

7 years ago
(In reply to comment #28)
> Quick update --- I'm currently working on bug 564086 which will allow the
> plugin subprocess to communicate directly with the chrome process.  In
> parallel, there are some other problems that need to be solved
>
> ...

That should definitely be the right approach to the problem.

(In reply to comment #21)
> > We may or may not want a new PluginLayer type eventually, but I don't
> I think so too...
> 
> Practically plugin-child calling SendShow, and in
> PluginInstanceParent::RecvShow we initialize mFrontSurface (as gfxXlibSurface
> with our XID) and call Invalidate
> 
> and in nsPluginInstanceOwner::InvalidateRect we call SetCurrentImage/SetData or
> something like that, and that should in Swap or similar ShadowImageLayer
> function send XID directly to Chrome process...
> 
> The only thing we need to do is to connect PluginInstanceParent::RecvShow and
> ShadowImageLayer::Swap and get back previous layer XID for recycling..

I checked if it possible to do that as a temporary hack for now, but the problem is that ShadowImageLayer::Swap just swaps internal used gfxSharedImageSurfaces. So it would probably easier to add a PluginLayer which also understands gfxXlibSurface but than there is still the problem that the ShadowLayer swap happens somewhere in BasicShadowLayerManager::EndTransaction called from nsDisplayList::PaintForFrame so it is not so easy to get the previous XID back into the PluginInstanceParent. But maybe it would work with three XIDs which are recycled. But it is a really hackish approach.

Comment 31

7 years ago
Created attachment 488368 [details]
Updated Image of Oleg Comment #21

Updated Image of Oleg Comment #21
Attachment #488053 - Attachment is obsolete: true

Comment 32

7 years ago
> Practically plugin-child calling SendShow, and in
> PluginInstanceParent::RecvShow we initialize mFrontSurface (as gfxXlibSurface
> with our XID) and call Invalidate

Sounds like the consensus is that plugins are the way to go.  

Sadly, I don't see any examples of their usage in the "gfx" folder.  Where is there documentation on how to integrate with the plugin system?   Is there any wiki links or visio diagrams that would be good to illustrate their usage.

Comment 33

7 years ago
Created attachment 488477 [details] [diff] [review]
Patch to add a PluginSurface (WIP v1)

WIP.

The idea is to have a PluginLayer which just forwards the surface from the plugin to the chrome process.
Attachment #487340 - Attachment is obsolete: true
Attachment #487340 - Flags: feedback?(romaxa)

Comment 34

7 years ago
Created attachment 488828 [details] [diff] [review]
Alternative approach using ImageLayer (WIP)

Requires bug 610155

Directly forward surface in ShadowableImageLayer when possible.
I don't understand the dependency on bug 610155 or the new patches.  Was the source of the tabs.remote=true overhead discovered?

Comment 36

7 years ago
I've now had a chance to look at both approaches, the atlternative approach using ImageLayer looks superior. It uses static variables to bypass all the layer cludge. Adding to an already overburdened deeply nested layer class hierarchy, as the PluginSurfaceLayer proposes does not look tenable.
deeply nested layer class hierarchy? The max depth is 3.

Comment 38

7 years ago
Are you including all the multi-inheritance in your count?
No, that doesn't increase the depth.

We could add some kind of PluginLayer. I think we will, in the future. But we don't need it here and now, as far as I can tell.

Comment 40

7 years ago
Created attachment 489007 [details]
Class Inheritance Chart of LayerManager and Layer
Attachment #488368 - Attachment is obsolete: true

Comment 41

7 years ago
>> We could add some kind of PluginLayer. I think we will, in the future.

Ok.  sounds fine.  I'm looking at how to both proposals at the moment.
 I didn't mean to criticize the depth of the class hiearchy, there is
nothing inherently wrong with it, I was just expressing a concern that
the proposal would duplicate an entire branch, which is a common
criticism of object orientated designs.

Comment 42

7 years ago
>> I don't understand the dependency on bug 610155 or the new patches.  Was the
>> source of the tabs.remote=true overhead discovered?

I dont think we have yet.  

Oleg's patch for 610155 seems relevent here.  Why couldn't we do something similar for flash? Why cant we just build upon his work.  I'm still studying it, but it looks to me exactly like what we need.  (In reply to comment #35)
The multiple classes you'd need to implement for PluginLayer are just the different chunks of functionality that any design would need: a way to render the plugin with and without GL, and a way to forward the PluginLayer from a content process to a compositing process.

Updated

7 years ago
Whiteboard: maemo6
This bug as spec'd depends on bug 598867, which we're not even planning to take for <video> in 4.0.  We need to either change tack of this bug to "share plugin surfaces more efficiently with the chrome process" which is more feasible for 4.0, then re-evaluate blocking status; or, blocking- this.
tracking-fennec: 2.0+ → ?

Updated

7 years ago
tracking-fennec: ? → 2.0-
You want to create a protocol that bridges the plugin subprocess and the the UI process.  Let's call that "PBridgeCompositorPlugin".  The syntax to set that up is

  PContent.ipdl:
    child spawns PPluginModule;
    //...

  PBridgeCompositorPlugin:
    bridges PContent, PPluginModule
    //...

You have to create the the bridge from within the process that has references to the two actors being bridged.  Here it's the content process: it can send messages to the compositor process (through ContentChild) and to the plugin subprocess (through PluginModuleParent).  The IPDL compiler infers this and should generate an interface like

  namespace PBridgeCompositorPlugin {
  bool Bridge(PContentChild*, PPluginModuleParent*);
  }

Then within the content process, your code will call that function.  Behind the scenes, messages will be sent to the compositor and plugin subprocesses, and IPDL-generated code will require Alloc*() methods to create the bridge endpoints.

Does this make sense?  If your implementation looks like this, what errors are you seeing?
(Assignee)

Comment 46

6 years ago
Created attachment 560821 [details] [diff] [review]
Plugin Bridge, v1, WIP

Tested on Linux X11 and Maemo works great
Assignee: jap → romaxa
Attachment #488477 - Attachment is obsolete: true
Attachment #488828 - Attachment is obsolete: true
Attachment #489007 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Depends on: 687373
(Assignee)

Comment 47

6 years ago
Created attachment 561122 [details] [diff] [review]
Plugin Parent LayerID AP + ObjectFrame setup call

Here is Plugin SetLayerID API which is passing unique shadowable Image LayerID provided in bug 598872 to PluginInstanceChild.
Attachment #561122 - Flags: review?(jones.chris.g)
(Assignee)

Updated

6 years ago
Depends on: 598872
No longer depends on: 598867
Comment on attachment 561122 [details] [diff] [review]
Plugin Parent LayerID AP + ObjectFrame setup call

>+    nsRefPtr<nsNPAPIPluginInstance> inst;
>+    GetPluginInstance(getter_AddRefs(inst));
>+    if (inst && imglayer->GetUniqueID()) {
>+      inst->SetLayerID(imglayer->GetUniqueID());
>+    }

This is going to set the ID (sending an IPC message) on every paint that involves a plugin instance, one message per plugin instance.  That's not good.  This should be done one when the container is initialized.

What happens if the PluginInstanceChild tries to send over a buffer before it has an ID set?
(Assignee)

Comment 49

6 years ago
> This is going to set the ID (sending an IPC message) on every paint that
> involves a plugin instance, one message per plugin instance.  That's not
> good.  This should be done one when the container is initialized.

I think that was before and not true now, before we had BuildLayer call on each paint, but now it called only when we need to create new layer.. at least my printf's show that..

> 
> What happens if the PluginInstanceChild tries to send over a buffer before
> it has an ID set?
This is ShadowableImageLayer and ID created in ctor.
(In reply to Oleg Romashin (:romaxa) from comment #49)
> > This is going to set the ID (sending an IPC message) on every paint that
> > involves a plugin instance, one message per plugin instance.  That's not
> > good.  This should be done one when the container is initialized.
> 
> I think that was before and not true now, before we had BuildLayer call on
> each paint, but now it called only when we need to create new layer.. at
> least my printf's show that..
> 

Would be good to know for sure.

> > 
> > What happens if the PluginInstanceChild tries to send over a buffer before
> > it has an ID set?
> This is ShadowableImageLayer and ID created in ctor.

Are you guaranteeing that UpdatePluginGeometry() or whatever it is that makes the plugin instance think it's visible is called after the layer is created?
... if that's the case, it needs to be documented.  Otherwise bad things will happen.
(Assignee)

Comment 52

6 years ago
> Are you guaranteeing that UpdatePluginGeometry() or whatever it is that
> makes the plugin instance think it's visible is called after the layer is
> created?
It is controlled by PluginInstanceOwner implementation and it call special function in order to suspend plugin painting... so plugin in that case will just stop calling invalidates and PluginInstanceChild will stop calling SendShow(or bridge::Show)...
In my implementation I'm just bridging SendShow calls...
If by some reason layout decide to destroy plugin layer, then it will be unregistered from BridgeParent, and we never call swap/paint for that layer, until layout create and send new layer ID.
(Assignee)

Comment 53

6 years ago
Created attachment 561156 [details] [diff] [review]
Plugin bridge impl wip2

Fixed multiple plugin's instance handling, destruction, updated to latest change in bug 598872...
Practically we need patches from bug  598872 and patches from this bug in order to make PluginBridge working with Shmem and X11 based plugins
Attachment #560821 - Attachment is obsolete: true
Attachment #561156 - Flags: feedback?(jones.chris.g)
(Assignee)

Comment 54

6 years ago
Created attachment 561568 [details] [diff] [review]
Plugin Layer API.

Kind of very simple exposure of PluginLayer API based derived from ImageLayer...
Does not provide any special type or name, just ImageLayer with additional method.

Derived all other ImageLayer implementations from PluginLayer in order to reduce dummy wrappers count.
Attachment #561568 - Flags: feedback?(roc)
Comment on attachment 561122 [details] [diff] [review]
Plugin Parent LayerID AP + ObjectFrame setup call

>diff --git a/layout/generic/nsObjectFrame.cpp b/layout/generic/nsObjectFrame.cpp
>--- a/layout/generic/nsObjectFrame.cpp
>+++ b/layout/generic/nsObjectFrame.cpp
>@@ -1626,16 +1626,21 @@ nsObjectFrame::BuildLayer(nsDisplayListB
>     }
> 
>     NS_ASSERTION(layer->GetType() == Layer::TYPE_IMAGE, "Bad layer type");
> 
>     ImageLayer* imglayer = static_cast<ImageLayer*>(layer.get());
>     UpdateImageLayer(container, r);
> 
>     imglayer->SetContainer(container);
>+    nsRefPtr<nsNPAPIPluginInstance> inst;
>+    GetPluginInstance(getter_AddRefs(inst));
>+    if (inst && imglayer->GetUniqueID()) {
>+      inst->SetLayerID(imglayer->GetUniqueID());
>+    }

I'm about 90% sure this is going to be called on every paint.  Explain to me why it's not going to be.
Attachment #561122 - Flags: review?(jones.chris.g)
Comment on attachment 561156 [details] [diff] [review]
Plugin bridge impl wip2

>diff --git a/dom/plugins/ipc/PBridgeCompositorPlugin.ipdl b/dom/plugins/ipc/PBridgeCompositorPlugin.ipdl
>+
>+namespace mozilla {
>+namespace plugins {
>+
>+struct SurfaceLayersDescriptorD3D10 {
>+  WindowsHandle handle;
>+};
>+

Don't duplicate all this stuff from PLayers.  Use PLayers directly
instead.  See below...

>+// (Bridge protocols can have different semantics than the endpoints
>+// they bridge)
>+rpc protocol PBridgeCompositorPlugin {

This shouldn't be rpc.  Use sync.

>+    bridges PContent, PPluginModule;
>+

... 

  manages PLayers;


The CreateBridge()/OpenPluginCompositorBridge() interaction doesn't
make sense to me.  It looks unnecessary.

I skimmed the rest and it looks mostly OK.
Attachment #561156 - Flags: feedback?(jones.chris.g) → feedback-
Is it really the right idea to have PluginLayers wrap ImageLayer? It seems to me that PluginLayers should have their own interface. For one thing, we really just want to be able to set a native surface for a PluginLayer, right? We don't need Images, or ImageContainers.
I mean "inherit from", not "wrap".
(Assignee)

Updated

6 years ago
Depends on: 688562
(Assignee)

Comment 59

6 years ago
Created attachment 562561 [details] [diff] [review]
API to forward unique ID to plugin instance
Attachment #561122 - Attachment is obsolete: true
Attachment #561568 - Attachment is obsolete: true
Attachment #562561 - Flags: review?(roc)
Attachment #561568 - Flags: feedback?(roc)
Comment on attachment 562561 [details] [diff] [review]
API to forward unique ID to plugin instance

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

Looks good. But don't push this patch until we have a working plugin solution, so I can see how it all fits together.

::: dom/plugins/ipc/PluginLibrary.h
@@ +120,5 @@
>  #if defined(MOZ_WIDGET_QT) && (MOZ_PLATFORM_MAEMO == 6)
>    virtual nsresult HandleGUIEvent(NPP instance, const nsGUIEvent&, bool*) = 0;
>  #endif
> +  /**
> +   * Set to PluginInstance Unique ID of layer which is used for instance rendering

"Send to PluginInstance"
Attachment #562561 - Flags: review?(roc) → review+

Comment 61

6 months ago
Resolving old bugs which are likely not relevant any more, since NPAPI plugins are deprecated.
Status: ASSIGNED → RESOLVED
Last Resolved: 6 months ago
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.