Last Comment Bug 506826 - Implement -moz-element(): using arbitrary elements as the source for CSS backgrounds
: Implement -moz-element(): using arbitrary elements as the source for CSS back...
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: Layout: Images (show other bugs)
: Trunk
: All All
: -- enhancement with 3 votes (vote)
: mozilla2.0b4
Assigned To: Markus Stange [:mstange]
:
:
Mentors:
Depends on: 591832 113577 background-size 479220 572680 572688 572689 572697 587336 591141 591304 592086 601999
Blocks: 547805 276431
  Show dependency treegraph
 
Reported: 2009-07-27 18:40 PDT by Ryo Kawaguchi
Modified: 2013-11-13 02:20 PST (History)
34 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch v1 (104.02 KB, patch)
2009-07-27 18:51 PDT, Ryo Kawaguchi
no flags Details | Diff | Splinter Review
reftests v1 (19.12 KB, patch)
2009-07-27 18:53 PDT, Ryo Kawaguchi
no flags Details | Diff | Splinter Review
patch v2 (106.50 KB, patch)
2009-07-28 17:06 PDT, Ryo Kawaguchi
no flags Details | Diff | Splinter Review
patch v3 with reftests (233.15 KB, patch)
2009-09-09 06:58 PDT, Ryo Kawaguchi
no flags Details | Diff | Splinter Review
part 1, v1: style system (24.17 KB, patch)
2010-06-01 14:02 PDT, Markus Stange [:mstange]
no flags Details | Diff | Splinter Review
part 1, v2: style system (24.05 KB, patch)
2010-06-02 09:33 PDT, Markus Stange [:mstange]
no flags Details | Diff | Splinter Review
part 1, v3 (25.73 KB, patch)
2010-06-05 16:51 PDT, Markus Stange [:mstange]
no flags Details | Diff | Splinter Review
part 1, v4: parsing (28.50 KB, patch)
2010-06-17 08:26 PDT, Markus Stange [:mstange]
dbaron: review-
Details | Diff | Splinter Review
part 2: Add a PAINT_ALL_CONTINUATIONS flag for nsLayoutUtils::PaintFrame. (4.02 KB, patch)
2010-06-17 08:27 PDT, Markus Stange [:mstange]
roc: review+
Details | Diff | Splinter Review
part 3: Add nsSVGIntegrationUtils::DrawPaintServer (12.57 KB, patch)
2010-06-17 08:29 PDT, Markus Stange [:mstange]
no flags Details | Diff | Splinter Review
part 4: -moz-element drawing (8.66 KB, patch)
2010-06-17 08:30 PDT, Markus Stange [:mstange]
no flags Details | Diff | Splinter Review
part 5: Ryo's reftests (48.11 KB, patch)
2010-06-17 08:33 PDT, Markus Stange [:mstange]
roc: review+
Details | Diff | Splinter Review
part 6: more reftests (26.21 KB, patch)
2010-06-17 08:35 PDT, Markus Stange [:mstange]
roc: review+
Details | Diff | Splinter Review
part 1, v5: parsing (29.15 KB, patch)
2010-08-08 14:45 PDT, Markus Stange [:mstange]
dbaron: review+
Details | Diff | Splinter Review
part 2, PAINT_ALL_CONTINUATIONS (3.40 KB, patch)
2010-08-08 14:46 PDT, Markus Stange [:mstange]
no flags Details | Diff | Splinter Review
part 3a: use a frame state bit for paint loop detection (3.68 KB, patch)
2010-08-08 14:48 PDT, Markus Stange [:mstange]
roc: review+
Details | Diff | Splinter Review
part 3b, add nsSVGIntegrationUtils::DrawPaintServer (12.96 KB, patch)
2010-08-08 14:51 PDT, Markus Stange [:mstange]
roc: review+
Details | Diff | Splinter Review
part 4: -moz-element drawing (7.29 KB, patch)
2010-08-08 14:53 PDT, Markus Stange [:mstange]
roc: review+
Details | Diff | Splinter Review
part 5: invalidate observers (4.93 KB, patch)
2010-08-08 14:54 PDT, Markus Stange [:mstange]
roc: review+
Details | Diff | Splinter Review
part 6: reftests (78.04 KB, patch)
2010-08-08 14:55 PDT, Markus Stange [:mstange]
no flags Details | Diff | Splinter Review
part 5: invalidate observers (5.90 KB, patch)
2010-08-09 05:05 PDT, Markus Stange [:mstange]
no flags Details | Diff | Splinter Review
part 1, v6: parsing (29.56 KB, patch)
2010-08-12 12:20 PDT, Markus Stange [:mstange]
dbaron: review+
Details | Diff | Splinter Review
complete -moz-element patch for approval (260.77 KB, patch)
2010-08-12 14:05 PDT, Markus Stange [:mstange]
dbaron: approval2.0+
Details | Diff | Splinter Review

Description Ryo Kawaguchi 2009-07-27 18:40:09 PDT
Implement the followings to allow use of an arbitrary element as the source for CSS background.

1) A new CSS image type, -moz-element(#foo). Normally 'foo' is the ID of
an element in the document to which the stylesheet applies.
2) A new DOM API, window.mozSetImageElement("foo", node). The node is
used instead of the element with ID foo when -moz-element(#foo) is used.

For more details discussion:

http://weblogs.mozillazine.org/roc/archives/2008/07/the_latest_feat.html

http://groups.google.com/group/mozilla.dev.platform/browse_thread/thread/d58cbafa83471800/b4b5ab1849ffe3af?lnk=gst&q=proposal%3A+using+arbitrary+elements+as+the+source+for+CSS+backgrounds
Comment 1 Ryo Kawaguchi 2009-07-27 18:51:50 PDT
Created attachment 390976 [details] [diff] [review]
patch v1

This patch is worked on top of the patch v4 for bug 113577.

This patch brings forward changes in roc's svg-integration repo to the latest trunk and implements "-moz-element()" CSS function to be used with "background" or "background-image" property. (mozSetImageElement() is not implemented yet.)

This passes all the reftests except for one. (I need to tweak DrawPaintServer()).
Comment 2 Ryo Kawaguchi 2009-07-27 18:53:48 PDT
Created attachment 390977 [details] [diff] [review]
reftests v1

Reftests for patch v1.

(I brought them forward from roc's repo and replaced url(#id) with -moz-element(#id)).
Comment 3 Ryo Kawaguchi 2009-07-28 12:35:58 PDT
Comment on attachment 390976 [details] [diff] [review]
patch v1

This patch also brings forward the feature to use gradient and pattern SVG frames as the source of CSS backgrounds.
Comment 4 Ryo Kawaguchi 2009-07-28 17:06:20 PDT
Created attachment 391236 [details] [diff] [review]
patch v2

I was thinking how I should introduce the image snapping logic (bug 458487) into paint server rendering since a half of the image snapping code is hidden inside imgFrame, which we are discouraged to directly access. Some options would be:

(1) Copy and paste the image snapping code from imgFrame::Draw() to nsSVGIntegrationUtils::DrawPaintServer(); the idea of duplicating over 100 lines of code does not sound so good.
  
(2) Move the image snapping code from imgFrame::Draw() to somewhere else that can be accessed from both imglib2 and layout components.

(3) Renders a paint server frame as an imgIContainer image and simply toss it to nsLayoutUtils::DrawImage(), which internally calls imgFrame::Draw(). An extra step of drawing a paint server on the surface of a temporary imgIContainer is not too bad, I think, because we anyway have to draw it on a temporary surface when tiling it.

I implemented the option (3) in patch v2 because it makes the most of the existing code base. But, the option (3) is a kind of cheating and may be a very bad idea. Any advice on this issue?

The patch v2 fails 5 reftests (all moz-element() related); the causes are the same: an unexpected scaling.
Comment 5 Ryo Kawaguchi 2009-09-09 06:58:01 PDT
Created attachment 399463 [details] [diff] [review]
patch v3 with reftests

I still need to clean up DrawPaintServer() and fix a leak and a few reftests.

Anyway, It's very close to finish.
I am hoping that I can finish this in a week after going back to Japan.
Comment 6 Markus Stange [:mstange] 2009-11-09 09:39:30 PST
Ryo, do you think you'll have time to finish this patch, or do you want somebody else to take over?
Comment 7 Ryo Kawaguchi 2009-11-10 22:35:14 PST
Markus, I'm sorry for this late reply. I will have some free time towards the end of this month, and am planning to work on this patch. However, if this patch needs be finished sometime soon, it's no problem for me that someone else takes it over.
Comment 8 Silvia Pfeiffer 2010-05-17 16:46:28 PDT
Will this be a means to use videos as a background, too? There are some pretty fancy examples of use of video as background listed here http://abduzeedo.com/web-inspiration-video-backgrounds, though these are all using flash, of course.
Comment 9 Markus Stange [:mstange] 2010-05-27 06:34:49 PDT
I'd like to work on this because I need it for the new Mac theme, see bug 547805.

I've looked at the v2 and v3 patches (attachment 391236 [details] [diff] [review] and
attachment 399463 [details] [diff] [review]) and have some questions:

 - What syntax have we decided on? v2 restricts the argument of -moz-element to
   #elementid, but v3 reuses url() parsing and thus accepts much more. The
   discussion in the newsgroup (see comment 0) seems to have come to the
   conclusion that we want the v2 approach.
 - What to do with the duplicated image snapping code? This problem is
   described in comment 4, and while Ryo went for option (3) in the v2 patch,
   the v3 patch uses option (1).
   Another option I thought of would be to move all the code from
   imgFrame::Draw to DrawImageInternal in nsLayoutUtils.cpp, remove the Draw
   method on imgFrame and imgIContainer and instead add a GetSurfaceForDrawing
   method to them which returns the gfxASurface that imgFrame::Draw would have
   used. Then we'd only have to fix up the one other caller of
   imgIContainer::Draw, nsBaseDragService. And GetSurfaceForDrawing would need
   some special indication for the single-color case so that we keep that
   optimization.
Comment 10 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-05-27 15:14:28 PDT
(In reply to comment #9)
>  - What syntax have we decided on? v2 restricts the argument of -moz-element to
>    #elementid, but v3 reuses url() parsing and thus accepts much more. The
>    discussion in the newsgroup (see comment 0) seems to have come to the
>    conclusion that we want the v2 approach.

Yeah, that's fine.

>  - What to do with the duplicated image snapping code? This problem is
>    described in comment 4, and while Ryo went for option (3) in the v2 patch,
>    the v3 patch uses option (1).
>    Another option I thought of would be to move all the code from
>    imgFrame::Draw to DrawImageInternal in nsLayoutUtils.cpp, remove the Draw
>    method on imgFrame and imgIContainer and instead add a GetSurfaceForDrawing
>    method to them which returns the gfxASurface that imgFrame::Draw would have
>    used.

That won't work well with SVG images. I think we need either 1 or 2.

I don't quite know how this will work with bug 547805. There, you want to align the rendering of -moz-element() based on the position of the referenced element, and these patches don't do that (and in many use cases we wouldn't want to do that). Maybe we will need an extension value for background-position, say -moz-align-with-source, that positions the center of a -moz-element() image at the center of its source element, if there is one?

Eventually I would like to extend this so it can be used for TabCandy, accelerated using layers. Let's not go there immediately, but I'd like to make sure the API doesn't prevent that. I think right now we're in good shape.
Comment 11 Markus Stange [:mstange] 2010-05-28 02:17:07 PDT
(In reply to comment #10)
> >  - What to do with the duplicated image snapping code? This problem is
> >    described in comment 4, and while Ryo went for option (3) in the v2 patch,
> >    the v3 patch uses option (1).
> >    Another option I thought of would be to move all the code from
> >    imgFrame::Draw to DrawImageInternal in nsLayoutUtils.cpp, remove the Draw
> >    method on imgFrame and imgIContainer and instead add a GetSurfaceForDrawing
> >    method to them which returns the gfxASurface that imgFrame::Draw would have
> >    used.
> 
> That won't work well with SVG images.

Why not? I've looked at imgContainerSVG::Draw in http://hg.mozilla.org/users/dholbert_mozilla.com/svg-patches/file/07ca124f2309/svg_images - we construct an empty gfxASurface and use presShell->RenderDocument to draw into it, so couldn't we just return that surface instead and draw it in nsLayoutUtils? Or do you mean that SVG images don't want the pixel snapping that imgFrame::Draw does?

> I don't quite know how this will work with bug 547805. There, you want to align
> the rendering of -moz-element() based on the position of the referenced
> element

Well, not really. I wasn't going to reference an element, but an image outside the document set by document.mozSetImageElement. And I think background-attachment: fixed will take care of the positioning nicely.

> Eventually I would like to extend this so it can be used for TabCandy,
> accelerated using layers.

Not sure what this will look like. Are you planning to create a separate layer for backgrounds that use -moz-element and then reflect the relation between that layer and the referenced element's layer in the layer tree? Or just reusing the retained layer of the referenced element when rendering the background?
Comment 12 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-05-28 03:13:02 PDT
(In reply to comment #11)
> Why not? I've looked at imgContainerSVG::Draw in
> http://hg.mozilla.org/users/dholbert_mozilla.com/svg-patches/file/07ca124f2309/svg_images
> - we construct an empty gfxASurface and use presShell->RenderDocument to draw
> into it, so couldn't we just return that surface instead and draw it in
> nsLayoutUtils? Or do you mean that SVG images don't want the pixel snapping
> that imgFrame::Draw does?

I haven't read those patches yet, so dholbert should comment. But for the non-tiled case, we shouldn't need a temporary surface. Rather than stretching a surface to fill the image area, instead SVG images are drawn to correctly fill the image area.

> > I don't quite know how this will work with bug 547805. There, you want to align
> > the rendering of -moz-element() based on the position of the referenced
> > element
> 
> Well, not really. I wasn't going to reference an element, but an image outside
> the document set by document.mozSetImageElement. And I think
> background-attachment: fixed will take care of the positioning nicely.

Good idea, background-attachment:fixed will work fine.

> > Eventually I would like to extend this so it can be used for TabCandy,
> > accelerated using layers.
> 
> Not sure what this will look like. Are you planning to create a separate layer
> for backgrounds that use -moz-element and then reflect the relation between
> that layer and the referenced element's layer in the layer tree?

Yes. When we reference an element with a layer, we should create a layer for the -moz-element background and for the referenced element (if possible) and hook them up in the layer tree so the retained layer subtree for the referenced element is used directly in the -moz-element background. That would let us accelerate everything with the GPU.
Comment 13 Markus Stange [:mstange] 2010-05-28 03:23:23 PDT
Okay. For option (2), what place is there that can be accessed from both imglib and from layout? Do I need to create that place first?
Or should I just go with code duplication for now?
Comment 14 Lars Gunther 2010-05-28 04:21:45 PDT
Remember the evangelism follow up: Add a test to Modernizr and tip off the author of http://caniuse.com/

In fact, an evangelism tracking bug to make sure Modernizr tests for everything Firefox supports might be a good idea...
Comment 15 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-05-28 15:26:47 PDT
Lars: Indeed. Do you want to file that bug? You might even fix it :-)

Markus: we could put shared snapping logic in gfx maybe?
Comment 16 Markus Stange [:mstange] 2010-06-01 14:02:52 PDT
Created attachment 448606 [details] [diff] [review]
part 1, v1: style system
Comment 17 Markus Stange [:mstange] 2010-06-01 14:16:52 PDT
Comment on attachment 448606 [details] [diff] [review]
part 1, v1: style system

Wait, I think this leaks strings.
Comment 18 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2010-06-01 16:10:45 PDT
Probably the nsStyleImage::mElementId.  However, style structs don't get destructors run, so instead of making nsStyleImage::mElementId own a string, you should make it point to the memory in the style rule.
Comment 19 Markus Stange [:mstange] 2010-06-02 09:33:32 PDT
Created attachment 448764 [details] [diff] [review]
part 1, v2: style system

(In reply to comment #18)
> However, style structs don't get destructors run

Not sure what you mean, I see a destructor called ~nsStyleImage().

> so instead of making nsStyleImage::mElementId own a string,
> you should make it point to the memory in the style rule.

Is that the memory of the nsCSSValue that gets passed to SetStyleImage? That's what I'm using in this patch now. Is that nsCSSValue guaranteed to outlive the nsStyleImage object filled from it?
Comment 20 Markus Stange [:mstange] 2010-06-03 13:15:22 PDT
(In reply to comment #15)
> Markus: we could put shared snapping logic in gfx maybe?

Moving it into gfxUtils seems to work, thanks.
Comment 21 Markus Stange [:mstange] 2010-06-04 12:49:23 PDT
Ryo's latest patch makes dynamic updates to out-of-document canvases work by having nsHTMLCanvasElement dispatch a "redraw" event that nsSVGRenderingObserver listens for. Is that a good approach? These events aren't standard so I'm not sure if it's a good idea to expose them to web content.
Comment 22 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-06-05 15:06:44 PDT
I don't think we should be firing DOM events here.

We should probably just add a private C++ API to nsHTMLCanvasElement that lets you add an observer that will be called back on updates.
Comment 23 Markus Stange [:mstange] 2010-06-05 16:51:33 PDT
Created attachment 449487 [details] [diff] [review]
part 1, v3

The previous patch was missing the implementations of nsStyleImage::IsOpaque(), IsComplete() and operator==.
Comment 24 Markus Stange [:mstange] 2010-06-07 08:44:46 PDT
Comment on attachment 449487 [details] [diff] [review]
part 1, v3

Still not done... I need to add nsChangeHint_UpdateEffects in CalcDifference when the referenced element has changed. I'll request review when everything runs and passes tests.
Comment 25 Markus Stange [:mstange] 2010-06-17 08:26:20 PDT
Created attachment 451960 [details] [diff] [review]
part 1, v4: parsing

Like the previous version, but with changes to CalcDifference that also add nsChangeHint_UpdateEffects when -moz-element changes. This needs changes to MaxDifference and maxHint nsStyleContext::CalcStyleDifference.
Comment 26 Markus Stange [:mstange] 2010-06-17 08:27:31 PDT
Created attachment 451961 [details] [diff] [review]
part 2: Add a PAINT_ALL_CONTINUATIONS flag for nsLayoutUtils::PaintFrame.
Comment 27 Markus Stange [:mstange] 2010-06-17 08:29:10 PDT
Created attachment 451962 [details] [diff] [review]
part 3: Add nsSVGIntegrationUtils::DrawPaintServer

This is the heart of the operation. It needs lots of things from the dependent bugs.
Comment 28 Markus Stange [:mstange] 2010-06-17 08:30:55 PDT
Created attachment 451963 [details] [diff] [review]
part 4: -moz-element drawing

Compute the right size, call Acquire/ReleaseReferenceFrame at the right times, and delegate drawing to nsLayoutUtils::DrawPixelSnapped or nsSVGIntegrationUtils::DrawPaintServer.
Comment 29 Markus Stange [:mstange] 2010-06-17 08:33:15 PDT
Created attachment 451964 [details] [diff] [review]
part 5: Ryo's reftests

These are the reftests from Ryo's patch, adapted and culled until they passed. There are several instance where I compare scaled things to unscaled things at the same position, so the scaled version is blurred. In order to get rid of the blurring I apply an SVG threshold filter.
Comment 30 Markus Stange [:mstange] 2010-06-17 08:35:57 PDT
Created attachment 451965 [details] [diff] [review]
part 6: more reftests

Some more reftests I made. These test dynamic changes, multiple -moz-element background images, rendering sharpness and how fractional sizes are dealt with.
Comment 31 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2010-06-22 16:46:59 PDT
Comment on attachment 451960 [details] [diff] [review]
part 1, v4: parsing

>+#define VARIANT_IMAGE (VARIANT_HUO | VARIANT_GRADIENT | \
>+                       VARIANT_IMAGE_RECT | VARIANT_ELEMENT)

I think it goes against the existing pattern to include VARIANT_INHERIT
inside VARIANT_IMAGE.  I'd rather you drop the INHERIT (H) and make the
callers write VARIANT_IMAGE | VARIANT_INHERIT.

>+  // If we detect a syntax error, we must match the opening parenthesis of the
>+  // function with the closing parenthesis and skip all the tokens in between.

This seems like documenting the obvious, since it comes right above
  SkipUntil(')');
I'd suggest removing the comment.

nsStyleBackground::CalcDifference is wrong since it needs to check the
background layers first, and loop through all layers before returning
NS_STYLE_HINT_VISUAL, so that an early return doesn't return
NS_STYLE_HINT_VISUAL when nsChangeHint_UpdateEffects is also needed.

nsStyleImage::GetElementId should return |const PRUnichar*| rather
than |const nsString|, which creates multiple unneeded temporaries.


I'd like to look at the new version of nsStyleBackground::CalcDifference,
so marking review-.
Comment 32 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-06-27 18:30:49 PDT
+   * @param aNominalSize how big the "image" should be, in appunits

Name this aPaintServerSize and document it as well as you did in DrawableFromPaintServer?

PaintFrameCallback doesn't repeat when aRepeat is true. Isn't that a bug?
Comment 33 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-06-27 18:36:46 PDT
Factor out the calls to nsLayoutUtils::GetGraphicsFilterForFrame in ImageRenderer::Draw?

+      if (mPaintServerFrame) {
+        mPaintingProperty->ReleaseReferencedFrame();
+      }

How can this already be non-null?

Otherwise looks great!
Comment 34 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-06-27 18:40:31 PDT
Comment on attachment 451965 [details] [diff] [review]
part 6: more reftests

Change the 1.1 zoom factor to 1.2. We want 60/zoom to be an integer.
Comment 35 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-06-27 18:43:19 PDT
So with this patch, referencing a <canvas>, <video> or <img> that has a frame will include borders and padding in the rendering, but if it doesn't have a frame we won't get those. CSS scaling might also be applied if there's a frame, but not if there isn't.

Some people might complain about that difference, but I guess it makes sense.
Comment 36 Markus Stange [:mstange] 2010-08-08 14:45:12 PDT
Created attachment 463964 [details] [diff] [review]
part 1, v5: parsing
Comment 37 Markus Stange [:mstange] 2010-08-08 14:46:30 PDT
Created attachment 463965 [details] [diff] [review]
part 2, PAINT_ALL_CONTINUATIONS

updated to trunk
Comment 38 Markus Stange [:mstange] 2010-08-08 14:48:04 PDT
Created attachment 463966 [details] [diff] [review]
part 3a: use a frame state bit for paint loop detection
Comment 39 Markus Stange [:mstange] 2010-08-08 14:51:56 PDT
Created attachment 463967 [details] [diff] [review]
part 3b, add nsSVGIntegrationUtils::DrawPaintServer

(In reply to comment #32)
> +   * @param aNominalSize how big the "image" should be, in appunits
> 
> Name this aPaintServerSize and document it as well as you did in
> DrawableFromPaintServer?

Done.

> PaintFrameCallback doesn't repeat when aRepeat is true. Isn't that a bug?

It didn't have to; repeating is handled by gfxCallbackDrawable. I got rid of the aRepeat argument for gfxDrawingCallbacks in bug 572680.
Comment 40 Markus Stange [:mstange] 2010-08-08 14:53:27 PDT
Created attachment 463968 [details] [diff] [review]
part 4: -moz-element drawing

(In reply to comment #33)
> Factor out the calls to nsLayoutUtils::GetGraphicsFilterForFrame in
> ImageRenderer::Draw?

Done.

> +      if (mPaintServerFrame) {
> +        mPaintingProperty->ReleaseReferencedFrame();
> +      }
> 
> How can this already be non-null?

It can't. Removed the check.
Comment 41 Markus Stange [:mstange] 2010-08-08 14:54:59 PDT
Created attachment 463969 [details] [diff] [review]
part 5: invalidate observers
Comment 42 Markus Stange [:mstange] 2010-08-08 14:55:43 PDT
Created attachment 463970 [details] [diff] [review]
part 6: reftests
Comment 43 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-08-08 16:36:42 PDT
Comment on attachment 463969 [details] [diff] [review]
part 5: invalidate observers

This is great!

I think you need an additional change to add InvalidateDirectRenderingObservers to WebGLContext::Invalidate.
Comment 44 Markus Stange [:mstange] 2010-08-09 05:05:26 PDT
Created attachment 464030 [details] [diff] [review]
part 5: invalidate observers

with that change
Comment 45 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2010-08-11 12:01:45 PDT
Comment on attachment 463964 [details] [diff] [review]
part 1, v5: parsing

I think I'd be more comfortable if nsStyleImage::mElementId owned a copy
of the string rather than pointing into the style rule's data; I think
pointing into the style rule's data is ok if nsStyleImage are always in
style structs, but given the current configuration (and the lack of
comments documenting a change) I'm guessing you didn't audit all uses of
nsStyleImage and the strings they're passed to make sure the lifetimes are
ok.

So I'd prefer making mElementId PRUnichar* (no const, but keep the
const in GetElementId), cloning the string in SetElementId, and freeing
it in SetNull.

(It might be worth filing a followup bug to see if we can make
nsStyleImage stop copying and refcounting, though.)


nsStyleBackground::CalcDifference is wrong; it needs to loop through
all layers before deciding to return NS_STYLE_HINT_VISUAL.

Also, in CalcDifference, could you make moreLayers and lessLayers
pointers rather than references?


r=dbaron with those changes, although I'd sort of like to look at the
new CalcDifference.
Comment 46 Markus Stange [:mstange] 2010-08-12 12:20:43 PDT
Created attachment 465331 [details] [diff] [review]
part 1, v6: parsing
Comment 47 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2010-08-12 12:52:50 PDT
Comment on attachment 465331 [details] [diff] [review]
part 1, v6: parsing

nsStyleImage::GetElementId should still return |const PRUnichar*|; you don't want the caller messing with the data that the nsStyleImage owns.

r=dbaron with that
Comment 48 Markus Stange [:mstange] 2010-08-12 14:05:14 PDT
Created attachment 465376 [details] [diff] [review]
complete -moz-element patch for approval

Requesting approval on all patches in this bug, bug 572680, bug 572689 and bug 572688.

From roc's email:
> It adds significant new capabilities to the platform. It demos well. It will be
> useful for theming and personas. It matches and surpasses Webkit's CSS canvas
> backgrounds feature. The code will rot if we don't land it. It doesn't do
> anything scary to our existing code. And Markus is quite capable of fixing any
> issues that come up.
Comment 50 Marco Bonardo [::mak] 2010-08-13 10:03:37 PDT
pushed additional changeset on mstange's request: http://hg.mozilla.org/mozilla-central/rev/27f263f62bfb
Comment 51 Eric Shepherd [:sheppy] 2010-08-18 13:49:54 PDT
Now documented:

https://developer.mozilla.org/en/CSS/-moz-element

And listed on the CSS Reference's Mozilla extensions page and Firefox 4 for developers.

Do let me know if I missed anything.
Comment 52 Alexei 2011-12-17 02:20:27 PST
I beg to add support for the values ​​in CSS content and cursor. And it is not justice. Also, border-radius. I also ask to add CSS to mask your browser.

Note You need to log in before you can comment on or make changes to this bug.