Closed
Bug 917755
Opened 11 years ago
Closed 11 years ago
Implement Node.getBoxQuads
Categories
(Core :: Layout, defect)
Tracking
()
RESOLVED
FIXED
mozilla31
People
(Reporter: roc, Assigned: roc)
References
(Blocks 2 open bugs)
Details
(Keywords: dev-doc-needed)
Attachments
(9 files, 15 obsolete files)
24.33 KB,
patch
|
MatsPalmgren_bugz
:
review+
|
Details | Diff | Splinter Review |
1.45 KB,
text/html
|
Details | |
10.34 KB,
patch
|
MatsPalmgren_bugz
:
review+
|
Details | Diff | Splinter Review |
9.38 KB,
patch
|
MatsPalmgren_bugz
:
review+
|
Details | Diff | Splinter Review |
1.60 KB,
patch
|
MatsPalmgren_bugz
:
review+
|
Details | Diff | Splinter Review |
10.01 KB,
patch
|
jst
:
review+
|
Details | Diff | Splinter Review |
13.56 KB,
patch
|
jst
:
review+
|
Details | Diff | Splinter Review |
12.06 KB,
patch
|
jst
:
review+
|
Details | Diff | Splinter Review |
7.02 KB,
patch
|
jst
:
review+
|
Details | Diff | Splinter Review |
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.
Updated•11 years ago
|
Keywords: dev-doc-needed
Assignee | ||
Comment 1•11 years ago
|
||
Attachment #807052 -
Flags: review?(matspal)
Assignee | ||
Comment 2•11 years ago
|
||
Attachment #807053 -
Flags: review?(matspal)
Assignee | ||
Comment 3•11 years ago
|
||
Attachment #807054 -
Flags: review?(matspal)
Assignee | ||
Comment 4•11 years ago
|
||
Attachment #807055 -
Flags: review?(matspal)
Assignee | ||
Updated•11 years ago
|
Attachment #807054 -
Attachment description: Add CoordinateTransformations, containing the real implementation of GetBoxQuads → Part 3: Add CoordinateTransformations, containing the real implementation of GetBoxQuads
Assignee | ||
Comment 5•11 years ago
|
||
Attachment #807056 -
Flags: review?(matspal)
Assignee | ||
Comment 6•11 years ago
|
||
Attachment #807057 -
Flags: review?(matspal)
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?
Attachment #807054 -
Flags: review?(bzbarsky)
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?
Attachment #807055 -
Flags: review?(bzbarsky)
Comment 9•11 years ago
|
||
Comment on attachment 807056 [details] [diff] [review]
Part 5: Implement nsINode::GetBoxQuads
One more webidl file.
Attachment #807056 -
Flags: review?(bzbarsky)
Updated•11 years ago
|
Attachment #807052 -
Flags: review?(matspal) → review+
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.
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.
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?
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).
Attachment #807054 -
Flags: review?(bzbarsky) → review+
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.
Attachment #807055 -
Flags: review?(bzbarsky) → review+
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.
Attachment #807056 -
Flags: review?(bzbarsky) → review+
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.
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.
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.
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.
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.
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.
Attachment #807053 -
Flags: review?(matspal)
Updated•11 years ago
|
Attachment #807054 -
Flags: review?(matspal)
Updated•11 years ago
|
Attachment #807055 -
Flags: review?(matspal)
Updated•11 years ago
|
Attachment #807057 -
Flags: review?(matspal)
Updated•11 years ago
|
Attachment #807056 -
Flags: review?(matspal)
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}
Assignee | ||
Comment 23•11 years ago
|
||
OK, I'll look into that.
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.
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.
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.
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?
Updated•11 years ago
|
Flags: needinfo?(roc)
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.
Assignee | ||
Comment 30•11 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).
Attachment #807052 -
Attachment is obsolete: true
Attachment #807053 -
Attachment is obsolete: true
Attachment #807054 -
Attachment is obsolete: true
Attachment #807055 -
Attachment is obsolete: true
Attachment #807056 -
Attachment is obsolete: true
Attachment #807057 -
Attachment is obsolete: true
Attachment #8356406 -
Flags: review?(matspal)
Assignee | ||
Comment 31•11 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.
Attachment #8356410 -
Flags: review?(matspal)
Assignee | ||
Comment 32•11 years ago
|
||
DOMRect is not pref-controlled since stuff like getBoundingClientRect already uses it. I don't have any qualms about these changes though.
Attachment #8356411 -
Flags: review?(matspal)
Assignee | ||
Comment 33•11 years ago
|
||
Attachment #8356413 -
Flags: review?(matspal)
Assignee | ||
Comment 34•11 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.
Attachment #8356415 -
Flags: review?(matspal)
Assignee | ||
Comment 35•11 years ago
|
||
Attachment #8356416 -
Flags: review?(matspal)
Assignee | ||
Comment 36•11 years ago
|
||
(In reply to Paul Rouget [:paul] from comment #29)
Sorry about the delay.
Comment 37•11 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.
Attachment #8356406 -
Flags: review?(matspal) → review-
Comment 38•11 years ago
|
||
Comment on attachment 8356410 [details] [diff] [review]
Part 2: Implement DOMPoint
I think a DOM peer should review this part.
Attachment #8356410 -
Flags: review?(matspal)
Comment 39•11 years ago
|
||
Comment on attachment 8356411 [details] [diff] [review]
Part 3: Implement DOMRect per spec
I think a DOM peer should review this part.
Attachment #8356411 -
Flags: review?(matspal)
Comment 40•11 years ago
|
||
Comment on attachment 8356413 [details] [diff] [review]
Part 4: Add DOMQuad implementation
I think a DOM peer should review this part.
Attachment #8356413 -
Flags: review?(matspal)
Comment 41•11 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.
Attachment #8356415 -
Flags: review?(matspal) → review+
Comment 42•11 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()}
Attachment #8356416 -
Flags: review?(matspal) → review+
Comment 43•11 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?
Comment 44•11 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.
Comment 45•11 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.
Comment 46•11 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.
Assignee | ||
Comment 47•11 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.
Flags: needinfo?(matspal)
Assignee | ||
Comment 48•11 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.
Assignee | ||
Comment 49•11 years ago
|
||
Attachment #8356406 -
Attachment is obsolete: true
Attachment #8388503 -
Flags: review?(matspal)
Assignee | ||
Comment 50•11 years ago
|
||
Attachment #8356410 -
Attachment is obsolete: true
Attachment #8388504 -
Flags: review?(mrbkap)
Assignee | ||
Comment 51•11 years ago
|
||
Attachment #8356411 -
Attachment is obsolete: true
Attachment #8388505 -
Flags: review?(mrbkap)
Assignee | ||
Comment 52•11 years ago
|
||
Attachment #8356413 -
Attachment is obsolete: true
Attachment #8388507 -
Flags: review?(mrbkap)
Assignee | ||
Comment 53•11 years ago
|
||
Attachment #8356415 -
Attachment is obsolete: true
Attachment #8388509 -
Flags: review?(matspal)
Assignee | ||
Comment 54•11 years ago
|
||
Attachment #8388510 -
Flags: review?(mrbkap)
Comment 55•11 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 ..."
Attachment #8388503 -
Flags: review?(matspal) → review+
Flags: needinfo?(matspal)
Updated•11 years ago
|
Attachment #8388509 -
Flags: review?(matspal) → review+
Assignee | ||
Comment 56•11 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.
Assignee | ||
Comment 57•11 years ago
|
||
Attachment #8389628 -
Flags: review?(matspal)
Assignee | ||
Comment 58•11 years ago
|
||
Attachment #8388504 -
Attachment is obsolete: true
Attachment #8388504 -
Flags: review?(mrbkap)
Attachment #8389630 -
Flags: review?(jst)
Assignee | ||
Comment 59•11 years ago
|
||
Attachment #8388505 -
Attachment is obsolete: true
Attachment #8388505 -
Flags: review?(mrbkap)
Attachment #8389631 -
Flags: review?(jst)
Assignee | ||
Comment 60•11 years ago
|
||
Attachment #8388507 -
Attachment is obsolete: true
Attachment #8388507 -
Flags: review?(mrbkap)
Attachment #8389632 -
Flags: review?(jst)
Assignee | ||
Comment 61•11 years ago
|
||
Attachment #8388510 -
Attachment is obsolete: true
Attachment #8388510 -
Flags: review?(mrbkap)
Attachment #8389633 -
Flags: review?(jst)
Updated•11 years ago
|
Attachment #8389628 -
Flags: review?(matspal) → review+
Updated•11 years ago
|
Attachment #8389630 -
Flags: review?(jst) → review+
Updated•11 years ago
|
Attachment #8389631 -
Flags: review?(jst) → review+
Updated•11 years ago
|
Attachment #8389632 -
Flags: review?(jst) → review+
Updated•11 years ago
|
Attachment #8389633 -
Flags: review?(jst) → review+
Comment 62•11 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.
Assignee | ||
Comment 63•11 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.
Assignee | ||
Comment 64•11 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
Comment 65•11 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
Assignee | ||
Comment 66•11 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
Comment 67•11 years ago
|
||
Assignee | ||
Comment 68•11 years ago
|
||
Revision 7c1d075bb7f6 landed this morning and managed to break my patches without causing a merge conflict.
Assignee | ||
Comment 69•11 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
Comment 70•11 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
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
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.
Flags: needinfo?(roc)
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.
Flags: needinfo?(roc)
Comment 73•10 years ago
|
||
Or we could enable in privileged code on all channels.
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.
Blocks: geometry-1
Comment 75•5 years ago
|
||
BCD PR 4828 submitted to add this to BCD.
Keywords: dev-doc-needed → dev-doc-complete
Comment 76•5 years ago
|
||
So... getBoxQuads() is only available in privileged code at this time?
Comment 77•5 years ago
|
||
With this only available in chrome right now, I'm going to back out the addition of getBoxQuads() to Firefox's BCD record, since it's not "available available."
Keywords: dev-doc-complete → dev-doc-needed
Comment 78•5 years ago
|
||
It's available to privileged code in release and beta; available to the web at large in nightly. Looks like bug 1107559 tracks enabling it in release builds.
You need to log in
before you can comment on or make changes to this bug.
Description
•