Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Description • 11 years ago

Node.getBoxQuads is a very powerful but fairly easy to use API for obtaining the CSS boxes for a Node relative to any other Node (or viewport).
The proposal is in this thread and I think we reached consensus:
http://lists.w3.org/Archives/Public/www-style/2013Aug/0607.html
I have an implementation of this which I will attach as soon as I've broken it up into a proper patch series. My implementation is quite complete except it doesn't handle SVG text properly yet ... I'll probably defer that to followup.

My implementation includes a barebones implementation of DOMPoint, hence this blocks 850805.

	Mats Palmgren (inactive)
	Comment 7 • 11 years ago

Comment on attachment 807054 [details] [diff] [review]
Part 3: Add CoordinateTransformations, containing the real implementation of GetBoxQuads

Regarding dom/webidl/Node.webidl: I would move the 'box' field last
since I suspect the webidl to C++ translator lays out the fields in
the same order causing a 32-bit gap on 64-bit platforms; and given
our jemalloc granularities that will cause this struct to use twice
as much memory as it would with 'box' last.

Other than that, I don't know webidl well enough to know if it could
be written in a better way or not, so I'll delegate review of that file
to Boris ... and tag along a question on webidl: does webidl mandate
an ABI?

	Mats Palmgren (inactive)
	Comment 8 • 11 years ago

Comment on attachment 807055 [details] [diff] [review]
Part 4: Add DOMPoint and DOMQuad implementations

There's some (simple) webidl files in this patch too, they look fine
to me, but could have a quick look on those too?

	Mats Palmgren (inactive)
	Comment 9 • 11 years ago

Comment on attachment 807056 [details] [diff] [review]
Part 5: Implement nsINode::GetBoxQuads

One more webidl file.

	Boris Zbarsky [:bzbarsky]
	Comment 10 • 11 years ago

> since I suspect the webidl to C++ translator lays out the fields in
> the same order

They're laid out in lexicographic order by name, since that's the order all operations on them happen in, so the dictionary member list is just sorted in that order.

We could change that if we care, but who's ever going to be heap-allocating a BoxQuadOptions anyway?  The only consumer I see here is getBoxQuads, which takes it as an argument: that would be a stack-allocation.

As for ABI, WebIDL doesn't mandate one.  Since we generate C++ code that we then compile against Gecko headers, as long as the API is right it will work with whatever ABI the headers are using.

	Mats Palmgren (inactive)
	Comment 11 • 11 years ago

Fair enough for BoxQuadOptions, I was wondering more in general if there's a tradeoff
between optimal packing of fields and access speed.  It sounds like we have the freedom
to sort the fields in any order we like in the C++ translation.

	Mats Palmgren (inactive)
	Comment 12 • 11 years ago

<fieldset><legend>...  has getBoxQuads().length = 1, shouldn't the legend be included?
(similar to <table><caption>...)

Should outer bullet frames be included? (currently excluded)

and ::before/::after?

	Boris Zbarsky [:bzbarsky]
	Comment 13 • 11 years ago

Comment on attachment 807054 [details] [diff] [review]
Part 3: Add CoordinateTransformations, containing the real implementation of GetBoxQuads

r=me on the WebIDL bits, but I believe this patch won't compile standalone because we only codegen dictionaries that are actually used by something.  You can add a method taking BoxQuadOptions to DummyBinding.webidl to avoid that problem (and remove it later in this patch queue, I guess).

	Boris Zbarsky [:bzbarsky]
	Comment 14 • 11 years ago

Comment on attachment 807055 [details] [diff] [review]
Part 4: Add DOMPoint and DOMQuad implementations

Should the DOMPoint ctor take unrestricted double?  Specifically, should it allow infinities?

r=me modulo that question, as long as it's answered.

	Boris Zbarsky [:bzbarsky]
	Comment 15 • 11 years ago

Comment on attachment 807056 [details] [diff] [review]
Part 5: Implement nsINode::GetBoxQuads

>+  mozilla::GetBoxQuads(this, aOptions, quads, aRv);

If after this line aRv.Failed(), should probably just return immediately without uselessly touching aResult.

r=me with that and a comment added to Node.webidl, both above the partial interface and at the top of the file, pointing to the relevant spec.

That said, if we wanted to we could only expose this on Document/Element/Text instead of on Node, and maybe even make it infallible.  I guess that will get sorted out in the spec.

	Mats Palmgren (inactive)
	Comment 16 • 11 years ago

(In reply to Mats Palmgren (:mats) from comment #12)
> <fieldset><legend>...  has getBoxQuads().length = 1, shouldn't the legend be
> included?
> (similar to <table><caption>...)

Actually, I think my suggestion might be wrong, and it's the <table><caption> that is
wrong.  I need to read up on the thread(s) discussing this feature first to understand
it better.  At the moment I'm thinking that the only boxes that should be included are
those associated with the element itself, i.e. not <legend> nor <caption> in the examples
above, but include bullets, ::before/::after etc.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 17 • 11 years ago

(In reply to Mats Palmgren (:mats) from comment #11)
> Fair enough for BoxQuadOptions, I was wondering more in general if there's a
> tradeoff
> between optimal packing of fields and access speed.  It sounds like we have
> the freedom
> to sort the fields in any order we like in the C++ translation.

Yes. If we ever do anything to optimize this, it should be done in the bindings codegen.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 18 • 11 years ago

(In reply to Mats Palmgren (:mats) from comment #16)
> Actually, I think my suggestion might be wrong, and it's the
> <table><caption> that is
> wrong.  I need to read up on the thread(s) discussing this feature first to
> understand
> it better.  At the moment I'm thinking that the only boxes that should be
> included are
> those associated with the element itself, i.e. not <legend> nor <caption> in
> the examples
> above, but include bullets, ::before/::after etc.

The thread doesn't really address this; we just assumed we should behave like getClientRects(). The table behavior for getClientRects() was introduced in bug 414190. It's a special case because visually a table element visually contains its caption and it would be a bit confusing to just return the inner table frame's bounds. <fieldset> <legend> is a similar sort of case. I will raise this on www-style.

::before/::after create anonymous children for the element, thus I think they should not contribute to the quad list.

I'm not sure about bullets. I think probably not but I'll mention it on the list.

(In reply to Boris Zbarsky [:bz] from comment #15)
> r=me with that and a comment added to Node.webidl, both above the partial
> interface and at the top of the file, pointing to the relevant spec.

The spec hasn't been written yet, but I think Simon Pieters is going to.

> That said, if we wanted to we could only expose this on
> Document/Element/Text instead of on Node, and maybe even make it infallible.
> I guess that will get sorted out in the spec.

We'd also have to make relativeTo a union type in that case. Could be done, but seems a little unnecessary.

(In reply to Boris Zbarsky [:bz] from comment #14)
> Should the DOMPoint ctor take unrestricted double?  Specifically, should it
> allow infinities?

I think not but I'll ask on the list.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 19 • 11 years ago

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #18)
> The thread doesn't really address this; we just assumed we should behave
> like getClientRects(). The table behavior for getClientRects() was
> introduced in bug 414190. It's a special case because visually a table
> element visually contains its caption and it would be a bit confusing to
> just return the inner table frame's bounds. <fieldset> <legend> is a similar
> sort of case.

Actually our getClientRects() behavior is explicitly mandated as a special case in the CSSOM spec:
http://dev.w3.org/csswg/cssom-view/#dom-element-getclientrects
fieldset/legend on the other hand is not. Still needs discussion on the list, though.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 20 • 11 years ago

Probably should hold off reviewing these patches until the spec discussion settles down and I update these patches to the consensus.

	Mats Palmgren (inactive)
	Comment 21 • 11 years ago

Comment on attachment 807053 [details] [diff] [review]
Part 2: Add nsLayoutUtils::TransformCSSPoints and nsLayoutUtils::GetFirstNonAnonymousFrame

Clearing these review requests until the www-style discussion has settled
on how it should work, please ask again later.

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]
	Comment 22 • 11 years ago

A problem with the current patch is that if the document is zoomed in to any extent then:

All points are:
{w: 1, x: Infinity, y: Infinity, z: 0}

All bounds are:
{bottom: NaN, height: NaN, left: Infinity, right: NaN, top: Infinity, width: NaN}

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 23 • 11 years ago

OK, I'll look into that.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 24 • 11 years ago

Sorry about the delay here; some spec details ended up being controversial, especially around the design of DOMRect. But we have basically converged, so I'll update these patches soon.

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]
	Comment 25 • 11 years ago

Is it correct that the margin box co-ordinates are transformed? As far as I understand, this box should remain in it's original location because it is not affected by the transform.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 26 • 11 years ago

If you request margin boxes, they will be transformed. I think that's the right thing. It would be weird for border-boxes to be transformed and margin-boxes for the same element not transformed.

	Paul Rouget [:paul]
	Comment 27 • 11 years ago

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #24)
> Sorry about the delay here; some spec details ended up being controversial,
> especially around the design of DOMRect. But we have basically converged, so
> I'll update these patches soon.

Any update on this?

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]
	Comment 28 • 11 years ago

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #22)
> A problem with the current patch is that if the document is zoomed in to any
> extent then:
> 
> All points are:
> {w: 1, x: Infinity, y: Infinity, z: 0}
> 
> All bounds are:
> {bottom: NaN, height: NaN, left: Infinity, right: NaN, top: Infinity, width:
> NaN}

This also happens at the default zoom level on Retina displays.

	Paul Rouget [:paul]
	Comment 29 • 10 years ago

Is there anything we can do here to help you?

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 30 • 10 years ago

This is basically unchanged from the old "part 2", except I fixed the bug found in comments #22 and #28 (need to force floating-point division instead of integer division).

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 31 • 10 years ago

This patch (and following patches) follows the spec in http://dev.w3.org/fxtf/geometry/.

It's simple stuff, so although I've made DOMPoint pref-controlled, I see no reason to not just let this go through to release. These geometry interfaces were quite heavily discussed and I think we just need to ship something to force them to stabilize.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 32 • 10 years ago

DOMRect is not pref-controlled since stuff like getBoundingClientRect already uses it. I don't have any qualms about these changes though.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 34 • 10 years ago

getBoxQuads IDL is given in http://dev.w3.org/csswg/cssom-view/ (designed by Simon). Unfortunately there's no description of the actual semantics there yet. There was quite a bit of discussion on www-style and as far as I can tell we reached a consensus on most edge cases; we certainly did for all the common cases. There may be some edge cases unresolved but my tests seem reasonably thorough.

I think we should land this so people can start using it. It's unlikely to create compatibility risk. I'll push on Simon to write a proper spec.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 36 • 10 years ago

(In reply to Paul Rouget [:paul] from comment #29)

Sorry about the delay.

	Mats Palmgren (inactive)
	Comment 37 • 10 years ago

Comment on attachment 8356406 [details] [diff] [review]
Part 1: Add nsLayoutUtils::TransformCSSPoints and nsLayoutUtils::GetFirstNonAnonymousFrame

>layout/base/nsLayoutUtils.h
>+  static TransformResult TransformCSSPoints(nsIFrame* aFromFrame, nsIFrame* aToFrame,
>+                                            uint32_t aPointCount, gfxPoint* aPoints);

Do we actually gain anything by using aPoints as an in/out parameter?
If not, then "const nsPoint* aAppPoints, CSSPoint* aCSSPoints" seems better.
Add @param doc comments for those with [in] / [out] for clarity.
Please point out that the resulting aCSSPoints values are undefined
unless the return value is TRANSFORM_SUCCEEDED.
And rename the method - TransformPoints? TransformToCSSPoints?

Perhaps we should also consider using nsTArray& instead of pointer +
length?  I think explicitly sized nsAutoTArray:s have performance on par
with stack POD arrays if used correctly (and they're safer).

>layout/base/nsLayoutUtils.cpp
>+nsLayoutUtils::TransformCSSPoints(nsIFrame* aFromFrame, nsIFrame* aToFrame,

Please add spaces around / and * operators in this method.

>+  double devPixelsPerCSSPixelFromFrame =
>+  double devPixelsPerCSSPixelToFrame =

Those two should probably be gfxFloat, in case we one day define
gfxFloat to be float rather than double in some configurations.

>+    // answer [...] its reciprocal

Please end the sentence with a period.

	Mats Palmgren (inactive)
	Comment 38 • 10 years ago

Comment on attachment 8356410 [details] [diff] [review]
Part 2: Implement DOMPoint

I think a DOM peer should review this part.

	Mats Palmgren (inactive)
	Comment 39 • 10 years ago

Comment on attachment 8356411 [details] [diff] [review]
Part 3: Implement DOMRect per spec

I think a DOM peer should review this part.

	Mats Palmgren (inactive)
	Comment 40 • 10 years ago

Comment on attachment 8356413 [details] [diff] [review]
Part 4: Add DOMQuad implementation

I think a DOM peer should review this part.

	Mats Palmgren (inactive)
	Comment 41 • 10 years ago

Comment on attachment 8356415 [details] [diff] [review]
Part 5: Implement getBoxQuads

I have only reviewed layout/base/GeometryUtils.*
The other files should be reviewed by a DOM peer.

>layout/base/GeometryUtils.h
>+#include "gfxPoint.h"

Can that be moved to GeometryUtils.cpp?

>layout/base/GeometryUtils.cpp
>+void GetBoxQuads(nsINode* aNode, ...
>+{
>+  nsIDocument* ownerDoc = aNode->OwnerDoc();
>+  nsIFrame* frame = GetFrameForNode(aNode);
>+  if (!frame) {
>+    // No boxes to return
>+    return;
>+  }

'ownerDoc' can be moved after the if-statement.

>+  if (!relativeToFrame) {
>+    aRv.Throw(NS_ERROR_DOM_NOT_FOUND_ERR);

The spec doesn't say we can throw here, is this really what we want?
(our GeometryUtils.webidl has [Throws] unlike the spec)  Please sort
this out with Simon and remove these aRv.Throw() calls if needed.
If you've already decided that throwing is desired and the spec is
lagging behind, then this looks OK.

r=mats, but I'd like to review AddBox again if TransformCSSPoints
changes in the earlier patch.  The rest looks good.

	Mats Palmgren (inactive)
	Comment 42 • 10 years ago

Comment on attachment 8356416 [details] [diff] [review]
Part 6: Add tests for getBoxQuads

LGTM, r=mats

Perhaps add tests that checks we throw the right thing for 
document.createDocumentFragment().getBoxQuads()
and
document.body.getBoxQuads({relativeTo:document.createDocumentFragment()}

	Patrick Brosset <:pbro>
	Comment 43 • 10 years ago

This bug is blocking bug 663778 (drawing a box-model highlighter for nodes in the DevTools) which we'd hope to land for the up coming release.
It'd be great if you could let us know if you have an ETA for this bug?

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]
	Comment 44 • 10 years ago

When a page contains an iframe at 100,100 that contains a node at 10,10 getBoxQuads returns 10,10.

I would have expected the points to be relative to the viewport position and not their parent frames.

	Boris Zbarsky [:bzbarsky]
	Comment 45 • 10 years ago

Michael, each frame has its own viewport.  So yes, the points are relative to the viewport position, for the relevant viewport.  Just like getBoundingClientRect(), client*, etc.

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]
	Comment 46 • 10 years ago

I am really sorry, I feel like a troll but attachment 8356406 [details] [diff] [review] did not fix the zoom / retina issue. Although it returns values they are not correct.

- The x and y co-ordinates on zoomed pages are too low at low zoom levels and too high at higher levels.
- The width and height have the same value as the non-zoomed div.

Use full zoom in this test, refresh the page, check the values and it is clear that they are not correct.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 47 • 10 years ago

(In reply to Mats Palmgren (:mats) from comment #37)
> Do we actually gain anything by using aPoints as an in/out parameter?
> If not, then "const nsPoint* aAppPoints, CSSPoint* aCSSPoints" seems better.

We shouldn't make the inputs be nsPoints since in bug 918189 we need to convert DOM-provided CSS pixel values in double format.

Given the input and output points are the same type and the same number, it seems a little simpler to just convert them in-place than to create two separate arrays for no particular reason.

Do you think "const gfxPoint* aInputPoints, const gfxPoint* aOutputPoints" would be better? If so, why?

> Add @param doc comments for those with [in] / [out] for clarity.
> Please point out that the resulting aCSSPoints values are undefined
> unless the return value is TRANSFORM_SUCCEEDED.

Sure.

> And rename the method - TransformPoints? TransformToCSSPoints?

Sure.

> Perhaps we should also consider using nsTArray& instead of pointer +
> length?  I think explicitly sized nsAutoTArray:s have performance on par
> with stack POD arrays if used correctly (and they're safer).

Yes, but they're also a bit more verbose and these are fixed-length arrays in all callers so I don't think it will be a problem.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 48 • 10 years ago

(In reply to Mats Palmgren (:mats) from comment #41)
> >+  if (!relativeToFrame) {
> >+    aRv.Throw(NS_ERROR_DOM_NOT_FOUND_ERR);
> 
> The spec doesn't say we can throw here, is this really what we want?
> (our GeometryUtils.webidl has [Throws] unlike the spec)  Please sort
> this out with Simon and remove these aRv.Throw() calls if needed.
> If you've already decided that throwing is desired and the spec is
> lagging behind, then this looks OK. 

Yeah, I'm pretty sure the spec will catch up here.

	Mats Palmgren (inactive)
	Comment 55 • 10 years ago

Comment on attachment 8388503 [details] [diff] [review]
Part 1: Add nsLayoutUtils::TransformCSSPoints and nsLayoutUtils::GetFirstNonAnonymousFrame

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #47)
> Given the input and output points are the same type and the same number, it
> seems a little simpler to just convert them in-place than to create two
> separate arrays for no particular reason.

Ah, I see that I mistakenly thought this method took app unit points
and produced CSS points which is why I suggested using separate types.

Changing CSSPoints in place is fine when it's the same type.

> > Perhaps we should also consider using nsTArray& instead of pointer +
> > length?  I think explicitly sized nsAutoTArray:s have performance on par
> > with stack POD arrays if used correctly (and they're safer).
> 
> Yes, but they're also a bit more verbose and these are fixed-length arrays
> in all callers so I don't think it will be a problem.

Yeah, I'm just a bit grumpy on adding more T* + length APIs after seeing too many
security bugs emanating from such APIs.  Fixed-length stack arrays is somewhat
easier to spot errors for though, so OK.

> layout/base/nsLayoutUtils.h
>+   * Transforms a list of points, given as gfxPoints representing CSS pixels,

Remove the part about gfxPoints since it now takes the correct type.
Maybe just shorten it to "Transforms a list of CSS points, from aFromFrame ..."

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 56 • 10 years ago

There's a few test failures, most of which are easy to fix:
https://tbpl.mozilla.org/?tree=Try&rev=c966a1395db9

One remaining issue is due to float imprecision causing test failures. We could fix this by using gfxPoint instead of CSSPoint to get double precision. But I think for now I'll just fix it by increasing the tolerance in the tests.

	Johnny Stenback (:jst)
	Comment 62 • 10 years ago

Looks all good! Given that this exposes new APIs to the web I'd appreciate an intent to ship email to dev.platform here. Thanks.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 63 • 10 years ago

(In reply to Johnny Stenback (:jst, jst@mozilla.com) from comment #62)
> Looks all good! Given that this exposes new APIs to the web I'd appreciate
> an intent to ship email to dev.platform here. Thanks.

Done.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 64 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/094decae8ff3
https://hg.mozilla.org/integration/mozilla-inbound/rev/002dbc057a74
https://hg.mozilla.org/integration/mozilla-inbound/rev/92e5d79ec57f
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b41ab878fcd
https://hg.mozilla.org/integration/mozilla-inbound/rev/8ad57cf09e75
https://hg.mozilla.org/integration/mozilla-inbound/rev/4771c2b309ad
https://hg.mozilla.org/integration/mozilla-inbound/rev/896bfaeab866
https://hg.mozilla.org/integration/mozilla-inbound/rev/1b7530ddb0af

	Carsten Book [:Tomcat]
	Comment 65 • 10 years ago

Hi,

sorry Roc had to backout this changes as part of https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=12e871cf1100 for test failures like https://tbpl.mozilla.org/php/getParsedLog.php?id=36383057&tree=Mozilla-Inbound - not sure if this is a problem in the test or this checkins here

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 66 • 10 years ago

Relanded with test fixed. On B2G window.open doesn't work (and on the emulator it causes weird crashes/timeouts).
https://hg.mozilla.org/integration/mozilla-inbound/rev/472cb7738e14
https://hg.mozilla.org/integration/mozilla-inbound/rev/b049571a7cce
https://hg.mozilla.org/integration/mozilla-inbound/rev/f3bbd98021f7
https://hg.mozilla.org/integration/mozilla-inbound/rev/5272cfbd63f3
https://hg.mozilla.org/integration/mozilla-inbound/rev/ada41f2f74b8
https://hg.mozilla.org/integration/mozilla-inbound/rev/5117e3f594e7
https://hg.mozilla.org/integration/mozilla-inbound/rev/43bceca43fb6
https://hg.mozilla.org/integration/mozilla-inbound/rev/98e31d225a5a

	Ryan VanderMeulen [:RyanVM]
	Comment 67 • 10 years ago

Backed out for bustage.
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e640765c3f4

https://tbpl.mozilla.org/php/getParsedLog.php?id=36462189&tree=Mozilla-Inbound

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 68 • 10 years ago

Revision 7c1d075bb7f6 landed this morning and managed to break my patches without causing a merge conflict.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 69 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/9b73b517cb53
https://hg.mozilla.org/integration/mozilla-inbound/rev/4388c77d8dab
https://hg.mozilla.org/integration/mozilla-inbound/rev/b5aeab9d58fa
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1e259b9983e
https://hg.mozilla.org/integration/mozilla-inbound/rev/9506a2117caa
https://hg.mozilla.org/integration/mozilla-inbound/rev/d5ffda84968f
https://hg.mozilla.org/integration/mozilla-inbound/rev/01597ae3e0aa
https://hg.mozilla.org/integration/mozilla-inbound/rev/9ae5146d1267

	Carsten Book [:Tomcat]
	Comment 70 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/9b73b517cb53
https://hg.mozilla.org/mozilla-central/rev/4388c77d8dab
https://hg.mozilla.org/mozilla-central/rev/b5aeab9d58fa
https://hg.mozilla.org/mozilla-central/rev/b1e259b9983e
https://hg.mozilla.org/mozilla-central/rev/9506a2117caa
https://hg.mozilla.org/mozilla-central/rev/d5ffda84968f
https://hg.mozilla.org/mozilla-central/rev/01597ae3e0aa
https://hg.mozilla.org/mozilla-central/rev/9ae5146d1267

	Patrick Brosset <:pbro>
	Comment 71 • 10 years ago

So I just learned from :philor that this API isn't enabled in release builds:
http://mxr.mozilla.org/mozilla-central/source/modules/libpref/src/init/all.js#1883

In the devtools, we have a nice box-model highlighter (bug 663778) that, before this landed, used to rely on a simple node.getBoxQuads polyfill. Now that the API is available, we removed that polyfill in bug 969306.
That bug is in 32, Aurora today, and will move to beta in less than 20 days.

Roc: what's the plan for node.getBoxQuads to be enabled in release builds?
Depending on this, we will need to re-introduce our polyfill and test for the "layout.css.getBoxQuads.enabled" flag to use the real API or the polyfill.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 72 • 10 years ago

We should just enable it in release builds. Filed bug 1033276 on that.

If that doesn't get resolved in a couple of weeks, I guess you'll need to revert to your polyfill before merging to Beta.

	Boris Zbarsky [:bzbarsky]
	Comment 73 • 10 years ago

Or we could enable in privileged code on all channels.

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]
	Comment 74 • 10 years ago

(In reply to Boris Zbarsky [:bz] from comment #73)
> Or we could enable in privileged code on all channels.

+1, bug 1033391 logged.